fix: Replace incorrect Alembic migration with proper initial schema by bokelley · Pull Request #10 · prebid/salesagent

bokelley · 2025-07-31T19:50:13Z

Summary

Fixed database initialization issues in Conductor workspaces by replacing broken Alembic migration
The previous migration was trying to ALTER non-existent tables, causing startup failures
Created proper initial migration that CREATEs all required tables from scratch

Test plan

Tested in Conductor workspace with fresh PostgreSQL database
All services start successfully (PostgreSQL, MCP server, Admin UI)
Database tables are created properly
No migration errors during startup

🤖 Generated with Claude Code

The previous migration (e2213dba0c9f) was incorrectly trying to ALTER non-existent tables. This commit replaces it with a proper initial migration that CREATEs all required tables. Changes: - Remove broken migration that assumed tables already existed - Add new initial_schema migration that creates all tables from scratch - Fix database initialization issues in Conductor workspaces This ensures clean database setup for new deployments. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

- Implement flattened asset structure removing nested requirements - Add asset_id field for unique asset identification within formats - Add required boolean field to each asset - Update all 5 foundational formats to use new asset-based structure - Remove backward compatibility with legacy specs field - Update AI creative format service to generate asset-based formats - Add validation tests for new format structure This aligns our implementation with the AdCP spec changes for cleaner, more structured creative format definitions that orchestrators can easily understand and map uploaded content to. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

…-spec-updates feat: Update creative formats to match AdCP spec PR #10

fix: Replace incorrect Alembic migration with proper initial schema

- Implement flattened asset structure removing nested requirements - Add asset_id field for unique asset identification within formats - Add required boolean field to each asset - Update all 5 foundational formats to use new asset-based structure - Remove backward compatibility with legacy specs field - Update AI creative format service to generate asset-based formats - Add validation tests for new format structure This aligns our implementation with the AdCP spec changes for cleaner, more structured creative format definitions that orchestrators can easily understand and map uploaded content to. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

…-spec-updates feat: Update creative formats to match AdCP spec PR #10

Address all 39 findings from inspect_bdd_steps.py across 6 step files: uc002_create_media_buy.py (10 findings): - Assert packages exist instead of silent if-guard skips (#1, prebid#4, prebid#6, prebid#7) - Strengthen success path with media_buy_id verification (#2) - Add DB verification after seller rejection (#3) - Re-commit creative.data mutation to DB (prebid#5) - Document proposal SPEC-PRODUCTION GAP with implementation notes (prebid#8, prebid#9, prebid#10) uc003_update_media_buy.py (4 findings): - Add max_daily_package_spend comparison when tenant config available (prebid#12) - Assert tenant exists for creative status validation (prebid#13) - Validate placement_ids non-empty and against product config (prebid#14) uc019_query_media_buys.py (5 findings): - Verify principal_id consistency in Given steps (prebid#18, prebid#20) - Validate date parsing in given_today_is (prebid#19) - Strengthen adapter/snapshot setup documentation (prebid#21, prebid#22) uc026_package_media_buy.py (2 findings): - Verify pricing_options content matches step parameter (prebid#27) - Verify format_ids content matches step parameter (prebid#28) then_error.py (2 findings): - Fix guard logic: allow Exception subclasses with .recovery (prebid#34) - Use _get_error_dict for consistent field extraction (prebid#35) then_media_buy.py (2 findings): - Explicit error path in status check (no silent fallthrough) (prebid#36) - Clarify seller event type assertions (prebid#37) Remaining 14 findings use correct SPEC-PRODUCTION GAP xfail pattern (try assertion first, xfail only on known gap with descriptive message).

…it reports Updates the three async-audit reports whose findings were superseded by the 2026-04-11 deep-think resolutions of Decisions 1, 7, and 9. The audit reports are committed deliverables (3e0afa0 / d895793) and remain the canonical record of the parallel-Opus audit round, so overwriting them silently would lose traceability of how the original findings evolved into the resolved decisions. Instead this commit adds reversal banners and corrected scope while preserving the original audit text inline. agent-a-scope-audit.md (56 net lines): §2.5 services row "background_sync_service.py" — replaces "+80 LOC full async conversion / MEDIUM risk" with "+200 LOC new module +30 LOC sync swap / HIGH risk (mitigated)" and names Decision 9's sync-bridge module path, pool sizing (2+3, 600s timeout), Spike 5.5 validation, and the structural guard allowlist. §2.5 services subtotal — adjusts "~+460 LOC" to "~+610 LOC" with the +150 LOC delta breakdown for background_sync_db.py + sync-bridge call site rewiring. §2.6 adapters intro — adds the Decision 1 Path B preamble explaining why the original full-async +345 LOC plan with the cascading async/ await contagion is no longer the plan. Names the 18 call sites in src/core/tools/*.py + 1 in operations.py:252, the dual session factory in database_session.py, the AuditLogger split, the anyio limiter tune to 80, and the structural guard. Points to foundation-modules.md §11.14 and async-pivot-checkpoint.md §3 "Adapters (Decision 1 Path B)" for full target state. Marks the old table "kept for traceability" with stale LOC deltas. §2.6 corrected scope table — new 11-row table at the end of §2.6 showing every adapter file's Decision-1 disposition (most stay sync, 0-15 LOC swap), the new file additions (database_session.py dual factory +60, AuditLogger split +25, lifespan threadpool tune +5, structural guard +120), and the corrected ~+385 LOC subtotal vs the original ~+345 LOC. Includes the "why Path B over full async" 4-bullet rationale (googleads sync-only, 4 of 5 adapters use requests, AdCP surface unchanged, threadpool capacity at 80 sufficient for ~50 concurrent media buys). §7 open questions — replaces all 9 questions with their resolved answers. Each question is preserved verbatim but the answer is rewritten to point to the resolution: D1 Path B (with foundation- modules §11.14 + checkpoint §3 cross-refs), D2 Audit 06 OVERRULE (KEEP DatabaseConnection), D3 custom factory-boy shim, D4 not a blocker (mechanical Wave 4), D5 Audit 06 SUBSTITUTE (delete database_schema.py + RuntimeError on product_pricing.py latent hotspot), D6 Decision 9 correction (3 consumer sites, not zero), D7 ContextManager refactor + Spike 4.5 + structural guard, D8 already correct (just async I/O upgrades), D9 sync-bridge + Spike 5.5 + structural guard. Adds a "LEDGER CLOSED" banner at the top. agent-b-risk-matrix.md (150 net lines): Section 1 risk matrix table — inserts new row "Risk #7.5: Background sync long-session multi-hour failure (Decision 9, 2026-04-11)" with H severity, Spike 5.5 detection, ~200 LOC sync-bridge effort, soft-blocker fallback to Option A asyncio-task-with-checkpointing. Section 2 per-risk deep dive — new "Risk #7.5 — Background sync long-session multi-hour failure" subsection between Risk #7 and Risk #8 (~110 lines). Six-part deep dive (root cause / detection / mitigation / fallback / AdCP compliance / point of no return) covering all three failure modes (pool_recycle rotation, identity map growth, Fly.io TCP keepalive expiry), the four Spike 5.5 test cases verbatim, the pool math across all three engines (60 peak within PG max_connections=100), and the shutdown ordering (request_shutdown → wait_for_shutdown(30s) → dispose_sync_bridge → engine.dispose). Names the v2.1+ sunset target (phase-per-session async refactor with sync_jobs table checkpointing). Risk #19 (DatabaseManager class) — adds RESOLVED banner at the top naming Decision 7 (the class is deleted entirely as part of the ContextManager refactor; only ContextManager subclassed it). Updates the mitigation section to "delete (Decision 7)" instead of "convert to async with __aenter__/__aexit__". Risk #28 (audit_logger module-level singleton) — adds PARTIALLY RESOLVED banner naming Decision 1 (Path B). Replaces the "naive convert all callers to async" mitigation with the _log_operation_sync + async public wrapper split. Includes the complete corrected code (~30 lines) showing how adapter code calls the sync internal directly while async code uses the threadpool- forwarding public wrapper, with both writing to the same audit_logs table via different session factories. agent-f-nonsurface-inventory.md (101 net lines): §1.1 (psycopg2 → asyncpg swap) — adds CORRECTED 2026-04-11 PARTIAL REVERSAL banner naming the three retained sync-psycopg2 paths (Decision 2 db_config pre-uvicorn, Decision 1 Path B sync factory, Decision 9 sync-bridge). Updates the "Change required" list to be additive (asyncpg ALONGSIDE, not replacing) and names the new structural guard test_architecture_no_runtime_psycopg2.py with its allowlist (db_config.py + database_session.py + background_sync_db.py). §1.2 (libpq build-time deps) — adds REVERSAL banner. Action F1.2.1 "remove libpq-dev + libpq5" is reversed; both stay in the Dockerfile to keep the door open for v2.1+ swap to source-built psycopg2. Adjusts net image size savings ~80MB → ~75MB. §1.4 (DatabaseConnection delete) — adds the most important reversal banner naming Audit 06 Decision 2 OVERRULE. Updates the callers list from `:65,133` to `:84,135` (file lines moved). Explains why the original "Option D delete + asyncio.run wrapper" recommendation is structurally wrong (pre-uvicorn loop conflict with the eventual uvicorn loop). Reverses Action F1.4.1 to KEEP DatabaseConnection. Reverses Action F1.4.2 to KEEP the bootstrap scripts as sync. Includes the structural guard allowlist code block (Python literal). Marks the priority field "REVERSED" so future readers see at a glance that this finding's original recommendation was wrong. Top surprises section (top of file) — adds CORRECTIONS APPLIED 2026-04-11 banner pointing to §1.1, §1.2, §1.4 reversals. Strikes through the "MUST fix before Wave 4 merge" claims on findings #1 and #2. Updates finding #2's line numbers from `:65, 133` to `:84, 135`. Strikes through finding #10's "remove libpq" claim and adds the v2.1+ rationale. Go/no-go impact list — strikes through the now-non-blocker findings (#1, #2, #10) and notes that finding #4 (PG version skew) REMAINS a blocker (Spike 2 still runs against asyncpg even though psycopg2 stays). The MUST bucket effort estimate stays at ~3-4 days but the shape changes — less swap-and-fallout, more dual-engine implementation. Adds Spike 5.5 to the new pre-Wave-0 budget. These three files plus the previous 6 commits (15be0b80 ledger, 034b3fc + 0a7a36b execution-details, 03e0c02 checkpoint, 1e876af foundation-modules §11.14, f5961c5 implementation- checklist) complete the propagation of Decisions 1, 7, and 9 across every plan file in the .claude/notes/flask-to-fastapi/ folder. The async-audit directory was the last unfollowed-up reference; with this commit the deep-think resolution trail is complete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…eware (Option A) Rewrites execution-details.md §1089-1093 (assumption #10) from double- submit-cookie semantics to Option A CSRFOriginMiddleware: 7 Origin-scenario tests, OIDC transit-cookie exception, exempt-list covering all 3 OAuth callbacks + MCP/A2A/REST/.well-known paths. Strips adcp_csrf cookie reads and X-CSRF-Token header sets from TestClient fixture + all 3 integration test recipes (POST-redirect-GET, AJAX JSON, file upload). Rewrites the Playwright assertion set from double-submit tokens to Origin-header validation semantics. Fixes the CSRF exempt list across 3 files where /admin/auth/callback (nonexistent route) was listed instead of /admin/auth/google/callback: - execution-details.md risk table - implementation-checklist.md §402 foundation module spec - adcp-safety.md §232 exempt-list example Adds /.well-known/ and /agent.json to the canonical exempt tuple (matching foundation-modules §11.7) in adcp-safety.md and implementation-checklist.md. Tightens folder CLAUDE.md Invariant 4 OAuth exception: async-def OAuth callbacks MUST wrap sync DB helpers in await run_in_threadpool(...); calling with get_db_session() directly inside async-def is forbidden (would block the event loop for every concurrent request). Deletes stray agent-output preamble at top of execution-details.md. discipline: N/A - docs-only Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…uards (L0-01) Per .claude/notes/flask-to-fastapi/L0-implementation-plan-v2.md §L0-01, landing rows #1-#16 of the §5.5 Structural Guards Inventory plus guard #34 (no_werkzeug_imports, reassigned to L0-01 per v2 plan §7.1 RATIFIED). Guards (each with AST scanner + main test + planted-violation meta-fixture): - test_architecture_no_flask_imports.py (#1, Captured→shrink) - test_architecture_handlers_use_sync_def.py (#2, frozen carve-out) - test_architecture_no_async_db_access.py (#3) - test_architecture_middleware_order.py (#4, DORMANT) - test_architecture_exception_handlers_complete.py (#5, DORMANT) - test_architecture_csrf_exempt_covers_adcp.py (#6, DORMANT) - test_architecture_approximated_middleware_path_gated.py (#7, DORMANT) - test_architecture_admin_routes_excluded_from_openapi.py (#8, DORMANT) - test_architecture_admin_routes_named.py (#9, DORMANT) - test_architecture_admin_route_names_unique.py (#10, DORMANT) - test_architecture_no_module_scope_create_app.py (#11, Captured→shrink) - test_architecture_scheduler_lifespan_composition.py (#12) - test_architecture_a2a_routes_grafted.py (#13) - test_architecture_form_getlist_parity.py (#14, Captured→shrink) - test_architecture_no_module_level_engine.py (#15, Captured→shrink) - test_architecture_no_direct_env_access.py (#16, Captured→shrink) - test_architecture_no_werkzeug_imports.py (#34, Captured→shrink) Shared helpers in tests/unit/architecture/_ast_helpers.py (walk_py_files, read_allowlist, relpath) per the DRY invariant in CLAUDE.md. Captured→shrink baselines seeded: - allowlists/no_flask_imports.txt (40 files importing flask) - allowlists/no_werkzeug_imports.txt (6 files importing werkzeug) - allowlists/module_scope_create_app.txt (7 tests/*.py files) - allowlists/no_form_getlist.txt (3 admin routers) - allowlists/no_module_level_engine.txt (2 legacy gam services) - allowlists/no_direct_env_access.txt (121 path:line call sites) Meta-guard `test_structural_guard_allowlist_monotonic.py` (§5.5 row #28) enforcing allowlist monotonicity will land in a later L0 work item. DORMANT guards (scanner present; repo state trivially compliant until L0-04..L0-18 foundation modules land): #4, #5, #6, #7, #8, #9, #10. Each has a planted-violation fixture that DOES fire the scanner; dormancy is about the repo state, not scanner correctness. Guard #15 landed as a new guard (the sibling test_architecture_no_module_level_get_engine.py referenced in the plan does not exist in the pre-L0 state, so #15 is not redundant with it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…llow-up Addresses items #6, #12, #16, #17, #19, #20, #23, #24 from the 2026-05-20 review (small mechanical fixes; no architectural change): - #6 + #17: ``extract_error_info`` envelope branch now coerces ``recovery`` through ``_coerce_recovery()``, which validates the value against ``typing.get_args(RecoveryHint)`` instead of a hard-coded tuple duplicating the Literal. Both the envelope and legacy ToolError branches go through the same helper, so a future RecoveryHint extension is picked up automatically. - #12: ``_body_contains_builder_call`` docstring now correctly says "N-level transitive call analysis with cycle detection via ``seen``" (was "1-level transitive", but the implementation actually walks the full call graph with cycle detection). - #16: dropped the duplicate ``logger.warning`` in ``on_message_send``'s AdCPError handler — ``_handle_explicit_skill`` already logs the same error (with audit log + activity feed) before raising. Two log lines for the same failure was noise. - #19: ``_translate_to_tool_error`` now uses ``raise error`` (not bare ``raise``) on its passthrough branches so the ``NoReturn`` contract holds even when called outside an active ``except`` block. - #20: ``_handle_tool_error`` defensively copies ``e.envelope`` before returning to ``JSONResponse`` (preserves the envelope-builder's immutability contract — the dict is owned by the AdCPToolError instance and may be referenced elsewhere). - #23: ``_handle_tool_exception`` Context extraction switched from ``hasattr(arg, "tenant_id")`` to explicit ``isinstance(arg, (FastMCPContext, ToolContext))`` — the hasattr check matched any Pydantic model with a ``tenant_id`` field, treating request bodies as Contexts. - #24: hoisted ``from fastmcp.exceptions import ToolError`` to the top imports in ``src/app.py``; dropped the mid-file ``# noqa: E402``. Local quality: 4682 passed, 1 skipped, 20 xfailed. Deferred (separate scope): - #5 byte-identical test, #7 delete per-boundary wrappers (30+ call sites), #9 decorator extraction, #10/14/25 move AdCPToolError to neutral module, #11 fail_step caller (#1311 coordination), #13 CWD-relative paths, #15 dead legacy fallback, #18 already softened in b5c4356, #21 status_code keyword-only, #22 synthetic mutation.

Addresses three related items from the 2026-05-20 structural follow-up review on PR #1306: - #14: `_handle_tool_error` was defined in api_v1.py but called only from app.py via lazy import. Moved (renamed to public `handle_tool_error`) plus its helpers (`_build_error_code_to_status`, `_ERROR_CODE_TO_STATUS`) to src/core/tool_error_logging.py — the module that already hosts AdCPToolError and extract_error_info. Underscore prefix dropped per Konstantine's ask. - #25: app.py now imports `handle_tool_error` eagerly at the top instead of the lazy `from src.routes.api_v1 import _handle_tool_error` inside tool_error_handler. No circular-import risk after the move (tool_error_logging has no transitive dependency on app). - #10: REST module (api_v1.py) no longer imports AdCPToolError. The class stays in tool_error_logging.py (alongside the boundary translator functions that build it). Konstantine's literal suggestion was to move AdCPToolError to src/core/exceptions.py; this commit takes the alternative he listed (REST no longer imports the MCP-boundary type) because adding fastmcp to exceptions.py would couple the neutral exceptions module to MCP. Cleanup in api_v1.py: - Dropped imports of JSONResponse, ToolError, AdCPError, AdCPToolError, extract_error_info, build_two_layer_error_envelope. All moved with the function or no longer used after the move. Test updates: - tests/unit/test_error_boundary_translation.py: 10 lazy imports of `_handle_tool_error` updated to direct imports of `handle_tool_error` from tool_error_logging. Docstring references updated. Adjacent AdCPToolError + handle_tool_error imports merged onto one line. Local quality: 4682 passed, 1 skipped, 20 xfailed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

bokelley merged commit 80504a6 into main Jul 31, 2025
1 check passed

bokelley deleted the test-worktree-setup branch July 31, 2025 19:58

bokelley mentioned this pull request Aug 1, 2025

feat: Update creative formats to match AdCP spec PR #10 #13

Merged

5 tasks

bokelley added a commit that referenced this pull request Aug 1, 2025

Merge pull request #13 from adcontextprotocol/feature/creative-format…

a3929e1

…-spec-updates feat: Update creative formats to match AdCP spec PR #10

bokelley added a commit that referenced this pull request Sep 15, 2025

Merge pull request #10 from adcontextprotocol/test-worktree-setup

35a9b75

fix: Replace incorrect Alembic migration with proper initial schema

bokelley added a commit that referenced this pull request Sep 15, 2025

Merge pull request #13 from adcontextprotocol/feature/creative-format…

cecf485

…-spec-updates feat: Update creative formats to match AdCP spec PR #10

KonstantinMirin mentioned this pull request May 1, 2026

feat: AdCP storyboard compliance — gap analysis and roadmap #1247

Open

bokelley mentioned this pull request May 6, 2026

fix(pre-commit): no-tenant-config regex over-matches webhook event names #1279

Closed

2 tasks

ChrisHuie mentioned this pull request May 14, 2026

refactor: complete error-emission architecture — typed AdCPError → spec two-layer envelope across MCP/A2A/REST #1304

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Replace incorrect Alembic migration with proper initial schema#10

fix: Replace incorrect Alembic migration with proper initial schema#10
bokelley merged 1 commit into
mainfrom
test-worktree-setup

bokelley commented Jul 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

bokelley commented Jul 31, 2025

Summary

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant