diff --git a/CLAUDE.md b/CLAUDE.md index cef73c73..b44207ee 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -773,15 +773,19 @@ kbagent notification detail --project NAME --subscription-id ID # flow's configuration JSON, so `flow detail` / `config detail` never showed them. The in-flow # `type: "notification"` TASK is a different mechanism and stays visible there. # Event names are kebab-case (job-failed, job-succeeded, job-succeeded-with-warning, -# job-processing-long + phase-job-* variants); --event is forwarded verbatim and NOT validated -# (the API declares EventName as an open string). Filter fields are dotted paths into the event +# job-processing-long + phase-job-* variants); --event is NOT validated (the API declares +# EventName as an open string). The service ACCEPTS ?event= and then IGNORES it (200 + the +# full list, verified live), so kbagent sends it AND narrows client-side -- a direct API +# caller must do the same or get a superset. Filter fields are dotted paths into the event # payload (job.component.id, job.configuration.id, branch.id, phase.id) -- --component-id / -# --config-id match those CLIENT-SIDE, since the API supports only ?event=. A subscription with +# --config-id likewise match CLIENT-SIDE. A subscription with # NO filters is project-wide and fires for EVERY job; those are dropped by --component-id/ # --config-id and counted in `project_wide_excluded` (plus a warning) so "who gets paged for # this flow" is never silently under-reported. Webhook recipients carry `url`, email carries -# `address` -- both render in the single `address` column. Subscriptions are project-level, not -# branch-scoped; a branch-specific one carries a branch.id filter shown in the Branch column. +# `address` -- both render in the single `address` column. The Branch column is populated on +# EVERY row, production included: the UI always writes a branch.id filter, and for production +# that value is the DEFAULT branch's numeric id -- compare against `branch list` to tell a +# production alert from a dev-branch one; presence alone means nothing. kbagent context kbagent init [--from-global] [--project ALIAS ...] diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index f25e71cf..d5128bc8 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -308,8 +308,10 @@ Requires the project to be added with its **master ('owner') Storage API token** ## Notification Subscriptions (Flow Notifications Tab) (since v0.86.0) - `notification list [--project NAME ...] [--event NAME] [--component-id ID] [--config-id ID]` -- fleet-wide list of Notification Service subscriptions: the recipients behind the Flow Builder's **Notifications tab** (bell icon -- Success / Error / Processing-delay / Warning cards). These live in a separate platform service (`notification.{stack}`, plain project Storage token, no elevated scope), **not** in the flow's `configuration` JSON, which is why `flow detail` / `config detail` never showed them. The in-flow `type: "notification"` **task** is a different mechanism and remains visible via `flow detail`. Row keys: `project_alias`, `subscription_id`, `event`, `component_id`, `config_id`, `config_name` (resolved), `branch_id`, `phase_id`, `channel`, `address`, `expires_at`, `scope`, `filters` - `notification detail --project NAME --subscription-id ID` -- one subscription with every filter printed verbatim, including threshold filters (`durationOvertimePercentage`) that have no dedicated column -- Event names are **kebab-case**: `job-failed`, `job-succeeded`, `job-succeeded-with-warning`, `job-processing-long`, plus the `phase-job-*` variants. `--event` is forwarded verbatim as `?event=` and deliberately **not** validated -- the API declares `EventName` as an open string, so a newer platform event works without a kbagent release -- `--component-id` / `--config-id` filter **client-side**: the API's only server-side filter is `?event=`. They match the subscription's own `job.component.id` / `job.configuration.id` filter values (dotted paths into the event payload -- not flat `componentId`/`configurationId` keys) +- Event names are **kebab-case**: `job-failed`, `job-succeeded`, `job-succeeded-with-warning`, `job-processing-long`, plus the `phase-job-*` variants. `--event` is deliberately **not** validated -- the API declares `EventName` as an open string, so a newer platform event works without a kbagent release +- **The service ignores `?event=`** -- verified live: a filtered request answers `200` with the project's FULL subscription list. kbagent sends the parameter anyway (a server-side fix would then cost nothing) and narrows client-side, so `--event` is correct from the CLI; a direct API caller gets a superset. See [gotchas.md](gotchas.md) +- `--component-id` / `--config-id` filter **client-side** too. They match the subscription's own `job.component.id` / `job.configuration.id` filter values (dotted paths into the event payload -- not flat `componentId`/`configurationId` keys) +- `branch_id` is populated on **every** row, production included -- the Flow Builder always writes a `branch.id` filter and uses the default branch's numeric id for production. A filled Branch column does NOT mean "dev-branch only"; cross-check `kbagent branch list` - A subscription with **no filters at all** is project-wide and fires for every job. Those are excluded by `--component-id`/`--config-id` and reported as `project_wide_excluded` (plus a warning in human mode), so "who gets paged when this flow breaks" is never silently under-reported -- see [gotchas.md](gotchas.md) - Exposed over `kbagent serve` as `GET /notifications` and `GET /notifications/{project}/{subscription_id}` - Read-only in this release; creating/deleting subscriptions is not exposed diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 8982b2c7..588d1539 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -1218,20 +1218,31 @@ events and emits a final `done` SSE frame mirroring the same record. - **Event names are kebab-case, and the field is an open string.** `job-failed`, `job-succeeded`, `job-succeeded-with-warning`, `job-processing-long`, plus the `phase-job-*` variants -- *not* the - camelCase `jobFailed` spelling. `--event` is forwarded verbatim as - `?event=` and deliberately not validated against a fixed list, because the - API declares `EventName` as `type: string` with no enum. A misspelled event - therefore returns an empty list, not an error -- if a query comes back - empty, re-check the spelling before concluding nobody is subscribed. + camelCase `jobFailed` spelling. `--event` is deliberately not validated + against a fixed list, because the API declares `EventName` as + `type: string` with no enum. A misspelled event therefore returns an empty + list, not an error -- if a query comes back empty, re-check the spelling + before concluding nobody is subscribed. +- **The service IGNORES `?event=`; kbagent narrows client-side** + (verified live, not from the swagger). A filtered request answers `200` with + the project's FULL subscription list -- the documented query parameter has + no effect. kbagent still sends it (so a server-side fix would cost nothing) + and then filters the rows itself, which is why `--event` behaves correctly + from the CLI. **Anything talking to the service directly must do the same + narrowing**, or "who gets paged on failure" silently answers with a superset + that includes success and processing-delay recipients. Same lesson as + Storage's `list_files`: a documented query parameter is a statement of + intent, not of behavior -- check what comes back. - **Filter fields are dotted paths into the event payload.** `job.component.id`, `job.configuration.id`, `branch.id`, `phase.id`, `durationOvertimePercentage` -- *not* flat keys like `configurationId` or `component`. Only the first four have dedicated output columns; everything else rides along in the raw `filters` list (`notification detail` prints it verbatim). -- **`--component-id` / `--config-id` filter client-side.** The API's only - server-side filter is `?event=`; everything else is applied by kbagent after - fetching the project's full subscription list. +- **`--component-id` / `--config-id` filter client-side** -- as does + `--event` (see above). The endpoint has no server-side filter that works at + all: kbagent always fetches the project's full subscription list and narrows + it locally. - **A subscription with NO filters is project-wide and fires for every job.** `filters` is optional in the schema (only `event` and `recipient` are required), so a catch-all "page me on any failure" subscription is legal and @@ -1250,11 +1261,16 @@ events and emits a final `done` SSE frame mirroring the same record. - **Webhook recipients carry `url`, email recipients carry `address`.** Both render in the single `address` column; a `--json` consumer reading the raw API would need to handle both keys. -- **Subscriptions are project-level, not branch-scoped.** There is no branch - query parameter. A branch-specific subscription carries a `branch.id` - filter, surfaced in the `branch_id` column. Configuration *names* are - resolved against the project's active branch, so a flow that exists only in - production shows a blank `config_name` while a dev branch is active. +- **`branch_id` is populated on EVERY subscription, production included** -- + a filled Branch column does NOT mean "dev-branch only" (verified live). + There is no branch query parameter, but the Flow Builder always writes a + `branch.id` filter, and for a production subscription that value is the + **default branch's numeric id** (e.g. `390737` for a project whose `Main` + branch has that id). To tell a production alert from a dev-branch one you + must compare against `kbagent branch list` for that project -- presence + alone tells you nothing. Configuration *names* are resolved against the + project's active branch, so a flow that exists only in production shows a + blank `config_name` while a dev branch is active. - **`config_name` is best-effort.** It is resolved by an exact `(component_id, config_id)` match, falling back to a config-ID lookup when the subscription has no component filter -- but only when unambiguous. Two diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index c0e26f87..6edd530c 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -37,7 +37,9 @@ "registered alias already holds -- no elevated scope, no manage token.", "`notification list` counts the project-wide subscriptions its scope filters hide, " 'so "who gets paged" is never under-reported. `--component-id` / `--config-id` ' - "filter client-side (the API's only server-side filter is `?event=`) and match the " + "filter client-side -- as does `--event`: the service accepts its documented " + "`?event=` parameter and then ignores it, answering 200 with the project's full " + "list (verified live), so kbagent sends it and narrows the rows itself. They match the " "subscription's own `job.component.id` / `job.configuration.id` filter values. A " "subscription with NO filters is project-wide and fires for every job in the " "project; those rows are excluded by those two flags but counted in " @@ -48,6 +50,11 @@ "declares `EventName` as an open string.", "`kbagent serve` mirrors the new group 1:1: `GET /notifications` and " "`GET /notifications/{project}/{subscription_id}`.", + 'A filled Branch column on `notification list` does not mean "dev-branch only". ' + "The Flow Builder writes a `branch.id` filter on EVERY subscription, and for a " + "production one that value is the default branch's own numeric id -- so `branch_id` " + "is populated on every row, production included. Cross-check `kbagent branch list` " + "for the project to tell a production alert from a dev-branch one.", ], "0.85.1": [ "Fix: `kbagent config new --push` no longer creates a broken configuration from a " diff --git a/src/keboola_agent_cli/client/notifications.py b/src/keboola_agent_cli/client/notifications.py index 6dc7c1ee..70a0eafc 100644 --- a/src/keboola_agent_cli/client/notifications.py +++ b/src/keboola_agent_cli/client/notifications.py @@ -38,10 +38,19 @@ class _NotificationsMixin(_CoreClient): def list_project_subscriptions(self, event: str | None = None) -> list[dict[str, Any]]: """List every notification subscription for the token's project. + .. warning:: + **The service IGNORES ``event``** -- verified against a live stack, + where a filtered request answers 200 with every subscription in the + project. The parameter is still sent because the swagger documents + it and a server-side fix would then cost nothing, but THIS METHOD + DOES NOT NARROW. Callers that need narrowing must filter the + returned list themselves; ``NotificationService`` does exactly that. + Args: - event: Optional event-name filter (e.g. ``job-failed``). Passed - through to the API as ``?event=``; a falsy value is omitted - entirely rather than sent as an empty parameter. + event: Optional event-name filter (e.g. ``job-failed``). Sent as + ``?event=``; a falsy value is omitted entirely rather than sent + as an empty parameter. See the warning above -- passing it does + not reduce the result. Returns: List of subscription dicts verbatim from the API. diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index 9cf66eb6..0e969da3 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -1009,15 +1009,22 @@ Event names are kebab-case: job-failed, job-succeeded, job-succeeded-with-warning, job-processing-long, and the phase-job-* variants. --event is NOT validated against that list (the API declares - EventName as an open string), it is forwarded verbatim as ?event=. - --component-id / --config-id filter CLIENT-SIDE on the subscription's - own job.component.id / job.configuration.id filter values; the API - supports only ?event=. A subscription with NO filters is project-wide - (scope="project-wide") and fires for every job -- those are excluded by - --component-id/--config-id and reported as project_wide_excluded so the - "who gets paged for this flow" answer is never silently incomplete. - Subscriptions are project-level, not branch-scoped; a branch-specific one - carries a branch.id filter, surfaced in the branch_id column. + EventName as an open string). + ALL filtering is CLIENT-SIDE, --event included: the service accepts + ?event= and then IGNORES it, answering 200 with the project's full + subscription list (verified live). kbagent still sends the parameter and + narrows the rows itself, so --event is correct from the CLI -- anything + calling the service directly must narrow too, or it gets a superset. + --component-id / --config-id match the subscription's own + job.component.id / job.configuration.id filter values. + A subscription with NO filters is project-wide (scope="project-wide") and + fires for every job -- those are excluded by --component-id/--config-id + and reported as project_wide_excluded so the "who gets paged for this + flow" answer is never silently incomplete. + branch_id is populated on EVERY row, production included: the Flow + Builder always writes a branch.id filter and uses the DEFAULT branch's + numeric id for production. A filled Branch column does NOT mean + "dev-branch only" -- cross-check `branch list` to tell them apart. kbagent notification detail --project NAME --subscription-id ID One subscription with every filter printed verbatim, including threshold diff --git a/src/keboola_agent_cli/services/notification_service.py b/src/keboola_agent_cli/services/notification_service.py index 17c4038b..975b75a2 100644 --- a/src/keboola_agent_cli/services/notification_service.py +++ b/src/keboola_agent_cli/services/notification_service.py @@ -217,7 +217,9 @@ def list_subscriptions( Args: aliases: Project aliases to query. ``None`` / empty means every registered project. - event: Event-name filter, applied server-side via ``?event=``. + event: Event-name filter. Sent as ``?event=`` AND applied + client-side, because the service ignores the parameter -- see + the comment in :meth:`_fetch_project_subscriptions`. component_id: Client-side filter on the subscription's ``job.component.id`` filter value (the API has no such filter). config_id: Client-side filter on ``job.configuration.id``. @@ -326,6 +328,16 @@ def _fetch_project_subscriptions( raw = client.list_project_subscriptions(event=event) rows = [_extract_subscription_fields(sub) for sub in raw] + # The live service IGNORES ``?event=`` -- verified against a real + # stack, where a filtered request answers 200 with every + # subscription in the project. The parameter is still sent (the + # swagger documents it, and a server-side fix would then cost + # nothing), but the narrowing has to happen here or --event + # answers "who gets paged on failure" with a superset that + # includes success recipients. + if event: + rows = [row for row in rows if row["event"] == event] + # Count only rows the filter actually DROPPED. A subscription # filtering on job.component.id alone is labelled project-wide # (scope keys off the config filter), but --component-id KEEPS diff --git a/tests/test_e2e.py b/tests/test_e2e.py index 0cacde3b..c06a607f 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -5507,6 +5507,15 @@ class TestE2ENotificationSubscriptions: exist. That makes the test meaningful on an empty project (it still proves the host derivation, auth, and envelope) and start covering the row path the day a subscription is added, without needing a rewrite. + + That vacuity is not free, and it already cost us once: the service turned + out to IGNORE its documented ``?event=`` filter, and the assertion that + would have caught it passed vacuously on a project with no subscriptions. + So the data-dependent assertions below ``pytest.skip`` with an explicit + reason rather than passing silently -- a skipped test in the report is a + visible gap; a green vacuous one is not. Populate the E2E project with a + couple of Notifications-tab subscriptions on different events to turn + them on. """ @pytest.fixture(autouse=True) @@ -5534,6 +5543,12 @@ def setup(self, tmp_path: Path) -> None: ) assert result.exit_code == 0, f"project add failed: {result.output}" + # L3 client for the canary below: it has to see the service's RAW + # answer, and every CLI path narrows before the caller sees anything. + from keboola_agent_cli.client import KeboolaClient + + self.client = KeboolaClient(stack_url=self.url, token=self.token) + def _run(self, *args: str) -> Any: return _invoke(self.config_dir, ["--json", *args]) @@ -5559,22 +5574,94 @@ def test_notification_list_returns_well_formed_envelope(self) -> None: if row["channel"] in ("email", "webhook"): assert row["address"], f"recipient missing an address: {row}" - def test_event_filter_narrows_without_error(self) -> None: - """`--event` is forwarded verbatim as ?event= and must not 400.""" + def test_event_filter_returns_only_that_event(self) -> None: + """Every returned row must match `--event`, whatever the API did. + + The Notification Service ACCEPTS ``?event=`` (200, no 400) and then + ignores it, returning the project's full subscription list. kbagent + narrows client-side, so this assertion is about kbagent's contract, + not the API's -- and it is the guard against anyone "simplifying" that + filter away on the strength of the swagger. + """ _step(2, "notification list --event job-failed") result = self._run("notification", "list", "--project", self.alias, "--event", "job-failed") data = _json_ok(result) assert all(row["event"] == "job-failed" for row in data["subscriptions"]) - def test_unknown_event_returns_empty_not_error(self) -> None: - """EventName is an open string -- a typo yields no rows, not a failure. + def test_event_filter_actually_narrows_a_mixed_project(self) -> None: + """The teeth of the previous test: prove it drops non-matching rows. + + `all(...)` over an empty or single-event list is vacuously true -- + exactly how the ignored-``?event=`` bug survived review. This one + needs a project carrying at least two distinct events and skips + loudly otherwise. + """ + _step(3, "notification list --event ") + everything = _json_ok(self._run("notification", "list", "--project", self.alias)) + rows = everything["subscriptions"] + events = {row["event"] for row in rows} + if len(events) < 2: + pytest.skip( + f"project has {len(events)} distinct notification event(s); " + "need >= 2 to prove --event narrows" + ) - This is exactly why the CLI does not validate `--event`: the check - would have to be a client-side allowlist that goes stale the moment - the platform adds an event. + target = sorted(events)[0] + expected = sum(1 for row in rows if row["event"] == target) + filtered = _json_ok( + self._run("notification", "list", "--project", self.alias, "--event", target) + ) + + assert len(filtered["subscriptions"]) == expected + assert len(filtered["subscriptions"]) < len(rows) + + def test_api_side_event_filter_is_still_ignored(self) -> None: + """Pin the upstream behavior the client-side narrowing exists for. + + This has to observe the service's RAW answer, so it calls the L3 + client directly -- every CLI path narrows before the caller sees + anything, and asserting that kbagent *sent* ``?event=`` would stay + true whether or not the service honors it, i.e. would be no canary + at all. + + If the service ever starts honoring the parameter, the assertion + below fails, which is the notification we want: the narrowing in + ``NotificationService`` becomes redundant and the ``gotchas.md`` + entry needs retiring. It is a canary, not a correctness requirement, + so it skips unless the project carries the mixed data needed to tell + the two behaviors apart. + """ + _step(4, "raw ?event= behavior canary") + everything = _json_ok(self._run("notification", "list", "--project", self.alias)) + events = {row["event"] for row in everything["subscriptions"]} + if len(events) < 2: + pytest.skip("need >= 2 distinct events to observe the API-side filter") + + target = sorted(events)[0] + raw = self.client.list_project_subscriptions(event=target) + raw_events = {str(sub.get("event", "")) for sub in raw} + + assert any(event != target for event in raw_events), ( + f"GET /project-subscriptions?event={target} came back containing only " + f"{target!r}: the Notification Service now HONORS the parameter. The " + "client-side narrowing in NotificationService is redundant, and the " + "gotchas.md / CLAUDE.md / context.py notes saying it is ignored are " + "now wrong -- retire them." + ) + + def test_unknown_event_returns_empty_not_error(self) -> None: + """A misspelled event yields no rows, not a failure and not everything. + + `EventName` is an open string in the schema, which is why the CLI does + not validate `--event` against an allowlist that would go stale the + moment the platform adds an event. The empty result is kbagent's doing: + the service answers a bogus `?event=` with 200 and the project's FULL + subscription list, so without the client-side narrowing a camelCase + typo like `jobFailed` would return every subscription in the project + and read as "these all fire on that event". """ - _step(3, "notification list --event ") + _step(5, "notification list --event ") result = self._run( "notification", "list", @@ -5589,7 +5676,7 @@ def test_unknown_event_returns_empty_not_error(self) -> None: def test_config_filter_counts_excluded_catchalls(self) -> None: """Project-wide subscriptions dropped by a scope filter must be counted.""" - _step(4, "notification list --component-id keboola.flow") + _step(6, "notification list --component-id keboola.flow") unfiltered = _json_ok(self._run("notification", "list", "--project", self.alias)) # Only the rows the filter DROPS are counted -- a project-wide # subscription that filters on keboola.flow survives and is shown. @@ -5615,7 +5702,7 @@ def test_config_filter_counts_excluded_catchalls(self) -> None: def test_detail_round_trips_a_listed_subscription(self) -> None: """`detail` must resolve the same row `list` reported.""" - _step(5, "notification detail -- round-trip from list") + _step(7, "notification detail -- round-trip from list") listed = _json_ok(self._run("notification", "list", "--project", self.alias)) if not listed["subscriptions"]: pytest.skip("project has no notification subscriptions to inspect") @@ -5637,7 +5724,7 @@ def test_detail_round_trips_a_listed_subscription(self) -> None: assert detail["address"] == expected["address"] def test_detail_of_missing_subscription_fails_cleanly(self) -> None: - _step(6, "notification detail -- unknown ID") + _step(8, "notification detail -- unknown ID") result = self._run( "notification", "detail", diff --git a/tests/test_notification_service.py b/tests/test_notification_service.py index 0a53b1f7..0c23d55c 100644 --- a/tests/test_notification_service.py +++ b/tests/test_notification_service.py @@ -325,7 +325,35 @@ def test_no_join_call_when_nothing_is_config_scoped(self) -> None: client.list_component_configs.assert_not_called() assert result["subscriptions"][0]["config_name"] == "" - def test_event_filter_is_forwarded_to_the_api(self) -> None: + def test_event_filter_is_applied_client_side(self) -> None: + """The live service IGNORES ``?event=`` and returns everything. + + Verified against a real stack: ``GET /project-subscriptions?event= + job-failed`` answers 200 with every subscription in the project, + including ``job-succeeded`` ones. The swagger documents the parameter, + so it is still sent -- but the rows must be narrowed here too, or + ``--event job-failed`` silently answers "who gets paged on failure" + with a superset that includes success recipients. + """ + client = MagicMock() + # The mock ignores the kwarg exactly like the live service does. + client.list_project_subscriptions.return_value = [ + FLOW_SUBSCRIPTION, + { + "id": "1240", + "event": "job-succeeded", + "filters": [{"field": "job.configuration.id", "value": "98765"}], + "recipient": {"channel": "email", "address": "ok@example.com"}, + }, + ] + _wire_configs(client, []) + + result = _make_service(client).list_subscriptions(event="job-failed") + + assert [s["event"] for s in result["subscriptions"]] == ["job-failed"] + + def test_event_filter_is_still_sent_to_the_api(self) -> None: + """Keep sending it: harmless today, correct for free if it is fixed.""" client = MagicMock() client.list_project_subscriptions.return_value = [] @@ -333,6 +361,16 @@ def test_event_filter_is_forwarded_to_the_api(self) -> None: client.list_project_subscriptions.assert_called_once_with(event="job-failed") + def test_unknown_event_narrows_to_nothing(self) -> None: + """A typo must not silently return the whole project.""" + client = MagicMock() + client.list_project_subscriptions.return_value = [FLOW_SUBSCRIPTION] + _wire_configs(client, []) + + result = _make_service(client).list_subscriptions(event="jobFailed") + + assert result["subscriptions"] == [] + def test_component_filter_is_applied_client_side(self) -> None: """The API has no component filter -- only ``?event=``.""" client = MagicMock()