From 167518f2589453d892745d6f3ddc64761cc60e3b Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Wed, 29 Oct 2025 07:24:19 -0400 Subject: [PATCH] feat: Add signals agent registry with unified MCP client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add comprehensive signals agent management system with registry pattern, unified MCP client utility, and admin UI for managing tenant-specific signals agents. Key Features: - SignalsAgent database model and registry for centralized agent management - Unified MCP client utility (src/core/utils/mcp_client.py) for consistent agent communication across creative agents, signals agents, etc. - Admin UI for adding/editing/deleting/testing signals agents - Migrate product catalog provider to use agent registry - Fix creative agent registry to use correct /mcp path suffix Architecture: - SignalsAgentRegistry pattern (mirrors CreativeAgentRegistry) - Unified MCP client with retry logic, auth handling, error handling - Database-backed agent configuration per tenant - Admin UI templates for agent management Database Changes: - Add signals_agents table with proper indexes and foreign keys - Merge migrations to resolve multiple heads - Remove deprecated brand manifest policy fields Testing: - Add integration tests for signals agent workflow - Add MCP client utility tests - Update signals discovery provider integration tests - Skip MCP server tests in CI (require local server) Fixes: - Fix creative agent default URL (add /mcp suffix) - Fix migration script to use 'heads' instead of 'head' - Update signals agent workflow tests to use unified MCP client - Remove deprecated signals provider tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .cz-config.md | 120 -- .github/workflows/commitizen-check.yml | 36 - .github/workflows/release.yml | 107 -- CHANGELOG.md | 35 - CLAUDE.md | 1296 ++++++++++++++--- README.md | 74 - .../versions/024_add_signals_agents_table.py | 57 + ...56a4aaa17_merge_signals_agents_and_main.py | 25 + ...hange_brand_manifest_policy_default_to_.py | 50 - ...33_add_brand_manifest_policy_to_tenants.py | 40 - ...8b051_merge_signals_agents_and_pricing_.py | 25 + ...b22_update_signals_agents_table_remove_.py | 46 + docker-compose.yml | 1 - product_catalog_providers/signals.py | 137 +- pyproject.toml | 13 - ...mas_v1_core_webhook-payload_json.json.meta | 9 +- ..._schemas_v1_enums_task-type_json.json.meta | 9 +- ...ia-buy_get-products-request_json.json.meta | 8 +- scripts/ops/migrate.py | 5 +- src/admin/app.py | 2 + src/admin/blueprints/policy.py | 11 - src/admin/blueprints/signals_agents.py | 302 ++++ src/core/creative_agent_registry.py | 206 +-- src/core/database/models.py | 46 +- src/core/signals_agent_registry.py | 299 ++++ src/core/tools/products.py | 84 +- src/core/utils/mcp_client.py | 259 ++++ templates/policy_settings.html | 23 - templates/policy_settings_comprehensive.html | 18 - templates/signals_agent_form.html | 211 +++ templates/signals_agents.html | 218 +++ templates/tenant_settings.html | 71 +- test_mcp_connection.py | 166 +++ tests/integration/test_mcp_client_util.py | 228 +++ tests/integration/test_mcp_protocol.py | 1 + .../test_signals_discovery_integration.py | 206 --- .../test_template_url_validation.py | 10 + .../test_signals_agent_workflow.py | 120 +- .../test_signals_discovery_integration.py | 132 ++ tests/unit/test_brand_manifest_policy.py | 377 ----- tests/unit/test_signals_discovery_provider.py | 472 ------ uv.lock | 127 +- 42 files changed, 3364 insertions(+), 2318 deletions(-) delete mode 100644 .cz-config.md delete mode 100644 .github/workflows/commitizen-check.yml delete mode 100644 .github/workflows/release.yml delete mode 100644 CHANGELOG.md create mode 100644 alembic/versions/024_add_signals_agents_table.py create mode 100644 alembic/versions/28a56a4aaa17_merge_signals_agents_and_main.py delete mode 100644 alembic/versions/378299ad502f_change_brand_manifest_policy_default_to_.py delete mode 100644 alembic/versions/6f05f4179c33_add_brand_manifest_policy_to_tenants.py create mode 100644 alembic/versions/fa617dd8b051_merge_signals_agents_and_pricing_.py create mode 100644 alembic/versions/fa7cc00c5b22_update_signals_agents_table_remove_.py create mode 100644 src/admin/blueprints/signals_agents.py create mode 100644 src/core/signals_agent_registry.py create mode 100644 src/core/utils/mcp_client.py create mode 100644 templates/signals_agent_form.html create mode 100644 templates/signals_agents.html create mode 100644 test_mcp_connection.py create mode 100644 tests/integration/test_mcp_client_util.py delete mode 100644 tests/integration/test_signals_discovery_integration.py create mode 100644 tests/integration_v2/test_signals_discovery_integration.py delete mode 100644 tests/unit/test_brand_manifest_policy.py delete mode 100644 tests/unit/test_signals_discovery_provider.py diff --git a/.cz-config.md b/.cz-config.md deleted file mode 100644 index b52c2e9c7..000000000 --- a/.cz-config.md +++ /dev/null @@ -1,120 +0,0 @@ -# Commitizen Configuration - -This project uses Commitizen for automated version management and changelog generation. - -## Commit Message Format - -Follow the [Conventional Commits](https://www.conventionalcommits.org/) format: - -``` -(): - -[optional body] - -[optional footer(s)] -``` - -## Commit Types - -| Type | Description | Version Bump | Example | -|------|-------------|--------------|---------| -| `feat` | New feature | minor (0.1.0 → 0.2.0) | `feat: add OAuth support` | -| `fix` | Bug fix | patch (0.1.0 → 0.1.1) | `fix: correct validation error` | -| `docs` | Documentation | none | `docs: update README` | -| `test` | Tests | none | `test: add unit tests` | -| `chore` | Tooling/build | none | `chore: update dependencies` | -| `refactor` | Code refactoring | none | `refactor: simplify auth logic` | -| `perf` | Performance | patch | `perf: optimize query` | -| `style` | Code style | none | `style: format with black` | -| `ci` | CI/CD changes | none | `ci: add test workflow` | -| `build` | Build system | none | `build: update docker config` | - -## Breaking Changes - -For breaking changes, use `!` after type or add `BREAKING CHANGE:` in footer: - -```bash -# Method 1: Using ! -git commit -m "feat!: redesign API format" - -# Method 2: Using footer -git commit -m "feat: redesign API format - -BREAKING CHANGE: Response format changed from {data} to {result}" -``` - -Breaking changes trigger a major version bump (0.1.0 → 1.0.0). - -## Scope (Optional) - -Use scope to specify what part of codebase is affected: - -```bash -git commit -m "feat(gam): add video creative support" -git commit -m "fix(auth): prevent token race condition" -git commit -m "docs(api): update MCP tool documentation" -``` - -Common scopes: `gam`, `auth`, `api`, `mcp`, `a2a`, `admin`, `db` - -## Commands - -```bash -# Interactive commit (recommended for beginners) -uv run cz commit - -# Check commit message format -uv run cz check - -# Check all commits in a range -uv run cz check --rev-range main..HEAD - -# Bump version (done automatically by CI) -uv run cz bump - -# Show current version -uv run cz version -``` - -## CI Automation - -- **On PR**: `commitizen-check.yml` validates all commits follow format -- **On merge to main**: `release.yml` automatically bumps version and updates changelog -- **No manual steps**: Just write good commit messages! - -## Examples - -### Good Commits ✅ - -```bash -feat: add webhook support for media buy events -fix: correct timezone handling in campaign scheduling -docs: add deployment guide for Kubernetes -test: add integration tests for A2A protocol -chore: update commitizen to v3.29.0 -refactor: simplify GAM adapter error handling -perf: optimize database query for large tenants -``` - -### Bad Commits ❌ - -```bash -update code # ❌ No type -feat add feature # ❌ Missing colon -FIX: bug # ❌ Type should be lowercase -feat(GAM): new feature # ❌ Scope should be lowercase -``` - -## Tips - -1. **Be specific**: "feat: add OAuth" is better than "feat: update auth" -2. **Use imperative mood**: "add feature" not "adds feature" or "added feature" -3. **Keep subject under 72 characters** -4. **Use body for details**: Subject is a summary, body explains why -5. **Reference issues**: "Closes #123" in footer to auto-close issues - -## Resources - -- [Conventional Commits Specification](https://www.conventionalcommits.org/) -- [Commitizen Documentation](https://commitizen-tools.github.io/commitizen/) -- [Angular Commit Guidelines](https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit) (similar format) diff --git a/.github/workflows/commitizen-check.yml b/.github/workflows/commitizen-check.yml deleted file mode 100644 index f5830ee2f..000000000 --- a/.github/workflows/commitizen-check.yml +++ /dev/null @@ -1,36 +0,0 @@ -name: Commitizen Check - -on: - pull_request: - branches: [main] - -jobs: - commitizen-check: - name: Check Conventional Commits - runs-on: ubuntu-latest - - steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: '3.12' - - - name: Install uv - run: | - curl -LsSf https://astral.sh/uv/0.4.18/install.sh | sh - echo "$HOME/.cargo/bin" >> $GITHUB_PATH - - - name: Install dependencies - run: | - export PATH="$HOME/.cargo/bin:$PATH" - uv sync - - - name: Check commits follow conventional commit format - run: | - export PATH="$HOME/.cargo/bin:$PATH" - # Check all commits in this PR - uv run cz check --rev-range origin/main..HEAD diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml deleted file mode 100644 index e5476ccf1..000000000 --- a/.github/workflows/release.yml +++ /dev/null @@ -1,107 +0,0 @@ -name: Release - -on: - push: - branches: - - main - workflow_dispatch: - -concurrency: ${{ github.workflow }}-${{ github.ref }} - -jobs: - release: - name: Bump Version and Create Release - runs-on: ubuntu-latest - permissions: - contents: write - pull-requests: write - id-token: write - - steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 - token: ${{ secrets.GITHUB_TOKEN }} - - - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: '3.12' - - - name: Install uv - run: | - curl -LsSf https://astral.sh/uv/0.4.18/install.sh | sh - echo "$HOME/.cargo/bin" >> $GITHUB_PATH - - - name: Install dependencies - run: | - export PATH="$HOME/.cargo/bin:$PATH" - uv sync - - - name: Configure Git - run: | - git config user.name "github-actions[bot]" - git config user.email "github-actions[bot]@users.noreply.github.com" - - - name: Check if version bump is needed - id: check_bump - run: | - export PATH="$HOME/.cargo/bin:$PATH" - # Check if there are any commits since last tag that warrant a version bump - LAST_TAG=$(git describe --tags --abbrev=0 2>/dev/null || echo "v0.1.0") - echo "Last tag: $LAST_TAG" - - # Check if there are feat/fix/BREAKING commits since last tag - if git log $LAST_TAG..HEAD --oneline | grep -qE '^[a-f0-9]+ (feat|fix|perf|refactor|BREAKING CHANGE)'; then - echo "needs_bump=true" >> $GITHUB_OUTPUT - echo "✅ Found commits that need version bump" - else - echo "needs_bump=false" >> $GITHUB_OUTPUT - echo "ℹī¸ No version bump needed (only chore/docs/test commits)" - fi - - - name: Bump version and update changelog - if: steps.check_bump.outputs.needs_bump == 'true' - run: | - export PATH="$HOME/.cargo/bin:$PATH" - - # Bump version (automatically determines bump type from commits) - uv run cz bump --yes --changelog - - # Get the new version - NEW_VERSION=$(uv run cz version -p) - echo "NEW_VERSION=$NEW_VERSION" >> $GITHUB_ENV - - echo "✅ Bumped to version $NEW_VERSION" - - - name: Push changes - if: steps.check_bump.outputs.needs_bump == 'true' - run: | - git push origin main --tags - echo "✅ Pushed version bump and tags to main" - - - name: Create GitHub Release - if: steps.check_bump.outputs.needs_bump == 'true' - uses: actions/create-release@v1 - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - with: - tag_name: v${{ env.NEW_VERSION }} - release_name: Release v${{ env.NEW_VERSION }} - body: | - ## Changes - - See [CHANGELOG.md](https://github.com/${{ github.repository }}/blob/main/CHANGELOG.md) for details. - draft: false - prerelease: false - - - name: Summary - run: | - if [ "${{ steps.check_bump.outputs.needs_bump }}" == "true" ]; then - echo "✅ Version bumped to ${{ env.NEW_VERSION }}" - echo "✅ Changelog updated" - echo "✅ Git tag created and pushed" - echo "✅ GitHub release created" - else - echo "ℹī¸ No version bump needed - no feat/fix/BREAKING commits found" - fi diff --git a/CHANGELOG.md b/CHANGELOG.md deleted file mode 100644 index 36d3a36de..000000000 --- a/CHANGELOG.md +++ /dev/null @@ -1,35 +0,0 @@ -# Changelog - -All notable changes to this project will be documented in this file. - -The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), -and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - -## [Unreleased] - -### Added -- Changeset system for automated version management -- CI workflows to enforce changeset requirements on PRs -- Automated version bump PR creation when changesets are merged - -## [0.1.0] - 2025-01-29 - -Initial release of the AdCP Sales Agent reference implementation. - -### Added -- MCP server implementation with AdCP v2.3 support -- A2A (Agent-to-Agent) protocol support -- Multi-tenant architecture with PostgreSQL -- Google Ad Manager (GAM) adapter -- Mock ad server adapter for testing -- Admin UI with Google OAuth authentication -- Comprehensive testing backend with dry-run support -- Real-time activity dashboard with SSE -- Workflow management system -- Creative management and approval workflows -- Audit logging -- Docker deployment support -- Extensive documentation - -[Unreleased]: https://github.com/adcontextprotocol/salesagent/compare/v0.1.0...HEAD -[0.1.0]: https://github.com/adcontextprotocol/salesagent/releases/tag/v0.1.0 diff --git a/CLAUDE.md b/CLAUDE.md index c7541f07c..986755a71 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,286 +1,1218 @@ -# AdCP Sales Agent - Development Guide +# AdCP Sales Agent Server - Development Guide -Quick reference for AI coding assistants with essential context and gotchas. See `/docs` for detailed documentation. +## 🚨 CRITICAL ARCHITECTURE PATTERNS -## External Resources & Services +### AdCP Schema Source of Truth +**🚨 MANDATORY**: The official AdCP specification at https://adcontextprotocol.org/schemas/v1/ is the **SINGLE SOURCE OF TRUTH** for all API schemas. -**AdCP Protocol** -- Spec: https://adcontextprotocol.org/schemas/v1/ -- Docs: https://adcontextprotocol.org/docs/ -- Current version: 2.2.0 (official), v1 schemas -- Cached schemas: `schemas/v1/` (checked into git) +**Schema Hierarchy:** +1. **Official Spec** (https://adcontextprotocol.org/schemas/v1/) - Primary source of truth +2. **Cached Schemas** (`schemas/v1/`) - Checked into git for offline validation +3. **Pydantic Schemas** (`src/core/schemas.py`) - MUST match official spec exactly -**Deployment (Reference Implementation)** -- Local: `docker-compose up` → localhost:8001/8080/8091 -- Sales Agent (ours): https://adcp-sales-agent.fly.dev (Fly.io, auto-deploys from `main`) -- Test Buyer Agent (ours): https://test-agent.adcontextprotocol.org (Fly.io, for E2E tests) +**Rules:** +- ✅ Always verify against official AdCP spec when adding/modifying schemas +- ✅ Use `tests/e2e/adcp_schema_validator.py` to validate responses +- ✅ Run `pytest tests/unit/test_adcp_contract.py` to check Pydantic schema compliance +- ❌ NEVER add fields not in the official spec +- ❌ NEVER make required fields optional (or vice versa) without spec verification +- ❌ NEVER bypass AdCP contract tests with `--no-verify` -**Known Test Agent Issues:** -- Auth failures with `create_media_buy` (see `docs/testing/postmortems/2025-10-04-test-agent-auth-bug.md`) -- `get_media_buy_delivery` expects `media_buy_id` (singular) instead of spec-compliant `media_buy_ids` (plural) -- When test agent is down, check: `fly logs --app test-agent` +**When schemas don't match:** +1. Check official spec: `https://adcontextprotocol.org/schemas/v1/media-buy/[operation].json` +2. Update Pydantic schema in `src/core/schemas.py` to match +3. Update cached schemas if official spec changed: Re-run schema validator +4. If spec is wrong, file issue with AdCP maintainers, don't work around it locally + +**Schema Update Process:** +```bash +# Check official schemas (they auto-download and cache) +pytest tests/e2e/test_adcp_compliance.py -v + +# Validate all Pydantic schemas match spec +pytest tests/unit/test_adcp_contract.py -v + +# If schemas are out of date, cached files are auto-updated on next run +# Commit any schema file changes that appear in schemas/v1/ +``` + +**Current Schema Version:** +- AdCP Version: 2.2.0 (official spec version) +- Schema Version: v1 +- Last Verified: 2025-10-22 +- Source: https://adcontextprotocol.org/schemas/v1/index.json +- Note: Internal "v2.4" references in codebase refer to feature evolution, not official spec versions + +--- + + +### PostgreSQL-Only Architecture +**🚨 DECISION**: This codebase uses **PostgreSQL exclusively**. We do NOT support SQLite. -## Critical Architecture Patterns +**Why:** +- Production uses PostgreSQL exclusively +- SQLite hides bugs (different JSONB behavior, no connection pooling, single-threaded) +- "No fallbacks - if it's in our control, make it work" (core principle) +- One database. One source of truth. No hidden bugs. -### AdCP Schema Compliance -**🚨 SINGLE SOURCE OF TRUTH**: Official AdCP spec at https://adcontextprotocol.org/schemas/v1/ +**What this means:** +- ✅ All database code assumes PostgreSQL (JSONB, connection pooling, etc.) +- ✅ All tests require PostgreSQL container (run via `./run_all_tests.sh ci`) +- ✅ Alembic migrations use PostgreSQL-specific syntax +- ❌ NO SQLite support - don't add it, don't test for it +- ❌ NO cross-database compatibility code - keep it simple -- All Pydantic schemas in `src/core/schemas.py` MUST match official spec exactly -- Validate: `pytest tests/unit/test_adcp_contract.py` -- Never add fields not in spec (common mistake: adding "convenience" fields) -- Never bypass pre-commit hooks with `--no-verify` +**If you see SQLite references:** +- Test files (`tests/smoke/`, `tests/unit/test_json_type.py`) - Legacy tests, ignore or delete +- Simulation tools (`tools/simulations/`) - Uses temp PostgreSQL, not SQLite +- Documentation - Outdated, needs removal -### PostgreSQL Only (No SQLite) -**Why**: SQLite hides bugs (different JSONB behavior, no connection pooling, single-threaded) +--- -- All tests require PostgreSQL: `./run_all_tests.sh ci` -- Alembic migrations use PostgreSQL-specific syntax -- Don't add cross-database compatibility code +### Schema Validation Modes (Environment-Based) +**🚨 CRITICAL**: Schema validation strictness changes based on `ENVIRONMENT` variable. -### Environment-Based Validation -**Why**: Strict validation breaks production when clients use newer schema versions +**The Problem:** +Strict schema validation (`extra="forbid"`) makes production fragile: +- Clients using newer schema versions get rejected +- Rolling updates require perfect coordination +- Forward compatibility breaks +- Production failures from harmless extra fields +**The Solution: Environment-Based Validation** ```bash -ENVIRONMENT=production # Lenient (extra="ignore") - forward compatible -ENVIRONMENT=development # Strict (extra="forbid") - catches bugs early +# .env +ENVIRONMENT=production # Lenient: extra="ignore" (forward compatible) +ENVIRONMENT=development # Strict: extra="forbid" (catches bugs early) +ENVIRONMENT=staging # Strict: extra="forbid" (catches bugs early) ``` -### Database Patterns +**Behavior:** +- **Production** (`ENVIRONMENT=production`): + - `extra="ignore"` - Unknown fields are silently dropped + - Clients can send future schema fields (forward compatible) + - Graceful degradation, no production failures + - Example: Client sends `adcp_version="1.8.0"` from v1.8 schema → accepted -**JSONType (mandatory):** -- Use `JSONType` for ALL JSON columns (never plain `JSON`) -- Why: Handles PostgreSQL JSONB properly, no manual `json.loads()` needed -- Implementation: `src/core/database/json_type.py` +- **Development/Staging** (default): + - `extra="forbid"` - Unknown fields raise validation errors + - Catches typos and bugs during development + - Enforces schema compliance in tests + - Example: Client sends `unknown_field="test"` → validation error + +**Implementation:** +All AdCP request/response models inherit from `AdCPBaseModel`: -**SQLAlchemy 2.0:** ```python -# ✅ CORRECT -stmt = select(Model).filter_by(field=value) -instance = session.scalars(stmt).first() +from src.core.schemas import AdCPBaseModel -# ❌ WRONG - deprecated -instance = session.query(Model).filter_by(field=value).first() +class CreateMediaBuyRequest(AdCPBaseModel): # Uses environment-aware validation + buyer_ref: str + packages: list[Package] + # ... fields +``` + +**Testing:** +```bash +# Test validation modes +uv run pytest tests/unit/test_schema_validation_modes.py -v + +# Production mode accepts extra fields +ENVIRONMENT=production pytest tests/unit/test_create_media_buy.py + +# Development mode rejects extra fields (default) +pytest tests/unit/test_create_media_buy.py ``` -**Integration tests:** +**When to Use Each Mode:** +- ✅ **Production**: Always use `ENVIRONMENT=production` (forward compatible) +- ✅ **Staging**: Use `ENVIRONMENT=staging` (catch issues before prod) +- ✅ **Development**: Default (no env var) (strict validation) +- ✅ **CI**: Default (strict validation catches bugs) + +**Why This Matters:** +1. **Forward Compatibility**: Clients using newer schemas don't break production +2. **Rolling Updates**: Can deploy server/client independently +3. **Development Safety**: Strict validation catches bugs in tests +4. **Production Stability**: No failures from harmless extra fields + +--- + +### Database JSON Fields Pattern (SQLAlchemy 2.0) +**🚨 MANDATORY**: All JSON columns MUST use `JSONType` for PostgreSQL JSONB handling. + +**The Solution: JSONType** +We have a custom `JSONType` TypeDecorator for PostgreSQL JSONB: + ```python -@pytest.mark.requires_db -def test_something(integration_db): # Use integration_db, not db_session - with get_db_session() as session: - # Real PostgreSQL database +# ✅ CORRECT - Use JSONType for ALL JSON columns +from src.core.database.json_type import JSONType + +class MyModel(Base): + __tablename__ = "my_table" + + # Use JSONType, not JSON + config = Column(JSONType, nullable=False, default=dict) + tags = Column(JSONType, nullable=True) + metadata = Column(JSONType) ``` -### MCP/A2A Shared Implementation -**Why**: Avoid code duplication between protocols +**❌ WRONG - Never use plain JSON type:** +```python +from sqlalchemy import JSON -All tools use shared `_tool_name_impl()`: +class MyModel(Base): + config = Column(JSON) # ❌ Will cause bugs! +``` + +**In Application Code:** ```python -# main.py -def _tool_impl(...) -> Response: - # Real implementation - ALL business logic here +# ✅ CORRECT - JSONType handles everything automatically +with get_db_session() as session: + model = session.query(MyModel).first() + + # Always receive dict/list, never string + assert isinstance(model.config, dict) + assert isinstance(model.tags, (list, type(None))) + + # No manual json.loads() needed! + config_value = model.config.get("key") + +# ❌ WRONG - Manual JSON parsing (old pattern) +import json +config = json.loads(model.config) if isinstance(model.config, str) else model.config +``` + +**Migration Strategy:** +1. **New code**: ALWAYS use `JSONType` for new JSON columns +2. **Existing code**: Convert to `JSONType` when you touch it +3. **No manual parsing**: Remove any `json.loads()` calls when converting + +**mypy + SQLAlchemy Plugin Compatibility:** +JSONType is fully compatible with mypy's SQLAlchemy plugin: + +```python +from typing import Optional +from sqlalchemy.orm import Mapped, mapped_column + +class MyModel(Base): + __tablename__ = "my_table" + + # mypy-compatible syntax (SQLAlchemy 2.0 style) + config: Mapped[dict] = mapped_column(JSONType, nullable=False, default=dict) + tags: Mapped[Optional[list]] = mapped_column(JSONType, nullable=True) +``` + +**Error Handling:** +JSONType uses fail-fast error handling: +- **Invalid JSON in database** → Raises `ValueError` (data corruption alert) +- **Non-dict/list types being stored** → Converts to `{}` with warning +- **Unexpected database types** → Raises `TypeError` + +This ensures data corruption is detected immediately, not hidden. + +**Current Status:** +- ✅ All 45 JSON columns in models.py use JSONType +- ✅ 30 comprehensive unit tests +- ✅ PostgreSQL fast-path optimization (~20% performance improvement) +- ✅ Production-ready with robust error handling + +**References:** +- Implementation: `src/core/database/json_type.py` +- Tests: `tests/unit/test_json_type.py` +- SQLAlchemy docs: [TypeDecorator](https://docs.sqlalchemy.org/en/20/core/custom_types.html#typedecorator) + +--- + +### MCP/A2A Shared Implementation Pattern +**🚨 MANDATORY**: All tools MUST use shared implementation to avoid code duplication. + +``` +CORRECT ARCHITECTURE: + MCP Tool (main.py) → _tool_name_impl() → [real implementation] + A2A Raw (tools.py) → _tool_name_impl() → [real implementation] + +WRONG - DO NOT DO THIS: + MCP Tool → [implementation A] + A2A Raw → [implementation B] ❌ DUPLICATED CODE! +``` + +**Pattern for ALL tools:** +1. **Implementation** in `main.py`: `def _tool_name_impl(...) -> ResponseType` + - Contains ALL business logic + - No `@mcp.tool` decorator + - Called by both MCP and A2A paths + +2. **MCP Wrapper** in `main.py`: `@mcp.tool() def tool_name(...) -> ResponseType` + - Thin wrapper with decorator + - Just calls `_tool_name_impl()` + +3. **A2A Raw Function** in `tools.py`: `def tool_name_raw(...) -> ResponseType` + - Imports and calls `_tool_name_impl()` from main.py + - Uses lazy import to avoid circular dependencies: `from src.core.main import _tool_name_impl` + +**Example:** +```python +# src/core/main.py +def _create_media_buy_impl(promoted_offering: str, ...) -> CreateMediaBuyResponse: + """Real implementation with all business logic.""" + # 600+ lines of actual implementation + return response @mcp.tool() -def tool(...) -> Response: - return _tool_impl(...) +def create_media_buy(promoted_offering: str, ...) -> CreateMediaBuyResponse: + """MCP tool wrapper.""" + return _create_media_buy_impl(promoted_offering, ...) -# tools.py -def tool_raw(...) -> Response: - from src.core.main import _tool_impl # Lazy import - return _tool_impl(...) +# src/core/tools.py +def create_media_buy_raw(promoted_offering: str, ...) -> CreateMediaBuyResponse: + """A2A raw function.""" + from src.core.main import _create_media_buy_impl # Lazy import + return _create_media_buy_impl(promoted_offering, ...) ``` -## Critical Gotchas +**Status of Current Tools:** +- ✅ `create_media_buy` - Uses shared `_create_media_buy_impl` +- ✅ `get_media_buy_delivery` - Uses shared `_get_media_buy_delivery_impl` +- ✅ `sync_creatives` - Uses shared `_sync_creatives_impl` +- ✅ `list_creatives` - Uses shared `_list_creatives_impl` +- ✅ `update_media_buy` - Uses shared `_update_media_buy_impl` +- ✅ `update_performance_index` - Uses shared `_update_performance_index_impl` +- ✅ `list_creative_formats` - Uses shared `_list_creative_formats_impl` +- ✅ `list_authorized_properties` - Uses shared `_list_authorized_properties_impl` -### Database Initialization Dependencies -**🚨 Products require CurrencyLimit + PropertyTag to exist first** +**All 8 AdCP tools now use shared implementations - NO code duplication!** + +## 🚨 CRITICAL REMINDERS + +### Deployment Architecture (Reference Implementation) +**NOTE**: This codebase can be hosted anywhere (Docker, Kubernetes, cloud providers, bare metal). The reference implementation uses: + +**THREE SEPARATE ENVIRONMENTS:** +- **Local Dev**: `docker-compose up/down` → localhost:8001/8080/8091 +- **Reference Sales Agent**: `fly deploy` → https://adcp-sales-agent.fly.dev + - This is OUR sales agent (publisher side) + - Hosted on Fly.io, auto-deploys from main branch +- **Test Buyer Agent**: https://test-agent.adcontextprotocol.org + - This is OUR test advertiser agent (buyer side) + - Also hosted on Fly.io by us + - Used for E2E testing of the complete AdCP flow + - When this is down, it affects our integration tests + - Check logs: `fly logs --app test-agent` (exact app name may vary) + +⚠ī¸ **All three are INDEPENDENT** - starting Docker does NOT affect production! + +**Your Deployment**: You can host this anywhere that supports: +- Docker containers (recommended) +- Python 3.11+ +- PostgreSQL (production and testing) +- We'll support your deployment approach as best we can + +**Known Test Agent Issues:** +- **`create_media_buy` auth failure** (2025-10-04): Rejects valid auth tokens. See [postmortem](docs/testing/postmortems/2025-10-04-test-agent-auth-bug.md) +- **`get_media_buy_delivery` parameter mismatch** (2025-10-04): Expects `media_buy_id` (singular) instead of spec-compliant `media_buy_ids` (plural) + +**When Test Agent is Down:** +- Check Fly.io logs first: `fly logs --app ` +- Check Fly.io status: `fly status --app ` +- Don't assume external infrastructure issue - we control both sides +- Check application logs before assuming network/external issues + +### Git Workflow - MANDATORY (Reference Implementation) +**❌ NEVER PUSH DIRECTLY TO MAIN** + +1. Work on feature branches: `git checkout -b feature/name` +2. Push branch and create PR: `gh pr create` +3. Wait for review and merge via GitHub UI +4. **Merging to main auto-deploys to reference production via Fly.io** + +## Project Overview + +Python-based AdCP V2.3 sales agent reference implementation with: +- **MCP Server** (8080): FastMCP-based tools for AI agents +- **Admin UI** (8001): Google OAuth secured web interface +- **A2A Server** (8091): Standard python-a2a agent-to-agent communication +- **Multi-Tenant**: Database-backed isolation with subdomain routing +- **Production Ready**: PostgreSQL, Docker deployment, health monitoring + +## Key Architecture Principles + +### 1. SQLAlchemy 2.0 Query Patterns - MANDATORY +**🚨 CRITICAL**: Use SQLAlchemy 2.0 patterns for all database queries. Legacy 1.x patterns are deprecated. +**Migration Status:** +- ✅ Stage 1 & 2 Complete: Infrastructure and helper functions migrated +- 🔄 In Progress: ~114 files with legacy patterns remain +- đŸŽ¯ Goal: All new code uses 2.0 patterns; convert legacy code as touched + +**Correct Pattern (SQLAlchemy 2.0):** ```python -# MUST create in this order: -1. Tenant -2. CurrencyLimit (at least USD) - needed for budget validation -3. PropertyTag (at least "all_inventory") - needed for property_tags array -4. Products (can now reference both) +from sqlalchemy import select + +# Single result +stmt = select(Model).filter_by(field=value) +instance = session.scalars(stmt).first() + +# Multiple results +stmt = select(Model).filter_by(field=value) +results = session.scalars(stmt).all() + +# With filter() instead of filter_by() +stmt = select(Model).where(Model.field == value) +instance = session.scalars(stmt).first() + +# Complex queries with joins +stmt = select(Model).join(Other).where(Model.field == value) +results = session.scalars(stmt).all() ``` -**Why**: -- Products validate budgets against currency limits -- AdCP spec requires products have `properties` OR `property_tags` (oneOf) -- Missing these causes "Must have at least one product" errors +**❌ WRONG - Legacy Pattern (SQLAlchemy 1.x):** +```python +# DO NOT USE - Deprecated in 2.0 +instance = session.query(Model).filter_by(field=value).first() +results = session.query(Model).filter_by(field=value).all() +``` + +**When to Migrate:** +1. **Always use 2.0 patterns for new code** - No exceptions +2. **Convert legacy code when touching it** - If you're editing a function with `session.query()`, convert it to `select()` + `scalars()` +3. **Helper functions available**: Use `get_or_404()` and `get_or_create()` from `database_session.py` (already migrated) -**Check in init scripts:** +**Common Conversions:** ```python -# Validate CurrencyLimit exists -stmt = select(CurrencyLimit).filter_by(tenant_id=tenant_id, currency_code="USD") -if not session.scalars(stmt).first(): - raise ValueError("Create CurrencyLimit before products") +# Pattern 1: Simple filter_by +- session.query(Model).filter_by(x=y).first() ++ stmt = select(Model).filter_by(x=y) ++ session.scalars(stmt).first() + +# Pattern 2: filter() with conditions +- session.query(Model).filter(Model.x == y).first() ++ stmt = select(Model).where(Model.x == y) ++ session.scalars(stmt).first() + +# Pattern 3: All results +- session.query(Model).filter_by(x=y).all() ++ stmt = select(Model).filter_by(x=y) ++ session.scalars(stmt).all() -# Validate PropertyTag exists -stmt = select(PropertyTag).filter_by(tenant_id=tenant_id, tag_id="all_inventory") -if not session.scalars(stmt).first(): - raise ValueError("Create PropertyTag before products") +# Pattern 4: Count +- session.query(Model).filter_by(x=y).count() ++ from sqlalchemy import func, select ++ stmt = select(func.count()).select_from(Model).where(Model.x == y) ++ session.scalar(stmt) ``` -### Admin UI Route Architecture -**Debugging tip**: Routes split between blueprints +**See Also:** +- PR #307 - Stage 1 & 2 migration examples +- [SQLAlchemy 2.0 Docs](https://docs.sqlalchemy.org/en/20/changelog/migration_20.html) + +### 2. AdCP Protocol Compliance - MANDATORY +**🚨 CRITICAL**: All client-facing models MUST be AdCP spec-compliant and tested. + +**Requirements:** +- Response models include ONLY AdCP spec-defined fields +- Use exact field names from AdCP schema +- Each model needs AdCP contract test in `tests/unit/test_adcp_contract.py` +- Use `model_dump()` for external responses, `model_dump_internal()` for database + +**When Adding New Models:** +1. Check AdCP spec at https://adcontextprotocol.org/docs/ +2. Add compliance test BEFORE implementing +3. Test with minimal and full field sets +4. Verify no internal fields leak + +**🚨 NEVER ADD FIELDS TO REQUEST/RESPONSE SCHEMAS WITHOUT SPEC VERIFICATION:** +- **Before adding ANY field to a Request/Response model**, verify it exists in the AdCP spec +- **Do NOT bypass pre-commit hooks with `--no-verify`** unless absolutely necessary +- **If you must use `--no-verify`**, manually run schema validation checks: + ```bash + # Run schema validation manually + .git/hooks/pre-commit + # Or specific checks: + pre-commit run verify-adcp-schema-sync --all-files + pre-commit run audit-required-fields --all-files + ``` +- **Example of WRONG approach**: Adding `creative_ids` to `CreateMediaBuyRequest` top-level + - ❌ NOT in AdCP spec - belongs in `Package.creative_ids` + - ✅ Correct: Use existing spec-compliant `Package.creative_ids` +**Common mistakes:** +- Adding "convenience" fields that seem useful but aren't in spec +- Assuming client's usage pattern means we should add fields +- Using `--no-verify` and not checking schema compliance manually + +See `docs/testing/adcp-compliance.md` for detailed test patterns. + +### 2. Admin UI Route Architecture +**⚠ī¸ DEBUGGING TIP**: Routes split between blueprints: - `settings.py`: POST operations for tenant settings - `tenants.py`: GET requests for tenant settings pages +Route mapping: ``` /admin/tenant/{id}/settings → tenants.py::tenant_settings() (GET) /admin/tenant/{id}/settings/adapter → settings.py::update_adapter() (POST) ``` -### No Quiet Failures -**🚨 ALWAYS fail loudly** +### 3. Multi-Tenancy & Adapters +- **Tenant Isolation**: Each publisher is isolated with own data +- **Principal System**: Advertisers have unique tokens per tenant +- **Adapter Pattern**: Base class with GAM, Kevel, Triton, Mock implementations +- **GAM Modular**: 250-line orchestrator delegating to specialized managers -```python -# ❌ WRONG - silent failure -if not self.supports_device_targeting: - logger.warning("Skipping device targeting...") - return # Silently doesn't fulfill contract! +### 4. Unified Workflow System +- **Single Source**: All work tracking uses `WorkflowStep` tables +- **No Task Models**: Eliminated deprecated `Task`/`HumanTask` (caused schema conflicts) +- **Dashboard**: Shows workflow activity instead of generic tasks -# ✅ CORRECT - explicit failure -if not self.supports_device_targeting and targeting.device_type_any_of: - raise TargetingNotSupportedException( - "Device targeting requested but not supported" - ) +### 5. Protocol Support +- **MCP**: FastMCP with header-based auth (`x-adcp-auth`) +- **A2A**: Standard `python-a2a` library (no custom protocol code) +- **Testing Backend**: Full AdCP testing spec implementation + +### 6. Push Notification (Webhook) Registration +**Both MCP and A2A support webhook registration:** + +- **MCP**: Pass `push_notification_config` parameter to `create_media_buy` +- **A2A**: Use `set_task_push_notification_config` endpoint (pushNotifications capability enabled) + +**Implementation Notes:** +- Parameter extraction follows A2A spec (snake_case attributes via Pydantic) +- Authentication format: `schemes` array + `credentials` string (HMAC-SHA256) +- Database persistence in `push_notification_configs` table +- Tenant setup script creates default USD currency limit (required for media buy creation) + +## Core Components + +``` +src/core/ + ├── main.py # FastMCP server + AdCP tools + ├── schemas.py # Pydantic models (AdCP-compliant) + └── database/ # Multi-tenant schema + +src/adapters/ + ├── base.py # Abstract adapter interface + ├── mock_ad_server.py + └── gam/ # Modular GAM implementation + +src/admin/ # Flask admin UI +src/a2a_server/ # python-a2a server implementation +``` + +## Configuration + +### Secrets (.env.secrets file - REQUIRED) +**🔒 Security**: All secrets MUST be in `.env.secrets` file (no env vars). + +```bash +# API Keys +GEMINI_API_KEY=your-key + +# OAuth (Admin UI) +GOOGLE_CLIENT_ID=your-id.apps.googleusercontent.com +GOOGLE_CLIENT_SECRET=your-secret +SUPER_ADMIN_EMAILS=user@example.com + +# GAM OAuth +GAM_OAUTH_CLIENT_ID=your-gam-id.apps.googleusercontent.com +GAM_OAUTH_CLIENT_SECRET=your-gam-secret + +# Approximated (Custom Domains) +APPROXIMATED_API_KEY=your-approximated-api-key +APPROXIMATED_PROXY_IP=37.16.24.200 # IP address of Approximated proxy cluster +APPROXIMATED_BACKEND_URL=adcp-sales-agent.fly.dev # Your backend server URL ``` -## Quick Commands +**Note**: +- Ports auto-configured by workspace setup script. +- `APPROXIMATED_PROXY_IP`: The IP address that clients should point their custom domains to via A record (your Approximated proxy cluster) +- `APPROXIMATED_BACKEND_URL`: Your actual backend server URL that Approximated proxies to + +### Database Schema +```sql +-- Core multi-tenant tables +tenants, principals, products, media_buys, creatives, audit_logs + +-- Workflow system (unified) +workflow_steps, object_workflow_mappings + +-- Legacy (deprecated) +-- tasks, human_tasks (DO NOT USE) +``` + +### Admin UI Settings Structure + +**Tenant Settings** (http://localhost:8001/tenant/{id}/settings): + +Settings are organized around clear conceptual buckets: + +**đŸĸ Account** (Who You Are) +- Organization name and subdomain +- Virtual host configuration +- Domain/email access control + +**đŸ–Ĩī¸ Ad Server** (Infrastructure) +- Adapter selection (GAM, Mock, Kevel) +- OAuth authentication +- Network codes and IDs +- Connection testing + +**📋 Policies & Workflows** (How You Operate) ⭐ **NEW** +- **Budget Controls**: Max daily budget +- **Naming Conventions**: Order/line item templates with live preview + - Works across all adapters (GAM, Mock, Kevel) + - Quick-start presets (Simple, Campaign-First, Detailed) + - Variable reference and validation +- **Approval Workflow**: Manual approval, human review +- **Features**: AXE signals, feature flags + +**📊 Inventory** (Ad Server Data) +- Sync inventory from ad server +- Browse ad units and placements + +**đŸ“Ļ Products** (Advertising Products) +- Product management and configuration + +**đŸ‘Ĩ Advertisers** (Principals) +- Advertiser/principal management +- Platform mappings + +**🔗 Integrations** (External Services) +- Slack webhooks +- Signals discovery agent +- API tokens + +**⚠ī¸ Danger Zone** (Destructive Operations) +- Delete tenant, reset data + +**Key Changes (October 2025):** +- Naming templates moved from Ad Server to Policies & Workflows +- Budget controls moved from Account to Policies & Workflows +- Feature flags moved from Account to Policies & Workflows +- Settings reduced from 11 to 8 sections +- Naming templates now work across all adapters + +## Common Operations + +### Running Locally +```bash +# Start all services +docker-compose up -d + +# View logs +docker-compose logs -f + +# Stop +docker-compose down +``` ### Testing ```bash -./run_all_tests.sh ci # Full suite with PostgreSQL (3-5 min, matches CI) -./run_all_tests.sh quick # Fast, no database (1 min) -pytest tests/unit/test_adcp_contract.py # AdCP compliance (run before commit) +# Recommended: Full test suite with PostgreSQL (matches CI/production) +./run_all_tests.sh ci + +# Fast iteration during development (skips database tests) +./run_all_tests.sh quick + +# Manual pytest commands +uv run pytest tests/unit/ # Unit tests only +uv run pytest tests/integration/ # Integration tests (needs database) +uv run pytest tests/e2e/ # E2E tests (needs database) + +# With coverage +uv run pytest --cov=. --cov-report=html + +# AdCP compliance (MANDATORY before commit) +uv run pytest tests/unit/test_adcp_contract.py -v ``` -### Development +### Database Migrations ```bash -docker-compose up -d # Start all services -docker-compose logs -f # View logs -uv run python migrate.py # Run migrations -pre-commit run --all-files # Check code quality +# Run migrations +uv run python migrate.py + +# Create new migration +uv run alembic revision -m "description" ``` -### Type Checking +**⚠ī¸ NEVER modify existing migrations after commit!** + +### Database Initialization Dependencies +**🚨 CRITICAL**: Products have implicit dependencies that MUST be satisfied before creation. + +**Dependency Chain:** +``` +Tenant → CurrencyLimit (required for budget validation) + → PropertyTag (required for property_tags array references) + → Products (require BOTH CurrencyLimit AND PropertyTag) +``` + +**Why These Dependencies Exist:** +1. **CurrencyLimit** (`currency_code="USD"`): Required for media buy budget validation + - Products are used in media buys which validate budgets against currency limits + - Missing currency limits cause "Must have at least one product" errors in E2E tests + - **Fix**: Always create at least one CurrencyLimit when creating a tenant + +2. **PropertyTag** (`tag_id="all_inventory"`): Required for property_tags array references + - Per AdCP v2.4 spec, products MUST have either `properties` OR `property_tags` (oneOf constraint) + - Most products use `property_tags=["all_inventory"]` as default + - Missing property tags cause referential integrity errors + - **Fix**: Always create at least one PropertyTag when creating a tenant + +**Validation in Init Scripts:** +The `scripts/setup/init_database_ci.py` script now validates these prerequisites: +```python +# 1. Check CurrencyLimit exists +stmt_currency = select(CurrencyLimit).filter_by(tenant_id=tenant_id, currency_code="USD") +currency_limit = session.scalars(stmt_currency).first() +if not currency_limit: + raise ValueError("Cannot create products: CurrencyLimit (USD) not found...") + +# 2. Check PropertyTag exists +stmt_tag = select(PropertyTag).filter_by(tenant_id=tenant_id, tag_id="all_inventory") +property_tag = session.scalars(stmt_tag).first() +if not property_tag: + raise ValueError("Cannot create products: PropertyTag 'all_inventory' not found...") +``` + +**When Creating New Tenants:** +1. Create Tenant +2. Create CurrencyLimit (at least USD) +3. Create PropertyTag (at least "all_inventory") +4. Create Products (can now safely reference currency and tags) + +**Failure Symptoms:** +- ❌ "Must have at least one product" in E2E tests +- ❌ Products created but budget validation fails +- ❌ Referential integrity errors on property_tags array + +## Development Best Practices + +### Code Style +- Use `uv` for dependencies +- Run pre-commit: `pre-commit run --all-files` +- Use type hints +- **🚨 NO hardcoded external system IDs** - use config/database +- **🛡ī¸ NO testing against production systems** + +### Type Checking with mypy +**🚨 MANDATORY**: When touching files, fix mypy errors in the code you modify. + +**Run mypy manually:** ```bash +# Check specific file uv run mypy src/core/your_file.py --config-file=mypy.ini + +# Check entire directory +uv run mypy src/core/ --config-file=mypy.ini + +# Check all source files +uv run mypy src/ --config-file=mypy.ini ``` -## Key Rules +**When modifying code:** +1. **Run mypy on files you change** - Fix errors introduced by your changes +2. **Fix nearby errors if easy** - Opportunistically improve type safety +3. **Use SQLAlchemy 2.0 Mapped[] annotations** for new ORM models: + ```python + from sqlalchemy.orm import Mapped, mapped_column + + class MyModel(Base): + # ✅ CORRECT - New SQLAlchemy 2.0 style + id: Mapped[int] = mapped_column(primary_key=True) + name: Mapped[str] = mapped_column(String(100)) + optional_field: Mapped[str | None] = mapped_column(nullable=True) + ``` + +**Common Fixes:** +- Add type hints to function signatures +- Use `| None` instead of `Optional[]` (Python 3.10+) +- Fix `Sequence[Model]` vs `list[Model]` return types +- Add missing imports -1. **Schema Compliance**: All client-facing models must match AdCP spec exactly -2. **No Quiet Failures**: Raise exceptions, don't silently skip features -3. **Test Before Commit**: Run unit tests + verify imports for ALL changes -4. **Fix Tests, Don't Skip**: Never use `skip_ci` or `--no-verify` to bypass failures -5. **PostgreSQL Only**: No cross-database compatibility code -6. **Shared Implementation**: MCP and A2A must call same `_impl()` function +**Current Status:** +- ✅ mypy installed with SQLAlchemy plugin +- ✅ Configuration in `mypy.ini` (lenient for incremental adoption) +- ⚠ī¸ ~1313 errors remaining (down from 2644) +- đŸŽ¯ Goal: Fix errors as we touch files, gradually improve type safety + +**Note**: Pre-commit hook disabled until error count is manageable. Run manually during development. + +### Schema Design +**Field Requirement Analysis:** +- Can this have a sensible default? +- Would clients expect this optional? +- Good defaults: `brief: str = ""`, `platforms: str = "all"` + +### Import Patterns +```python +# ✅ Always use absolute imports +from src.core.schemas import Principal +from src.core.database.database_session import get_db_session +from src.adapters import get_adapter +``` + +### Database Patterns +- Use context managers for sessions +- Explicit commits: `session.commit()` +- Use `attributes.flag_modified()` for JSONB updates + +### ⛔ NO QUIET FAILURES +**CRITICAL**: NEVER silently skip requested features. + +```python +# ❌ WRONG - Silent failure +if not self.supports_device_targeting: + logger.warning("Skipping...") + +# ✅ CORRECT - Explicit failure +if not self.supports_device_targeting and targeting.device_type_any_of: + raise TargetingNotSupportedException("Cannot fulfill buyer contract.") +``` ## Testing Guidelines +### Test Organization +``` +tests/ +├── unit/ # Fast, isolated (mock external deps only) +├── integration/ # Database + services (real DB, mock external APIs) +├── e2e/ # Full system tests +└── ui/ # Admin UI tests +``` + +### Database Test Fixtures - MANDATORY + +**🚨 CRITICAL**: Use the correct fixture based on test type. + +**Integration Tests** (tests/integration/): +```python +# ✅ CORRECT - Use integration_db fixture +@pytest.mark.requires_db +def test_something(integration_db): + """Integration test with real PostgreSQL database.""" + with get_db_session() as session: + # Your test code using real database + tenant = Tenant(...) + session.add(tenant) + session.commit() +``` + +**Unit Tests** (tests/unit/): +```python +# ✅ CORRECT - Mock database calls +def test_something(): + """Unit test with mocked database.""" + with patch('src.core.database.database_session.get_db_session') as mock_db: + # Your test code with mocked database + pass +``` + +**⚠ī¸ Common Mistakes:** + +```python +# ❌ WRONG - Don't use db_session in integration tests +def test_something(db_session): # This expects DATABASE_URL already set + tenant = Tenant(...) + db_session.add(tenant) # Will fail in CI + +# ❌ WRONG - Don't use @pytest.mark.requires_db in tests/unit/ +# Unit tests should mock the database, not use a real one +``` + +**When to use each:** +- **integration_db**: Integration tests that need real PostgreSQL (CI sets this up) +- **db_session**: Legacy fixture, being phased out - use integration_db instead +- **Mock**: Unit tests - mock get_db_session() for fast, isolated tests + +**Fixture Details:** +- `integration_db`: Creates isolated PostgreSQL database per test (via tests/integration/conftest.py) +- `db_session`: Expects DATABASE_URL to be set, returns session (via tests/conftest_db.py) +- Unit tests use auto-mock fixture that mocks get_db_session() automatically + +**Migration Path:** +- ✅ New integration tests: Always use `integration_db` +- ✅ Existing integration tests: Convert from `db_session` to `integration_db` when touched +- ✅ Unit tests with `@pytest.mark.requires_db`: Consider moving to tests/integration/ + +### Quality Enforcement +**🚨 Pre-commit hooks enforce:** +- Max 10 mocks per test file +- AdCP compliance tests for all client-facing models +- MCP contract validation (minimal params work) +- No skipped tests (except `skip_ci`) +- No `.fn()` call patterns + +### What Makes a Good Unit Test + +**✅ Good unit tests:** +- Test YOUR code's logic and behavior +- Use minimal mocking (only for external dependencies) +- Have clear, specific assertions about actual behavior +- Would catch real bugs if the implementation changed +- Test edge cases and error conditions +- Are fast (<100ms per test) + +**❌ Bad unit tests (DELETE THESE):** +- Test Python's built-in behavior (`assert True`, `assert 1+1==2`) +- Test data structures without testing any code (`assert dict["key"] == "value"`) +- Define functions inside tests that aren't used (`def validate_x(): ...`) +- Have generic names like `test_basic_functionality()` +- Just check that imports work without testing behavior + +**Examples:** + +```python +# ❌ BAD - Testing Python itself +def test_basic_functionality(): + assert True + +def test_list_operations(): + test_list = [1, 2, 3] + assert len(test_list) == 3 # This tests Python, not our code! + +# ✅ GOOD - Testing our code's behavior +def test_1x1_placeholder_accepts_any_creative_size(): + """1x1 placeholder should accept any creative size (wildcard behavior).""" + manager = GAMCreativesManager(mock_client, "advertiser_123", dry_run=True) + + asset = {"creative_id": "c1", "width": 728, "height": 90} + placeholders = {"pkg1": [{"size": {"width": 1, "height": 1}}]} + + errors = manager._validate_creative_size_against_placeholders(asset, placeholders) + assert errors == [] # This tests OUR business logic! +``` + +### Critical Testing Patterns +1. **MCP Tool Roundtrip**: Test with real Pydantic objects, not mock dicts +2. **AdCP Compliance**: Every client model needs contract test +3. **Integration over Mocking**: Use real DB, mock only external services +4. **Test What You Import**: If imported, test it's callable +5. **Test YOUR Code**: Don't test Python, test your implementation +6. **Never Skip or Weaken Tests**: Fix the underlying issue, never bypass with `skip_ci` or `--no-verify` +7. **Roundtrip Tests for Testing Hooks**: Every operation using `apply_testing_hooks` MUST have a roundtrip test + +**🚨 MANDATORY**: When CI tests fail, FIX THE TESTS PROPERLY. Skipping or weakening tests to make CI pass is NEVER acceptable. The tests exist to catch real issues - if they fail, there's a problem that needs fixing, not hiding. + +### Testing Hooks Roundtrip Pattern (MANDATORY) + +**Rule**: If an operation uses `apply_testing_hooks()`, it MUST have a roundtrip test. + +**Why**: Testing hooks add extra fields (`is_test`, `dry_run`, `test_session_id`, `response_headers`) that can break response reconstruction. + +**Pattern**: +```python +def test_{operation}_with_testing_hooks_roundtrip(): + """Test {Operation}Response survives apply_testing_hooks roundtrip.""" + # 1. Create valid response + response = {Operation}Response(field1="value", field2="value") + + # 2. Convert to dict + response_data = response.model_dump_internal() + + # 3. Apply testing hooks (adds extra fields) + testing_ctx = TestingContext(dry_run=True, test_session_id="test") + campaign_info = {"start_date": datetime.now(), "end_date": datetime.now(), "total_budget": 1000} + modified_data = apply_testing_hooks(response_data, testing_ctx, "{operation}", campaign_info) + + # 4. Filter out testing hook fields + valid_fields = {"field1", "field2", "field3"} # Only schema fields + filtered_data = {k: v for k, v in modified_data.items() if k in valid_fields} + + # 5. Reconstruct response - this MUST not raise validation error + reconstructed = {Operation}Response(**filtered_data) + + # 6. Verify reconstruction + assert reconstructed.field1 == "value" +``` + +**Examples**: +- `tests/integration/test_create_media_buy_roundtrip.py` +- `test_create_media_buy_response_survives_testing_hooks_roundtrip()` +- `test_get_media_buy_delivery_survives_testing_hooks_roundtrip()` + +**Enforcement**: Pre-commit hook `check-roundtrip-tests` verifies all operations using `apply_testing_hooks` have roundtrip tests. + +**When Adding Testing Hooks**: +1. Add `apply_testing_hooks` to your operation +2. Immediately write roundtrip test (before committing) +3. Pre-commit hook will fail if test is missing + +### Testing Workflow - MANDATORY for Refactoring + +**⚠ī¸ CRITICAL**: Pre-commit hooks can't catch import errors or integration issues. You MUST run the full test suite for refactorings. + +#### Before Committing Code Changes + **For ALL changes:** ```bash +# 1. Run unit tests (fast, always required) uv run pytest tests/unit/ -x -python -c "from src.core.tools import your_function" # Verify imports + +# 2. Verify imports work (catches missing imports) +python -c "from src.core.tools import get_products_raw" # Example +python -c "from src.core.main import _get_products_impl" # For impl functions ``` **For refactorings (shared implementation, moving code, import changes):** ```bash -uv run pytest tests/integration/ -x # REQUIRED - catches real bugs +# 3. Run integration tests (REQUIRED - catches real bugs) +uv run pytest tests/integration/ -x + +# 4. Run specific A2A/MCP tests if you changed those paths +uv run pytest tests/integration/test_a2a*.py -x +uv run pytest tests/integration/test_mcp*.py -x ``` -**Why unit tests alone aren't enough:** -- Unit tests pass with mocked imports (don't catch missing imports) -- Unit tests don't execute real code paths (don't catch integration bugs) -- Real example: Refactored `get_products_raw`, unit tests passed, integration tests caught missing import +**For critical changes (protocol changes, schema updates):** +```bash +# 5. Run full suite including e2e +uv run pytest tests/ -x +``` -**Pre-commit hooks check:** -- Code formatting (black, isort) -- Max 10 mocks per test file -- AdCP contract tests exist for all client models -- No skipped tests (except marked `skip_ci`) +#### Why This Matters -**Pre-push hook checks:** -- Migration heads only (fast, automatic) -- Tests run in CI, not in hook +**Unit tests alone are NOT enough because:** +- ✅ Unit tests pass with mocked imports (don't catch missing imports) +- ✅ Unit tests don't execute real code paths (don't catch integration bugs) +- ✅ Unit tests use fake data (don't catch validation issues) -## Project Structure +**Real example from this codebase:** +```python +# Refactored get_products_raw to use GetProductsRequest +# Unit tests: PASSED ✓ (they mock everything) +# Integration tests: FAILED ✗ (import GetProductsRequest was missing) +# CI caught it after 2 failed runs +``` +#### Pre-Push Hook + +**✅ MIGRATION CHECKS ONLY:** +The pre-push hook performs a fast migration head check. No tests run in the hook - tests run in CI. + +```bash +# Pre-push hook checks: +🔍 Checking for multiple migration heads... +✅ Migration heads OK +✅ Pre-push checks passed! + +💡 To run tests locally before pushing: + ./run_all_tests.sh quick # Fast (~1 min, no database) + ./run_all_tests.sh ci # Full (~3-5 min, with PostgreSQL) ``` -src/core/ # MCP server, schemas, database -src/adapters/ # GAM, Kevel, Mock ad server implementations - └── gam/ # Modular GAM (250-line orchestrator + managers) -src/admin/ # Flask admin UI (Google OAuth secured) -src/a2a_server/ # Agent-to-agent server (python-a2a) + +**🔧 Setup Pre-Push Hook:** +```bash +./scripts/setup/setup_hooks.sh ``` -## Documentation +**Philosophy:** +- **Pre-commit**: Code quality (formatting, linting) - fast, automatic +- **Pre-push**: Migration checks - fast, automatic +- **CI**: Full test suite - runs on GitHub Actions +- **Manual**: Developers can run tests locally when needed -**Detailed guides:** -- Architecture: `docs/ARCHITECTURE.md` -- Testing patterns: `docs/testing/` -- Setup: `docs/SETUP.md` -- Deployment: `docs/deployment.md` -- Security: `docs/security.md` +**Test Modes (Manual):** -## Adapter Pricing Support +**Quick Mode:** +- Fast validation: unit tests + integration tests (no database) +- Skips database-dependent tests (marked with `@pytest.mark.requires_db`) +- No PostgreSQL container required +- ~1 minute -**GAM**: CPM, VCPM, CPC, FLAT_RATE -- Auto line item type selection based on pricing + guarantees -- FLAT_RATE → SPONSORSHIP with CPD translation -- VCPM → STANDARD only (GAM restriction) +**CI Mode:** +- Starts PostgreSQL container automatically (postgres:15) +- Runs ALL tests including database-dependent tests +- Exactly matches GitHub Actions +- ~3-5 minutes + +**Command Reference:** +```bash +./run_all_tests.sh quick # Quick mode - fast, no PostgreSQL +./run_all_tests.sh ci # CI mode - full test suite with PostgreSQL +``` -**Mock**: All AdCP pricing models (CPM, VCPM, CPCV, CPP, CPC, CPV, FLAT_RATE) +**Why No Tests in Pre-Push?** +- Developers should be able to push quickly +- CI catches all test failures automatically +- Pre-commit already handles code quality +- Developers can manually run tests when they want +- Keeps workflow fast and uninterrupted -See `docs/ARCHITECTURE.md` for detailed pricing matrices. +See `docs/testing/` for detailed patterns and case studies. -## Configuration +## Pre-Commit Hooks -**Secrets** (`.env.secrets` - REQUIRED): ```bash -GEMINI_API_KEY=... -GOOGLE_CLIENT_ID=... -GOOGLE_CLIENT_SECRET=... -GAM_OAUTH_CLIENT_ID=... -GAM_OAUTH_CLIENT_SECRET=... -APPROXIMATED_API_KEY=... # Custom domains -APPROXIMATED_PROXY_IP=37.16.24.200 # Proxy cluster IP -APPROXIMATED_BACKEND_URL=adcp-sales-agent.fly.dev # Backend URL +# Run all checks +pre-commit run --all-files + +# Specific checks +pre-commit run no-excessive-mocking --all-files +pre-commit run adcp-contract-tests --all-files ``` -**Database schema:** -```sql --- Core multi-tenant -tenants, principals, products, media_buys, creatives, audit_logs +**When hooks fail**: Fix the issue, don't bypass with `--no-verify`. --- Workflow system (unified) -workflow_steps, object_workflow_mappings +## Adapter Pricing Model Support + +### GAM Adapter +**Supported Pricing Models**: CPM, VCPM, CPC, FLAT_RATE + +**✅ FULLY IMPLEMENTED**: End-to-end pricing support with automatic line item type selection, dynamic cost type assignment, and goal unit configuration. --- DEPRECATED (don't use) -tasks, human_tasks +#### Pricing Model Details + +| AdCP Model | GAM Cost Type | Line Item Types | Goal Unit Type | Use Case | +|------------|---------------|-----------------|----------------|----------| +| **CPM** | CPM | All types (STANDARD, SPONSORSHIP, NETWORK, PRICE_PRIORITY, BULK, HOUSE) | IMPRESSIONS | Cost per 1,000 impressions - most common | +| **VCPM** | VCPM | STANDARD only | VIEWABLE_IMPRESSIONS | Cost per 1,000 viewable impressions - viewability-based | +| **CPC** | CPC | STANDARD, SPONSORSHIP, NETWORK, PRICE_PRIORITY | CLICKS | Cost per click - performance-based | +| **FLAT_RATE** | CPD (internal) | SPONSORSHIP | IMPRESSIONS | Fixed campaign cost - internally translates to CPD (total / days) | + +**Not Supported**: CPCV, CPV, CPP (GAM API limitations - video completion and GRP metrics not available) + +**Note**: CPD (Cost Per Day) is a GAM cost type but NOT exposed as an AdCP pricing model. It's used internally to translate FLAT_RATE pricing. + +**Implementation Status**: +- ✅ Pricing validation at adapter level +- ✅ Automatic line item type selection based on pricing + guarantees +- ✅ Dynamic cost type assignment (CPM, VCPM, CPC, CPD) +- ✅ Dynamic goal unit types (IMPRESSIONS, VIEWABLE_IMPRESSIONS, CLICKS) +- ✅ FLAT_RATE → CPD rate calculation (total_budget / campaign_days) +- ✅ Comprehensive unit tests (22 tests in `test_gam_pricing_compatibility.py`) +- ✅ Integration tests (6 tests in `test_gam_pricing_models_integration.py`) + +#### Line Item Type Selection + +GAM adapter **automatically selects** the appropriate line item type based on: +1. **Pricing model** (FLAT_RATE → SPONSORSHIP, VCPM → STANDARD, others → based on delivery guarantee) +2. **Delivery guarantee** (guaranteed_impressions → STANDARD, else PRICE_PRIORITY) +3. **Product override** (implementation_config.line_item_type, validated for compatibility) + +**Automatic Selection Logic**: +- FLAT_RATE pricing → SPONSORSHIP line item (priority 4) with CPD translation +- VCPM pricing → STANDARD line item (priority 8) - VCPM only works with STANDARD +- Guaranteed CPM/CPC → STANDARD line item (priority 8) +- Non-guaranteed CPM/CPC → PRICE_PRIORITY line item (priority 12) + +**Manual Override** (via product configuration): +```json +{ + "implementation_config": { + "line_item_type": "NETWORK", // Override default selection + "cost_type": "CPC", // Must be compatible with line_item_type + // ... other config + } +} ``` -## Git Workflow +**Validation**: Incompatible pricing + line item type combinations are rejected with clear error messages. + +#### Compatibility Matrix + +| Line Item Type | Supported Pricing | Priority | Guaranteed | +|----------------|------------------|----------|------------| +| STANDARD | CPM, CPC, VCPM | 8 | ✅ Yes | +| SPONSORSHIP | CPM, CPC, CPD | 4 | ✅ Yes | +| NETWORK | CPM, CPC, CPD | 16 | ❌ No | +| PRICE_PRIORITY | CPM, CPC | 12 | ❌ No | +| BULK | CPM only | 12 | ❌ No | +| HOUSE | CPM only | 16 | ❌ No (filler) | + +**Source**: Google Ad Manager API v202411 CostType specification + +### Mock Adapter +**Supported**: All AdCP pricing models (CPM, VCPM, CPCV, CPP, CPC, CPV, FLAT_RATE) +- Both fixed and auction pricing +- All currencies +- Simulates appropriate metrics per pricing model +- Used for testing and development + +--- + +## Deployment + +### Hosting Options +This application can be hosted anywhere: +- **Docker** (recommended) - Works on any Docker-compatible platform +- **Kubernetes** - Full k8s manifests supported +- **Cloud Providers** - AWS, GCP, Azure, DigitalOcean, etc. +- **Bare Metal** - Direct Python deployment with systemd/supervisor +- **Platform Services** - Fly.io, Heroku, Railway, Render, etc. + +See `docs/deployment.md` for platform-specific guides. + +### Reference Implementation Deployment (Fly.io) +**🚨 For reference implementation maintainers: Fly.io auto-deploys from main branch** + +```bash +# Check status +fly status --app adcp-sales-agent -1. Create branch: `git checkout -b feature/name` -2. Make changes, test locally -3. Push and create PR: `gh pr create` -4. Wait for review and merge via GitHub UI -5. **Merging to main auto-deploys to Fly.io production** +# View logs +fly logs --app adcp-sales-agent -**❌ Never push directly to main** +# Manual deploy (rarely needed) +fly deploy --app adcp-sales-agent + +# Update secrets +fly secrets set GEMINI_API_KEY="key" --app adcp-sales-agent +``` + +### Deployment Checklist (General) +1. ✅ Create feature branch +2. ✅ Test locally: `docker-compose up` +3. ✅ Run tests: `uv run pytest` +4. ✅ Check migrations: `uv run python migrate.py` +5. ✅ Create Pull Request (if contributing to reference implementation) +6. ✅ Deploy to your environment +7. ✅ Monitor logs and health endpoints +8. ✅ Verify database migrations ran successfully + +## Documentation + +For detailed information, see: +- **Architecture**: `docs/ARCHITECTURE.md` +- **Setup**: `docs/SETUP.md` +- **Development**: `docs/DEVELOPMENT.md` +- **Testing**: `docs/testing/` +- **Troubleshooting**: `docs/TROUBLESHOOTING.md` +- **Security**: `docs/security.md` +- **A2A Implementation**: `docs/a2a-implementation-guide.md` +- **Deployment**: `docs/deployment.md` + +## Quick Reference + +### MCP Client Example +```python +from fastmcp.client import Client, StreamableHttpTransport + +headers = {"x-adcp-auth": "token"} +transport = StreamableHttpTransport(url="http://localhost:8080/mcp/", headers=headers) +client = Client(transport=transport) + +async with client: + products = await client.tools.get_products(brief="video ads") + result = await client.tools.create_media_buy(product_ids=["prod_1"], ...) +``` + +### A2A Server Pattern +```python +# ✅ ALWAYS use create_flask_app() for A2A servers +from python_a2a.server.http import create_flask_app +app = create_flask_app(agent) # Provides all standard endpoints +``` + +### Admin UI Access +```bash +# Local: http://localhost:8001 +# Reference Production: https://admin.sales-agent.scope3.com +# Your Production: Configure based on your hosting setup +``` -## Common Mistakes to Avoid +## Support -1. **Adding non-spec fields to AdCP schemas** - Always verify against official spec first -2. **Using SQLite for tests** - Always use PostgreSQL -3. **Creating products without CurrencyLimit/PropertyTag** - Create dependencies first -4. **Duplicating MCP/A2A code** - Use shared `_impl()` functions -5. **Silent failures** - Always raise exceptions for unsupported features -6. **Using `session.query()`** - Use SQLAlchemy 2.0 `select()` + `scalars()` -7. **Using plain `JSON` type** - Use `JSONType` for all JSON columns -8. **Unit tests only for refactoring** - Run integration tests too -9. **Bypassing hooks with `--no-verify`** - Fix the issue, don't bypass -10. **Using `ENVIRONMENT=production` locally** - Use development mode for strict validation +- Check documentation in `/docs` +- Review test examples in `/tests` +- Consult adapter implementations in `/src/adapters` diff --git a/README.md b/README.md index 2088c3a3c..95c3d8687 100644 --- a/README.md +++ b/README.md @@ -236,80 +236,6 @@ We welcome contributions! Please see our [Development Guide](docs/DEVELOPMENT.md - Code style guidelines - Creating pull requests -### Commitizen for Version Management - -This project uses [Commitizen](https://commitizen-tools.github.io/commitizen/) for automated version management and changelog generation. - -**Commit Message Format:** - -All commits must follow the [Conventional Commits](https://www.conventionalcommits.org/) format: - -``` -(): - - - -