diff --git a/docs/development/WORKFLOW.md b/docs/development/WORKFLOW.md index 2384d56..7dccf43 100644 --- a/docs/development/WORKFLOW.md +++ b/docs/development/WORKFLOW.md @@ -85,3 +85,16 @@ Each group's PR closes its own sub-issue: `Closes #` (fires the board's Done automation for that item). Once every sub-issue under the parent is closed, close the parent too — it's just a tracking issue at that point. + +## 6. Local lint enforcement before push + +`scripts/git-hooks/pre-push` mirrors each service's CI lint step +(`black --check`, `mypy`, `flake8`) locally, scoped to whichever +service(s) the push actually touches (a `shared/` change checks all four). +One-time setup: + +``` +git config core.hooksPath scripts/git-hooks +``` + +Bypass with `git push --no-verify` when intentionally needed. diff --git a/openspec/changes/audit-mixin-coverage-guard/.openspec.yaml b/openspec/changes/audit-mixin-coverage-guard/.openspec.yaml index b4b3ece..92dfce5 100644 --- a/openspec/changes/audit-mixin-coverage-guard/.openspec.yaml +++ b/openspec/changes/audit-mixin-coverage-guard/.openspec.yaml @@ -1,2 +1,7 @@ schema: spec-driven created: 2026-09-01 +priority: high +author: Arutsh +depends_on: + - audit-mixin-rollout-tier4 + - shared-feat-299-audit-mixin-rollout-tier3 diff --git a/openspec/changes/shared-feat-299-audit-mixin-rollout-tier3/tasks.md b/openspec/changes/shared-feat-299-audit-mixin-rollout-tier3/tasks.md index 95b44b2..5caa7ba 100644 --- a/openspec/changes/shared-feat-299-audit-mixin-rollout-tier3/tasks.md +++ b/openspec/changes/shared-feat-299-audit-mixin-rollout-tier3/tasks.md @@ -1,12 +1,12 @@ One task group = one GitHub ticket = one PR, merged before the next group starts. -## 1. ai service — depends on audit-mixin-auto-population being merged +## 1. ai service — depends on audit-mixin-auto-population being merged — Issue #301 -- [ ] 1.1 Add Alembic migration adding nullable `created_by`/`updated_by` to `AIAuditLog`, `AIPrompt`, `UserProviderKey`, `CustomerAiDefaults` (via `AuditColumnsMixin`, non-`id` PK), `PrivilegedAccessLog`. -- [ ] 1.2 Update each model class to inherit `AuditMixin`/`AuditColumnsMixin` as appropriate. -- [ ] 1.3 For `PrivilegedAccessLog`, check whether an existing actor/subject field already captures the acting user; if so, assert it matches the auto-populated `created_by` in a test rather than treating them as unrelated. -- [ ] 1.4 Add/update tests confirming `created_by` is populated on creation for each of the 5 models, and `updated_by` behaves per the model's mutability (populated on update for mutable models, stays `NULL` for the append-only `AIAuditLog`/`PrivilegedAccessLog`). -- [ ] 1.5 Run `services/ai`'s test suite clean; PR merged. +- [x] 1.1 Add Alembic migration adding nullable `created_by`/`updated_by` to `AIAuditLog`, `AIPrompt`, `UserProviderKey`, `CustomerAiDefaults` (via `AuditColumnsMixin`, non-`id` PK), `PrivilegedAccessLog`. +- [x] 1.2 Update each model class to inherit `AuditMixin`/`AuditColumnsMixin` as appropriate. +- [x] 1.3 For `PrivilegedAccessLog`, check whether an existing actor/subject field already captures the acting user; if so, assert it matches the auto-populated `created_by` in a test rather than treating them as unrelated. +- [x] 1.4 Add/update tests confirming `created_by` is populated on creation for each of the 5 models, and `updated_by` behaves per the model's mutability (populated on update for mutable models, stays `NULL` for the append-only `AIAuditLog`/`PrivilegedAccessLog`). +- [x] 1.5 Run `services/ai`'s test suite clean; PR merged. ## 2. chat service — depends on 1 diff --git a/services/ai/app/models/audit_log.py b/services/ai/app/models/audit_log.py index 46f2c6e..45268bf 100644 --- a/services/ai/app/models/audit_log.py +++ b/services/ai/app/models/audit_log.py @@ -4,15 +4,14 @@ from sqlalchemy.dialects.postgresql import JSON from sqlalchemy.orm import mapped_column, Mapped from app.models.base import Base +from shared.db.audit_mixin import AuditMixin import shared.db.type_decorators as t -class AIAuditLog(Base): +class AIAuditLog(Base, AuditMixin): __tablename__ = "ai_audit_logs" - id: Mapped[t.GUID] = mapped_column( - t.GUID(), primary_key=True, default=lambda: str(uuid.uuid4()) - ) + id: Mapped[uuid.UUID] = mapped_column(t.GUID(), primary_key=True, default=lambda: uuid.uuid4()) customer_id: Mapped[t.GUID] = mapped_column(t.GUID(), nullable=False, index=True) user_id: Mapped[t.GUID] = mapped_column(t.GUID(), nullable=False, index=True) prompt_version: Mapped[str] = mapped_column(String, nullable=False) diff --git a/services/ai/app/models/customer_ai_defaults.py b/services/ai/app/models/customer_ai_defaults.py index e6df571..893a4fa 100644 --- a/services/ai/app/models/customer_ai_defaults.py +++ b/services/ai/app/models/customer_ai_defaults.py @@ -4,10 +4,11 @@ from sqlalchemy.orm import mapped_column, Mapped from app.models.base import Base +from shared.db.audit_mixin import AuditColumnsMixin import shared.db.type_decorators as t -class CustomerAiDefaults(Base): +class CustomerAiDefaults(Base, AuditColumnsMixin): __tablename__ = "customer_ai_defaults" customer_id: Mapped[t.GUID] = mapped_column(t.GUID(), primary_key=True) diff --git a/services/ai/app/models/privileged_access_log.py b/services/ai/app/models/privileged_access_log.py index bcd4f5d..ebddb4c 100644 --- a/services/ai/app/models/privileged_access_log.py +++ b/services/ai/app/models/privileged_access_log.py @@ -5,17 +5,16 @@ from sqlalchemy.orm import Mapped, mapped_column from app.models.base import Base +from shared.db.audit_mixin import AuditMixin import shared.db.type_decorators as t -class PrivilegedAccessLog(Base): +class PrivilegedAccessLog(Base, AuditMixin): """Append-only — no update/delete path exists anywhere in the app.""" __tablename__ = "privileged_access_logs" - id: Mapped[t.GUID] = mapped_column( - t.GUID(), primary_key=True, default=lambda: str(uuid.uuid4()) - ) + id: Mapped[uuid.UUID] = mapped_column(t.GUID(), primary_key=True, default=lambda: uuid.uuid4()) actor_user_id: Mapped[t.GUID] = mapped_column(t.GUID(), nullable=False, index=True) customer_id: Mapped[t.GUID] = mapped_column(t.GUID(), nullable=False, index=True) method: Mapped[str] = mapped_column(String, nullable=False) diff --git a/services/ai/app/models/prompt.py b/services/ai/app/models/prompt.py index 262f53f..f7c6048 100644 --- a/services/ai/app/models/prompt.py +++ b/services/ai/app/models/prompt.py @@ -3,15 +3,14 @@ from sqlalchemy import Boolean, DateTime, String, Text from sqlalchemy.orm import mapped_column, Mapped from app.models.base import Base +from shared.db.audit_mixin import AuditMixin import shared.db.type_decorators as t -class AIPrompt(Base): +class AIPrompt(Base, AuditMixin): __tablename__ = "ai_prompts" - id: Mapped[t.GUID] = mapped_column( - t.GUID(), primary_key=True, default=lambda: str(uuid.uuid4()) - ) + id: Mapped[uuid.UUID] = mapped_column(t.GUID(), primary_key=True, default=lambda: uuid.uuid4()) name: Mapped[str] = mapped_column(String, nullable=False, index=True) version: Mapped[str] = mapped_column(String, nullable=False) is_active: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False) diff --git a/services/ai/app/models/user_provider_key.py b/services/ai/app/models/user_provider_key.py index bfe125a..f668c13 100644 --- a/services/ai/app/models/user_provider_key.py +++ b/services/ai/app/models/user_provider_key.py @@ -5,15 +5,14 @@ from sqlalchemy.orm import mapped_column, Mapped, relationship from app.models.base import Base +from shared.db.audit_mixin import AuditMixin import shared.db.type_decorators as t -class UserProviderKey(Base): +class UserProviderKey(Base, AuditMixin): __tablename__ = "user_provider_keys" - id: Mapped[t.GUID] = mapped_column( - t.GUID(), primary_key=True, default=lambda: str(uuid.uuid4()) - ) + id: Mapped[uuid.UUID] = mapped_column(t.GUID(), primary_key=True, default=lambda: uuid.uuid4()) user_id: Mapped[t.GUID] = mapped_column(t.GUID(), nullable=False, index=True) customer_id: Mapped[t.GUID | None] = mapped_column(t.GUID(), nullable=True, index=True) provider_id: Mapped[t.GUID] = mapped_column( diff --git a/services/ai/migrations/versions/017_tier3_audit_columns.py b/services/ai/migrations/versions/017_tier3_audit_columns.py new file mode 100644 index 0000000..edefc97 --- /dev/null +++ b/services/ai/migrations/versions/017_tier3_audit_columns.py @@ -0,0 +1,51 @@ +"""Add created_by/updated_by (audit-mixin-rollout-tier3, group 1) + +Revision ID: 017_tier3_audit_columns +Revises: 016_ai_provider_models +Create Date: 2026-09-20 00:00:00.000000 + +""" + +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + +from shared.db.type_decorators import GUID + +revision: str = "017_tier3_audit_columns" +down_revision: Union[str, Sequence[str], None] = "016_ai_provider_models" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + +# table -> extra AuditMixin/AuditColumnsMixin columns it doesn't already have, +# beyond created_by/updated_by (added to every table below). +_EXTRA_COLUMNS = { + "ai_audit_logs": ["updated_at"], + "ai_prompts": ["updated_at"], + "user_provider_keys": [], + "customer_ai_defaults": ["created_at"], + "privileged_access_logs": ["updated_at"], +} + + +def _column(name: str) -> sa.Column: + if name in ("created_at", "updated_at"): + return sa.Column(name, sa.DateTime(timezone=True), nullable=True) + return sa.Column(name, GUID(), nullable=True) + + +def upgrade() -> None: + for table, extra in _EXTRA_COLUMNS.items(): + op.add_column(table, _column("created_by")) + op.add_column(table, _column("updated_by")) + for name in extra: + op.add_column(table, _column(name)) + + +def downgrade() -> None: + for table, extra in _EXTRA_COLUMNS.items(): + op.drop_column(table, "created_by") + op.drop_column(table, "updated_by") + for name in extra: + op.drop_column(table, name) diff --git a/services/ai/tests/conftest.py b/services/ai/tests/conftest.py index d0dbc4c..86658cc 100644 --- a/services/ai/tests/conftest.py +++ b/services/ai/tests/conftest.py @@ -21,16 +21,28 @@ from main import app # noqa: E402 from app.models.base import Base # noqa: E402 from app.models.privileged_access_log import PrivilegedAccessLog # noqa: E402 +from app.models.audit_log import AIAuditLog # noqa: E402 +from app.models.prompt import AIPrompt # noqa: E402 +from app.models.user_provider_key import UserProviderKey # noqa: E402 +from app.models.customer_ai_defaults import CustomerAiDefaults # noqa: E402 +from app.models.ai_provider import AIProvider # noqa: E402 + +_DB_TABLES = [ + PrivilegedAccessLog.__table__, + AIAuditLog.__table__, + AIPrompt.__table__, + AIProvider.__table__, + UserProviderKey.__table__, + CustomerAiDefaults.__table__, +] @pytest.fixture def db(): - """Real in-memory sqlite session covering PrivilegedAccessLog — sync, - matching this sink's deliberately sync design (see - app/services/privileged_access_audit.py). Add tables here as more tests - need a real DB session for this service.""" + """Real in-memory sqlite session — sync, matching this service's sync + audit sinks. Add tables to _DB_TABLES as more tests need a real DB session.""" engine = create_engine("sqlite:///:memory:") - Base.metadata.create_all(engine, tables=[PrivilegedAccessLog.__table__]) + Base.metadata.create_all(engine, tables=_DB_TABLES) return sessionmaker(bind=engine)() diff --git a/services/ai/tests/test_tier3_audit_columns.py b/services/ai/tests/test_tier3_audit_columns.py new file mode 100644 index 0000000..8f6d756 --- /dev/null +++ b/services/ai/tests/test_tier3_audit_columns.py @@ -0,0 +1,211 @@ +"""audit-mixin-rollout-tier3 group 1: created_by/updated_by population for +AIAuditLog, AIPrompt, UserProviderKey, CustomerAiDefaults, PrivilegedAccessLog.""" + +import uuid +from datetime import datetime, timezone + +from app.models.audit_log import AIAuditLog +from app.models.prompt import AIPrompt +from app.models.user_provider_key import UserProviderKey +from app.models.customer_ai_defaults import CustomerAiDefaults +from app.models.privileged_access_log import PrivilegedAccessLog +from app.models.ai_provider import AIProvider +from shared.security.current_user_context import reset_current_user_id, set_current_user_id + + +def _now(): + return datetime.now(timezone.utc) + + +class TestAIAuditLogAppendOnly: + def test_created_by_populated_on_insert(self, db): + user_id = uuid.uuid4() + token = set_current_user_id(user_id) + try: + log = AIAuditLog( + customer_id=str(uuid.uuid4()), + user_id=str(uuid.uuid4()), + prompt_version="v1", + input_text="text", + provider="anthropic", + model="claude", + success=True, + duration_ms=1, + created_at=_now(), + ) + db.add(log) + db.commit() + finally: + reset_current_user_id(token) + + assert log.created_by == user_id + + def test_updated_by_stays_null_with_no_update_path(self, db): + log = AIAuditLog( + customer_id=str(uuid.uuid4()), + user_id=str(uuid.uuid4()), + prompt_version="v1", + input_text="text", + provider="anthropic", + model="claude", + success=True, + duration_ms=1, + created_at=_now(), + ) + db.add(log) + db.commit() + + assert log.updated_by is None + + +class TestAIPromptMutable: + def test_created_by_populated_on_insert(self, db): + user_id = uuid.uuid4() + token = set_current_user_id(user_id) + try: + prompt = AIPrompt( + name="excel_extraction", + version="v1", + system_prompt="sys", + user_template="tmpl", + created_at=_now(), + ) + db.add(prompt) + db.commit() + finally: + reset_current_user_id(token) + + assert prompt.created_by == user_id + + def test_updated_by_populated_on_update(self, db): + prompt = AIPrompt( + name="excel_extraction", + version="v1", + system_prompt="sys", + user_template="tmpl", + created_at=_now(), + ) + db.add(prompt) + db.commit() + + updater_id = uuid.uuid4() + token = set_current_user_id(updater_id) + try: + prompt.is_active = True + db.commit() + finally: + reset_current_user_id(token) + + assert prompt.updated_by == updater_id + + +class TestUserProviderKeyMutable: + def _make_provider(self, db): + provider = AIProvider(name="anthropic", display_name="Anthropic") + db.add(provider) + db.commit() + return provider + + def test_created_by_populated_on_insert(self, db): + provider = self._make_provider(db) + user_id = uuid.uuid4() + token = set_current_user_id(user_id) + try: + key = UserProviderKey( + user_id=str(uuid.uuid4()), + provider_id=provider.id, + created_at=_now(), + updated_at=_now(), + ) + db.add(key) + db.commit() + finally: + reset_current_user_id(token) + + assert key.created_by == user_id + + def test_updated_by_populated_on_update(self, db): + provider = self._make_provider(db) + key = UserProviderKey( + user_id=str(uuid.uuid4()), + provider_id=provider.id, + created_at=_now(), + updated_at=_now(), + ) + db.add(key) + db.commit() + + updater_id = uuid.uuid4() + token = set_current_user_id(updater_id) + try: + key.label = "renamed" + db.commit() + finally: + reset_current_user_id(token) + + assert key.updated_by == updater_id + + +class TestCustomerAiDefaultsMutable: + def test_created_by_populated_on_insert(self, db): + user_id = uuid.uuid4() + token = set_current_user_id(user_id) + try: + defaults = CustomerAiDefaults(customer_id=str(uuid.uuid4()), updated_at=_now()) + db.add(defaults) + db.commit() + finally: + reset_current_user_id(token) + + assert defaults.created_by == user_id + + def test_updated_by_populated_on_update(self, db): + defaults = CustomerAiDefaults(customer_id=str(uuid.uuid4()), updated_at=_now()) + db.add(defaults) + db.commit() + + updater_id = uuid.uuid4() + token = set_current_user_id(updater_id) + try: + defaults.platform_fallback_enabled = True + db.commit() + finally: + reset_current_user_id(token) + + assert defaults.updated_by == updater_id + + +class TestPrivilegedAccessLogAppendOnly: + def test_created_by_matches_existing_actor_field(self, db): + """actor_user_id already captures the acting user (task 1.3) — the + automatically-populated created_by should equal it, not diverge.""" + actor_id = uuid.uuid4() + token = set_current_user_id(actor_id) + try: + log = PrivilegedAccessLog( + actor_user_id=str(actor_id), + customer_id=str(uuid.uuid4()), + method="PUT", + path="/api/v1/ai/settings", + created_at=_now(), + ) + db.add(log) + db.commit() + finally: + reset_current_user_id(token) + + assert log.created_by == actor_id + assert str(log.actor_user_id) == str(actor_id) + + def test_updated_by_stays_null_with_no_update_path(self, db): + log = PrivilegedAccessLog( + actor_user_id=str(uuid.uuid4()), + customer_id=str(uuid.uuid4()), + method="GET", + path="/api/v1/ai/settings", + created_at=_now(), + ) + db.add(log) + db.commit() + + assert log.updated_by is None