Skip to content

fix(api): stop writing pre-013 investigation columns and remove approvals - #183

Merged
bordumb merged 2 commits into
mainfrom
claude/practical-chaum-bbb68b
Sep 27, 2026
Merged

bordumb merged 2 commits into
mainfrom
claude/practical-chaum-bbb68b

Conversation

@bordumb

@bordumb bordumb commented Sep 27, 2026

Copy link
Copy Markdown
Owner

Summary

Since 013_unified_investigation.sql, investigations only has id, tenant_id, alert, main_branch_id, outcome, created_at, created_by, root_hash plus a generated status. Four call sites still used the old schema and failed at runtime:

  1. Snapshot import (POST /investigations/import) wrote a value into the generated status column (GeneratedAlwaysError). It now leaves status to derive from outcome.

  2. EE spawn_investigation automation action queried investigations.issue_id and inserted status/source/metadata. It now:

    • builds a valid AnomalyAlert from the issue and starts the investigation through InvestigationStarterService (which inserts the row and starts the Temporal workflow);
    • links the issue via issue_investigation_runs (trigger_type='rule') and reuses an existing run if there is one;
    • takes the datasource from a datasource_id action param, or the tenant's only active datasource. Any other case fails the action, and so does a datasource from another tenant.

    ExecutionContext gains a required investigation_starter. Note: execute_actions has no production caller yet.

  3. AppDatabase.create_investigation: no callers; deleted.

  4. The approvals stack: deleted. It targeted the pre-013 approval_requests table, which 013 re-keyed on branch/snapshot ids that Temporal investigations never create, and nothing creates approval requests. Temporal's human-in-the-loop path is POST /investigations/{id}/input. Removed:

    • the /approvals routes;
    • the AppDatabase approval methods and update_investigation_status;
    • the dashboard pending_approvals stat (API + card);
    • the unused approval notification producers (service, email, Slack);
    • ContextReviewPage with its route, the approval link in notifications, and the settings toggles.

OpenAPI: only the approvals paths, their 7 schemas and pending_approvals were removed from the committed spec. A full just generate-client pulls in thousands of lines of unrelated drift. The client was regenerated from the edited spec with orval, and the 23 stale approval files and index exports were removed by hand.

Tests

Written first against the real migrated schema (migrated_db), and each failed for the expected reason before its fix:

  • tests/integration/api/test_investigation_snapshot_import.py: failed with GeneratedAlwaysError.
  • EE tests/integration/core/automation/test_spawn_investigation.py, 6 cases: failed with column "issue_id" does not exist.

tests/integration/conftest.py is byte-identical to the one in #162. EE integration tests load that module by path, since both packages have a tests.integration package.

Verified on main 8cd71078:

  • CE pytest: 2122 passed, 38 skipped
  • EE pytest: 470 passed
  • Integration: 7/7
  • mypy: clean
  • ruff and ruff format (venv 0.14 and hook 0.3.0): clean
  • Frontend: tsc, eslint and vitest all clean

Merge notes

🤖 Generated with Claude Code

@vercel

vercel Bot commented Sep 27, 2026

Copy link
Copy Markdown

Deployment failed for project dataing with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/bordumbs-projects?upgradeToPro=build-rate-limit

…vals

Since 013_unified_investigation.sql the investigations table only has
id, tenant_id, alert, main_branch_id, outcome, created_at, created_by,
root_hash and a generated status. Four call sites still used the old schema:

- Snapshot import inserted a value into the generated status column
  (GeneratedAlwaysError). It now lets status derive from outcome.
- The EE spawn_investigation automation action queried investigations.issue_id
  and inserted status/source/metadata. It now builds a valid AnomalyAlert from
  the issue and starts the investigation through InvestigationStarterService
  (row + Temporal workflow), links it via issue_investigation_runs
  (trigger_type 'rule'), reuses an existing run, and takes the datasource from
  a datasource_id param or the tenant's only datasource.
  ExecutionContext gains a required investigation_starter.
- AppDatabase.create_investigation had no callers; deleted.
- The approvals stack targeted the pre-013 approval_requests table, which 013
  re-keyed on branch/snapshot ids that Temporal investigations never create,
  and nothing creates approval requests. Removed the /approvals routes, the
  AppDatabase approval methods and update_investigation_status, the dashboard
  pending_approvals stat, the unused approval notification producers
  (service, email, Slack), and the ContextReviewPage UI, route, notification
  link and settings entries. The committed OpenAPI spec loses only the
  approvals paths/schemas and pending_approvals; the client is regenerated
  from it and the stale approval model files are removed.

Tests run against the real migrated schema via the migrated_db fixture.
tests/integration/conftest.py is byte-identical to the one in #162; EE
integration tests load that module by path.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
This PR deletes the approvals routes. The route policy from #150 (merged first) still listed their four POST routes, so its coverage check failed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
@bordumb
bordumb force-pushed the claude/practical-chaum-bbb68b branch from 5aeb687 to 1c8a7cf Compare September 27, 2026 20:18
@vercel

vercel Bot commented Sep 27, 2026

Copy link
Copy Markdown

Deployment failed for project dataing-docs with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/bordumbs-projects?upgradeToPro=build-rate-limit

@vercel

vercel Bot commented Sep 27, 2026

Copy link
Copy Markdown

Deployment failed for project dataing-app with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/bordumbs-projects?upgradeToPro=build-rate-limit

@bordumb
bordumb merged commit bf18334 into main Sep 27, 2026
4 of 7 checks passed
github-actions Bot pushed a commit that referenced this pull request Sep 27, 2026
## [1.24.1](v1.24.0...v1.24.1) (2026-09-27)

### Bug Fixes

* **api:** stop writing pre-013 investigation columns and remove approvals ([#183](#183)) ([bf18334](bf18334)), closes [#162](#162) [#150](#150)
bordumb added a commit that referenced this pull request Sep 27, 2026
Follow-up to #183. These call sites still read columns that
013_unified_investigation.sql dropped and failed at runtime:

- Dashboard: get_dashboard_stats filtered on completed_at and on statuses
  the generated status column never takes. Migration 036 adds
  investigations.completed_at, stamped by a trigger the first time outcome
  is set; active = no outcome, completed today = completed_at today.
  list_investigations now returns summaries from the alert JSONB (primary
  dataset, metric display name or anomaly type, severity), "unknown" for
  imported replays. The frontend called /dashboard/stats without /api/v1,
  read camelCase keys the API never sends and fell back to mock numbers;
  it now uses the generated client and shows "—" and an error on failure.
- RBAC: PermissionService matched datasource grants on
  investigations.data_source_id, so every access check raised. It now
  compares with alert->>'datasource_id'.
- Fix feedback export: the investigation context read investigations.issue_id;
  it now names the issue from issue_investigation_runs.
- EE runbooks: generation from an issue read issues.resolution/metadata and
  investigations.issue_id/synthesis; it now uses resolution_note,
  issue_labels and the latest linked investigation's outcome, and the route
  records that investigation. Runbook responses also decode JSONB text,
  which failed validation on every runbook route.

Integration tests run against the migrated schema via migrated_db.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
bordumb added a commit that referenced this pull request Sep 27, 2026
…191)

Follow-up to #183. These call sites still read columns that
013_unified_investigation.sql dropped and failed at runtime:

- Dashboard: get_dashboard_stats filtered on completed_at and on statuses
  the generated status column never takes. Migration 036 adds
  investigations.completed_at, stamped by a trigger the first time outcome
  is set; active = no outcome, completed today = completed_at today.
  list_investigations now returns summaries from the alert JSONB (primary
  dataset, metric display name or anomaly type, severity), "unknown" for
  imported replays. The frontend called /dashboard/stats without /api/v1,
  read camelCase keys the API never sends and fell back to mock numbers;
  it now uses the generated client and shows "—" and an error on failure.
- RBAC: PermissionService matched datasource grants on
  investigations.data_source_id, so every access check raised. It now
  compares with alert->>'datasource_id'.
- Fix feedback export: the investigation context read investigations.issue_id;
  it now names the issue from issue_investigation_runs.
- EE runbooks: generation from an issue read issues.resolution/metadata and
  investigations.issue_id/synthesis; it now uses resolution_note,
  issue_labels and the latest linked investigation's outcome, and the route
  records that investigation. Runbook responses also decode JSONB text,
  which failed validation on every runbook route.

Integration tests run against the migrated schema via migrated_db.

Signed-off-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
bordumb added a commit that referenced this pull request Sep 27, 2026
Brings main up to the 1.24.5 release. tests/integration/conftest.py was
added on both sides (#183 landed the same migrated_dsn fixture); main's
copy is kept, since its docstring already says CI applies the migrations.

The 1.21.0-1.24.5 releases ran with the old release config, which bumps
every pyproject.toml without re-locking, so uv.lock is re-locked in the
next commit.

Signed-off-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bordumb added a commit that referenced this pull request Sep 28, 2026
* test(ee): run EE integration tests on a migrated database

The EE integration tests never ran in CI: the root addopts deselect
`integration`, and Backend CI's integration job only ran the CE suite.
test_audit_logging.py also connected to DATABASE_URL directly and
skipped on any connection error, when no tenant row existed, and when
the EE app failed to import.

- test_audit_logging.py runs on migrated_db, which EE's
  tests/integration/conftest.py (from #183) loads from the CE conftest.
  audit_logs.tenant_id has no foreign key, so each test uses a fresh
  tenant id instead of looking up a tenant row.
  test_audit_log_has_recent_entries only ever skipped on a fresh
  database. It is replaced by test_audit_log_created_for_api_call,
  which posts through AuditMiddleware and checks the stored entry.
- test_sso_flow.py mocks every repository and needs no database, so its
  two classes lose the `integration` marker and run with the unit tests.
- ci-backend.yml: the integration job also runs the EE integration
  tests. DATABASE_URL moves to job level.

Against pgvector/pgvector:pg16, EE integration gives 8 passed, 0
skipped; without a database it used to skip 5. The default EE run gains
the 12 SSO tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>

* chore(flow): track fn-68.4, EE integration tests on a migrated database

Adds task fn-68.4 (done, evidence 9d8b2ff2) and moves EE integration
tests from the epic's follow-ups into its scope.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>

* test(core): run the integrations schema test on migrated_db

test_integrations_schema.py (#148) connected to DATABASE_URL directly
and skipped on any error. In CI that URL is the service's empty `test`
database, so it always skipped with `relation "tenants" does not
exist`, and the NOT NULL check on integration signing secrets never
ran. It now uses migrated_db and creates its tenant without catching
errors.

Against pgvector/pgvector:pg16, CE integration gives 63 passed, 0
skipped (was 62 passed, 1 skipped).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>

* test(ee): give EE integration tests the psql fixture migrated_dsn needs

#181 made the CE migrated_dsn fixture depend on a session-scoped psql
fixture. EE's integration conftest only re-exported migrated_dsn and
migrated_db, so every EE test on migrated_db errored with
"fixture 'psql' not found". Nothing noticed, because CI never ran the
EE integration tests; this branch adds them.

Against pgvector/pgvector:pg16: EE integration 16 passed (was 3 passed,
13 errors); CE integration 85 passed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>

---------

Signed-off-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant