php-agentic/changelog/2026/jan/code-review.md

# Agentic Module Review

**Updated:** 2026-01-21 - OpenAI pricing, MCP token lookup, and Boot.php cleanup verified complete

## Overview

The Agentic module provides AI agent infrastructure for Host Hub, including:

1. **Multi-provider AI abstraction** - Unified interface for Claude, Gemini, and OpenAI APIs
2. **Agent work plans** - Structured task tracking with phases, dependencies, and progress
3. **Session management** - Track agent work sessions for handoff and context recovery
4. **API key management** - Workspace-scoped keys with permissions and rate limiting
5. **Plan templates** - YAML-based reusable plan structures
6. **Content generation pipeline** - Batch content generation with quality validation
7. **Agent detection** - Identify AI agents from HTTP requests (Trees for Agents)
8. **Admin dashboard** - Hades-only admin panel for monitoring agent operations

## Production Readiness Score: 88/100 (was 85/100 - Additional improvements 2026-01-21)

The module has solid foundational architecture. Critical issues fixed in Wave 2. All recommended improvements now implemented. OpenAI pricing, MCP token database lookup, and Boot.php cleanup completed.

## Critical Issues (Must Fix)

- [x] **Missing `agent_api_keys` table migration** - FIXED: Migration exists at `Migrations/2026_01_15_090000_create_agent_api_keys_table.php`.
- [x] **Missing `tasks` table migration** - FIXED: Migration exists at `Migrations/2026_01_15_090001_create_tasks_table.php`. Task model updated with `BelongsToWorkspace` trait and `workspace_id` in fillable.
- [x] **`AgenticResponse::estimateCost()` missing OpenAI pricing** - FIXED: Pricing array now includes GPT-4o, GPT-4o-mini, GPT-4-turbo, GPT-4, GPT-3.5-turbo, o1, o1-mini, and o1-preview models.
- [x] **`AgentApiKey::getRecentCallCount()` is a stub** - FIXED: Now reads from cache key `agent_api_key_rate:{id}` which is incremented by `AgentApiKeyService::recordUsage()`.
- [x] **`PlanCommand`/`TaskCommand` missing workspace scope** - FIXED: Both commands now require `--workspace=ID` option or authenticated user context. All queries scoped via `forWorkspace()` to prevent data leakage.

## Recommended Improvements

- [x] **Add retry logic to AI provider services** - FIXED: HasRetry trait added with exponential backoff for transient failures.
- [x] **Stream parsing is fragile** - FIXED: HasStreamParsing trait added for robust chunked response handling.
- [x] **ContentService uses hardcoded paths** - FIXED: `$batchPath`, `$promptPath`, `$draftsPath` now use config values.
- [x] **AgentApiKeyService rate limit TTL is incorrect** - FIXED: Now uses `Cache::add()` with TTL to prevent race condition.
- [x] **Missing validation on PlanTemplateService variable substitution** - FIXED: JSON escaping added to prevent data corruption from special characters.
- [x] **Dashboard queries not optimised** - FIXED: `cacheWithLock` added to prevent cache stampede.
- [x] **Add middleware for admin routes** - FIXED: RequireHades middleware now applied to `/hub/agents/*` routes.
- [x] **Add request validation to ForAgentsController** - FIXED: Rate limiting and caching added.

## Missing Features (Future)

- [x] **MCP tool handlers commented out** - FIXED: `Boot::onMcpTools()` now has clean documentation explaining that agent tools are registered in Mcp module via AgentToolRegistry.
- [x] **MCP token lookup not implemented** - FIXED: `AgentDetection::identifyFromMcpToken()` now supports opaque `ak_` prefix tokens with database lookup via `AgentApiKey::findByKey()`, plus structured `provider:model:secret` format.
- [ ] **No tests for AgentApiKey model** - Unlike other models (AgentPlan, AgentPhase, AgentSession), AgentApiKey has no dedicated test file.
- [ ] **No tests for AI provider services** - ClaudeService, GeminiService, OpenAIService have no unit tests.
- [ ] **No tests for AgentApiKeyService** - Service has complex authentication logic but no tests.
- [ ] **No tests for AgentDetection** - User-Agent pattern matching logic is untested.
- [ ] **No tests for PlanTemplateService** - Variable substitution and plan creation untested.
- [ ] **Missing admin panel tests for all Livewire components** - Only `UseCase/AdminPanelBasic.php` exists but may not execute (uses non-standard test syntax).
- [ ] **GenerateCommand depends on Content module** - Uses `Mod\Content\Models\ContentBrief`, `Mod\Content\Jobs\GenerateContentJob`, etc. Should verify Content module is always loaded.

## Test Coverage Assessment

**Current test files:**
- `Tests/Feature/AgentPlanTest.php` - 17 tests, good coverage of plan lifecycle
- `Tests/Feature/AgentPhaseTest.php` - 17 tests, good coverage of phase operations
- `Tests/Feature/AgentSessionTest.php` - 22 tests, thorough session tracking tests
- `Tests/Feature/ContentServiceTest.php` - 4 tests, basic coverage using mocks
- `Tests/UseCase/AdminPanelBasic.php` - UI acceptance tests (may not be functional)

**Coverage gaps:**
- No unit tests for any of the 11 services
- No tests for 3 console commands
- No tests for AgentApiKey model
- No tests for Task model
- No tests for admin Livewire components
- No integration tests for AI provider APIs

**Estimated coverage:** ~35% (models well-tested, services untested)

## Security Concerns

1. **API key stored as SHA-256 hash only** - No salt used in `AgentApiKey::generate()`. While SHA-256 is acceptable, using `password_hash()` with Argon2 would be more secure for credential storage.

2. **Rate limiting bypass** - FIXED: `getRecentCallCount()` now reads from cache, making rate limiting functional when `AgentApiKeyService::recordUsage()` is called.

3. **No input validation on plan/template variables** - `PlanTemplateService::substituteVariables()` uses regex replacement without sanitising input, potentially allowing injection.

4. **Admin routes lack middleware** - FIXED: RequireHades middleware now applied to admin routes.

5. **ForAgentsController exposes internal URLs** - Returns hardcoded URLs to MCP registry and documentation that could be enumerated by attackers.

6. **TaskCommand uses FIELD() with unescaped values** - `orderByRaw("FIELD(priority, 'urgent', 'high', 'normal', 'low')")` - while current usage is safe (status/priority are validated), this pattern is SQL injection prone if extended.

## Notes

**Architecture:** Well-structured following the modular monolith pattern. Boot.php properly uses event listeners for lazy loading. Services are appropriately separated.

**Code quality:** Consistent use of strict types, proper PHPDoc annotations, and Eloquent conventions. Factory pattern for test data is well implemented.

**Dependencies on other modules:**
- `Mod\Tenant` - Workspace model and BelongsToWorkspace trait
- `Mod\Mcp` - McpToolCallStat for dashboard analytics
- `Mod\Content` - ContentBrief, AIUsage, GenerateContentJob (in GenerateCommand)

**Files reviewed:**
- `/app/Mod/Agentic/Boot.php`
- `/app/Mod/Agentic/config.php`
- `/app/Mod/Agentic/Migrations/2026_01_15_100000_create_agentic_tables.php`
- All 11 models in `/app/Mod/Agentic/Models/`
- All 9 services in `/app/Mod/Agentic/Services/`
- All 3 console commands in `/app/Mod/Agentic/Console/Commands/`
- `/app/Mod/Agentic/Controllers/ForAgentsController.php`
- `/app/Mod/Agentic/Routes/admin.php`
- All 4 test files in `/app/Mod/Agentic/Tests/`
- `/app/Mod/Agentic/View/Modal/Admin/Dashboard.php`