refactor: add REVIEW annotations for dead/debug code across frontend and backend
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
0751be8e61
commit
cc14f25e69
@ -18,17 +18,22 @@ import os
|
|||||||
import re
|
import re
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
import asyncio
|
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 pprint
|
||||||
|
|
||||||
import requests
|
import requests
|
||||||
from dotenv import load_dotenv
|
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 mcp_server_adapter import MCPToolAdapter # Import from current directory for easier testing without package structure
|
||||||
from backend.agent.mcp_server_adapter import MCPToolAdapter
|
from backend.agent.mcp_server_adapter import MCPToolAdapter
|
||||||
|
|
||||||
# ── mcp server initialization ────────────────────────────────────────────────────────────────
|
# ── mcp server initialization ────────────────────────────────────────────────────────────────
|
||||||
adapter = MCPToolAdapter()
|
adapter = MCPToolAdapter()
|
||||||
|
# REVIEW: debug print — remove before shipping.
|
||||||
print("MCPToolAdapter created. Listing all tools from servers...")
|
print("MCPToolAdapter created. Listing all tools from servers...")
|
||||||
asyncio.run(adapter.initialize_all_servers())
|
asyncio.run(adapter.initialize_all_servers())
|
||||||
|
# REVIEW: debug print — remove before shipping.
|
||||||
print("listed tools from all servers")
|
print("listed tools from all servers")
|
||||||
|
|
||||||
load_dotenv()
|
load_dotenv()
|
||||||
@ -59,10 +64,12 @@ def build_all_tool_description() -> str:
|
|||||||
``"- <tool_name>: <description>"``.
|
``"- <tool_name>: <description>"``.
|
||||||
"""
|
"""
|
||||||
all_tools = adapter.get_all_tools()
|
all_tools = adapter.get_all_tools()
|
||||||
|
# REVIEW: debug print — remove before shipping.
|
||||||
print(f"Building tool description for {len(all_tools)} tools.")
|
print(f"Building tool description for {len(all_tools)} tools.")
|
||||||
|
|
||||||
descriptions = []
|
descriptions = []
|
||||||
for tool in all_tools:
|
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}")
|
pprint.pprint(f"{tool}")
|
||||||
descriptions.append(f"- {tool['tool_name']}: {tool['tool_description']}")
|
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}"
|
return f"DONE: {summary}"
|
||||||
|
|
||||||
try:
|
try:
|
||||||
|
# REVIEW: debug print — remove before shipping.
|
||||||
print(f"Trying to call tool '{tool_name}' in dispatch_tool through MCPToolAdapter...")
|
print(f"Trying to call tool '{tool_name}' in dispatch_tool through MCPToolAdapter...")
|
||||||
result = await adapter.call_tool(tool_name, arguments)
|
result = await adapter.call_tool(tool_name, arguments)
|
||||||
|
|
||||||
|
# REVIEW: debug print — remove before shipping.
|
||||||
print(f"Raw result from tool '{tool_name}': {result}")
|
print(f"Raw result from tool '{tool_name}': {result}")
|
||||||
|
|
||||||
if result.isError:
|
if result.isError:
|
||||||
@ -572,6 +581,8 @@ class CodingAgent:
|
|||||||
})
|
})
|
||||||
self.pending_action = None
|
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():
|
def main():
|
||||||
"""Example of how to use the CodingAgent in a simple loop."""
|
"""Example of how to use the CodingAgent in a simple loop."""
|
||||||
agent = CodingAgent()
|
agent = CodingAgent()
|
||||||
@ -598,7 +609,8 @@ def main():
|
|||||||
feedback = input("Enter feedback for the agent: ")
|
feedback = input("Enter feedback for the agent: ")
|
||||||
agent.reject(feedback)
|
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"]:
|
if result["is_done"]:
|
||||||
print("Task completed.")
|
print("Task completed.")
|
||||||
break
|
break
|
||||||
|
|||||||
@ -100,8 +100,10 @@ class MCPToolAdapter:
|
|||||||
await session.initialize()
|
await session.initialize()
|
||||||
print(f"Session initialized for {server_name}. Requesting tools...")
|
print(f"Session initialized for {server_name}. Requesting tools...")
|
||||||
result = await session.list_tools()
|
result = await session.list_tools()
|
||||||
|
# REVIEW: debug print — remove before shipping.
|
||||||
print(f"Tools received from {server_name}: {result}")
|
print(f"Tools received from {server_name}: {result}")
|
||||||
tools = result.tools
|
tools = result.tools
|
||||||
|
# REVIEW: duplicate print — identical message already printed inside the `async with` block above.
|
||||||
print(f"Tools received from {server_name}: {result}")
|
print(f"Tools received from {server_name}: {result}")
|
||||||
|
|
||||||
for tool in tools:
|
for tool in tools:
|
||||||
@ -195,6 +197,9 @@ class MCPToolAdapter:
|
|||||||
populated (connections are opened per-call). It is kept as a placeholder
|
populated (connections are opened per-call). It is kept as a placeholder
|
||||||
for a future persistent-connection implementation.
|
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():
|
for server_name, (transport_gen, session) in self.exit_stack.items():
|
||||||
try:
|
try:
|
||||||
await session.__aexit__(None, None, None)
|
await session.__aexit__(None, None, None)
|
||||||
|
|||||||
@ -15,6 +15,9 @@ blocks dangerous imports and builtins before spawning any subprocess.
|
|||||||
"""
|
"""
|
||||||
|
|
||||||
import ast
|
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
|
from datetime import datetime
|
||||||
import subprocess
|
import subprocess
|
||||||
import io
|
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."
|
return f"Valid Syntax, but with safety concerns: {static_analysis_result}; code execution is not allowed."
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
return f"Error during code safety analysis: {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 ───────────────────────────────────────────────────────────
|
# ── Run the server ───────────────────────────────────────────────────────────
|
||||||
|
|||||||
@ -69,6 +69,8 @@ def get_file_tree(dir_path: str=ALLOWED_DIR) -> str:
|
|||||||
"""
|
"""
|
||||||
try:
|
try:
|
||||||
safe_dir = _safe_path(dir_path)
|
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:
|
if not safe_dir:
|
||||||
return f"Error: Invalid directory path '{dir_path}'."
|
return f"Error: Invalid directory path '{dir_path}'."
|
||||||
elif not safe_dir.exists():
|
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))
|
lines.append(_tree(entry, prefix + extension))
|
||||||
return "\n".join(lines)
|
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)
|
return _tree(dir_path)
|
||||||
|
|
||||||
|
|
||||||
@ -235,6 +241,8 @@ def create_new_directory(path: str) -> str:
|
|||||||
if resolved.exists():
|
if resolved.exists():
|
||||||
return f"Error: File '{path}' already 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 != "":
|
if resolved.suffix != None and resolved.suffix != "":
|
||||||
return f"Error: can only create directories, got '{resolved.suffix}'."
|
return f"Error: can only create directories, got '{resolved.suffix}'."
|
||||||
|
|
||||||
|
|||||||
@ -37,6 +37,7 @@ class ChatManager:
|
|||||||
"""Return a copy of the conversation history."""
|
"""Return a copy of the conversation history."""
|
||||||
return list(self.chat_history)
|
return list(self.chat_history)
|
||||||
|
|
||||||
|
# REVIEW: dead code — clear_history() is never called anywhere in the codebase.
|
||||||
def clear_history(self) -> None:
|
def clear_history(self) -> None:
|
||||||
"""Wipe the conversation history (starts a fresh chat)."""
|
"""Wipe the conversation history (starts a fresh chat)."""
|
||||||
self.chat_history = []
|
self.chat_history = []
|
||||||
@ -107,6 +108,10 @@ class ChatManager:
|
|||||||
self.add_message("assistant", f"Error: {error_msg}")
|
self.add_message("assistant", f"Error: {error_msg}")
|
||||||
raise Exception(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:
|
def get_chat_display(self) -> list:
|
||||||
"""Return a copy of the history suitable for display in the UI."""
|
"""Return a copy of the history suitable for display in the UI."""
|
||||||
return [
|
return [
|
||||||
|
|||||||
@ -35,6 +35,7 @@ class DebugLogger:
|
|||||||
"""Reset the log — call before each new execution."""
|
"""Reset the log — call before each new execution."""
|
||||||
self.logs = []
|
self.logs = []
|
||||||
|
|
||||||
|
# REVIEW: dead code — format_debug_output() is never called anywhere in the codebase.
|
||||||
def format_debug_output(self, output: dict) -> str:
|
def format_debug_output(self, output: dict) -> str:
|
||||||
"""Format an ExecutionEngine result dict into a human-readable string.
|
"""Format an ExecutionEngine result dict into a human-readable string.
|
||||||
|
|
||||||
|
|||||||
@ -7,7 +7,8 @@ touching the filesystem, preventing path-traversal attacks.
|
|||||||
import streamlit as st
|
import streamlit as st
|
||||||
from pathlib import Path
|
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 = Path("workspace")
|
||||||
WORKSPACE.mkdir(exist_ok=True)
|
WORKSPACE.mkdir(exist_ok=True)
|
||||||
|
|
||||||
@ -139,6 +140,8 @@ class FileManager:
|
|||||||
with open(file_path, "r") as f:
|
with open(file_path, "r") as f:
|
||||||
return f.read()
|
return f.read()
|
||||||
except FileNotFoundError:
|
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}")
|
st.error(f"File not found: {relative_path}")
|
||||||
return ""
|
return ""
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
|
|||||||
@ -12,6 +12,10 @@ class SystemPrompter:
|
|||||||
"""
|
"""
|
||||||
|
|
||||||
@staticmethod
|
@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:
|
def generate_prompt(file_context: dict | None = None) -> str:
|
||||||
"""Build a system prompt, optionally embedding a file's content.
|
"""Build a system prompt, optionally embedding a file's content.
|
||||||
|
|
||||||
|
|||||||
@ -44,6 +44,8 @@ def main():
|
|||||||
|
|
||||||
st.title("Lightweight code editor")
|
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
|
# Re-run init_state to cover any keys that might have been missed on cold start
|
||||||
init_state()
|
init_state()
|
||||||
|
|
||||||
|
|||||||
@ -22,6 +22,8 @@ def _run_async(coro):
|
|||||||
Returns:
|
Returns:
|
||||||
The return value of the coroutine.
|
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:
|
try:
|
||||||
# Reuse the loop that is already running (e.g. inside pytest-asyncio).
|
# Reuse the loop that is already running (e.g. inside pytest-asyncio).
|
||||||
loop = asyncio.get_running_loop()
|
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",
|
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):
|
if st.button("Start Agent", type="primary", use_container_width=True):
|
||||||
|
# REVIEW: commented-out code — remove if not needed.
|
||||||
#loop = asyncio.new_event_loop()
|
#loop = asyncio.new_event_loop()
|
||||||
#asyncio.set_event_loop(loop)
|
#asyncio.set_event_loop(loop)
|
||||||
if task.strip():
|
if task.strip():
|
||||||
@ -353,6 +356,8 @@ def render_normal_chat():
|
|||||||
if st.button("🗑️ Clear Chat"):
|
if st.button("🗑️ Clear Chat"):
|
||||||
_clear_chat_dialog()
|
_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")
|
st.toggle("Agent Mode", key="agent_mode")
|
||||||
|
|
||||||
# 5h — Settings expander: file context toggle, model, token limit, custom prompt.
|
# 5h — Settings expander: file context toggle, model, token limit, custom prompt.
|
||||||
|
|||||||
@ -8,6 +8,8 @@ from pathlib import Path
|
|||||||
|
|
||||||
from backend.managers.file_manager import FileManager
|
from backend.managers.file_manager import FileManager
|
||||||
from backend.managers.execution_engine import ExecutionEngine
|
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
|
from backend.managers.debug_logger import DebugLogger
|
||||||
|
|
||||||
# Maps file extensions to Ace editor language modes for syntax highlighting.
|
# 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.
|
# 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]:
|
if code != st.session_state.files_content[file_path]:
|
||||||
st.session_state.files_content[file_path] = code
|
st.session_state.files_content[file_path] = code
|
||||||
|
|
||||||
|
|||||||
@ -375,6 +375,7 @@ def render_sidebar():
|
|||||||
if st.button("Add Folder", key="btn_add_folder", use_container_width=True):
|
if st.button("Add Folder", key="btn_add_folder", use_container_width=True):
|
||||||
_add_folder_dialog("")
|
_add_folder_dialog("")
|
||||||
|
|
||||||
|
# REVIEW: bare `return` at end of void function — no-op; can be removed.
|
||||||
return
|
return
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@ -43,25 +43,33 @@ def init_state():
|
|||||||
# Ordered list of absolute file paths currently open as editor tabs.
|
# Ordered list of absolute file paths currently open as editor tabs.
|
||||||
# The list order determines the visual tab order in the UI.
|
# The list order determines the visual tab order in the UI.
|
||||||
if "open_files" not in st.session_state:
|
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 = []
|
st.session_state.open_files = []
|
||||||
|
|
||||||
# Dict mapping absolute file path → current editor content (may differ from
|
# Dict mapping absolute file path → current editor content (may differ from
|
||||||
# disk if the user has unsaved changes).
|
# disk if the user has unsaved changes).
|
||||||
if "files_content" not in st.session_state:
|
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 = {}
|
st.session_state.files_content = {}
|
||||||
|
|
||||||
# Absolute path of the file whose tab is currently active in the editor.
|
# 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.
|
# 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:
|
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
|
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().
|
# Index of the active tab — kept in sync with active_file for st.tabs().
|
||||||
if "active_tab" not in st.session_state:
|
if "active_tab" not in st.session_state:
|
||||||
st.session_state.active_tab = 0
|
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:
|
if "is_editing" not in st.session_state:
|
||||||
st.session_state.is_editing = False
|
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:
|
if "code_suggestions" not in st.session_state:
|
||||||
st.session_state.code_suggestions = []
|
st.session_state.code_suggestions = []
|
||||||
|
|
||||||
@ -120,5 +128,7 @@ def init_state():
|
|||||||
st.session_state.exec_results = {}
|
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__":
|
if __name__ == "__main__":
|
||||||
init_state()
|
init_state()
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user