--- name: gza-code-review-full description: Comprehensive pre-release code review assessing test coverage, code duplication, and component interactions allowed-tools: Read, Glob, Grep, Bash(uv run pytest:*), Bash(uv run python:*), Bash(uv run mypy:*), Bash(ls:*), Bash(wc:*) version: 1.0.0 public: false ---

# Full Codebase Code Review

Perform a comprehensive code review of the gza codebase, suitable for pre-release assessment. This review covers: 1. Unit test coverage 2. Functional test coverage 3. Code duplication 4. Component interaction patterns 5. Error handling consistency 6. API/interface consistency 7. Configuration and hardcoding audit 8. Logging and observability 9. Resource management 10. Type safety

## When to Use

- Before a release to assess codebase health - When you want a comprehensive quality check - To identify areas needing more tests or refactoring

## Output

Write findings to `reviews/<timestamp>-code-review-full-<model>.md` in the project root, where `<timestamp>` is the current date/time in `YYYYmmddHHMMSS` format and `<model>` is a short identifier for the model performing the review (e.g., `reviews/20260305114139-code-review-full-opus-4-6.md`). Use your own model name/ID to derive the short identifier.

## Process

### Step 1: Inventory the codebase

Map out the source modules and test files:

1. **List all source modules:** ```bash ls -la src/gza/*.py ls -la src/gza/providers/*.py ```

2. **List all test files:** ```bash ls -la tests/*.py ls -la tests_integration/*.py 2>/dev/null || echo "No integration tests dir" ```

3. **Create a mapping** of source file → test file(s): - `db.py` → `test_db.py` - `cli.py` → `test_cli.py` - etc.

4. **Identify untested modules** - source files with no corresponding test file

### Step 2: Assess unit test coverage

For each source module:

1. **Read the source file** to understand its public interface (functions, classes, methods)

2. **Read the corresponding test file** (if exists)

3. **Check coverage by listing:** - Functions/methods that ARE tested - Functions/methods that are NOT tested - Edge cases that aren't covered (error paths, boundary conditions)

4. **Run the tests** to verify they pass: ```bash uv run pytest tests/ -v --tb=short ```

Focus especially on: - **`db.py`** - Core task storage, critical for correctness - **`cli.py`** - User-facing commands, all subcommands should have tests - **`runner.py`** - Task execution logic - **`git.py`** - Git operations (mocked tests preferred) - **`github.py`** - GitHub integration

### Step 3: Assess functional test coverage

Functional tests verify end-to-end workflows. Check for:

1. **Core workflows that should have integration tests:** - Creating a task → running it → verifying completion - Task dependencies (task B waits for task A) - PR creation workflow - Review workflow - Improve workflow

2. **Read `tests_integration/`** (if exists) to see what's covered

3. **Identify missing functional tests** - workflows documented in AGENTS.md that aren't tested

### Step 4: Analyze code duplication

Look for patterns of duplicated code:

1. **Search for similar code blocks:** - Similar function signatures doing similar things - Copy-pasted error handling - Repeated patterns that could be extracted

2. **Check specific areas prone to duplication:** - CLI command handlers (do they share common patterns that could be unified?) - Database queries (repeated query patterns) - Git operations (similar git command sequences)

3. **Use grep to find suspicious patterns:** ```bash # Find similar function definitions grep -n "def.*task" src/gza/*.py

# Find repeated patterns grep -n "subprocess.run" src/gza/*.py grep -n "click.echo" src/gza/cli.py ```

4. **Read AGENTS.md** section on "Single code path principle" and verify it's followed

### Step 5: Check error handling consistency

Review how errors are handled across the codebase:

1. **Identify error handling patterns:** ```bash # Find exception raising grep -n "raise " src/gza/*.py

# Find try/except blocks grep -n "except " src/gza/*.py

# Find custom exceptions grep -rn "class.*Exception" src/gza/ grep -rn "class.*Error" src/gza/ ```

2. **Check for consistency:** - Are errors handled uniformly? (always raise vs sometimes return None) - Are custom exceptions used where appropriate vs generic `Exception`? - Do error messages provide actionable information? - Are exceptions caught too broadly? (`except Exception` vs specific types)

3. **Look for problematic patterns:** - Silent failures (bare `except:` or `except: pass`) - Swallowed exceptions without logging - Inconsistent error return values (None vs empty list vs raise) - Missing error handling on I/O operations

4. **Document findings:** - List any inconsistencies in error handling approach - Note functions that should raise but return None (or vice versa) - Identify error messages that aren't helpful for debugging

### Step 6: Check API/interface consistency

Review function signatures and naming conventions:

1. **Check naming consistency:** ```bash # Find all public function definitions grep -n "^def " src/gza/*.py grep -n " def " src/gza/*.py | grep -v "__" ```

2. **Look for inconsistencies:** - Similar operations with different names (`get_task` vs `fetch_task` vs `retrieve_task`) - Parameter ordering inconsistencies (does `db` come first or last?) - Return type inconsistencies (objects vs dicts vs tuples)

3. **Check function signatures:** - Do similar functions have similar signatures? - Are there functions with too many parameters (>5)? - Are boolean parameters used where enums would be clearer?

4. **Review public interfaces:** - Are module `__all__` exports defined? - Is it clear what's public vs private? (underscore prefix convention) - Are there functions that should be private but aren't?

### Step 7: Audit configuration and hardcoding

Look for magic values that should be configurable:

1. **Find hardcoded values:** ```bash # Find numeric literals (potential magic numbers) grep -En "[^a-zA-Z_][0-9]{2,}[^0-9]" src/gza/*.py

# Find string literals that might be paths or config grep -n '"/.*"' src/gza/*.py grep -n "'/.*'" src/gza/*.py ```

2. **Check for:** - Magic numbers (timeouts, retry counts, limits) - Hardcoded file paths - Hardcoded URLs or endpoints - Default values that should be configurable

3. **Review path handling:** - Are paths constructed safely using `pathlib`? - Are there string concatenations for paths? (`dir + "/" + file`) - Are relative vs absolute paths handled correctly?

4. **Check configuration loading:** - Is `config.py` the single source for configuration? - Are there config values scattered in other modules? - Are defaults documented?

### Step 8: Review logging and observability

Assess the ability to debug and monitor the system:

1. **Check logging usage:** ```bash # Find logging calls grep -n "logging\." src/gza/*.py grep -n "logger\." src/gza/*.py grep -n "log\." src/gza/*.py

# Find print statements (should these be logs?) grep -n "print(" src/gza/*.py ```

2. **Assess logging quality:** - Is there consistent logging for key operations? - Can you trace a task's execution through the logs? - Are log levels used appropriately? (debug vs info vs warning vs error) - Are there operations that fail silently without logging?

3. **Check for sensitive data exposure:** ```bash # Look for potential credential logging grep -in "api.key\|token\|password\|secret\|credential" src/gza/*.py ``` - Are API keys, tokens, or passwords properly excluded from logs? - Are there any `repr()` or `str()` methods that might expose secrets?

4. **Review error logging:** - Are exceptions logged with stack traces where needed? - Are error messages actionable? - Is there enough context to debug issues?

### Step 9: Check resource management

Look for resource leaks and cleanup issues:

1. **Check file handling:** ```bash # Find file operations grep -n "open(" src/gza/*.py grep -n "with open" src/gza/*.py ``` - Are all file opens using context managers (`with`)? - Are there any `open()` calls without corresponding `close()`?

2. **Check database connections:** ```bash grep -n "connect(" src/gza/*.py grep -n "cursor" src/gza/*.py ``` - Are database connections properly closed? - Are cursors managed with context managers? - Is there connection pooling or is it connect-per-operation?

3. **Check subprocess management:** ```bash grep -n "subprocess" src/gza/*.py grep -n "Popen" src/gza/*.py ``` - Are subprocesses properly waited on? - Are there potential zombie processes? - Are stdin/stdout/stderr handles closed?

4. **Check for memory issues:** - Are there unbounded caches or growing lists? - Are large objects cleaned up after use? - Are there circular references that prevent garbage collection?

5. **Check temp file cleanup:** ```bash grep -n "tempfile\|mktemp\|NamedTemporaryFile" src/gza/*.py ``` - Are temp files cleaned up after use? - Are temp directories removed?

### Step 10: Assess type safety

Review type hints and type correctness:

1. **Check type hint coverage:** ```bash # Find functions without return type hints grep -n "def.*):$" src/gza/*.py

# Find functions with type hints grep -n "def.*) ->" src/gza/*.py ```

2. **Run mypy (if configured):** ```bash uv run mypy src/gza/ --ignore-missing-imports 2>&1 | head -100 ```

3. **Look for type safety issues:** - Functions with `Any` types that could be more specific - `Optional` types without proper `None` checks - Type: ignore comments (are they justified?) - Inconsistent types (function returns `str | None` but callers don't check)

4. **Check for common type issues:** ```bash # Find potential None issues grep -n "\.get(" src/gza/*.py # dict.get returns Optional grep -n "or None" src/gza/*.py grep -n "if.*is None" src/gza/*.py ```

### Step 11: Analyze component interaction patterns

Understand how modules interact and assess the clarity of these interactions:

1. **Map the import graph:** ```bash grep -h "^from gza" src/gza/*.py | sort | uniq -c | sort -rn grep -h "^import gza" src/gza/*.py | sort | uniq -c | sort -rn ```

2. **Identify the layering:** - Which modules are "lower level" (few dependencies)? - Which are "higher level" (many dependencies)? - Are there circular dependencies?

3. **Check separation of concerns:** - Does `cli.py` only handle CLI concerns, delegating to other modules? - Does `db.py` only handle database concerns? - Does `runner.py` only handle execution concerns?

4. **Look for unclear interfaces:** - Functions with too many parameters - Functions that do too many things - Tight coupling between modules that should be loosely coupled

5. **Document the interaction patterns:** ``` cli.py → db.py (task CRUD) cli.py → runner.py (task execution) runner.py → providers/* (AI execution) runner.py → git.py (git operations) etc. ```

### Step 12: Compile the review report

Create a structured report at `reviews/code-review-full.md`:

```markdown # Gza Code Review - Pre-Release Assessment

Date: YYYY-MM-DD Reviewer: Claude

## Executive Summary

[2-3 sentence overview of codebase health]

## Test Coverage

### Unit Tests

| Module | Test File | Coverage Assessment | |--------|-----------|---------------------| | db.py | test_db.py | Good - covers CRUD, queries | | cli.py | test_cli.py | Partial - missing `gza pr` tests | | ... | ... | ... |

#### Well-Tested Areas - [List modules/features with good coverage]

#### Under-Tested Areas - [List modules/featur