feat(notification): audit Flow Notifications-tab subscriptions (#600)

[padak]

Closes #600.

Adds a read-only kbagent notification command group over the Notification Service, closing the last unauditable notification surface: the Flow Builder's Notifications tab (bell icon — Success / Error / Processing-delay / Warning cards).

Those recipients live in a separate platform service, 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 stays visible there.

kbagent notification list [--project ALIAS ...] [--event NAME] [--component-id ID] [--config-id ID]
kbagent notification detail --project ALIAS --subscription-id ID

Layers

Follows the queue / schedule precedents exactly — no new abstraction.

Layer	File	What
L3	client/notifications.py + client/_core.py	_NotificationsMixin over a notification.{stack} sibling host derived by _derive_service_url, plus the _notification_request / sub-client / close() plumbing
L2	services/notification_service.py	Parallel fan-out with per-project error accumulation; config-name join
L1	commands/notification.py	Thin Typer layer, registered under the Flows help panel and as read-only ops in the permission registry
REST	server/routers/notifications.py	GET /notifications and GET /notifications/{project}/{subscription_id} — the 1:1 kbagent serve mirror CONTRIBUTING.md requires

Authenticates with the plain project Storage token every registered alias already holds — no elevated scope, no manage token.

Wire-format corrections

The issue's draft got three details wrong; all were verified against the service's public swagger and are written up in the issue thread.

1. Event names are kebab-case — job-failed, job-succeeded, job-succeeded-with-warning, job-processing-long and the phase-job-* variants, not jobFailed. EventName is also an open string with no enum, so --event is forwarded verbatim and deliberately not validated against a client-side allowlist that would go stale the moment the platform adds an event.
2. Filter fields are dotted paths into the event payload — job.component.id, job.configuration.id, branch.id, phase.id, durationOvertimePercentage — not flat configurationId / component keys. This was the one that mattered: with the flat keys the resolved flow_name column would have been silently empty on every row.
3. Webhook recipients carry url, email recipients carry address. Both normalize into one address column.

Both of the issue's open questions are answered: branch.id is a supported filter (so subscriptions can be branch-scoped even though the endpoint has no branch parameter), and filters is optional — a filter-less project-wide subscription is legal and common.

One design decision beyond the issue

--component-id / --config-id exclude project-wide subscriptions (the ones with no filters) — but those also page for the config being audited. Silently dropping them would answer "who gets paged when this flow breaks" wrongly, which is the exact failure mode the issue was opened about. They are therefore counted in project_wide_excluded and surfaced as a warning in human mode.

Two smaller ones:

• The config-name join is skipped entirely when no row is config-scoped. The list_components_with_configs payload is proportional to the whole project, and a project full of catch-alls would otherwise pay for a join that can only produce blanks.
• config_name resolves by exact (component_id, config_id) match, falling back to a config-ID lookup only when unambiguous. Two components sharing a config ID, a deleted parent, or a failed lookup all yield "" rather than a guess — a wrong flow name in an alert audit is worse than a blank one.

Review round 1

Four findings from the automated review, all verified before acting — fixed in 8b931d6:

• project_wide_excluded over-reported. scope keys off the config filter alone, so a subscription filtering only on job.component.id is labelled project-wide — but --component-id keeps it. The counter summed every project-wide row rather than only the dropped ones, so a visible row was also warned about as hidden. Now counted in the same pass that partitions the rows.

• Component column rendered literal [dim]any[/dim]. The fallback markup was inside escape(). Moved out, matching _config_cell.

• No REST routes. Genuine CONTRIBUTING.md violation — no skip applies to plain JSON reads. Router added (see the table above).

• Config-name join fetched the whole project. list_components_with_configs sends include=configuration,rows to map an ID to a display name. Now one list_component_configs per distinct component when every config-scoped row names its component; the whole-project listing is used only for a bare-config-ID row, where resolving it does require searching every component.

  One detail in that finding's reasoning was wrong and is worth recording: ScheduleService uses the same list_components_with_configs call (schedule_service.py:521, :585) and documents the trade-off — it needs the config bodies. The stale list_component_configs mention in its module docstring is what the comparison picked up. The optimization stands on its own here regardless, since this service only needs names.

Testing

make check green: 5620 passed.

• tests/test_notification_client.py (10) — host derivation, query params, path quoting, sub-client lifecycle
• tests/test_notification_service.py (27) — field extraction, fan-out, client-side filters, join fallbacks, exclusion counter, error accumulation
• tests/test_notification_cli.py (12) — JSON envelope, human rendering, warnings, exit codes, --deny-writes
• tests/test_e2e.py::TestE2ENotificationSubscriptions (6) — envelope contract, --event round-trip, unknown-event-returns-empty, exclusion counter, detail round-trip

Live verification: GET https://notification.europe-west3.gcp.keboola.com/project-subscriptions → HTTP 200 with a plain project Storage token, fanned out across 4 real projects on a GCP stack, no errors.

Not verified live: row shaping against real data — the projects available to me have zero subscriptions, so the E2E class asserts the envelope contract unconditionally and the per-row shape only when rows exist. It starts covering the row path the day a subscription exists in an E2E fixture, without needing a rewrite. The same gap means it is still unconfirmed whether the Flow Builder UI actually writes a branch.id filter when a subscription is created inside a dev branch — the schema supports it, the producer is unverified.

Scope

Read-only, per the issue's stated non-goals. POST / DELETE /project-subscriptions are a natural follow-up. POST /notifications (send a direct notification) is deliberately out — it needs a Manage API application token with the notifications:send-notification scope, so it belongs with the rest of the default-deny manage-token surface, not here.

Docs

Synced per convention #17: CLAUDE.md, context.py, commands-reference.md, SKILL.md, gotchas.md, keboola-expert.md. Changelog entry + version bump to 0.86.0.

Two limits bit during the doc sync and are fixed in the second commit: the changelog headline truncates at 160 chars, and Claude Desktop rejects a skill description over 1024 characters (the description was already at ~997).

Note for a follow-up

CLAUDE.md convention #17 lists context.py and the ## All CLI Commands section as silent-drift risks "not CI-enforced". They are enforced today by scripts/check_command_sync.py — it failed on both while I was building this. Left alone here to keep the diff on-topic; happy to fix in a separate PR.

[devin-ai-integration]

Devin Review found 4 potential issues.

[padak]

Review of #615 — feat(notification): audit Flow Notifications-tab subscriptions (#600)

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR adds a new read-only kbagent notification command group (list/detail) across all three layers plus the kbagent serve REST mirror, closing a real audit gap: Flow Builder Notifications-tab recipients live in a separate platform service and were previously invisible to flow detail/config detail. Layering, permission registration, and the entire hand-maintained plugin-sync surface (CLAUDE.md, commands/context.py, keboola-expert.md §2 matrix row, gotchas.md, commands-reference.md, SKILL.md description/table) are all present and consistent, make check is green (5633 passed), and the diff shows a healthy iteration history already responding to a prior automated review round (Devin) that fixed an over-counting bug in project_wide_excluded. My own pass found one residual edge case in that same counter that the prior fix didn't close — see NB-1 — plus a couple of small cosmetic notes. Verdict: COMMENT (no blocking findings).

Verdict

• Verdict: COMMENT
• Blocking findings: 0
• Non-blocking findings: 2
• Nits: 1

Blocking findings

(none)

Non-blocking findings

[NB-1] src/keboola_agent_cli/services/notification_service.py:116 + :306-313 — project_wide_excluded still over-counts when --component-id is filtered against a subscription scoped to a different, specific component

scope is derived purely from whether a subscription has a job.configuration.id filter (services/notification_service.py:116: SCOPE_CONFIG if config_id else SCOPE_PROJECT_WIDE), ignoring whether it does carry a job.component.id filter for some other component. The exclusion-counting loop (:306-313) then counts any unmatched row whose scope == SCOPE_PROJECT_WIDE as "would also fire for the audited config/component."

Concretely: a project with subscription A = {filters: [{field: "job.component.id", value: "keboola.ex-db-snowflake"}]} (no config filter) is unambiguously scoped to a different component and can never fire for a keboola.flow job. Running notification list --component-id keboola.flow still counts subscription A in project_wide_excluded and the human-mode warning at commands/notification.py:178 ("They have no configuration filter, so they also fire for this one") is factually wrong for this row — it will never fire "for this one."

This is a narrower residual of the exact bug class the prior Devin-review fix (commit e6f12b5) addressed for matched rows; that fix correctly stopped counting kept rows as excluded, but didn't add the extra check needed for unmatched-but-provably-irrelevant rows (an explicit, non-matching component_id filter). tests/test_notification_service.py::test_component_filter_is_applied_client_side (line ~329) exercises exactly this scenario but only asserts the kept-row list, not project_wide_excluded, so the gap is untested. Not blocking because the bias is still toward over- rather than under-reporting (the PR's stated safety invariant holds), but it will produce misleading "who else gets paged" noise for the exact incident-response workflow this feature targets. Suggested fix: exclude a row from the project_wide_excluded count when it carries its own non-matching, present component_id filter (i.e., only count rows where the missing dimension is genuinely ambiguous, not ones whose explicit filter proves them irrelevant).

[NB-2] src/keboola_agent_cli/services/notification_service.py:120-122 — new tuple[...] return is a candidate for a small dataclass

_build_config_indexes(...) -> tuple[dict[tuple[str, str], str], dict[str, list[str]]] is a newly-added, heterogeneous two-element tuple (by_pair vs names_by_config_id) — exactly the case CONTRIBUTING.md's "Return values" section calls out ("even two-element tuples should use a dataclass when the values are semantically distinct"). A tiny @dataclass(frozen=True) with by_pair / names_by_config_id fields would make the two call sites (_stamp_config_names) self-documenting instead of relying on unpacking order. Low priority — the function is _-private with two call sites and both are in the same file.

Nits

• [NIT-1] src/keboola_agent_cli/services/notification_service.py:284 — _fetch_project_subscriptions returns a 4-tuple (alias, rows_or_error_dict, excluded, ok) on success but a 2-tuple (alias, error_dict) on failure (see the except branches around line ~360). Both shapes are consumed positionally in list_subscriptions (result[1], result[2]), which works only because _run_parallel sorts successes/errors into separate buckets first. This is grandfathered under the BaseService parallel-result convention (tuple[str, ...] | tuple[str, dict]) so it's not a Code-Quality-Patterns violation, but the differing arity between the two tuple shapes in the same function is easy to trip over on a future edit — worth a one-line comment at the two return sites noting the arity contract if this file is touched again.

Verification log

• git rev-parse --abbrev-ref HEAD → claude/issue-600-3d974b (matches <branch>), working tree clean, gh pr view 615 --json state → OPEN ✓
• gh auth status → authenticated as padak ✓
• Read CONTRIBUTING.md (Checklist: Adding a New CLI Command, Plugin synchronization map, Releasing a new version), CLAUDE.md convention #17 + ## All CLI Commands, plugins/kbagent/agents/keboola-expert.md §1/§3 (and confirmed §2 already carries the new matrix row) ✓
• gh pr view 615 --json title,body,files,additions,deletions,... → 26 files, +2213/-7, feat(notification): ... conventional prefix matches (new read-only feature) ✓
• gh pr diff 615 → 2576 lines, reviewed in full ✓
• git diff main...HEAD -- pyproject.toml changelog.py plugin.json marketplace.json → version bumped 0.85.1 → 0.86.0 consistently across all four files; changelog entry present and well-formed (3 bullets, each starting with a recognized style); survived the rebase onto #614 (d6c6ef8) intact ✓
• Layer-violation greps (typer/formatter in services, httpx in commands, formatter/typer in clients) on the diff → all empty ✓
• Magic-number / raw-error-code-string / bare-except / stray-print() / token-leakage greps on the diff → empty except two raw error_code="..." string literals, both confirmed to be in test files (tests/test_notification_cli.py:295, tests/test_notification_service.py:429,466,507) constructing mock KeboolaApiError objects, not production code — make check-error-codes (part of make check) passed, confirming these are not flagged ✓
• New -> tuple[...] return added in this diff → exactly one hit, _build_config_indexes (see NB-2); the two tuple[Any, ...] / tuple[str, ...] | tuple[str, dict] worker-callback shapes match the grandfathered BaseService parallel-result convention and are correctly skipped ✓
• make check (background, ~124s) → 5633 passed, 12 skipped, 158 deselected, exit 0 — lint, format, typecheck, skill freshness, version sync, command-sync (permissions/CLAUDE.md/context.py/commands-reference.md drift gate), changelog-check, error-code enum, sentinel-guards, full test suite all green ✓
• uv run python scripts/check_sentinel_guards.py --list → OK: ... all 10 guards covered by SESSION_UNSUPPORTED_FEATURES (8 entries) — the new _notification_client sub-client + _notification_request plumbing did not trip the session-sentinel guard (bearer-capable like billing/queue, correctly classified) ✓
• Read services/notification_service.py in full (406 lines) and traced _matches_scope / scope classification / exclusion-counting logic line by line against the commit history (git show e6f12b5, the prior Devin-review fix) to confirm what was already fixed vs. what remains — see NB-1 ✓
• Read _stamp_config_names join-path selection logic (per-component list_component_configs vs whole-project list_components_with_configs) and cross-checked against tests/test_notification_service.py (test_component_scoped_join_uses_the_cheap_per_component_listing, test_component_less_row_falls_back_to_the_whole_project_listing, test_each_component_is_listed_once_for_many_subscriptions, test_no_join_call_when_nothing_is_config_scoped) → logic and tests both correct: whole-project listing is used only when at least one config-scoped row lacks a component filter, otherwise one call per distinct component ✓
• Read commands/notification.py (254 lines) → thin, both JSON and human-mode paths present, permission callback matches the schedule/branch precedent exactly (check_cli_permission(ctx, "notification") → notification.list/notification.detail keys, both registered as "read" in permissions.py) ✓
• server/routers/notifications.py, test_server_router_calls.py, test_server_smoke.py diffs → both CLI commands have a 1:1 REST mirror (GET /notifications, GET /notifications/{project}/{subscription_id}), registered in server/app.py (router include + OpenAPI tag) and server/dependencies.py (ServiceRegistry.notification), with request-forwarding tests and a smoke-test path entry ✓
• Attempted live reproduction against a real Keboola project: no credentialed config.json / registered project was available in this sandbox environment (no ~/.kbagent/config-dir with a live token was present or accessible without handling a token myself, which is out of scope per policy) — could not independently hit a live notification.{stack} host. The claimed wire-format corrections (kebab-case event names, dotted filter paths, address vs url recipient discrimination) are stated in the PR description as verified against the service's public swagger with a link to the issue thread; I did not independently re-verify against the swagger. Flagging as unverified rather than asserting correctness.

Open questions for the author

• Was [NB-1]'s scenario (a component-scoped-but-config-less catch-all for a different component, audited via --component-id for another component) considered and deliberately left as an acceptable over-report, or is it worth a follow-up fix? Given the PR already iterated once on this exact counter with the automated reviewer, a quick disposition either way (fix now vs. tracked as a known limitation in gotchas.md) would close the loop cleanly.

[padak]

All three findings addressed in 6a2aac6. Disposition on the open question first, since it was the substantive one.

NB-1 — fixed, not accepted as a known limitation

Not deliberate. The prior round fixed the kept-row half of this counter and I stopped there without walking the dropped side, so this is the same bug class left half-closed — a fair catch.

_could_also_fire() now gates the count: a dropped row increments project_wide_excluded only when the mismatch comes from an absent constraint. A subscription carrying its own explicit job.component.id for a different component is provably irrelevant to the audited one and is no longer counted.

What still over-counts, deliberately: a subscription filtering on a component but not a config, dropped by --config-id, is counted even though the audited config might belong to a different component. Deciding that needs a config→component lookup the counter does not have, and the ambiguity is real rather than provable either way — so it stays biased toward over-reporting.

test_component_filter_is_applied_client_side did exercise this scenario while asserting only the kept list, exactly as you noted; it now asserts the counter too, plus a dedicated test_subscription_naming_another_component_is_not_counted.

NB-2 — fixed

Checked CONTRIBUTING.md before acting: line 127 ("even two-element tuples should use a dataclass when the values are semantically distinct") and the PR checklist item "No new tuple[...] returns" both apply, and CLAUDE.md rule 0 makes CONTRIBUTING binding — so this is a convention violation I introduced, not a preference. Replaced with a ConfigNameIndex dataclass. It also gave the second index somewhere to explain itself: names_by_config_id exists only so the component-less fallback can detect an ambiguous match instead of guessing at one.

NIT-1 — fixed

Documented the arity contract in the docstring and at both return sites, naming the len(result) == 2 discrimination in _run_parallel as the reason the two shapes cannot move independently.

On the unverified wire-format claims

Reasonable flag given you had no credentialed project. For the record, both halves were verified earlier in the session that produced this branch: the swagger was fetched from https://notification.eu-central-1.keboola.com/docs/swagger.yaml (kebab-case event names with no enum on EventName, dotted job.component.id / job.configuration.id / branch.id / phase.id filter fields, RecipientChannel_Email.address vs RecipientChannel_Webhook.url), and notification list was run against four real projects on a GCP stack — GET https://notification.europe-west3.gcp.keboola.com/project-subscriptions returned HTTP 200 with a plain project Storage token.

The gap that remains, and which the PR description states: those projects have zero subscriptions, so row shaping is exercised only against swagger-shaped fixtures, and it is still unconfirmed whether the Flow Builder UI actually writes a branch.id filter when a subscription is created inside a dev branch. The schema supports it; the producer is unverified.

make check green after the fixes: 5634 passed.

🤖 Addressed by Claude Code