Skip to content

fix(api): declare the watcher notification vocabulary - #558

Open
craig-dt wants to merge 1 commit into
Vigil-SOC:mainfrom
craig-dt:fix/553-watcher-notification-vocabulary
Open

fix(api): declare the watcher notification vocabulary#558
craig-dt wants to merge 1 commit into
Vigil-SOC:mainfrom
craig-dt:fix/553-watcher-notification-vocabulary

Conversation

@craig-dt

@craig-dt craig-dt commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What changed

case_watchers.notification_preferences is read at services/case_notification_service.py:297-298:

prefs = watcher.notification_preferences or {}
if prefs.get(notification_type, True):  # Default to True

notification_type is a runtime argument, not a literal — so the keys that lookup honours are exactly the set of types passed to notify_watchers. Nothing in the repo declared that set: no constant, no enum, no Pydantic type, no TypeScript type, no SQL default, no seed row, no test. The available switches could only be learned by finding every call site by hand.

The API made this worse rather than hiding it. WatcherAdd.notification_preferences was a bare Optional[Dict], so any JSON object was accepted and persisted verbatim. A caller sending {"case_assigned": false} got a 200 and reasonably concluded assignment notices were suppressed. They are not — case_assigned, comment_mention and stale_case call create_notification directly and never consult the column. The key was stored and never read.

This PR:

  • Declares the vocabulary as WATCHER_NOTIFICATION_TYPES next to the lookup that consumes it (new_comment, sla_warning).
  • Rejects unknown keys at the API boundary, so a caller learns immediately instead of trusting a switch that does nothing.
  • Tightens the value type to bool, which the bare Dict never enforced.
  • Warns rather than raises in notify_watchers on an unregistered type. The new_comment call site runs inside add_comment's try/except, which rolls back and returns None — raising there would trade a silently ineffective preference for a silently dropped comment. It sends, matching the absent-key default.

No behaviour change for any existing caller. to_dict() still emits notification_preferences; a request omitting the field is still valid; the opt-out default (absent key → send) is unchanged.

Why this is not the schema migration #553 asked for

#553 proposed promoting the column to typed boolean columns. That was declined after tracing every reader and writer — the issue's own first acceptance criterion. Three findings:

  1. The key set is not fixed. It is keyed by notification_type, so typed columns would cap an open vocabulary and cost a schema migration for every new notification type.
  2. No row anywhere has data. add_watcher is insert-only and returns early on an existing row, discarding incoming preferences; there is no PATCH/PUT route; the frontend posts {user_id} only (frontend/src/services/api.ts:271-272). The migration would have moved nothing.
  3. The prescribed mechanism does not apply. case_watchers has no DDL anywhere — grep -rn "case_watchers" database/ helm/ returns only database/models.py:1066. The table comes from Base.metadata.create_all(), so an ALTER TABLE in a numbered database/init/ file would run in the dbInit Job before the table exists and hard-fail.

So: no schema change, no migration, no init SQL, no Helm work. The audit doc is corrected accordingly in #546, which also picks up a related finding — six of the promotion target tables have no init SQL at all, so several already-filed issues prescribe a mechanism that cannot reach them.

How this was tested

tests/unit/test_watcher_notification_vocabulary.py — 13 tests, no database, so they run in the main PR gate.

Verified to fail before the change and pass after: 6 failed pre-change, 13 pass post-change. The 7 that passed pre-change are deliberate regression guards on behaviour this PR must not alter (absent key sends, NULL column sends, False suppresses, per-type scoping).

Coverage:

  • The vocabulary as a change-detector, so adding a notify_watchers call site without registering its type is visible in review.
  • Types that bypass preferences (case_assigned, comment_mention, stale_case) are asserted absent, since listing them would advertise a switch that does nothing.
  • The opt-out default: absent key and NULL column both still send.
  • Unregistered type warns and still sends.
  • Unknown key → 422 through FastAPI; the error names the accepted set.

The route test is itself evidence of the bug: pre-change it reached real Postgres and failed on a foreign-key violation — proof that an unknown key passed validation and got as far as the database. Post-change it never reaches the service layer.

Regressions: unit 1088 passed with the same 17 pre-existing failures as clean main; security 84 passed with the same 1. Both baselined in a throwaway worktree at 311790e and diffed by test name — zero new failures.

Lint: flake8 reports 0 findings on added lines; black --diff wants to touch none of them; the new import sits where isort wants it. black was deliberately not run across backend/api/cases.py or services/case_notification_service.py — both are pre-existing-dirty and a bulk pass would rewrite unrelated code.

Follow-ups found while tracing (not in this PR)

Filed separately rather than riding along:

  • CaseNotification(metadata=...) is silently swallowed — the column is notification_metadata, and hasattr(cls, "metadata") is True (the declarative MetaData), so the kwarg sets a shadowing instance attribute instead of raising. Every notification's metadata is dropped. Confirmed empirically. Same failure family as Case timeline events silently discarded: in-place JSONB appends are never persisted #543.
  • add_watcher discards preferences for an existing watcher, and no update route exists — so preferences are unsettable after creation.
  • Frontend Watcher.added_at vs backend created_at; "Watching since" always renders from undefined.
  • scripts/migrate_schema.py is invoked by no deploy path, so ORM-only column drift is silent until a human runs it.

Closes #553


Analysis by Claude (Claude Code), reviewed by @craig-dt before posting. All file:line citations were verified against main at 311790e.

🤖 Generated with Claude Code

case_watchers.notification_preferences is read at
services/case_notification_service.py with

    prefs = watcher.notification_preferences or {}
    if prefs.get(notification_type, True):

where notification_type is a runtime argument, not a literal. Nothing in
the repo declared which keys that lookup honours -- no constant, no enum,
no Pydantic type, no TypeScript type, no SQL default, no seed row, no
test. The available switches could only be learned by finding every
notify_watchers call site by hand.

The API made that worse rather than hiding it. WatcherAdd typed the field
as a bare Optional[Dict], so any JSON object was accepted and persisted
verbatim. A caller sending {"case_assigned": false} got a 200 and
reasonably concluded assignment notices were suppressed. They are not:
case_assigned, comment_mention and stale_case all call
create_notification directly and never consult the column. The key was
stored and never read.

Declares the vocabulary as WATCHER_NOTIFICATION_TYPES next to the lookup
that consumes it, and rejects unknown keys at the API boundary so the
caller learns immediately instead of trusting a switch that does nothing.
Also tightens the value type to bool, which the bare Dict never enforced.

notify_watchers warns rather than raises on an unregistered type. The
new_comment call site runs inside add_comment's try/except, which rolls
back and returns None -- raising there would trade a silently ineffective
preference for a silently dropped comment. It sends, matching the
absent-key default.

Issue Vigil-SOC#553 proposed promoting this column to typed boolean columns. That
was declined after tracing every reader and writer, which is the issue's
own first acceptance criterion:

  - The key set is not fixed. It is keyed by notification_type, so typed
    columns would cap an open vocabulary and cost a schema migration per
    new notification type.
  - No row anywhere has data. add_watcher is insert-only and returns
    early on an existing row, discarding incoming preferences; there is
    no PATCH route; the frontend posts {user_id} only. Promoting would
    have migrated nothing.
  - The migration mechanism the issue prescribes does not apply.
    case_watchers has no DDL anywhere -- the table comes from
    Base.metadata.create_all(), not database/init/. An ALTER TABLE in a
    numbered init file would run in the dbInit Job before the table
    exists and hard-fail.

So no schema change, no migration, no init SQL and no Helm work. No API
response change: to_dict() still emits notification_preferences, and a
request that omits the field is still valid.

Tests, all verified to fail before the change and pass after (6 failed
pre-change, 13 pass post-change):

  tests/unit/test_watcher_notification_vocabulary.py -- no database, so
  it runs in the main PR gate. Pins the vocabulary as a change-detector,
  the opt-out default (absent key and NULL column both still send), the
  per-type scoping, warn-but-still-send on an unregistered type, and the
  422 for an unknown key. The route test previously reached real
  Postgres and failed on a foreign-key violation, which is itself the
  proof that an unknown key passed validation and got as far as the
  database.

Regressions: unit 1088 passed with the same 17 pre-existing failures as
clean main, and security 84 with the same 1 -- both baselined in a
throwaway worktree at 311790e, zero new. flake8 reports 0 findings on
added lines; black wants to touch none of them; the new import sits
where isort wants it. black was not run across either existing file --
both are pre-existing-dirty and a bulk pass would rewrite unrelated code.

Closes Vigil-SOC#553
Relates to Vigil-SOC#468

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Craig <craig@deeptempo.ai>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

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.

case_watchers.notification_preferences: promote the flag set to real columns

1 participant