feat(llc): board approval gates — GH#8214 by mrveiss · Pull Request #8458 · mrveiss/AutoBot-AI

mrveiss · 2026-05-23T10:01:03Z

Summary

ApprovalType enum (hire/strategy/budget_override/sprint_close) added to llc/models/enums.py
LLCApproval SQLAlchemy model in llc/models/approval.py with all required fields
Alembic migration 20260523_023 creating approvaltype + approvalstatus PG enums and llc_approvals table with compound indexes
ApprovalService in llc/services/approval.py with request_approval, decide, get_pending, and best-effort Redis pub/sub events on llc:approval_requested / llc:approval_decided
requires_approval(gate_type) decorator for gating async service methods; attaches __approval_gate__ metadata for introspection
API routes: POST /llc/approvals, GET /llc/approvals?company_id=..., POST /llc/approvals/{id}/decide
llc/api/__init__.py updated to include the new approvals router

Test plan

17 unit tests in llc/tests/test_approvals.py — all passing (pytest 17 passed in 0.28s)
Service layer: request, decide (approve/reject), get_pending, Redis unavailable survival
requires_approval decorator: valid approval passes, missing ID raises, wrong type raises, non-approved status raises, metadata attachment
Syntax check: python3 -m py_compile on all 5 new/changed files
Migration revision 20260523_023 with proper down_revision = "20260523_022" and checkfirst=True enum creation

Closes GH#8214

🤖 Generated with Claude Code

The previous commit put create_llc_work_items at 022 and llc_goals at 024, but create_llc_work_items has sa.ForeignKey("llc_goals.id") — PostgreSQL requires the referenced table to exist at CREATE TABLE time, so that order would fail with "relation llc_goals does not exist". Correct linear chain (respects FK dependencies): 021 (heartbeat_wakeup) — existing anchor 022 (llc_goals) — no FK deps, must come first 023 (organization_llc_extensions) — no FK deps on LLC tables 024 (create_llc_work_items) — FK to llc_goals.id, must come last Head is still 024; downstream PRs (#8457, #8458, #8459) still need down_revision = "20260523_024". Co-Authored-By: Paperclip <noreply@paperclip.ing>

mrveiss · 2026-05-23T10:26:40Z

Code Review — CHANGES REQUIRED

7 confirmed bugs found across security, correctness, and deployment categories. Do not merge until items 1–2 are resolved.

CRITICAL — deployment blockers

1. Duplicate Alembic revision ID
migrations/versions/20260523_023_create_llc_approvals.py:25

revision: str = "20260523_023"

20260523_023_organization_llc_extensions.py already exists on Dev_new_gui with the same revision string. After merge, alembic upgrade head fails with:

CommandError: Multiple revisions found for '20260523_023'

Fix: Use 20260523_025 (next free slot after 024_create_llc_work_items).

2. Stale down_revision creates a two-headed branch
migrations/versions/20260523_023_create_llc_approvals.py:26

down_revision: Union[str, None] = "20260523_022"

Current chain on Dev_new_gui is 022 → 023_org_ext → 024. Pointing back to 022 forks the chain — Alembic refuses upgrade head with "Multiple head revisions are present".

Fix: down_revision = "20260523_024" to append cleanly.

HIGH — security + correctness

3. requires_approval decorator has no tenant scope check
llc/services/approval.py:~580

The wrapper validates approval.type and approval.status but never checks that approval.company_id matches the operation's target company. A user in company A can pass a valid HIRE approval from company A to gate a @requires_approval(HIRE) action for company B → cross-tenant authorization bypass.

Fix: The decorator must compare approval.company_id against the intended target company.

4. decide() has no SELECT FOR UPDATE — concurrent-decide race
llc/services/approval.py:~132

Two concurrent calls both SELECT the same PENDING row, both pass the guard, last writer wins. Two contradictory Redis events fire for the same approval_id.

Fix: Add .with_for_update() to the SELECT in decide().

5. Redis publish fires before DB commit
llc/services/approval.py:~94, ~163

Both request_approval and decide call _publish() after flush() but still inside async with session.begin(). If commit subsequently fails, the DB rolls back but the Redis event was already broadcast.

Fix: Move _publish() outside the transaction (post-commit), or document as explicitly best-effort and note the ordering risk.

MEDIUM — HTTP 500 on invalid client input

6. request_approval — bare uuid.UUID() calls without try/except
llc/api/approvals.py:91–94

uuid.UUID(body.company_id) and uuid.UUID(body.requested_by_agent_id) are not guarded. Malformed UUID → ValueError → 500. list_pending and decide_approval both guard their UUID parses.

Fix: Type the Pydantic fields as uuid.UUID, or wrap in try/except ValueError → HTTPException(422).

7. decide_approval — ValueError for decided_by_agent_id escapes the try block
llc/api/approvals.py:135

uuid.UUID(body.decided_by_agent_id) is inside a try block that only catches ApprovalNotFoundError / ApprovalStateError — ValueError propagates as 500.

Fix: Same as #6.

Note: The goals_router concern was investigated and refuted — gh pr merge --squash uses 3-way merge so Dev_new_gui's goals_router addition would be preserved in the final result.

) After moving _publish calls out of session.begin(), tests that call the service directly must also call publish_requested/publish_decided explicitly to verify Redis events — mirrors the route-layer call pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

mrveiss · 2026-05-23T10:37:02Z

MVA-871 fixes applied ✅

All 7 bugs fixed across 2 commits on this branch:

Commit `d334c62` — 6 bugs

Migration renamed 20260523_023_create_llc_approvals.py → 20260523_026_create_llc_approvals.py; revision 20260523_026, down_revision 20260523_025 (slots after llc_agent_budgets which owns 025)
Cross-tenant auth bypass fixed — requires_approval now checks approval.company_id == target_company_id
Concurrent decide race fixed — decide() SELECT uses .with_for_update()
Bugs 4 & 5: ValueError → 422 — ApprovalRequest.company_id, requested_by_agent_id, ApprovalDecision.decided_by_agent_id typed as uuid.UUID in Pydantic models
Redis publish moved post-commit — publish_requested()/publish_decided() are now called by routes after session.begin() exits

Commit `4b5c2a8` — test fixes

All 17 tests pass — two publish-event tests updated to call the new explicit publish methods

17 passed in 0.17s

mrveiss · 2026-05-23T11:01:30Z

Re-review — 6/7 fixes confirmed, 1 merge-order constraint

All 7 originally reported bugs have been fixed:

✅ Migration revision ID unique: revision="20260523_027"
✅ down_revision="20260523_026" (correct — depends on PR feat(llc): immutable activity log — model, service, API, migration (GH#8216) #8457)
✅ decide() uses SELECT FOR UPDATE
✅ @requires_approval checks approval.company_id against target company
✅ UUID fields typed as uuid.UUID in Pydantic models → 422 on invalid input
✅ Redis publish moved post-commit (publish_requested / publish_decided called after session.begin() exits)

Merge-order constraint: This PR's migration has down_revision="20260523_026". Dev_new_gui currently ends at 025. PR #8457 (GH#8216, immutable activity log) adds revision 026.

PR #8457 must be merged first. Once #8457 is on Dev_new_gui, this PR can be squash-merged cleanly.

- Add ApprovalType enum (hire/strategy/budget_override/sprint_close) to enums.py - LLCApproval SQLAlchemy model (llc_approvals table) - Alembic migration 20260523_023: approvaltype + approvalstatus enums + table - ApprovalService: request_approval, decide, get_pending + Redis pub/sub events - requires_approval(gate_type) decorator for gating async methods - API routes POST/GET /llc/approvals, POST /llc/approvals/{id}/decide - 17 unit tests (all passing) covering service layer and decorator Co-Authored-By: Paperclip <noreply@paperclip.ing>

- Rename migration 20260523_023→026, down_revision 022→025 (fits after llc_agent_budgets which now occupies 025) - requires_approval decorator: verify approval.company_id matches operation's target company_id to block cross-tenant auth bypass - decide(): add .with_for_update() to prevent concurrent-decide race - ApprovalRequest/ApprovalDecision: type company_id, requested_by_agent_id, decided_by_agent_id as uuid.UUID so Pydantic returns 422 on invalid input - Move _publish calls out of session.begin() — publish_requested/ publish_decided are now called by the routes after the transaction commits Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

) After moving _publish calls out of session.begin(), tests that call the service directly must also call publish_requested/publish_decided explicitly to verify Redis events — mirrors the route-layer call pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Migration was revision=026/down=025 which collides with PR #8457 (activity log). After hotfix PR #8464 establishes chain ...→025(agent_budgets), approvals is next at 027. Co-Authored-By: Paperclip <noreply@paperclip.ing>

Both 026 (activity log) and 027 (approvals) had down_revision=025, which would create two Alembic heads on merge. Activity log (026) merges first, so approvals (027) chains off it: 025→026(activity_log)→027(approvals). Co-Authored-By: Paperclip <noreply@paperclip.ing>

github-actions · 2026-05-23T11:10:17Z

⚠️ SSOT Configuration Compliance: Violations Found

Metric	Count
Total Violations	7
SSOT Violations (high priority)	7
Other Violations	0

⚠️ 7 values have SSOT config equivalents!

These should be replaced with SSOT config imports:

Python:

from src.config.ssot_config import config
# Use: config.vm.main, config.port.backend, config.backend_url

TypeScript:

import config from '@/config/ssot-config'
// Use: config.vm.main, config.port.backend, config.backendUrl

📖 See SSOT_CONFIG_GUIDE.md for documentation.

…new_gui 028 (approvals, PR #8458) merged into Dev_new_gui after this branch was cut. Secrets was using revision=027/down=026 which collides with approvals. Linear chain: 025→026(activity_log)→027(approvals)→028(secrets). Co-Authored-By: Paperclip <noreply@paperclip.ing>

…17) (#8467) * feat(llc): company-scoped secrets — versioned, encrypted, access control (GH#8217) - LLCSecret SQLAlchemy model: id, company_id, name (unique per co.), encrypted value (LargeBinary), version (auto-increments on update), created_by_agent_id, created_at, updated_at, revoked_at (soft-delete). - HKDF-SHA256 key derivation: per-company Fernet key from LLC_SECRET_MASTER_KEY env var; cross-company decryption is cryptographically impossible. - Alembic migration 20260523_027 (down_revision=20260523_026). - SecretService extending LLCServiceBase: set/get/revoke/list; SecretNotFound raised for missing or revoked secrets; activity_log wired when injected. - API routes /llc/secrets/{company_id}: list (no values), POST set, GET /{name} (decrypted value on explicit request only), DELETE /{name} revoke. X-Agent-Company-Id header enforces per-company access control. - 11 unit tests covering encrypt/decrypt round-trips, version increment, revoked-secret reactivation, cross-company isolation, missing-key error. - Wired into llc.api and llc.models __init__.py. Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(migrations): rename secrets migration to 028/down=027, merge Dev_new_gui 028 (approvals, PR #8458) merged into Dev_new_gui after this branch was cut. Secrets was using revision=027/down=026 which collides with approvals. Linear chain: 025→026(activity_log)→027(approvals)→028(secrets). Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>

mrveiss mentioned this pull request May 23, 2026

hotfix(migrations): fix duplicate LLC Alembic revision IDs (20260523_022) #8460

Closed

mrveiss mentioned this pull request May 23, 2026

hotfix(migrations): fix 4 duplicate LLC Alembic revision IDs (20260523_022) #8464

Merged

mrveiss mentioned this pull request May 23, 2026

hotfix(migrations): fix 4 duplicate LLC Alembic revision IDs + FK chain (#8460 v2) #8466

Merged

mrveiss and others added 5 commits May 23, 2026 14:09

mrveiss force-pushed the issue-8214 branch from 921009c to f2d4173 Compare May 23, 2026 11:10

mrveiss merged commit 8699683 into Dev_new_gui May 23, 2026
1 check failed

mrveiss deleted the issue-8214 branch May 23, 2026 11:10

mrveiss mentioned this pull request May 23, 2026

feat(llc): Board approval gates — hire, strategy, budget override, sprint close #8214

Closed

10 tasks

mrveiss mentioned this pull request Sep 25, 2026

ci(migrations): the migration gate ran on an unpinned SQLAlchemy — fixed; the path-filter blind spot remains #17490

Closed

4 tasks

Navigation Menu

Repository navigation

feat(llc): board approval gates — GH#8214 - #8458

feat(llc): board approval gates — GH#8214#8458
mrveiss merged 5 commits into
Dev_new_guifrom
issue-8214

mrveiss commented May 23, 2026

Uh oh!

mrveiss commented May 23, 2026

Uh oh!

mrveiss commented May 23, 2026

Uh oh!

mrveiss commented May 23, 2026

Uh oh!

Uh oh!

github-actions Bot commented May 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mrveiss commented May 23, 2026

Summary

Test plan

Uh oh!

mrveiss commented May 23, 2026

Code Review — CHANGES REQUIRED

CRITICAL — deployment blockers

HIGH — security + correctness

MEDIUM — HTTP 500 on invalid client input

Uh oh!

mrveiss commented May 23, 2026

MVA-871 fixes applied ✅

Commit d334c62 — 6 bugs

Commit 4b5c2a8 — test fixes

Uh oh!

mrveiss commented May 23, 2026

Re-review — 6/7 fixes confirmed, 1 merge-order constraint

Uh oh!

Uh oh!

github-actions Bot commented May 23, 2026

⚠️ SSOT Configuration Compliance: Violations Found

⚠️ 7 values have SSOT config equivalents!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Commit `d334c62` — 6 bugs

Commit `4b5c2a8` — test fixes