From 0d47e9da4ddd596dd9417ef5a48dfb61278b40eb Mon Sep 17 00:00:00 2001 From: Petr Date: Wed, 19 Aug 2026 23:36:34 -0400 Subject: [PATCH 1/2] fix(notification): narrow --event client-side; the service ignores ?event= Found by exercising the merged 0.86.0 feature against a real project with actual Notifications-tab subscriptions. The Notification Service ACCEPTS its documented `?event=` query parameter and then ignores it: `GET /project-subscriptions?event=job-failed` answers 200 with the project's FULL subscription list. `--event job-failed` therefore returned success and processing-delay recipients too -- a silent superset, which for an audit command is worse than an error: it looks like the answer to "who gets paged when this breaks" and is not. A camelCase typo like `jobFailed` returned everything rather than nothing. Same trap issue #600 itself documents for Storage's `list_files`: a documented query parameter states intent, not behavior. The parameter is still sent (a server-side fix would then cost nothing), and the rows are narrowed here. `branch_id` also turned out to be populated on EVERY subscription, production included -- the Flow Builder always writes a `branch.id` filter and uses the default branch's own numeric id for production. The shipped docs said presence implied a branch-specific subscription, which would have led an agent to read a live production alert as dev-branch-only. Corrected in gotchas.md, CLAUDE.md, context.py and commands-reference.md. Confirmed correct against the same live data: kebab-case event names, dotted filter paths (`job.component.id`, `job.configuration.id`, `branch.id`), webhook `url` normalizing into the `address` column, `durationOvertimePercentage >= 0.7` surviving into `detail`, and the config-name join. E2E is where this should have been caught, so it is strengthened rather than just patched: the existing `all(row["event"] == ...)` assertion was vacuously true on a project with no subscriptions, which is exactly how the bug survived. Data-dependent assertions now `pytest.skip` with an explicit reason instead of passing silently, plus a test that proves `--event` actually drops rows on a mixed project and a canary that flips to failing if the service ever starts honoring the parameter. Unit coverage pins the client-side narrowing with a mock that ignores the kwarg exactly like the live service does. 0.86.0 is on main but not yet released, so this folds into that entry rather than shipping a 0.86.1 for a bug no user ever saw. --- CLAUDE.md | 14 ++- .../kbagent/references/commands-reference.md | 6 +- .../skills/kbagent/references/gotchas.md | 35 ++++-- src/keboola_agent_cli/changelog.py | 5 + src/keboola_agent_cli/commands/context.py | 25 +++-- .../services/notification_service.py | 10 ++ tests/test_e2e.py | 104 ++++++++++++++++-- tests/test_notification_service.py | 40 ++++++- 8 files changed, 201 insertions(+), 38 deletions(-) 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..fa439062 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -1218,11 +1218,21 @@ 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 @@ -1250,11 +1260,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..31cc6429 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -48,6 +48,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/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..a0f2e719 100644 --- a/src/keboola_agent_cli/services/notification_service.py +++ b/src/keboola_agent_cli/services/notification_service.py @@ -326,6 +326,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..d76365c3 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) @@ -5559,22 +5568,95 @@ 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" + ) + + 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 this client-side filter exists for. - 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. + If the service ever starts honoring ``?event=``, this test flips to + failing -- which is the notification we want: the client-side filter + becomes redundant (harmless, but the comment explaining it is then + wrong) and the gotchas entry needs retiring. It is a canary, not a + correctness requirement, so it skips unless the project has 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)) + rows = everything["subscriptions"] + events = {row["event"] for row in rows} + if len(events) < 2: + pytest.skip("need >= 2 distinct events to observe the API-side filter") + + target = sorted(events)[0] + raw = _invoke( + self.config_dir, + [ + "--json", + "--verbose", + "notification", + "list", + "--project", + self.alias, + "--event", + target, + ], + ) + # The request really is sent with the parameter -- the narrowing that + # reaches the user is kbagent's, not the service's. + assert f"event={target}" in raw.output + + 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 +5671,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 +5697,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 +5719,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() From 91f569599ddac42d55e24c7d051878a5155a94b2 Mon Sep 17 00:00:00 2001 From: Petr Date: Wed, 19 Aug 2026 23:45:06 -0400 Subject: [PATCH 2/2] fix(notification): purge the stale "server-side ?event=" claim; give the canary teeth Two findings from Devin's review of this PR. The first is a self-inflicted one worth naming: three surfaces still carried the pre-fix wording, sitting next to the new note that says the opposite. The changelog line survived because the previous pass ran two replacements in one script and the SECOND assert raised before write_text() -- so the first, successful replacement was silently discarded. This pass writes each replacement immediately for that reason. Also corrected beyond what was flagged: `NotificationService.list_subscriptions`' own docstring still said the event filter was "applied server-side", and the L3 `list_project_subscriptions` docstring described the parameter without warning that it does nothing. That last one matters most -- SDK callers get the raw client with no narrowing behind it, so its docstring now carries an explicit warning that the method DOES NOT narrow. The second is fair and the test was worthless as written: `test_api_side_event_filter_is_still_ignored` asserted that kbagent SENT `?event=`, which stays true whether or not the service honors it -- no canary at all. It also depended on --verbose logging reaching `result.output`, which is unreliable: `logging.basicConfig` is a no-op once the root logger has a handler from an earlier `_invoke` in the same process, and CliRunner's stderr capture varies by click version. It now calls the L3 client directly, so it observes the service's RAW answer -- every CLI path narrows before a caller sees anything -- and asserts that a filtered request still returns a non-matching event. If the service starts honoring the parameter, that fails with a message naming exactly what to retire. --- .../skills/kbagent/references/gotchas.md | 7 ++- src/keboola_agent_cli/changelog.py | 4 +- src/keboola_agent_cli/client/notifications.py | 15 ++++- .../services/notification_service.py | 4 +- tests/test_e2e.py | 55 ++++++++++--------- 5 files changed, 52 insertions(+), 33 deletions(-) diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index fa439062..588d1539 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -1239,9 +1239,10 @@ events and emits a final `done` SSE frame mirroring the same record. `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 diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index 31cc6429..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 " 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/services/notification_service.py b/src/keboola_agent_cli/services/notification_service.py index a0f2e719..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``. diff --git a/tests/test_e2e.py b/tests/test_e2e.py index d76365c3..c06a607f 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -5543,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]) @@ -5611,39 +5617,38 @@ def test_event_filter_actually_narrows_a_mixed_project(self) -> None: assert len(filtered["subscriptions"]) < len(rows) def test_api_side_event_filter_is_still_ignored(self) -> None: - """Pin the upstream behavior this client-side filter exists for. - - If the service ever starts honoring ``?event=``, this test flips to - failing -- which is the notification we want: the client-side filter - becomes redundant (harmless, but the comment explaining it is then - wrong) and the gotchas entry needs retiring. It is a canary, not a - correctness requirement, so it skips unless the project has the mixed - data needed to tell the two behaviors apart. + """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)) - rows = everything["subscriptions"] - events = {row["event"] for row in rows} + 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 = _invoke( - self.config_dir, - [ - "--json", - "--verbose", - "notification", - "list", - "--project", - self.alias, - "--event", - target, - ], + 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." ) - # The request really is sent with the parameter -- the narrowing that - # reaches the user is kbagent's, not the service's. - assert f"event={target}" in raw.output def test_unknown_event_returns_empty_not_error(self) -> None: """A misspelled event yields no rows, not a failure and not everything.