feat(notification): audit Flow Notification subscriptions fleet-wide (#600) - #602
Draft
padak wants to merge 1 commit into
Draft
feat(notification): audit Flow Notification subscriptions fleet-wide (#600)#602padak wants to merge 1 commit into
padak wants to merge 1 commit into
Conversation
…600) The Flow Builder Notifications tab -- the bell icon's Success / Error / Processing-delay cards -- is backed by the Notification Service, not by the flow's configuration, so flow detail and config detail never could show it. It was the one notification surface with no CLI path at all: on the 20-project / 276-flow fleet that prompted the issue, every other surface was auditable in minutes while the recipients that actually page someone when production breaks required opening each flow in the UI by hand. kbagent notification list [--project ALIAS ...] [--event NAME] [--component-id ID] [--config-id ID] [--branch ID] Reads GET /project-subscriptions on the derived notification.{stack} host with a plain Storage token, fans out across every registered project in parallel, and collects per-project failures in `errors` instead of aborting the run. Mirrored as GET /notifications on `kbagent serve`. Read-only by construction: the service's create/delete endpoints change who gets paged when production breaks, and the HTTP dispatcher takes no method argument -- the same guarantee _billing_get gives against a real-money top-up. Three contract details follow the service's own OpenAPI rather than the shapes proposed in the issue: event names are kebab-case (job-failed, not jobFailed) and free-form rather than an enum; subscriptions bind to a flow through dotted filter fields (job.component.id, job.configuration.id, branch.id); and a recipient is a discriminated union where email carries `address` and webhook carries `url`. Two shapes that would otherwise read as broken rows are reported honestly: a subscription with no config filter is the catch-all (scope project-wide), and one pointing at a deleted configuration keeps its id with an empty config_name -- a finding, not an error. The endpoint is not branch-scoped, so --branch filters client-side and is never inferred from the project's active branch; inheriting it would silently hide the production recipients the audit exists to check.
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
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.
What
Closes the last flow-notification blind spot described in #600. The Flow Builder Notifications tab — the bell icon's Success / Error / Processing-delay cards — is backed by the Notification Service, not by the flow's
configuration, soflow detailandconfig detailnever could show it. On the 20-project / 276-flow fleet in the issue, every other notification surface (owner emails in descriptions, in-flowtype: "notification"tasks, email-sender component configs) was auditable in minutes; these recipients — the ones that actually page someone when production breaks — required opening each flow in the UI by hand.Reads
GET /project-subscriptionson the derivednotification.{stack}host with a plain Storage token (no elevated scope), fans out over every registered project in parallel, and collects per-project failures inerrorsrather than aborting the run. Mirrored asGET /notificationsonkbagent serve, per the 1:1 convention.Follows the structure the issue proposed and the
billing creditsPR (#597) established:client/notifications.pymixin →services/notification_service.py→commands/notification.py, pluspermissions.py(notification.list = read, safe under--deny-writes).Read-only by construction. The service also exposes create/delete for subscriptions — changing who gets paged when production breaks.
_notification_gethardcodes the verb like_billing_getdoes against a real-money top-up, so no future caller can reach the write path through the dispatcher at all.Contract details that differ from the issue's proposal
I fetched the service's public OpenAPI rather than coding to the shapes in the issue text, and three of them differ. Each one would have produced a command that silently returns nothing:
jobFailed,jobSucceeded, …job-failed,job-succeeded,job-succeeded-with-warning,job-processing-long,phase-job-*configurationId/componentjob.component.id,job.configuration.id,branch.id{channel, address}emailcarriesaddress,webhookcarriesurleventis typed as a free-form string, not an enum, so no client-side allow-list is imposed: an unknown value gets the API's own 400, and a newly shipped event type works the day Keboola ships it.The issue's two open questions, answered
Are subscriptions branch-aware? Yes, but as a filter, not a scope: a dev-branch subscription carries a
branch.idfilter, a production one carries none, and the list endpoint returns both together. So--branchfilters client-side and — unlike every other branch-aware command — is never inferred from the project's active branch. Inheriting it would silently hide the production recipients an audit exists to check; there is a test pinning exactly that. A branch id is meaningful in one project only, hence--branchrequires exactly one--project(exit 2 otherwise).Do project-wide subscriptions exist? Yes — a subscription with no config filter fires for every job in the project. It degrades to
scope: "project-wide"with an emptyconfig_idrather than erroring, and is often the most important row in an audit. Likewise a subscription pointing at a deleted configuration keeps its id with an emptyconfig_name— a finding, not an error.Name resolution cost
config_nameis joined with onelist_component_configscall per distinct (branch, component) pair actually referenced — in practice one, and none at all when every subscription is project-wide.ScheduleService'slist_components_with_configswas deliberately not reused: it downloads every configuration body in the project (megabytes on a 276-flow project) to recover a handful of names. Grouping by branch matters because a dev-branch config is invisible from production, and looking it up in the wrong branch would report a live flow as deleted.How it was tested
make checkgreen: lint, format, typecheck, skill, version, command-sync, changelog, error-codes, sentinel-guards, file-size, 5786 tests passed.tests/test_notification_client.py(7) — derived host,X-StorageApi-Tokenon the wire,?event=passthrough, clean URL without it, wrapped/garbage payload tolerance, and a test asserting the dispatcher exposes nomethodparameter.tests/test_notification_service.py(16) — every row shape above, both recipient shapes, threshold filters surviving verbatim, per-branch name resolution, client-side filters, fan-out with a failing project, client cleanup, unknown alias.tests/test_notification_cli.py(13) — JSON envelope, human table, project-wide labelling, dangling-config fallback, argument forwarding,--branchguard, per-project warnings at exit 0,--deny-writes.tests/test_server_router_calls.py(3) — query params → service kwargs, envelope returned verbatim.tests/test_e2e.py::TestE2ENotificationList(3) — envelope shape and invariants against a live project; row assertions run only if the project has subscriptions, so it starts covering the populated path without a rewrite.No live-API verification of a populated list: that needs a project with Notifications-tab recipients, which I do not have. The wire contract comes from the service's published OpenAPI, and every shape it documents is covered by a test.
Doc surfaces (convention #17)
Updated:
context.pyAGENT_CONTEXT,CLAUDE.md,SKILL.md(viamake skill-gen),commands-reference.md,gotchas.md, newnotification-workflow.md,changelog.py.Not updated — needs a maintainer decision:
plugins/kbagent/agents/keboola-expert.md. It is at 61,991 bytes against the 62,000-byte budget enforced bytests/test_agent_prompt.py— 9 bytes free, so a new command-group row does not fit without trimming content I would only be guessing is stale. CONTRIBUTING's guidance is to trim rather than raise the cap, or split into per-domain specialists. The practical gap is small: the skill runskbagent contextas its first step, andAGENT_CONTEXTdoes carry the new group in full.Note for the maintainer
Changelog entries sit under
0.84.2, the current top key, sincemainis exactlyv0.84.2and there is no unreleased key. Renumber at release time. The version is not bumped.Fixes #600