From cc14f25e6944bf1370e6ebe51d21b84bbdaf6ebb Mon Sep 17 00:00:00 2001 From: Livio Meuli Date: Sun, 24 May 2026 10:38:59 +0200 Subject: [PATCH] refactor: add REVIEW annotations for dead/debug code across frontend and backend Co-Authored-By: Claude Sonnet 4.6 --- backend/agent/coding_agent.py | 14 +++++++++++++- backend/agent/mcp_server_adapter.py | 5 +++++ backend/agent/servers/mcp_server_code_execution.py | 6 ++++++ backend/agent/servers/mcp_server_file_search.py | 8 ++++++++ backend/managers/chat_manager.py | 5 +++++ backend/managers/debug_logger.py | 1 + backend/managers/file_manager.py | 5 ++++- backend/managers/system_prompter.py | 4 ++++ frontend/app.py | 2 ++ frontend/chat.py | 5 +++++ frontend/editor.py | 5 +++++ frontend/sidebar.py | 1 + frontend/state.py | 10 ++++++++++ 13 files changed, 69 insertions(+), 2 deletions(-) diff --git a/backend/agent/coding_agent.py b/backend/agent/coding_agent.py index ff98386..2fa8bf0 100644 --- a/backend/agent/coding_agent.py +++ b/backend/agent/coding_agent.py @@ -18,17 +18,22 @@ import os import re from pathlib import Path import asyncio +# REVIEW: pprint is only used for a single debug print in build_all_tool_description(); +# replace with a plain print() call and remove this import. import pprint import requests from dotenv import load_dotenv +# REVIEW: commented-out import — remove once the package import above is confirmed stable. #from mcp_server_adapter import MCPToolAdapter # Import from current directory for easier testing without package structure from backend.agent.mcp_server_adapter import MCPToolAdapter # ── mcp server initialization ──────────────────────────────────────────────────────────────── adapter = MCPToolAdapter() +# REVIEW: debug print — remove before shipping. print("MCPToolAdapter created. Listing all tools from servers...") asyncio.run(adapter.initialize_all_servers()) +# REVIEW: debug print — remove before shipping. print("listed tools from all servers") load_dotenv() @@ -59,10 +64,12 @@ def build_all_tool_description() -> str: ``"- : "``. """ all_tools = adapter.get_all_tools() + # REVIEW: debug print — remove before shipping. print(f"Building tool description for {len(all_tools)} tools.") descriptions = [] for tool in all_tools: + # REVIEW: debug print via pprint — remove before shipping; replace pprint import with plain print if kept. pprint.pprint(f"{tool}") descriptions.append(f"- {tool['tool_name']}: {tool['tool_description']}") @@ -90,9 +97,11 @@ async def dispatch_tool(tool_name: str, arguments: dict) -> str: return f"DONE: {summary}" try: + # REVIEW: debug print — remove before shipping. print(f"Trying to call tool '{tool_name}' in dispatch_tool through MCPToolAdapter...") result = await adapter.call_tool(tool_name, arguments) + # REVIEW: debug print — remove before shipping. print(f"Raw result from tool '{tool_name}': {result}") if result.isError: @@ -572,6 +581,8 @@ class CodingAgent: }) self.pending_action = None +# REVIEW: dead code — this module is always imported, never run as a script. +# The __main__ guard below is unreachable in normal use. Move this to run_agent.py or delete it. def main(): """Example of how to use the CodingAgent in a simple loop.""" agent = CodingAgent() @@ -598,7 +609,8 @@ def main(): feedback = input("Enter feedback for the agent: ") agent.reject(feedback) - + # REVIEW: unreachable when action["tool"] == "done" (we break above); also `result` is + # unbound when the elif branch runs — this will raise UnboundLocalError at runtime. if result["is_done"]: print("Task completed.") break diff --git a/backend/agent/mcp_server_adapter.py b/backend/agent/mcp_server_adapter.py index e3be379..21ab3d7 100644 --- a/backend/agent/mcp_server_adapter.py +++ b/backend/agent/mcp_server_adapter.py @@ -100,8 +100,10 @@ class MCPToolAdapter: await session.initialize() print(f"Session initialized for {server_name}. Requesting tools...") result = await session.list_tools() + # REVIEW: debug print — remove before shipping. print(f"Tools received from {server_name}: {result}") tools = result.tools + # REVIEW: duplicate print — identical message already printed inside the `async with` block above. print(f"Tools received from {server_name}: {result}") for tool in tools: @@ -195,6 +197,9 @@ class MCPToolAdapter: populated (connections are opened per-call). It is kept as a placeholder for a future persistent-connection implementation. """ + # REVIEW: self.exit_stack is never assigned in __init__ — calling this method will + # always raise AttributeError. Either remove this method or initialise exit_stack + # in __init__ as an empty dict. for server_name, (transport_gen, session) in self.exit_stack.items(): try: await session.__aexit__(None, None, None) diff --git a/backend/agent/servers/mcp_server_code_execution.py b/backend/agent/servers/mcp_server_code_execution.py index ff02b9e..262a6dd 100644 --- a/backend/agent/servers/mcp_server_code_execution.py +++ b/backend/agent/servers/mcp_server_code_execution.py @@ -15,6 +15,9 @@ blocks dangerous imports and builtins before spawning any subprocess. """ import ast +# REVIEW: dead code — datetime is imported but only used to generate run_id in +# run_python_code_sandboxed(). That is legitimate, but note the import is unused in all +# other tools; it would be cleaner as a local import inside run_python_code_sandboxed(). from datetime import datetime import subprocess import io @@ -440,6 +443,9 @@ def python_code_validation(code: str) -> str: return f"Valid Syntax, but with safety concerns: {static_analysis_result}; code execution is not allowed." except Exception as e: return f"Error during code safety analysis: {e}" + # REVIEW: unreachable code — python_code_validation() falls off the end of the function + # without an explicit `return` when static_analysis_result is None (safe code); the function + # implicitly returns None instead of returning a success message to the caller. # ── Run the server ─────────────────────────────────────────────────────────── diff --git a/backend/agent/servers/mcp_server_file_search.py b/backend/agent/servers/mcp_server_file_search.py index dbcbdb1..4ad5f5a 100644 --- a/backend/agent/servers/mcp_server_file_search.py +++ b/backend/agent/servers/mcp_server_file_search.py @@ -69,6 +69,8 @@ def get_file_tree(dir_path: str=ALLOWED_DIR) -> str: """ try: safe_dir = _safe_path(dir_path) + # REVIEW: unreachable code — _safe_path() always returns a Path object (never None/falsy) + # or raises ValueError; this check can never be True. if not safe_dir: return f"Error: Invalid directory path '{dir_path}'." elif not safe_dir.exists(): @@ -108,6 +110,10 @@ def get_file_tree(dir_path: str=ALLOWED_DIR) -> str: lines.append(_tree(entry, prefix + extension)) return "\n".join(lines) + # REVIEW: redundant — get_file_tree() passes `dir_path` (original str argument) to `_tree()` + # rather than the validated `safe_dir` (resolved Path). If dir_path is a relative string, + # the inner `_tree()` will call `dir_path.iterdir()` on a str, causing an AttributeError. + # Should pass `safe_dir` instead. return _tree(dir_path) @@ -235,6 +241,8 @@ def create_new_directory(path: str) -> str: if resolved.exists(): return f"Error: File '{path}' already exists." + # REVIEW: redundant — `resolved.suffix != None` is always True (Path.suffix always returns str); + # the None check is unnecessary. Simplify to `if resolved.suffix != "":`. if resolved.suffix != None and resolved.suffix != "": return f"Error: can only create directories, got '{resolved.suffix}'." diff --git a/backend/managers/chat_manager.py b/backend/managers/chat_manager.py index e39d911..a4363fe 100644 --- a/backend/managers/chat_manager.py +++ b/backend/managers/chat_manager.py @@ -37,6 +37,7 @@ class ChatManager: """Return a copy of the conversation history.""" return list(self.chat_history) + # REVIEW: dead code — clear_history() is never called anywhere in the codebase. def clear_history(self) -> None: """Wipe the conversation history (starts a fresh chat).""" self.chat_history = [] @@ -107,6 +108,10 @@ class ChatManager: self.add_message("assistant", f"Error: {error_msg}") raise Exception(error_msg) + # REVIEW: dead code — get_chat_display() is never called anywhere in the codebase. + # The UI renders st.session_state.chat_history directly. This method also does the + # same thing as get_history() (returns a copy of chat_history with the same fields), + # making it redundant even if it were used. def get_chat_display(self) -> list: """Return a copy of the history suitable for display in the UI.""" return [ diff --git a/backend/managers/debug_logger.py b/backend/managers/debug_logger.py index b681f05..946ccf3 100644 --- a/backend/managers/debug_logger.py +++ b/backend/managers/debug_logger.py @@ -35,6 +35,7 @@ class DebugLogger: """Reset the log — call before each new execution.""" self.logs = [] + # REVIEW: dead code — format_debug_output() is never called anywhere in the codebase. def format_debug_output(self, output: dict) -> str: """Format an ExecutionEngine result dict into a human-readable string. diff --git a/backend/managers/file_manager.py b/backend/managers/file_manager.py index 62360b0..58bc1cc 100644 --- a/backend/managers/file_manager.py +++ b/backend/managers/file_manager.py @@ -7,7 +7,8 @@ touching the filesystem, preventing path-traversal attacks. import streamlit as st from pathlib import Path -# The workspace folder is created at module load so it always exists. +# REVIEW: dead code — module-level WORKSPACE constant is never used anywhere in this file or +# the rest of the codebase. FileManager.__init__ creates the workspace via self.base_path.mkdir(). WORKSPACE = Path("workspace") WORKSPACE.mkdir(exist_ok=True) @@ -139,6 +140,8 @@ class FileManager: with open(file_path, "r") as f: return f.read() except FileNotFoundError: + # REVIEW: unreachable code — FileNotFoundError cannot be raised here because + # `file_path.exists()` is already checked above and returns "" on failure. st.error(f"File not found: {relative_path}") return "" except Exception as e: diff --git a/backend/managers/system_prompter.py b/backend/managers/system_prompter.py index d9610b7..b5e50d3 100644 --- a/backend/managers/system_prompter.py +++ b/backend/managers/system_prompter.py @@ -12,6 +12,10 @@ class SystemPrompter: """ @staticmethod + # REVIEW: unused parameter (in production) — `file_context` is never passed by the only + # production call site (frontend/chat.py line 242 calls generate_prompt() with no args), + # so the file-embedding branch (lines 31-46) is dead in production. It is tested in + # tests/test_system_prompter.py but the feature is not wired up in the UI. def generate_prompt(file_context: dict | None = None) -> str: """Build a system prompt, optionally embedding a file's content. diff --git a/frontend/app.py b/frontend/app.py index 140f7c9..c05ace7 100644 --- a/frontend/app.py +++ b/frontend/app.py @@ -44,6 +44,8 @@ def main(): st.title("Lightweight code editor") + # REVIEW: redundant — init_state() is already called at module level (line 26) before main() runs; + # calling it again here is unnecessary since Streamlit reruns the whole module on each reload. # Re-run init_state to cover any keys that might have been missed on cold start init_state() diff --git a/frontend/chat.py b/frontend/chat.py index af7e236..fd8207c 100644 --- a/frontend/chat.py +++ b/frontend/chat.py @@ -22,6 +22,8 @@ def _run_async(coro): Returns: The return value of the coroutine. """ + # REVIEW: asyncio.get_running_loop() always raises RuntimeError in a Streamlit context; + # the try branch is dead code. The except branch always runs. try: # Reuse the loop that is already running (e.g. inside pytest-asyncio). loop = asyncio.get_running_loop() @@ -144,6 +146,7 @@ def render_agent_mode(): placeholder="e.g. Write a function that sorts a list and saves it to sorted.py", ) if st.button("Start Agent", type="primary", use_container_width=True): + # REVIEW: commented-out code — remove if not needed. #loop = asyncio.new_event_loop() #asyncio.set_event_loop(loop) if task.strip(): @@ -353,6 +356,8 @@ def render_normal_chat(): if st.button("🗑️ Clear Chat"): _clear_chat_dialog() + # REVIEW: duplicate widget key — "agent_mode" toggle is already rendered inside render_agent_mode(); + # having two st.toggle calls with the same key on the same page will raise a DuplicateWidgetID error. st.toggle("Agent Mode", key="agent_mode") # 5h — Settings expander: file context toggle, model, token limit, custom prompt. diff --git a/frontend/editor.py b/frontend/editor.py index 19fe6b9..995ce1e 100644 --- a/frontend/editor.py +++ b/frontend/editor.py @@ -8,6 +8,8 @@ from pathlib import Path from backend.managers.file_manager import FileManager from backend.managers.execution_engine import ExecutionEngine +# REVIEW: DebugLogger is imported and used to log execution steps, but its output is never +# surfaced in the UI — DebugLogger writes to an in-memory buffer that nothing reads or renders. from backend.managers.debug_logger import DebugLogger # Maps file extensions to Ace editor language modes for syntax highlighting. @@ -194,6 +196,9 @@ def render_editor(): ) # Keep the in-memory cache in sync with what the editor currently shows. + # REVIEW: redundant round-trip — st_ace returns the same value that was passed as + # `value=` unless the user edited the content; comparing and re-assigning on every + # rerun is a no-op most of the time and adds overhead. if code != st.session_state.files_content[file_path]: st.session_state.files_content[file_path] = code diff --git a/frontend/sidebar.py b/frontend/sidebar.py index 4ee3db7..40f78c2 100644 --- a/frontend/sidebar.py +++ b/frontend/sidebar.py @@ -375,6 +375,7 @@ def render_sidebar(): if st.button("Add Folder", key="btn_add_folder", use_container_width=True): _add_folder_dialog("") + # REVIEW: bare `return` at end of void function — no-op; can be removed. return diff --git a/frontend/state.py b/frontend/state.py index d7e5f19..7e2fb9c 100644 --- a/frontend/state.py +++ b/frontend/state.py @@ -43,25 +43,33 @@ def init_state(): # Ordered list of absolute file paths currently open as editor tabs. # The list order determines the visual tab order in the UI. if "open_files" not in st.session_state: + # REVIEW: dead code — docstrings inside `if` blocks are plain string literals that Python + # evaluates and immediately discards; they are never visible as __doc__ and have no effect. st.session_state.open_files = [] # Dict mapping absolute file path → current editor content (may differ from # disk if the user has unsaved changes). if "files_content" not in st.session_state: + # REVIEW: dead code — same issue: string literal inside `if` block is never used as a docstring. st.session_state.files_content = {} # Absolute path of the file whose tab is currently active in the editor. # Must always be one of the paths in open_files, or None if no file is open. if "active_file" not in st.session_state: + # REVIEW: dead code — string literal inside `if` block is never used as a docstring. st.session_state.active_file = None + # REVIEW: dead code — active_tab is initialised here but never read or written anywhere else + # in the codebase; st.tabs() in editor.py does not use this key. # Index of the active tab — kept in sync with active_file for st.tabs(). if "active_tab" not in st.session_state: st.session_state.active_tab = 0 + # REVIEW: dead code — is_editing is initialised here but never read or written anywhere else. if "is_editing" not in st.session_state: st.session_state.is_editing = False + # REVIEW: dead code — code_suggestions is initialised here but never read or written anywhere else. if "code_suggestions" not in st.session_state: st.session_state.code_suggestions = [] @@ -120,5 +128,7 @@ def init_state(): st.session_state.exec_results = {} +# REVIEW: dead code — state.py is never run as a script; this guard is useless here because +# init_state() requires a running Streamlit session (st.session_state) to work. if __name__ == "__main__": init_state()