FIRE-813 | Gradio UI | MCP Support #110
Conversation
…-transport-support
…-transport-support
…-transport-support
…RE-813-mcp-gradio
…RE-813-mcp-gradio
…RE-813-mcp-gradio
yuval-qf
had a problem deploying
to
rogue-sanity-ci-secrets
GitHub Actions
Failure
yuval-qf
temporarily deployed
to
rogue-sanity-ci-secrets
GitHub Actions
Inactive
![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 (4)
rogue/ui/components/config_screen.py (3)
171-176: Map error labels to Pydantic field names to show field errors.AgentConfig errors use keys like evaluated_agent_url / evaluated_agent_credentials. Current keys won’t match, so errors fall to the general label.
- error_labels = { - "agent_url": agent_url_error, - "auth_credentials": auth_credentials_error, - "judge_llm_api_key": judge_llm_api_key_error, - # "huggingface_api_key": huggingface_api_key_error, - } + error_labels = { + "evaluated_agent_url": agent_url_error, + "evaluated_agent_credentials": auth_credentials_error, + "judge_llm_api_key": judge_llm_api_key_error, + # "huggingface_api_key": huggingface_api_key_error, + }
178-181: Do not log secrets; mask sensitive values.update_state logs raw values, leaking credentials/API keys. Mask sensitive keys or drop the value from logs. As per coding guidelines.
- def update_state(state, key, value): - logger.info(f"Updating state: {key} = {value}") + SENSITIVE_KEYS = { + "evaluated_agent_credentials", + "auth_credentials", + "judge_llm_api_key", + "qualifire_api_key", + } + + def update_state(state, key, value): + safe_value = "[REDACTED]" if key in SENSITIVE_KEYS else value + logger.debug(f"Updating state: {key} = {safe_value}") state["config"][key] = value return state
78-83: Change default in config_screen.py to match app.py and codebase patterns.The mismatch is confirmed: config_screen.py line 81 defaults deep_test_mode to True, while rogue/ui/app.py line 134 defaults to False. The codebase standardizes on False across sdks/python/rogue_sdk/types.py, rogue/run_cli.py, and rogue/models/cli_input.py, making False the canonical default. Changing config_screen.py to False removes the inconsistency between initial form state and loaded config state.
- value=config_data.get("deep_test_mode", True), + value=config_data.get("deep_test_mode", False),.rogue/user_config.json (1)
1-13: Remove .rogue/user_config.json from git tracking and create .gitignore to prevent future commits.The file is currently committed to git (
H .rogue/user_config.json) with no .gitignore to protect it. Since it's a configuration file carrying credentials and internal URLs, it must be excluded from version control:
- Remove from tracking:
git rm --cached .rogue/user_config.json- Create
.gitignorewith entry:.rogue/- Commit the .gitignore update
🧹 Nitpick comments (5)
rogue/ui/app.py (1)
105-151: Optional: annotate functions.Consider adding return/param type hints for get_app and load_and_update_ui to satisfy repo typing rules. As per coding guidelines.
- def load_and_update_ui(): + from typing import Any, Dict + def load_and_update_ui() -> Dict[Any, Any]: ... - def get_app(workdir: Path, rogue_server_url: str): + from typing import Any + def get_app(workdir: Path, rogue_server_url: str) -> Any:rogue/ui/components/config_screen.py (4)
183-196: Align state keys with AgentConfig fields for consistency and error handling.Use evaluated_* keys so state mirrors persisted config and error mapping.
- for component, key in [ - (agent_url, "agent_url"), + for component, key in [ + (agent_url, "evaluated_agent_url"), (protocol, "protocol"), (transport, "transport"), (interview_mode, "interview_mode"), - (auth_type, "auth_type"), - (auth_credentials, "auth_credentials"), + (auth_type, "evaluated_agent_auth_type"), + (auth_credentials, "evaluated_agent_credentials"), (service_llm, "service_llm"), (judge_llm, "judge_llm"), (judge_llm_api_key, "judge_llm_api_key"), # (huggingface_api_key, "huggingface_api_key"), (deep_test_mode, "deep_test_mode"), (parallel_runs, "parallel_runs"), ]:
213-219: Preserve selected transport when still valid for the new protocol.Currently resets to default on any protocol change. Keep current value if compatible; otherwise, fallback.
- def update_transport_choices(protocol_val): + def update_transport_choices(protocol_val, current_transport_val): """Update transport choices based on selected protocol.""" selected_protocol = Protocol(protocol_val) transport_choices = [t.value for t in PROTOCOL_TO_TRANSPORTS[selected_protocol]] - default_transport = selected_protocol.get_default_transport().value - return gr.update(choices=transport_choices, value=default_transport) + default_transport = selected_protocol.get_default_transport().value + new_value = ( + current_transport_val + if current_transport_val in transport_choices + else default_transport + ) + return gr.update(choices=transport_choices, value=new_value)And wire the current transport as input:
- protocol.change( + protocol.change( fn=update_transport_choices, - inputs=[protocol], + inputs=[protocol, transport], outputs=[transport], )
247-261: Optionally validate protocol/transport before saving.Defensive check ensures persisted config is compatible even if UI state gets out of sync.
- config = AgentConfig( + # Defensive compatibility check + try: + proto_enum = Protocol(protocol_val) + trans_enum = Transport(transport_val) + except Exception: + proto_enum = Protocol.A2A + trans_enum = proto_enum.get_default_transport() + if trans_enum not in PROTOCOL_TO_TRANSPORTS[proto_enum]: + trans_enum = proto_enum.get_default_transport() + + config = AgentConfig( evaluated_agent_url=url, evaluated_agent_auth_type=auth_t, evaluated_agent_credentials=creds, service_llm=service_llm_val, judge_llm=llm, judge_llm_api_key=llm_key, deep_test_mode=deep_test_mode_val, - protocol=protocol_val, - transport=transport_val, + protocol=proto_enum, + transport=trans_enum, interview_mode=interview_mode_val, parallel_runs=parallel_runs_val, business_context="", )
13-16: Add type hints to public function.Annotate create_config_screen to satisfy repo typing checks. As per coding guidelines.
-from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any, Tuple ... -def create_config_screen( - shared_state: "State", - tabs_component: "Tabs", -) : +def create_config_screen( + shared_state: "State", + tabs_component: "Tabs", +) -> Tuple[Any, ...]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.rogue/user_config.json(1 hunks)rogue/ui/app.py(4 hunks)rogue/ui/components/config_screen.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format Python code with Black
Ensure code passes flake8 linting
Run mypy with the repository configuration for static typing
Run Bandit security checks using .bandit.yaml configuration
Use isort import conventions for import ordering
Add type hints to all function signatures
Follow PEP 8 naming (snake_case for variables/functions, PascalCase for classes)
Use try/except around code that may raise exceptions
Files:
rogue/ui/components/config_screen.pyrogue/ui/app.py
🧬 Code graph analysis (2)
rogue/ui/components/config_screen.py (1)
sdks/python/rogue_sdk/types.py (4)
AgentConfig(104-136)AuthType(23-48)Protocol(68-79)get_default_transport(74-79)
rogue/ui/app.py (1)
sdks/python/rogue_sdk/types.py (5)
AuthType(23-48)Protocol(68-79)Transport(82-93)is_valid_for_protocol(92-93)get_default_transport(74-79)
🔇 Additional comments (9)
.rogue/user_config.json (1)
2-3: Defaults align with new Protocol/Transport.mcp + streamable_http are consistent with SDK defaults.
rogue/ui/app.py (5)
4-4: Import addition looks correct.AuthType, Protocol, Transport are used below.
38-49: Wiring protocol/transport from config screen: good.Tuple expansion matches added UI fields.
123-125: Compatibility fallback is correct.Good: invalid transport falls back to protocol default.
131-133: UI sync for protocol/transport values is correct.Using .value ensures dropdowns get strings.
157-158: Output list update looks consistent.Adding protocol and transport to app.load outputs matches inputs.
rogue/ui/components/config_screen.py (3)
36-45: Protocol dropdown: good addition.Choices derived from Protocol enum and defaulting to A2A is fine.
58-64: Transport dropdown initialization looks correct.Choices and default align with selected protocol.
323-336: Return tuple update is correct.Protocol and Transport included; ordering consistent with get_app consumer.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
yuval-qf
had a problem deploying
to
rogue-sanity-ci-secrets
GitHub Actions
Failure
yuval-qf
temporarily deployed
to
rogue-sanity-ci-secrets
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rogue/ui/components/config_screen.py (2)
180-184: Stop logging secrets in state updates.logger.info currently logs plaintext values (incl. auth creds, API keys). Mask sensitive keys.
Apply:
- def update_state(state, key, value): - logger.info(f"Updating state: {key} = {value}") + def update_state(state: dict, key: str, value): + sensitive = {"auth_credentials", "judge_llm_api_key", "huggingface_api_key", "qualifire_api_key"} + display = "***redacted***" if key in sensitive else value + logger.info(f"Updating state: {key} = {display}") state["config"][key] = value return stateAs per coding guidelines.
173-179: Map error labels to AgentConfig field names so field‑level errors render.Pydantic loc keys are model fields (e.g., evaluated_agent_url, evaluated_agent_credentials). Current keys won’t match.
Apply:
- error_labels = { - "agent_url": agent_url_error, - "auth_credentials": auth_credentials_error, - "judge_llm_api_key": judge_llm_api_key_error, - # "huggingface_api_key": huggingface_api_key_error, - } + error_labels = { + "evaluated_agent_url": agent_url_error, + "evaluated_agent_credentials": auth_credentials_error, + "judge_llm_api_key": judge_llm_api_key_error, + # "huggingface_api_key": huggingface_api_key_error, + }
♻️ Duplicate comments (2)
rogue/ui/components/config_screen.py (1)
46-59: Nice: guarded initial transport value against invalid config (resolves prior).The fallback to the protocol default when not in choices prevents Gradio mis-selections.
rogue/ui/app.py (1)
120-134: Safe enum parsing + validity check implemented (resolves prior).
🧹 Nitpick comments (8)
rogue/ui/components/config_screen.py (4)
60-66: Transport dropdown OK, but note state sync.Programmatic updates won’t trigger change events; see next comment for keeping shared_state in sync when protocol changes.
215-226: Make transport update robust and keep state in sync; preserve user’s selection if still valid.
- Add try/except around enum parsing. As per coding guidelines.
- Preserve current transport if valid for the new protocol; else fallback.
- Update shared_state["config"]["transport"] when protocol changes.
Apply:
- def update_transport_choices(protocol_val): - """Update transport choices based on selected protocol.""" - selected_protocol = Protocol(protocol_val) - transport_choices = [t.value for t in PROTOCOL_TO_TRANSPORTS[selected_protocol]] - default_transport = selected_protocol.get_default_transport().value - return gr.update(choices=transport_choices, value=default_transport) + def update_transport_choices(protocol_val: str, current_transport_val: str, state: dict): + """Update transport choices based on selected protocol.""" + try: + selected_protocol = Protocol(protocol_val) + except Exception: + selected_protocol = Protocol.A2A + transport_choices = [t.value for t in PROTOCOL_TO_TRANSPORTS[selected_protocol]] + preserved = current_transport_val if current_transport_val in transport_choices else selected_protocol.get_default_transport().value + # sync shared state (programmatic UI updates don't fire .change) + state.setdefault("config", {})["transport"] = preserved + return gr.update(choices=transport_choices, value=preserved), state @@ - protocol.change( - fn=update_transport_choices, - inputs=[protocol], - outputs=[transport], - ) + protocol.change( + fn=update_transport_choices, + inputs=[protocol, transport, shared_state], + outputs=[transport, shared_state], + )
249-296: Improve save_config robustness and error surfacing.
- Guard dump_config I/O. As per coding guidelines.
- Fix misleading log message.
[Suggest changes inline below.]try: config = AgentConfig( @@ config_dict = config.model_dump(mode="json") - dump_config(state, config) + try: + dump_config(state, config) + except Exception as io_err: + logger.exception("Failed to persist configuration") + return { + **label_updates, + general_error_label: gr.update( + value=f"**Failed to save config:** {io_err}", visible=True + ), + shared_state: state, + } state["config"] = config_dict @@ - except ValidationError as e: + except ValidationError as e: @@ - else: - logger.exception("Unhandled validation error") + else: + logger.exception("Validation error without a mapped label")
13-16: Add type hints to new/changed functions.Please add annotations for create_config_screen return type and inner functions (update_state, toggle_auth_credentials, update_transport_choices, save_config). Helps mypy/flake8.
Example:def create_config_screen(shared_state: "State", tabs_component: "Tabs") -> tuple[gr.Component, ...]: ...Also applies to: 215-221, 228-243
rogue/ui/app.py (4)
132-134: Normalize loaded config back into state to avoid divergence.If transport is corrected or defaulted, write normalized values into state["config"] so UI and state stay consistent.
Apply:
config = load_config(state) - state["config"] = config + state["config"] = config @@ if not transport_val.is_valid_for_protocol(protocol_val): transport_val = protocol_val.get_default_transport() + # persist normalized values into state/config for downstream consumers + state["config"]["protocol"] = protocol_val.value + state["config"]["transport"] = transport_val.valueAlso applies to: 114-115, 139-141
105-115: Guard load_config to handle read/parse failures gracefully.Avoid blank screen on I/O/JSON errors; return defaults and surface a log.
Apply:
- def load_and_update_ui(): + def load_and_update_ui(): state = { @@ - config = load_config(state) + try: + config = load_config(state) + except Exception as e: + # fall back to defaults; keep app usable + print(f"[load_config] Failed to load config: {e}") + config = {}As per coding guidelines.
120-131: Optional: log when invalid enum values are corrected.Helps troubleshoot user-edited configs.
Example:
if raw_protocol != protocol_val.value or raw_transport != transport_val.value: print(f"[config] Normalized protocol={protocol_val.value} transport={transport_val.value}")
105-115: Add simple return type hints for callbacks (optional).Annotate load_and_update_ui -> dict[str, Any] for mypy friendliness.
Also applies to: 159-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rogue/ui/app.py(4 hunks)rogue/ui/components/config_screen.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format Python code with Black
Ensure code passes flake8 linting
Run mypy with the repository configuration for static typing
Run Bandit security checks using .bandit.yaml configuration
Use isort import conventions for import ordering
Add type hints to all function signatures
Follow PEP 8 naming (snake_case for variables/functions, PascalCase for classes)
Use try/except around code that may raise exceptions
Files:
rogue/ui/app.pyrogue/ui/components/config_screen.py
🧬 Code graph analysis (2)
rogue/ui/app.py (1)
sdks/python/rogue_sdk/types.py (4)
Protocol(68-79)Transport(82-93)is_valid_for_protocol(92-93)get_default_transport(74-79)
rogue/ui/components/config_screen.py (1)
sdks/python/rogue_sdk/types.py (4)
AgentConfig(104-136)AuthType(23-48)Protocol(68-79)get_default_transport(74-79)
🔇 Additional comments (5)
rogue/ui/components/config_screen.py (2)
36-45: Protocol dropdown wiring looks good.Choices/initial value align with Protocol enum values.
300-307: Wiring protocol/transport through save and return tuple looks correct.Also applies to: 325-329, 258-260, 231-233
rogue/ui/app.py (3)
4-4: Import additions LGTM.
38-40: UI wiring includes protocol/transport correctly.Also applies to: 165-167
142-143: Align deep_test_mode defaults between app.py and config_screen.pyThe inconsistency is confirmed:
app.pyline 142 defaults toFalse, whileconfig_screen.pyline 83 defaults toTrue. Throughout the codebase (types.py, cli_input.py, run_cli.py, and evaluator agent modules), the consistent default isFalse. Update config_screen.py to match this standard default for consistency.
Description
Motivation and Context
Type of Change
Changes Made
Screenshots/Examples (if applicable)
Checklist
uv run black .to format my codeuv run flake8 .and fixed all issuesuv run mypy --config-file .mypy.ini .and addressed type checking issuesuv run bandit -c .bandit.yaml -r .for security checksuv run pytestand all tests passTesting
Test Configuration:
Test Steps:
1.
2.
3.
Additional Notes
Related Issues/PRs