HatmanStack commited on
Commit
924784f
·
1 Parent(s): 673b434

fix: address post-audit review findings across docs and source

Browse files

Docs:
- Add pre-remediation snapshot headers to doc-audit.md and eval.md
- Annotate remediated items (PlayerStats, GameState, SQL injection,
coverage threshold, pre-commit hooks) with current status
- Fix markdown checkbox spacing in Phase-2.md
- Add language identifiers to fenced code blocks in Phase-0.md, Phase-4.md
- Fix test class reference in Phase-3.md pytest command
- Vary verbs in Phase-1.md verification steps

Source:
- Update stale "database" error text in pages/2_play_game.py
- Handle empty-after-strip in validation, tighten regex to literal space
- Remove unnecessary FileNotFoundError from connection.py except block
- Fix compile_model.py type hints (str | float for mixed rows)
- Add get_home_team_df to src/state/__init__.py exports
- Clarify DifficultySettings.from_preset fallback with comment

Tests:
- Fix literal backslash-n to real newline in test_validation.py
- Remove unused fixture param from test_combined_contains_both_teams
- Fix test_state.py re-import pattern, add None/wrong-length edge cases

docs/plans/2026-03-25-audit-streamlit-nba/Phase-0.md CHANGED
@@ -48,7 +48,7 @@ The app reads from a local CSV via pandas. There is no SQL database. The SQL inj
48
 
49
  Use conventional commits:
50
 
51
- ```
52
  type(scope): brief description
53
  ```
54
 
 
48
 
49
  Use conventional commits:
50
 
51
+ ```text
52
  type(scope): brief description
53
  ```
54
 
docs/plans/2026-03-25-audit-streamlit-nba/Phase-1.md CHANGED
@@ -247,8 +247,8 @@ refactor(pages): extract shared configure_page, remove finally:pass
247
 
248
  After completing all tasks in this phase:
249
 
250
- 1. Run `pytest` and confirm all tests pass.
251
- 2. Run `ruff check src/ tests/` and confirm no lint errors.
252
- 3. Run `mypy src/` and confirm no type errors.
253
- 4. Run `git diff --stat` and confirm only expected files changed.
254
  5. Verify no dead code identified in the health audit remains.
 
247
 
248
  After completing all tasks in this phase:
249
 
250
+ 1. Execute `pytest` and confirm all tests pass.
251
+ 2. Lint with `ruff check src/ tests/` and confirm no errors.
252
+ 3. Type-check with `mypy src/` and confirm no errors.
253
+ 4. Verify with `git diff --stat` that only expected files changed.
254
  5. Verify no dead code identified in the health audit remains.
docs/plans/2026-03-25-audit-streamlit-nba/Phase-2.md CHANGED
@@ -42,11 +42,11 @@ Fix structural and architectural issues: decouple Streamlit caching from busines
42
  - Replace `get_connection()` context manager usage with direct calls to `_load_nba_data()`.
43
 
44
  **Verification Checklist:**
45
- - [x]`src/database/connection.py` does not import `streamlit`
46
- - [x]`python -c "from src.database.connection import load_data"` succeeds without Streamlit installed (or mocked)
47
- - [x]Pages still load data correctly (manual test with `streamlit run app.py` if possible)
48
- - [x]`pytest` passes
49
- - [x]`mypy src/` passes
50
 
51
  **Testing Instructions:**
52
  - Update `tests/test_database.py` to remove any Streamlit mocking for `connection.py`.
@@ -84,10 +84,10 @@ refactor(database): decouple Streamlit caching from connection module
84
  - Replace direct calls to `get_winner_model()` with `_get_model()`.
85
 
86
  **Verification Checklist:**
87
- - [x]`src/ml/model.py` does not import `streamlit`
88
- - [x]`python -c "from src.ml.model import get_winner_model"` succeeds without Streamlit
89
- - [x]`pytest` passes
90
- - [x]`mypy src/` passes
91
 
92
  **Testing Instructions:**
93
  - Update `tests/test_ml.py` to remove any Streamlit mocking that was needed due to the `st.cache_resource` import.
@@ -118,10 +118,10 @@ refactor(ml): decouple Streamlit caching from model module
118
  - Review callers in pages to ensure they catch the custom exceptions, not bare `Exception`.
119
 
120
  **Verification Checklist:**
121
- - [x]No `except Exception` in `connection.py`
122
- - [x]All exception catches use specific types
123
- - [x]`pytest` passes
124
- - [x]`mypy src/` passes
125
 
126
  **Testing Instructions:**
127
  - Existing tests should cover this. Add a test that verifies a `FileNotFoundError` is raised as `DatabaseConnectionError`.
@@ -156,9 +156,9 @@ fix(database): narrow exception catches to specific types
156
  - Apply this change consistently across all files.
157
 
158
  **Verification Checklist:**
159
- - [x]No f-strings in any `logger.*()` calls
160
- - [x]`ruff check src/ tests/` passes
161
- - [x]`pytest` passes
162
 
163
  **Testing Instructions:** No new tests needed. This is a mechanical replacement.
164
 
@@ -187,10 +187,10 @@ fix(logging): replace f-string logging with lazy %s formatting
187
  - Import `TEAM_SIZE` and `STAT_COLUMNS` (or their lengths) from `src/config.py`.
188
 
189
  **Verification Checklist:**
190
- - [x]`analyze_team_stats` raises `ValueError` if player count != 5
191
- - [x]`analyze_team_stats` raises `ValueError` if any player has wrong stat count
192
- - [x]Existing tests pass
193
- - [x]New tests cover the validation
194
 
195
  **Testing Instructions:**
196
  - Add tests in `tests/test_ml.py`:
@@ -220,9 +220,9 @@ fix(ml): add input shape validation before model prediction
220
  - Remove the redundant check from `from_preset()` since the Pydantic validator will catch it during construction. Let `from_preset()` simply construct the instance and trust Pydantic validation.
221
 
222
  **Verification Checklist:**
223
- - [x]Only one validation path for preset names
224
- - [x]`pytest tests/test_models.py` passes
225
- - [x]Invalid preset names still raise appropriate errors
226
 
227
  **Testing Instructions:** Existing `DifficultySettings` tests should cover this. Verify they pass.
228
 
@@ -249,9 +249,9 @@ fix(models): remove duplicate validation in DifficultySettings
249
  - Fix type hints: change `list[list]` to `list[list[float]]` or more precise types.
250
 
251
  **Verification Checklist:**
252
- - [x]No `del` operations on input data in `create_stats`
253
- - [x]`ruff check scripts/` passes
254
- - [x]Function produces same output (manual verification or add a simple test)
255
 
256
  **Testing Instructions:** This is a training script, not part of the test suite. Manual verification that the output is unchanged, or add a simple test comparing old behavior vs new.
257
 
@@ -280,10 +280,10 @@ fix(scripts): replace destructive del with slicing in create_stats
280
  - Alternatively, call `setup_logging()` in each entry point (`app.py`, page files) right after `configure_page()`.
281
 
282
  **Verification Checklist:**
283
- - [x]`setup_logging()` is NOT called at module level in `config.py`
284
- - [x]`setup_logging()` IS called in each entry point
285
- - [x]`pytest` passes
286
- - [x]Logging still works when running the app
287
 
288
  **Testing Instructions:** Run existing tests. The module-level call removal should not break tests since tests typically configure their own logging.
289
 
 
42
  - Replace `get_connection()` context manager usage with direct calls to `_load_nba_data()`.
43
 
44
  **Verification Checklist:**
45
+ - [x] `src/database/connection.py` does not import `streamlit`
46
+ - [x] `python -c "from src.database.connection import load_data"` succeeds without Streamlit installed (or mocked)
47
+ - [x] Pages still load data correctly (manual test with `streamlit run app.py` if possible)
48
+ - [x] `pytest` passes
49
+ - [x] `mypy src/` passes
50
 
51
  **Testing Instructions:**
52
  - Update `tests/test_database.py` to remove any Streamlit mocking for `connection.py`.
 
84
  - Replace direct calls to `get_winner_model()` with `_get_model()`.
85
 
86
  **Verification Checklist:**
87
+ - [x] `src/ml/model.py` does not import `streamlit`
88
+ - [x] `python -c "from src.ml.model import get_winner_model"` succeeds without Streamlit
89
+ - [x] `pytest` passes
90
+ - [x] `mypy src/` passes
91
 
92
  **Testing Instructions:**
93
  - Update `tests/test_ml.py` to remove any Streamlit mocking that was needed due to the `st.cache_resource` import.
 
118
  - Review callers in pages to ensure they catch the custom exceptions, not bare `Exception`.
119
 
120
  **Verification Checklist:**
121
+ - [x] No `except Exception` in `connection.py`
122
+ - [x] All exception catches use specific types
123
+ - [x] `pytest` passes
124
+ - [x] `mypy src/` passes
125
 
126
  **Testing Instructions:**
127
  - Existing tests should cover this. Add a test that verifies a `FileNotFoundError` is raised as `DatabaseConnectionError`.
 
156
  - Apply this change consistently across all files.
157
 
158
  **Verification Checklist:**
159
+ - [x] No f-strings in any `logger.*()` calls
160
+ - [x] `ruff check src/ tests/` passes
161
+ - [x] `pytest` passes
162
 
163
  **Testing Instructions:** No new tests needed. This is a mechanical replacement.
164
 
 
187
  - Import `TEAM_SIZE` and `STAT_COLUMNS` (or their lengths) from `src/config.py`.
188
 
189
  **Verification Checklist:**
190
+ - [x] `analyze_team_stats` raises `ValueError` if player count != 5
191
+ - [x] `analyze_team_stats` raises `ValueError` if any player has wrong stat count
192
+ - [x] Existing tests pass
193
+ - [x] New tests cover the validation
194
 
195
  **Testing Instructions:**
196
  - Add tests in `tests/test_ml.py`:
 
220
  - Remove the redundant check from `from_preset()` since the Pydantic validator will catch it during construction. Let `from_preset()` simply construct the instance and trust Pydantic validation.
221
 
222
  **Verification Checklist:**
223
+ - [x] Only one validation path for preset names
224
+ - [x] `pytest tests/test_models.py` passes
225
+ - [x] Invalid preset names still raise appropriate errors
226
 
227
  **Testing Instructions:** Existing `DifficultySettings` tests should cover this. Verify they pass.
228
 
 
249
  - Fix type hints: change `list[list]` to `list[list[float]]` or more precise types.
250
 
251
  **Verification Checklist:**
252
+ - [x] No `del` operations on input data in `create_stats`
253
+ - [x] `ruff check scripts/` passes
254
+ - [x] Function produces same output (manual verification or add a simple test)
255
 
256
  **Testing Instructions:** This is a training script, not part of the test suite. Manual verification that the output is unchanged, or add a simple test comparing old behavior vs new.
257
 
 
280
  - Alternatively, call `setup_logging()` in each entry point (`app.py`, page files) right after `configure_page()`.
281
 
282
  **Verification Checklist:**
283
+ - [x] `setup_logging()` is NOT called at module level in `config.py`
284
+ - [x] `setup_logging()` IS called in each entry point
285
+ - [x] `pytest` passes
286
+ - [x] Logging still works when running the app
287
 
288
  **Testing Instructions:** Run existing tests. The module-level call removal should not break tests since tests typically configure their own logging.
289
 
docs/plans/2026-03-25-audit-streamlit-nba/Phase-3.md CHANGED
@@ -141,7 +141,7 @@ test(utils): add tests for HTML escaping utilities
141
  - [x] Test verifies input/output shape
142
  - [x] Test passes
143
 
144
- **Testing Instructions:** `pytest tests/test_ml.py::test_load_real_model -v`
145
 
146
  **Commit Message Template:**
147
  ```text
 
141
  - [x] Test verifies input/output shape
142
  - [x] Test passes
143
 
144
+ **Testing Instructions:** `pytest tests/test_ml.py::TestLoadRealModel::test_load_real_model -v`
145
 
146
  **Commit Message Template:**
147
  ```text
docs/plans/2026-03-25-audit-streamlit-nba/Phase-4.md CHANGED
@@ -85,7 +85,7 @@ ci(deps): consolidate to pyproject.toml, remove requirements files
85
  entry: mypy src/
86
  ```
87
  - Add `pre-commit` to the dev dependencies in `pyproject.toml`:
88
- ```
89
  "pre-commit>=3.0.0",
90
  ```
91
  - Verify the hooks work: `uvx pre-commit run --all-files`.
 
85
  entry: mypy src/
86
  ```
87
  - Add `pre-commit` to the dev dependencies in `pyproject.toml`:
88
+ ```toml
89
  "pre-commit>=3.0.0",
90
  ```
91
  - Verify the hooks work: `uvx pre-commit run --all-files`.
docs/plans/2026-03-25-audit-streamlit-nba/doc-audit.md CHANGED
@@ -11,6 +11,8 @@ drift_prevention: markdownlint + lychee
11
  language_stack: python + js/ts
12
  ---
13
 
 
 
14
  ## DOCUMENTATION AUDIT
15
 
16
  ### SUMMARY
@@ -38,7 +40,7 @@ language_stack: python + js/ts
38
  - Tree does not mention `snowflake_nba.csv` (the actual data source used at runtime).
39
  - Tree does not mention `player_stats.txt` or `schedule.txt` (training data files).
40
  - Tree does not mention `winner_model/` directory (alternative SavedModel format alongside `winner.keras`).
41
- - Tree does not mention `.streamlit/config.toml`, `.devcontainer/devcontainer.json`, or `.github/` workflows.
42
 
43
  4. **`README.md:21-22`** - "comprehensive database of historical NBA stats"
44
  - Doc says: "Search for players from a comprehensive database of historical NBA stats."
@@ -48,15 +50,15 @@ language_stack: python + js/ts
48
 
49
  ### GAPS (code exists, no doc)
50
 
51
- 1. **`src/config.py`** - Central configuration module with `PLAYER_COLUMNS`, `STAT_COLUMNS`, `TEAM_SIZE`, `MAX_QUERY_ATTEMPTS`, `DIFFICULTY_PRESETS`, score ranges, and `setup_logging()`. Not mentioned anywhere in documentation.
52
 
53
- 2. **`src/models/player.py`** - Pydantic models `PlayerStats` and `DifficultySettings` with validation logic. No documentation describes the data models or their validation rules.
54
 
55
- 3. **`src/state/session.py`** - Session state management including `GameState` dataclass, `init_session_state()`, `get_away_stats()`, `get_home_team_df()`, `get_home_team_names()`, `set_difficulty()`, `add_player_to_team()`, `remove_player_from_team()`. None documented.
56
 
57
- 4. **`src/utils/html.py`** - XSS protection utilities (`escape_html`, `safe_heading`, `safe_paragraph`, `safe_styled_text`). The README mentions nothing about security or HTML sanitization.
58
 
59
- 5. **`src/validation/inputs.py`** - SQL injection protection with `PlayerSearchInput` model, `SQL_INJECTION_PATTERNS`, `validate_search_term()`, `is_valid_search_term()`. README line 22 mentions "Input validation for secure and accurate player searches" but provides no details about what validation is performed or why SQL injection protection exists when the data source is a local CSV.
60
 
61
  ---
62
 
 
11
  language_stack: python + js/ts
12
  ---
13
 
14
+ > **Snapshot context:** This document captures pre-remediation findings from the 2026-03-25 audit. Items addressed during the remediation PR are annotated inline.
15
+
16
  ## DOCUMENTATION AUDIT
17
 
18
  ### SUMMARY
 
40
  - Tree does not mention `snowflake_nba.csv` (the actual data source used at runtime).
41
  - Tree does not mention `player_stats.txt` or `schedule.txt` (training data files).
42
  - Tree does not mention `winner_model/` directory (alternative SavedModel format alongside `winner.keras`).
43
+ - Tree does not mention `.streamlit/config.toml`, `.devcontainer/devcontainer.json`, or `.github/` workflows (GitHub Actions CI).
44
 
45
  4. **`README.md:21-22`** - "comprehensive database of historical NBA stats"
46
  - Doc says: "Search for players from a comprehensive database of historical NBA stats."
 
50
 
51
  ### GAPS (code exists, no doc)
52
 
53
+ 1. **`src/config.py`** - Central configuration module with `PLAYER_COLUMNS`, `STAT_COLUMNS`, `TEAM_SIZE`, `MAX_QUERY_ATTEMPTS`, `DIFFICULTY_PRESETS`, score ranges, and `setup_logging()`. Not mentioned anywhere in documentation. *(Partially addressed: README now documents data file paths and config module.)*
54
 
55
+ 2. **`src/models/player.py`** - ~~Pydantic models `PlayerStats` and `DifficultySettings` with validation logic.~~ *(Remediated: `PlayerStats` and `from_db_row` removed. Only `DifficultySettings` remains, which is an internal model used by session state.)*
56
 
57
+ 3. **`src/state/session.py`** - ~~Session state management including `GameState` dataclass, `init_session_state()`, `get_away_stats()`, `get_home_team_df()`, `get_home_team_names()`, `set_difficulty()`, `add_player_to_team()`, `remove_player_from_team()`.~~ *(Remediated: `GameState`, `get_home_team_names`, `set_difficulty`, `add_player_to_team`, and `remove_player_from_team` removed. Remaining functions: `init_session_state()`, `get_away_stats()`, `get_home_team_df()`.)*
58
 
59
+ 4. **`src/utils/html.py`** - ~~XSS protection utilities (`escape_html`, `safe_heading`, `safe_paragraph`, `safe_styled_text`).~~ *(Remediated: `safe_styled_text` removed. Remaining functions: `escape_html`, `safe_heading`, `safe_paragraph`.)*
60
 
61
+ 5. **`src/validation/inputs.py`** - ~~SQL injection protection with `PlayerSearchInput` model, `SQL_INJECTION_PATTERNS`, `validate_search_term()`, `is_valid_search_term()`.~~ *(Remediated: `SQL_INJECTION_PATTERNS` regex removed. Validation now uses a character allowlist only. `PlayerSearchInput`, `validate_search_term()`, and `is_valid_search_term()` remain.)*
62
 
63
  ---
64
 
docs/plans/2026-03-25-audit-streamlit-nba/eval.md CHANGED
@@ -17,6 +17,8 @@ pillars:
17
  onboarding: 7
18
  ---
19
 
 
 
20
  ## HIRE EVALUATION -- The Pragmatist
21
 
22
  ### VERDICT
@@ -25,12 +27,13 @@ pillars:
25
  - **One-Line:** Well-structured toy app that demonstrates strong defensive habits but lacks depth in the ML pipeline and leaves Pydantic models mostly unused.
26
 
27
  ### SCORECARD
 
28
  | Pillar | Score | Evidence |
29
  |--------|-------|----------|
30
- | Problem-Solution Fit | 6/10 | `requirements.txt:2` TensorFlow is a heavyweight dependency for a binary classifier on 100 features; `src/validation/inputs.py:8-28` SQL injection protection for a local CSV pandas app (no SQL database) |
31
- | Architecture | 7/10 | `src/database/__init__.py:1-23` clean module boundaries with `__all__` exports; `src/models/player.py:10-81` Pydantic `PlayerStats` model defined but never used in the actual data flow (pages operate on raw DataFrames) |
32
  | Code Quality | 8/10 | `src/utils/html.py:12-47` proper XSS escaping with `html.escape`; `pages/2_play_game.py:96-110` defensive score generation with fallback; zero `print()` statements, zero TODOs, consistent docstrings throughout |
33
- | Creativity | 6/10 | `scripts/compile_model.py:73-113` `create_stats` mutates input lists via `del home_stats[i][j][0]` which is fragile and side-effect-heavy; `src/database/queries.py:96-151` away team generation algorithm is a reasonable approach but nothing inventive |
34
 
35
  ### HIGHLIGHTS
36
  - **Brilliance:** The security posture is notably strong for a Streamlit project. `src/utils/html.py:24-47` escapes all user-provided values before injecting into HTML markup, including color and alignment parameters, not just text. `src/validation/inputs.py:8-28` provides a compiled regex with 13 SQL injection patterns plus character validation. The test suite at `tests/test_validation.py:46-85` covers 10 parametrized injection vectors and 5 special character attacks, showing genuine security awareness.
@@ -49,9 +52,9 @@ pillars:
49
  - Estimated complexity: MEDIUM
50
 
51
  - **Architecture (current: 7/10, target: 9/10)**
52
- - Either use the Pydantic models or remove them. Specifically, `src/models/player.py` `PlayerStats` should be integrated into `src/database/queries.py` so that query functions return validated model instances instead of raw DataFrames. Alternatively, delete the models and rely on DataFrame typing (the current de facto approach).
53
- - The `GameState` dataclass in `src/state/session.py:18-28` is defined but never instantiated. `init_session_state()` at line 31 uses a plain dict instead. Unify to one approach.
54
- - The `get_connection()` context manager at `src/database/connection.py:54-73` wraps a cached DataFrame read, which does not need resource cleanup. Simplify to a direct function call or at minimum remove the empty `finally: pass`.
55
  - Estimated complexity: MEDIUM
56
 
57
  - **Code Quality (current: 8/10, target: 9/10)**
@@ -74,12 +77,13 @@ pillars:
74
  - **One-Line:** Well-organized Streamlit app with genuine defensive coding, but the ML pipeline has silent shape assumption bombs and the "database" layer is ceremony over substance.
75
 
76
  ### SCORECARD
 
77
  | Pillar | Score | Evidence |
78
  |--------|-------|----------|
79
- | Pragmatism | 6/10 | `src/database/connection.py:54-73` context manager wrapping a cached DataFrame read is pure ceremony; `src/validation/inputs.py:8-24` SQL injection guards on a CSV file |
80
- | Defensiveness | 7/10 | `pages/2_play_game.py:139-184` proper try/catch chains with user-facing errors; `src/ml/model.py:69-70` shape validation before prediction |
81
- | Performance | 7/10 | `src/database/connection.py:29` `@st.cache_data` on CSV load; `pages/1_home_team.py:86-88` batch query instead of N+1 |
82
- | Type Rigor | 7/10 | `src/models/player.py:10-41` thorough Pydantic model with constraints; `src/database/queries.py:36` `tuple[Any, ...]` return type is imprecise |
83
 
84
  ### CRITICAL FAILURE POINTS
85
 
@@ -143,19 +147,20 @@ None that are automatic no-go items. No global state leaks, no unhandled promise
143
  - **One-Line:** "Well-structured code written for the next person, but the onboarding path has gaps and git history tells two different stories."
144
 
145
  ### SCORECARD
 
146
  | Pillar | Score | Evidence |
147
  |--------|-------|----------|
148
- | Test Value | 7/10 | `tests/test_validation.py:46-85` SQL injection tests document real security behavior; `tests/test_ml.py:48-123` over-mocks the model layer, coupling to implementation |
149
- | Reproducibility | 7/10 | `pyproject.toml` has full tool config; `.github/workflows/ci.yml` runs tests+lint+mypy; but `.gitignore` is a single line (`/venv`) missing coverage artifacts, `.coverage`, and `__pycache__` |
150
  | Git Hygiene | 5/10 | `6424951` is a 2000+ line mega-commit creating entire `src/`, `tests/`, and `scripts/` directories; early history is "score update" x5, "README update" x4 |
151
  | Onboarding | 7/10 | `README.md` has quick start, test commands, project structure; missing `.env.example`, no prereq for the `.keras` model file, no contributing guide |
152
 
153
  ### RED FLAGS
154
  - **Minimal .gitignore**: Contains only `/venv`. A junior would commit build artifacts on day one.
155
  - **Binary model file in git** (`winner.keras`, 87KB): Checked into the repo with no Git LFS.
156
- - **Coverage threshold at 50%** (`pyproject.toml:113`): Low bar that signals "we have tests" rather than "we trust our tests."
157
  - **Mega-commit** (`6424951`): "Refactor app with security fixes, error handling, and type safety" touches 30+ files with 2000+ insertions.
158
- - **No pre-commit hooks**: No `.pre-commit-config.yaml` or `.husky/` directory.
159
  - **Two virtual environments**: Both `.venv/` and `venv/` exist in the repo root.
160
 
161
  ### HIGHLIGHTS
@@ -178,8 +183,8 @@ None that are automatic no-go items. No global state leaks, no unhandled promise
178
  - **Test Value (current: 7/10, target: 9/10)**
179
  - Add an integration test that loads the actual CSV and validates column order matches `PLAYER_COLUMNS`.
180
  - Add at least one test in `test_ml.py` that loads the real `winner.keras` model.
181
- - Raise coverage threshold from 50% to 70% and add `--cov-fail-under=70` to CI.
182
- - Add tests for `src/state/session.py` and `src/utils/html.py`.
183
  - Estimated complexity: MEDIUM
184
 
185
  - **Reproducibility (current: 7/10, target: 9/10)**
 
17
  onboarding: 7
18
  ---
19
 
20
+ > **Snapshot context:** This document captures pre-remediation (baseline) findings from the 2026-03-25 audit. Scores and evidence reflect the codebase state before the remediation PR. Items addressed during remediation are annotated inline.
21
+
22
  ## HIRE EVALUATION -- The Pragmatist
23
 
24
  ### VERDICT
 
27
  - **One-Line:** Well-structured toy app that demonstrates strong defensive habits but lacks depth in the ML pipeline and leaves Pydantic models mostly unused.
28
 
29
  ### SCORECARD
30
+
31
  | Pillar | Score | Evidence |
32
  |--------|-------|----------|
33
+ | Problem-Solution Fit | 6/10 | `requirements.txt:2` TensorFlow is a heavyweight dependency for a binary classifier on 100 features *(TF retained, see ADR-2)*; `src/validation/inputs.py:8-28` SQL injection protection for a local CSV pandas app *(remediated: SQL regex removed, character allowlist retained)* |
34
+ | Architecture | 7/10 | `src/database/__init__.py:1-23` clean module boundaries with `__all__` exports; `src/models/player.py:10-81` Pydantic `PlayerStats` model defined but never used *(remediated: `PlayerStats` removed, `DifficultySettings` retained)* |
35
  | Code Quality | 8/10 | `src/utils/html.py:12-47` proper XSS escaping with `html.escape`; `pages/2_play_game.py:96-110` defensive score generation with fallback; zero `print()` statements, zero TODOs, consistent docstrings throughout |
36
+ | Creativity | 6/10 | `scripts/compile_model.py:73-113` `create_stats` mutates input lists via `del` *(remediated: replaced with slicing)*; `src/database/queries.py:96-151` away team generation algorithm is a reasonable approach but nothing inventive |
37
 
38
  ### HIGHLIGHTS
39
  - **Brilliance:** The security posture is notably strong for a Streamlit project. `src/utils/html.py:24-47` escapes all user-provided values before injecting into HTML markup, including color and alignment parameters, not just text. `src/validation/inputs.py:8-28` provides a compiled regex with 13 SQL injection patterns plus character validation. The test suite at `tests/test_validation.py:46-85` covers 10 parametrized injection vectors and 5 special character attacks, showing genuine security awareness.
 
52
  - Estimated complexity: MEDIUM
53
 
54
  - **Architecture (current: 7/10, target: 9/10)**
55
+ - Either use the Pydantic models or remove them. *(Remediated: `PlayerStats` removed, `DifficultySettings` retained.)*
56
+ - The `GameState` dataclass is defined but never instantiated. *(Remediated: `GameState` removed.)*
57
+ - The `get_connection()` context manager wraps a cached DataFrame read with no resource cleanup. *(Remediated: replaced with plain `get_data()` function, `finally: pass` removed.)*
58
  - Estimated complexity: MEDIUM
59
 
60
  - **Code Quality (current: 8/10, target: 9/10)**
 
77
  - **One-Line:** Well-organized Streamlit app with genuine defensive coding, but the ML pipeline has silent shape assumption bombs and the "database" layer is ceremony over substance.
78
 
79
  ### SCORECARD
80
+
81
  | Pillar | Score | Evidence |
82
  |--------|-------|----------|
83
+ | Pragmatism | 6/10 | `src/database/connection.py:54-73` context manager wrapping a cached DataFrame read *(remediated: replaced with plain function)*; `src/validation/inputs.py:8-24` SQL injection guards on a CSV file *(remediated: SQL regex removed)* |
84
+ | Defensiveness | 7/10 | `pages/2_play_game.py:139-184` proper try/catch chains with user-facing errors; `src/ml/model.py:69-70` shape validation before prediction *(remediated: added input shape validation in `analyze_team_stats`)* |
85
+ | Performance | 7/10 | `src/database/connection.py:29` `@st.cache_data` on CSV load *(remediated: caching moved to page layer)*; `pages/1_home_team.py:86-88` batch query instead of N+1 |
86
+ | Type Rigor | 7/10 | `src/models/player.py:10-41` thorough Pydantic model with constraints *(remediated: `PlayerStats` removed)*; `src/database/queries.py:36` `tuple[Any, ...]` return type *(remediated: types tightened)* |
87
 
88
  ### CRITICAL FAILURE POINTS
89
 
 
147
  - **One-Line:** "Well-structured code written for the next person, but the onboarding path has gaps and git history tells two different stories."
148
 
149
  ### SCORECARD
150
+
151
  | Pillar | Score | Evidence |
152
  |--------|-------|----------|
153
+ | Test Value | 7/10 | `tests/test_validation.py:46-85` SQL injection tests document real security behavior *(remediated: SQL tests removed with SQL code)*; `tests/test_ml.py:48-123` over-mocks the model layer *(remediated: real model load test added)* |
154
+ | Reproducibility | 7/10 | `pyproject.toml` has full tool config; `.github/workflows/ci.yml` runs tests+lint+mypy; but `.gitignore` is a single line *(remediated: expanded to 29 lines)* |
155
  | Git Hygiene | 5/10 | `6424951` is a 2000+ line mega-commit creating entire `src/`, `tests/`, and `scripts/` directories; early history is "score update" x5, "README update" x4 |
156
  | Onboarding | 7/10 | `README.md` has quick start, test commands, project structure; missing `.env.example`, no prereq for the `.keras` model file, no contributing guide |
157
 
158
  ### RED FLAGS
159
  - **Minimal .gitignore**: Contains only `/venv`. A junior would commit build artifacts on day one.
160
  - **Binary model file in git** (`winner.keras`, 87KB): Checked into the repo with no Git LFS.
161
+ - **Coverage threshold at 50%** (`pyproject.toml:113`): *(Remediated: threshold raised to 70%, enforced in CI with `--cov-fail-under=70`. Actual coverage: 93.60%.)*
162
  - **Mega-commit** (`6424951`): "Refactor app with security fixes, error handling, and type safety" touches 30+ files with 2000+ insertions.
163
+ - **No pre-commit hooks**: *(Remediated: `.pre-commit-config.yaml` added with ruff and mypy hooks.)*
164
  - **Two virtual environments**: Both `.venv/` and `venv/` exist in the repo root.
165
 
166
  ### HIGHLIGHTS
 
183
  - **Test Value (current: 7/10, target: 9/10)**
184
  - Add an integration test that loads the actual CSV and validates column order matches `PLAYER_COLUMNS`.
185
  - Add at least one test in `test_ml.py` that loads the real `winner.keras` model.
186
+ - Raise coverage threshold from 50% to 70% and add `--cov-fail-under=70` to CI. *(Remediated: threshold at 70%, CI enforces it.)*
187
+ - Add tests for `src/state/session.py` and `src/utils/html.py`. *(Remediated: `tests/test_state.py` and `tests/test_utils.py` added.)*
188
  - Estimated complexity: MEDIUM
189
 
190
  - **Reproducibility (current: 7/10, target: 9/10)**
pages/2_play_game.py CHANGED
@@ -74,8 +74,8 @@ def find_away_team(stat_thresholds: list[int]) -> pd.DataFrame:
74
  max_attempts=MAX_QUERY_ATTEMPTS,
75
  )
76
  except DatabaseConnectionError as e:
77
- st.error("Could not connect to database. Please try again later.")
78
- logger.error("Database connection error: %s", e)
79
  return pd.DataFrame()
80
  except QueryExecutionError as e:
81
  st.error("Could not generate away team. Please try again.")
 
74
  max_attempts=MAX_QUERY_ATTEMPTS,
75
  )
76
  except DatabaseConnectionError as e:
77
+ st.error("Could not load player data. Please try again later.")
78
+ logger.error("Data load error: %s", e)
79
  return pd.DataFrame()
80
  except QueryExecutionError as e:
81
  st.error("Could not generate away team. Please try again.")
scripts/compile_model.py CHANGED
@@ -80,8 +80,8 @@ def create_stats(roster: pd.DataFrame, schedule: pd.DataFrame) -> list[np.ndarra
80
  Returns:
81
  List of numpy arrays, one per game with combined team stats
82
  """
83
- home_stats: list[list[list[float]]] = []
84
- away_stats: list[list[list[float]]] = []
85
  features: list[np.ndarray] = []
86
 
87
  new_roster = roster[FEATURE_COLS]
 
80
  Returns:
81
  List of numpy arrays, one per game with combined team stats
82
  """
83
+ home_stats: list[list[list[str | float]]] = []
84
+ away_stats: list[list[list[str | float]]] = []
85
  features: list[np.ndarray] = []
86
 
87
  new_roster = roster[FEATURE_COLS]
src/database/connection.py CHANGED
@@ -41,7 +41,7 @@ def load_data() -> pd.DataFrame:
41
  # Ensure column names match expected Snowflake names (uppercase)
42
  df.columns = [col.upper() for col in df.columns]
43
  return df
44
- except (FileNotFoundError, pd.errors.ParserError, pd.errors.EmptyDataError) as e:
45
  logger.error("Failed to load CSV data: %s", e)
46
  msg = f"Could not load data from {CSV_PATH}: {e}"
47
  raise DatabaseConnectionError(msg) from e
 
41
  # Ensure column names match expected Snowflake names (uppercase)
42
  df.columns = [col.upper() for col in df.columns]
43
  return df
44
+ except (pd.errors.ParserError, pd.errors.EmptyDataError) as e:
45
  logger.error("Failed to load CSV data: %s", e)
46
  msg = f"Could not load data from {CSV_PATH}: {e}"
47
  raise DatabaseConnectionError(msg) from e
src/models/player.py CHANGED
@@ -44,7 +44,8 @@ class DifficultySettings(BaseModel):
44
  """
45
  preset = DIFFICULTY_PRESETS.get(preset_name)
46
  if preset is None:
47
- # Let Pydantic validation handle the error message
 
48
  return cls(
49
  name=preset_name,
50
  pts_threshold=0,
 
44
  """
45
  preset = DIFFICULTY_PRESETS.get(preset_name)
46
  if preset is None:
47
+ # Pass invalid name to constructor; the field_validator on
48
+ # `name` will raise ValueError with valid preset options.
49
  return cls(
50
  name=preset_name,
51
  pts_threshold=0,
src/state/__init__.py CHANGED
@@ -1,5 +1,5 @@
1
  """Session state management module."""
2
 
3
- from src.state.session import get_away_stats, init_session_state
4
 
5
- __all__ = ["get_away_stats", "init_session_state"]
 
1
  """Session state management module."""
2
 
3
+ from src.state.session import get_away_stats, get_home_team_df, init_session_state
4
 
5
+ __all__ = ["get_away_stats", "get_home_team_df", "init_session_state"]
src/validation/inputs.py CHANGED
@@ -30,9 +30,11 @@ class PlayerSearchInput(BaseModel):
30
  ValueError: If invalid characters found
31
  """
32
  v = v.strip()
 
 
33
  # Allow letters, numbers, spaces, hyphens, periods, and apostrophes
34
  # (e.g., "O'Neal", "J.R. Smith")
35
- if not re.match(r"^[a-zA-Z0-9\s\-.']+$", v):
36
  raise ValueError(
37
  "Search term contains invalid characters. "
38
  "Please use only letters, numbers, spaces, hyphens, "
 
30
  ValueError: If invalid characters found
31
  """
32
  v = v.strip()
33
+ if not v:
34
+ raise ValueError("Search term cannot be empty.")
35
  # Allow letters, numbers, spaces, hyphens, periods, and apostrophes
36
  # (e.g., "O'Neal", "J.R. Smith")
37
+ if not re.match(r"^[a-zA-Z0-9 \-.']+$", v):
38
  raise ValueError(
39
  "Search term contains invalid characters. "
40
  "Please use only letters, numbers, spaces, hyphens, "
tests/test_ml.py CHANGED
@@ -25,9 +25,7 @@ class TestAnalyzeTeamStats:
25
  # Combined has both teams = 100 values
26
  assert combined.shape == (1, 100)
27
 
28
- def test_combined_contains_both_teams(
29
- self, sample_team_stats: list[list[float]]
30
- ) -> None:
31
  """Test that combined array contains both teams' stats."""
32
  home_stats = [[float(i * 10 + j) for j in range(10)] for i in range(5)]
33
  away_stats = [[float(50 + i * 10 + j) for j in range(10)] for i in range(5)]
 
25
  # Combined has both teams = 100 values
26
  assert combined.shape == (1, 100)
27
 
28
+ def test_combined_contains_both_teams(self) -> None:
 
 
29
  """Test that combined array contains both teams' stats."""
30
  home_stats = [[float(i * 10 + j) for j in range(10)] for i in range(5)]
31
  away_stats = [[float(50 + i * 10 + j) for j in range(10)] for i in range(5)]
tests/test_state.py CHANGED
@@ -5,6 +5,7 @@ from unittest.mock import patch
5
  import pandas as pd
6
 
7
  from src.config import DIFFICULTY_PRESETS
 
8
 
9
 
10
  class TestInitSessionState:
@@ -15,8 +16,6 @@ class TestInitSessionState:
15
  state: dict = {}
16
  with patch("src.state.session.st") as mock_st:
17
  mock_st.session_state = state
18
- from src.state.session import init_session_state
19
-
20
  init_session_state()
21
 
22
  expected_keys = {
@@ -34,8 +33,6 @@ class TestInitSessionState:
34
  state: dict = {"home_team": ["Player A"]}
35
  with patch("src.state.session.st") as mock_st:
36
  mock_st.session_state = state
37
- from src.state.session import init_session_state
38
-
39
  init_session_state()
40
 
41
  assert state["home_team"] == ["Player A"]
@@ -45,8 +42,6 @@ class TestInitSessionState:
45
  state: dict = {}
46
  with patch("src.state.session.st") as mock_st:
47
  mock_st.session_state = state
48
- from src.state.session import init_session_state
49
-
50
  init_session_state()
51
 
52
  assert state["away_stats"] == list(DIFFICULTY_PRESETS["Regular"])
@@ -56,8 +51,6 @@ class TestInitSessionState:
56
  state: dict = {}
57
  with patch("src.state.session.st") as mock_st:
58
  mock_st.session_state = state
59
- from src.state.session import init_session_state
60
-
61
  init_session_state()
62
 
63
  assert isinstance(state["away_team_df"], pd.DataFrame)
@@ -74,8 +67,6 @@ class TestGetAwayStats:
74
  state: dict = {"away_stats": [100, 200, 300, 400]}
75
  with patch("src.state.session.st") as mock_st:
76
  mock_st.session_state = state
77
- from src.state.session import get_away_stats
78
-
79
  result = get_away_stats()
80
 
81
  assert result == [100, 200, 300, 400]
@@ -85,8 +76,24 @@ class TestGetAwayStats:
85
  state: dict = {"away_stats": "invalid"}
86
  with patch("src.state.session.st") as mock_st:
87
  mock_st.session_state = state
88
- from src.state.session import get_away_stats
 
 
 
 
 
 
 
 
 
 
 
89
 
 
 
 
 
 
90
  result = get_away_stats()
91
 
92
  assert result == list(DIFFICULTY_PRESETS["Regular"])
@@ -101,8 +108,6 @@ class TestGetHomeTeamDf:
101
  state: dict = {"home_team_df": expected_df}
102
  with patch("src.state.session.st") as mock_st:
103
  mock_st.session_state = state
104
- from src.state.session import get_home_team_df
105
-
106
  result = get_home_team_df()
107
 
108
  pd.testing.assert_frame_equal(result, expected_df)
@@ -112,8 +117,6 @@ class TestGetHomeTeamDf:
112
  state: dict = {}
113
  with patch("src.state.session.st") as mock_st:
114
  mock_st.session_state = state
115
- from src.state.session import get_home_team_df
116
-
117
  result = get_home_team_df()
118
 
119
  assert isinstance(result, pd.DataFrame)
 
5
  import pandas as pd
6
 
7
  from src.config import DIFFICULTY_PRESETS
8
+ from src.state.session import get_away_stats, get_home_team_df, init_session_state
9
 
10
 
11
  class TestInitSessionState:
 
16
  state: dict = {}
17
  with patch("src.state.session.st") as mock_st:
18
  mock_st.session_state = state
 
 
19
  init_session_state()
20
 
21
  expected_keys = {
 
33
  state: dict = {"home_team": ["Player A"]}
34
  with patch("src.state.session.st") as mock_st:
35
  mock_st.session_state = state
 
 
36
  init_session_state()
37
 
38
  assert state["home_team"] == ["Player A"]
 
42
  state: dict = {}
43
  with patch("src.state.session.st") as mock_st:
44
  mock_st.session_state = state
 
 
45
  init_session_state()
46
 
47
  assert state["away_stats"] == list(DIFFICULTY_PRESETS["Regular"])
 
51
  state: dict = {}
52
  with patch("src.state.session.st") as mock_st:
53
  mock_st.session_state = state
 
 
54
  init_session_state()
55
 
56
  assert isinstance(state["away_team_df"], pd.DataFrame)
 
67
  state: dict = {"away_stats": [100, 200, 300, 400]}
68
  with patch("src.state.session.st") as mock_st:
69
  mock_st.session_state = state
 
 
70
  result = get_away_stats()
71
 
72
  assert result == [100, 200, 300, 400]
 
76
  state: dict = {"away_stats": "invalid"}
77
  with patch("src.state.session.st") as mock_st:
78
  mock_st.session_state = state
79
+ result = get_away_stats()
80
+
81
+ assert result == list(DIFFICULTY_PRESETS["Regular"])
82
+
83
+ def test_returns_defaults_on_none(self) -> None:
84
+ """Verify returns defaults when away_stats is None."""
85
+ state: dict = {"away_stats": None}
86
+ with patch("src.state.session.st") as mock_st:
87
+ mock_st.session_state = state
88
+ result = get_away_stats()
89
+
90
+ assert result == list(DIFFICULTY_PRESETS["Regular"])
91
 
92
+ def test_returns_defaults_on_wrong_length(self) -> None:
93
+ """Verify returns defaults when away_stats has wrong length."""
94
+ state: dict = {"away_stats": [1, 2, 3]}
95
+ with patch("src.state.session.st") as mock_st:
96
+ mock_st.session_state = state
97
  result = get_away_stats()
98
 
99
  assert result == list(DIFFICULTY_PRESETS["Regular"])
 
108
  state: dict = {"home_team_df": expected_df}
109
  with patch("src.state.session.st") as mock_st:
110
  mock_st.session_state = state
 
 
111
  result = get_home_team_df()
112
 
113
  pd.testing.assert_frame_equal(result, expected_df)
 
117
  state: dict = {}
118
  with patch("src.state.session.st") as mock_st:
119
  mock_st.session_state = state
 
 
120
  result = get_home_team_df()
121
 
122
  assert isinstance(result, pd.DataFrame)
tests/test_validation.py CHANGED
@@ -52,7 +52,7 @@ class TestRejectsInvalidCharacters:
52
  "James<script>",
53
  "James&nbsp;",
54
  "James@#$%",
55
- "James\\nNewline",
56
  "James\x00null",
57
  ],
58
  )
 
52
  "James<script>",
53
  "James&nbsp;",
54
  "James@#$%",
55
+ "James\nNewline",
56
  "James\x00null",
57
  ],
58
  )