feat: alert API connectors with scheduled daily imports - #337
Draft
Chouffe wants to merge 45 commits into
Draft
Conversation
…sponses api_get raises only on unparsable JSON, so a non-2xx response with a valid JSON error body comes back as a dict where a list is expected. Validate the shape and treat it as a normal verify failure instead of letting a TypeError escape the org-upsert loop and 500 the endpoint.
…line ImportConfig.annotation_api_token was inert: post_records_to_annotation_api and the three annotation_management call sites each resolved credentials from the environment and logged in themselves, so a caller-supplied token could never take effect. The worker self-mints a JWT precisely so that no plaintext annotation-API password has to exist in its environment, and an inert field would have silently defeated that. Both now accept an optional auth_token; None keeps the previous env-credential behaviour, so the CLI is unaffected. Also move the per-organization bookkeeping out of the posting try/except, so a bug in the accounting can never be reported as a transfer failure, and document that alerts_skipped covers both the pre-fetch filter and POST-time 409s.
…ixture update_user_password lacked the WORKER_USERNAME guard that update_user and delete_user already have; with the worker now active, that endpoint was the one remaining way to log in as the attribution-only worker identity. The worker-auth pinning test now runs seed_default_users instead of a fixture that only mimicked its shape, so it actually fails if the seed regresses.
…URL hostname import_connector's try/except only wrapped the run_import call; a DB failure anywhere else (org lookup, token mint, skip-set reads, coverage writes) would still propagate and break the "never raises" contract the sweep depends on. Wrap those sections too, with rollback before returning where a transaction may be left aborted. Also fix ANNOTATION_API_INTERNAL_URL's default host: the compose service is named annotation_api, not api, so http://api:5050 never resolved.
Replace legacy gray/red/emerald/amber Tailwind colours with the fire-lookout palette (pine, signal, ash, line, char) per DESIGN.md. Hatch colour now matches the signal token exactly, mirroring the precedent in ObjectFilmstrip's 'no source found' hole.
…l to legend
- verify's sample_date is a bare UTC date string; formatDate() re-parses it
local and rolls it back a day for negative-offset viewers, so render it
raw like dateFrom/dateEnd instead.
- the coverage-heatmap test only checked cell count and that Ardeche
appeared somewhere on the page, which would still pass if a disabled org
leaked into the heatmap; scope the assertion to the heatmap's own
coverage-cell-{organization_id}- testid prefix for Aveyron's absence.
- the legend was missing the partial (some imported, some failed) state.
format:check was failing on these five files (created/modified across the alert-api-connector tasks) even though the rest of src/ is prettier-clean. Formatting only, no behavior change.
…tics - Key imported/empty on alerts_fetched instead of alerts_imported: a re-run inside the trailing window re-fetches already-imported alerts and files them as skipped, so alerts_imported alone flipped a fully-covered day back to grey once it aged out of the newest run. - Only fall back to not-enabled when there is no coverage row: the initial sweep imports days preceding enabled_at, and those real rows were being masked by the dashed "not enabled yet" state.
Client built a 31-day window (dateEnd - 30); server default is date_end - 29. Both were labelled "last 30 days" but disagreed by one day.
…alls api_get had no timeout at all, and connector_verify's _probe (three sequential api_get calls plus the token exchange) ran behind asyncio.to_thread with no outer bound either. A connector pointed at a host that black-holes packets could occupy an asyncio default-executor thread forever; that pool is bounded, so repeated attempts would eventually stall every other to_thread consumer in the API process. - api_get now passes timeout=30 to requests.get — generous enough for a legitimately slow list endpoint, but no longer unbounded. This also bounds the worker's daily import path, which shares this client. - verify_connector wraps the probe in asyncio.wait_for(..., timeout=_PROBE_TIMEOUT_SECONDS=100), covering the worst-case sum of the four sequential calls' individual timeouts with a small buffer. - Added a test confirming the resulting TimeoutError is caught by the existing except Exception block and recorded as a normal verify failure, same as any other probe error.
shared.py imports dotenv at module scope and is now on the worker's import path (worker.py -> connector_import -> runner -> shared), but it only resolved transitively via pydantic-settings. Same failure mode already hit this branch once for pyyaml: if pydantic-settings ever drops the dependency, the worker fails to boot.
…nnector # Conflicts: # annotation_api/scripts/data_transfer/ingestion/alert_api/import.py # annotation_api/src/app/main.py
The runner's object-split summary reads every key main's #324 added, so a stub predating it raised KeyError inside run_import.
Ports main's #324 into runner.py, where the object-split summary line moved when the importer was extracted into a library.
…nnector # Conflicts: # annotation_api/scripts/data_transfer/ingestion/alert_api/import.py
…s a non-list
A non-admin alert-API credential authenticates fine and then fails only at
/organizations/, with {"detail": "Incompatible token scope."}. The shape guard
collapsed that to "unexpected response shape", discarding the one thing that
tells an operator to swap in an admin account. Name the failing call and pass
the detail through; GET responses carry no request body, so the password still
cannot leak into the message.
…e coverage heatmap The native title attribute was slow and unstyled; replace it with a single fixed-position bubble (a per-cell CSS bubble would clip inside the overflow-x-auto container). The cell text moves to aria-label. Also recolor 'covered, no alerts' from ash to pale pine: it is a success state, and at 16px the ash square was indistinguishable from the dashed not-enabled outline, which is now the only grey state. Legend updated in sync.
…nnector Three conflicts, all in the importer scripts main evolved while this branch moved them into runner.py: import.py keeps the runner-based main(); annotation_management.py adopts main's required-token design (7d75de0) — resolve_auth_token's env-login fallback is deleted since every caller (CLI and worker) already resolves one token up front via ImportConfig.annotation_api_token; the test file follows main, whose does-not-log-in + token-forwarded pair supersedes our fallback-guard test.
…nnector Four textual conflicts plus two the merge could not see. config.py / docker-compose.yml: both sides added adjacent env settings, so both are kept — except the worker's AUTOANNOTATE_MODEL_PATH override, which main deleted on purpose in #367 (the code default now points at the baked weights, and holding the real value in compose is what hid the broken default for three weeks). Re-adding it here would restore exactly that trap. sequence_fetching.py: the branch split fetch_all_sequences_within into a listing half and a detection-fetching half; main added its temporal_scores_unsupported warning at what is now the seam. It goes in fetch_detections_for_sequences, which both the CLI and the connector path still funnel through, so neither loses the warning and no signature changes. import.py: taken from the branch, which replaced main's inline pipeline with run_import(). That deletes the file main had been editing, so its three temporal-score reporting changes are ported into runner.py instead: the dropped-verdict warning after the split summary, the refresh counters (with a failed refresh feeding ImportResult.error, matching main's exit 1), and the refresh lines in both the nothing-imported panel and the final summary. The refresh behaviour itself needed no porting — it lives in shared.py and object_split.py, which merged cleanly and are what runner.py calls. The two invisible ones: both branches minted the same alembic revision id c5d6e7f8a9b0 off b4c5d6e7f8a9, so the merged tree had a duplicate id and two heads and the API would not boot. Main's is deployed, so the connector migration is re-parented onto it as d6e7f8a9b0c1; alembic heads now reports one. And the runner test's split_all_records stub omitted main's new dropped_temporal_scores key, which the stub's own comment warns about. Verified: 858 backend tests, 1351 frontend tests, type-check, eslint, ruff.
… filename The merge resolution re-parented the connector migration onto main's temporal-score migration, both branches having minted the same revision id c5d6e7f8a9b0 off b4c5d6e7f8a9. The `git mv` staged the rename, but the edit that actually changes revision/down_revision inside the file was never staged, so the commit carried the new filename with the old ids — still a duplicate id and still two heads, and the API still cannot boot. Local verification missed it because docker-compose-dev.yml bind-mounts ./src/migrations, so the container ran the working tree (which was correct) rather than the commit. Only CI, which builds from the commit, saw it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes daily alert-API ingestion self-service instead of a manual CLI ritual.
An admin registers a connector (alert API base URL + one credential pair) at
/connectors, discovers that API's organizations, toggles which to ingest, and the worker imports them daily — recording per-(connector, organization, day) coverage that renders as a heatmap.Design:
docs/specs/2026-08-06-alert-api-connector-design.mdWhat changes
Backend
alert_api_connectors,alert_api_connector_organizations,alert_api_import_coverage(one migration,c5d6e7f8a9b0)./api/v1/connectors, with the alert-API password write-only —ConnectorReadexposeshas_password: booland no password field of any kind.POST /{id}/verifyauthenticates, discovers organizations, and reports "saw sequences from N of M organizations".POST /connectors/test— stateless pre-save credential check (token exchange + organization listing, nothing persisted, bounded at 25s — deliberately under the frontend's 30s axios timeout). Catches both real failure modes: wrong password, and an org-scoped credential, whose"Incompatible token scope."detail is passed through verbatim (design:docs/specs/2026-08-07-connector-test-connection-design.md). The service function ischeck_connector_credentials, nottest_…— pytest collects importedtest_-prefixed names as tests.schedule_connector_imports), one job per enabled connector.Importer
scripts/…/alert_api/runner.py, shared by the CLI and the worker.make import-alert-apibehaves identically — flags, defaults, output, and exit codes all preserved.organization_ids,skip_platform_alert_ids) applied before the per-sequence detection fetch, so re-running a covered day costs one listing call and zero detection calls. Previously "already exists" was only discovered at POST time, after paying for every fetch.Frontend
/connectorslist and detail pages (superuser-gated), plus the coverage heatmap.position: fixedelement — a per-cell CSS bubble would be clipped by theoverflow-x-autocontainer; cell text lives onaria-label), and "covered, no alerts" recolored from ash to pale pine: it's a success state, and at 16px the ash square was indistinguishable from the dashed "not enabled yet" outline, now the only grey state.max-w-5xlcap forced horizontal scrolling on the 7-column table).Design decisions worth reviewing
Credentials are encrypted, not hashed. The worker needs the plaintext to log into the alert API, so passwords are Fernet-encrypted at rest under a new
CONNECTOR_SECRET_KEY. The threat addressed is a database dump — a backup, a copy pulled for debugging — not host compromise. If the key is unset, connector create/update returns 400 and the worker skips them, so existing deployments keep working untouched.The worker self-mints a JWT rather than holding a password, so no plaintext credential lives in its environment. This required making the seeded
workeruser active, becauseapp/api/dependencies.pyaliasesget_current_user = get_current_active_user— an inactive identity cannot call the endpoints it posts to. Login remains blocked by the random password discarded at seed time, and all four user-mutation routes 403 on it. ClosingPATCH /users/{id}/passwordwas part of this change; it had been open on the rationale that the worker couldn't log in anyway, which this PR invalidated.Verify reports a count, not a boolean. The whole "one connector, many organizations" model rests on an unverified premise: that one admin account can list sequences across every organization. Seeing one org on a quiet day proves nothing; four of seven proves it works. Collapsing that to a pass/fail tick would destroy the only signal that tests the premise.
A zero-alert day is recorded, not omitted.
okwith zero counts is deliberately distinct from a day never attempted — that distinction is why there's no separate run-history table, and why the heatmap can show gaps at all.Daily, not hourly.
trailing_days(default 3) is both the re-check window and the catch-up mechanism: a worker down at 03:00 loses nothing, because the next run re-covers that date.Testing
test_connector_import— passes in isolation and on immediate re-run; ordering interaction, not a code defect.)pyyamlandpython-dotenvbeing dev-only dependencies on a path now shipped to the worker).Merged with
maintwice. First merge: a docstring inmain.py, andimport.py, where main's #324 (same_frame_merges) had to be ported intorunner.pysince the summary line moved there. Second merge: main's required-tokenannotation_management.py(#7d75de0) was adopted wholesale and this branch'sresolve_auth_tokenenv-login fallback deleted — every caller (CLI and worker) already resolves one token up front intoImportConfig.annotation_api_token, so the fallback guarded nothing.Verified against production (2026-08-07)
The cross-organization premise holds. Probing the real alert API with the admin credential, using verify's exact probe shape (one day,
limit=200,risk_score=extreme):organizations_seen_in_sample, each of 2026-07-31 → 2026-08-06One admin credential lists sequences across a dozen organizations at once, so per-org credentials are not needed and the connector model stands as designed. Incidentally confirmed on real data:
camera_id→organization_idresolved for 100% of sequences,from_datereturns exactly one calendar day, and every organization dict carries theid/namekeys the upsert reads.A non-admin credential fails in a way worth naming. It authenticates fine, then
/organizations/answers{"detail": "Incompatible token scope."}— which the shape guard was collapsing into a bare "unexpected response shape". Connectors require an admin-scoped credential and nothing in the UI says so, so that detail is the only signal telling an operator which credential to swap in; it's now passed through, along with the name of the call that failed.The refactored importer runs on real records.
make import-alert-apifor 2026-08-05 against the local stack, end to end, no failures:Note the credential dependency: run with the org-scoped
ALERT_API_LOGIN(sdis-07) the same day yields 5 sequences and 0 cross-deduped siblings — cross-dedup is unreachable without a cross-org credential, so CLI testing to date never exercised the path the worker will take.The worker path itself ran live, end to end.
import_connector(the exact function the 03:00 job calls) executed against the production alert API through a real connector configured in the UI — create with Test connection → verify → enablesdis-07+sdis-12:okcoverage cells on the heatmapKnown gaps
/connectors/testendpoint sidesteps this by bounding itself at 25s.)Incompatible token scope.before saving, which is the practical guard.