feat(notification): write path — create/delete/replace-recipient subscriptions (#690) - #695
Merged
Conversation
Add create_subscription / delete_subscription / replace_subscription_recipient to NotificationService, building on the Task 1 client write methods. replace_subscription_recipient creates the new subscription before deleting the old one (reverse of the issue's delete-then-recreate sketch): a failed delete leaves a recoverable duplicate instead of risking a lost alert, so it is reported via old_deleted=False plus a warning rather than raised. Old event/filters/expiresAt are carried over verbatim; only the recipient changes.
The delete-old cleanup in replace_subscription_recipient only caught KeboolaApiError, but _do_request lets raw transport failures (httpx ReadError/WriteError/RemoteProtocolError/ProxyError) propagate as-is. After a successful create, such an exception would escape the method, drop new_subscription_id from the response, and risk a retry minting a third subscription. Widen the catch to Exception, log at warning, and keep reporting via old_deleted=False + a warning instead of raising. Also guard a non-dict old recipient the same way the read path does (avoids an AttributeError in favor of the intended ConfigError), and add test coverage for a positive expiresAt pass-through and the non-dict-recipient path that was previously untested.
Adds POST /notifications/{project}, DELETE /notifications/{project}/{subscription_id},
and POST /notifications/{project}/{subscription_id}/replace-recipient, wired to the
notification service's create_subscription/delete_subscription/replace_subscription_recipient
methods (Task 2). Request bodies follow the flows.py pydantic BaseModel convention.
No permission-engine wiring, matching every other non-/auth/* router.
create_subscription's config_id parameter is str | None (the CLI and serve both only ever pass a string). The filter-order test violated that by passing config_id=98765 as an int, which ty flagged as invalid-argument-type. Switch it to config_id="98765" and keep branch_id=4242 as a real int (its declared type), so the assertion still exercises _build_filters's str(branch_id) stringification.
- Update the OpenAPI tag description for the notifications router to reflect the write path (create/delete/replace-recipient), not just the original read-only audit, and regenerate docs/web-server-endpoints.md - Add a test covering the invalid --channel branch of `notification replace-recipient` - Add a pre-flight sweep to the E2E write-path round-trip test so a killed run doesn't leak a project-wide job-failed subscription - Clarify the historical CLAUDE.md notification comment block as describing the read path only, since a write-path block now sits above it
padak
commented
Aug 25, 2026
padak
left a comment
Member
Author
There was a problem hiding this comment.
Review of #695 — feat(notification): write path — create/delete/replace-recipient subscriptions (#690)
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR adds the write path (create / delete / replace-recipient) to the kbagent notification group across all four layers (client, service, CLI, serve router), closes issue #690, and threads every documented silent-drift surface (context.py, CLAUDE.md, commands-reference.md, gotchas.md, keboola-expert.md matrix row, SKILL.md table, permissions.py OPERATION_REGISTRY) with (since vNEXT) tags. It is unusually thorough: the replace-recipient create-before-delete ordering rationale is documented in three places (service docstring, gotchas.md, CLAUDE.md) and matches the actual implementation; the E2E test performs a full self-cleaning create→detail→replace-recipient→delete round trip with a pre-flight leak-sweep. make check passes (6198 tests), the layer boundaries are clean, and I verified live that the permission engine and the --channel validation behave exactly as documented. Verdict: APPROVE — no blocking findings, one minor non-blocking test-coverage nit.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 1
- Nits: 1
Blocking findings
(none)
Non-blocking findings
[NB-1] tests/test_notification_service.py:596-771 — new service tests never assert client.close()
TestCreateSubscription, TestDeleteSubscription, and TestReplaceSubscriptionRecipient all mock client = MagicMock() and exercise the try/finally: client.close() pattern in the service (confirmed present at src/keboola_agent_cli/services/notification_service.py in all three new methods), but none of the ~15 new test methods calls client.close.assert_called_once(). CONTRIBUTING.md > Testing Guidelines explicitly lists this as a required assertion. The implementation is correct (I read the finally blocks), so this is a coverage gap, not a bug — but it means a future regression that drops the finally would go undetected by these new tests.
Nits
[NIT-1]src/keboola_agent_cli/services/notification_service.py:365-576—create_subscription/delete_subscription/replace_subscription_recipientall return a baredict[str, Any]rather than a typed model/dataclass. This mirrors the pre-existinglist_subscriptions/get_subscription_detailshape in the same file (grandfathered), so not a new-tuple violation and not something this PR should be asked to fix alone — flagging only becausereplace_subscription_recipient's return shape (old_subscription_id/new_subscription_id/old_address/old_deleted/warnings/**row) is dense enough that a@dataclasswould read better at call sites; author's call whether it's worth a follow-up.
Verification log
gh pr view 695 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN, basemain, 18 files changed, +1970/-67, title usesfeat(notification):prefix matching the change (new commands = new feature) ✓- Read
CONTRIBUTING.md(Checklist: Adding a New CLI Command, Plugin synchronization map, Releasing a new version) andCLAUDE.mdconvention #17 +## All CLI Commandsbefore reviewing ✓ git rev-parse --abbrev-ref HEAD→claude/issue-690-pr-d250e8, matches<branch>✓ (no checkout needed)- Layer-violation greps (typer/formatter in services, httpx in commands, formatter/typer in clients) → all empty ✓
git diff main...HEAD -- commands/notification.py→ 3 new@notification_app.command(...)decorators:create,delete,replace-recipient✓- Silent-drift surface check (all present, correctly
(since vNEXT)tagged where applicable):permissions.pyOPERATION_REGISTRY→notification.create: write,notification.delete: destructive,notification.replace-recipient: write✓commands/context.pyAGENT_CONTEXT→ full section added under "Notification Subscriptions" ✓CLAUDE.md## All CLI Commands→ 3 new command signatures + explanatory block ✓plugins/kbagent/agents/keboola-expert.md§2 → one new matrix row for the group ("Add / remove / re-point a notification recipient"), correctly a single row not per-command; byte size 51570 / 70000 budget ✓plugins/kbagent/skills/kbagent/references/commands-reference.md→ 3 new bullets ✓plugins/kbagent/skills/kbagent/references/gotchas.md→ new section "Notification subscriptions can now be written..." tagged(since vNEXT)✓plugins/kbagent/skills/kbagent/SKILL.mddecision table → 3 new rows (CI-checked,make skill-checkpassed) ✓docs/web-server-endpoints.md→ regenerated (231→234 operations),make endpoints-checkpassed viamake check✓server/routers/notifications.py→ 3 new routes (POST /notifications/{project},DELETE .../{subscription_id},POST .../{subscription_id}/replace-recipient) matching the CLI 1:1 ✓
grep -E '\-> ?tuple\['on new diff hunks → none found (no new bare-tuple returns) ✓- Convention checks (magic numbers, raw
error_code="..."insrc/, bareexcept:,print(), token leakage) → all clean; the 3 rawerror_code="..."hits found are inside test files constructing mockKeboolaApiErrorobjects, not production code, andmake check-error-codes(part ofmake check) passed ✓ make check→ exit 0,6198 passed, 12 skipped(unit test job), lint/format/typecheck/skill/version/command-sync/endpoints/changelog/error-codes/sentinel-guards/file-size all green ✓- Test coverage by layer: service 19 new
def test_, CLI 16 new, client 4 new,test_server_router_calls.py5 new,tests/test_e2e.py1 new (test_create_replace_recipient_delete_round_trip, a full write-path round trip) ✓ — all four checklist layers covered - Live behavior verification (against a throwaway local
--config-dir, no real project needed since these are argument-validation / permission-engine paths):kbagent --json notification create --project doesnotexist --event job-failed --channel bogus --address a@b.com→{"error":{"code":"INVALID_ARGUMENT",...}}, exit 2 — matches PR's documentedINVALID_ARGUMENTclaim for the CLI layer ✓kbagent --deny-writes --json notification create --project doesnotexist --event job-failed --channel email --address a@b.com→{"error":{"code":"PERMISSION_DENIED",...}}, exit 6 — confirmsnotification.createis correctly gated aswriteand blocked ✓kbagent notification create/delete/replace-recipient --help→ option lists matchCLAUDE.md's documented signatures exactly ✓- Could not exercise the real create→delete→replace-recipient round trip against a live Keboola project (no credentials in this environment); the author's PR description reports a live run against E2E project 5946 with all steps passing, and
tests/test_e2e.py::TestE2ENotificationSubscriptions::test_create_replace_recipient_delete_round_tripencodes the same flow with self-cleanup — treating this as sufficiently verified rather than re-running it myself
- Reviewed the 3 self-review fixup commits already on the branch (
af9731dawiden delete-guard toException,cd593cf0fix anint/strtype mismatch in a test,1215c62aclose review findings incl. OpenAPI tag description + E2E pre-flight sweep) — evidence the author already ran a self-review pass before this posting, consistent withCONTRIBUTING.md> "Self-review before tagging a human reviewer" ✓
Open questions for the author
(none)
CONTRIBUTING.md's Testing Guidelines require verifying client.close() for the try/finally client-close pattern, but the new create/delete/replace tests never asserted it -- a future regression dropping the finally would go undetected. Add the assertion where it is real signal: one happy-path test per method (create_subscription, delete_subscription, replace_subscription_recipient), delete_subscription's own propagated-404 path (the client must still close even though the exception escapes the method), and replace_subscription_recipient's three try-block error paths where the client was already open when the error hit -- the blank/unknown-channel ConfigError raised before the replacement is created, both non-KeboolaApiError and KeboolaApiError delete-failure branches, and the non-dict-recipient guard. Left the resolve_projects-before-open ConfigError tests (invalid channel, unknown alias) untouched: the client factory is never invoked on those paths, so a close() assertion there would be trivially true rather than real coverage.
Member
Author
|
Review findings addressed:
|
padak
added a commit
that referenced
this pull request
Aug 25, 2026
padak
added a commit
that referenced
this pull request
Aug 25, 2026
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
1 participant
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.
Closes #690.
What
Adds the write path to the
kbagent notificationgroup so the audit workflow from #600 can also fix what it finds:Extends the exact layers #615 built — no new abstraction:
client/notifications.py):create_project_subscription(POST/project-subscriptions,expiresAtcamelCase,filtersomitted when empty) anddelete_project_subscription(DELETE with a path-quoted id), mirroring the existing read methods.services/notification_service.py):create_subscriptionbuildsfiltersfrom friendlycomponent_id/config_id/branch_idparams (same dotted-path constants the read path parses) and returns the canonical audit row;delete_subscriptionis a thin wrapper;replace_subscription_recipientimplements the motivating "fix a stale recipient" case.commands/notification.py): three thin commands; the detail rendering is extracted into a shared_print_subscriptionhelper (detail output unchanged). Invalid--channelis a structuredINVALID_ARGUMENT, exit 2.delete/replace-recipientprompt for confirmation unless--yes(or--json).notification.create= write,notification.replace-recipient= write,notification.delete= destructive — so--deny-writesblocks all three and--deny-destructiveblocks onlydelete, matching the gate the issue asked for.POST /notifications/{project},DELETE /notifications/{project}/{subscription_id},POST /notifications/{project}/{subscription_id}/replace-recipient(read routes already existed; body convention mirrorsrouters/flows.py; no permission-engine wiring, consistent with every router except/auth/*).docs/web-server-endpoints.mdregenerated. Note: channel validation lives in the CLI command layer, so over REST an invalid channel surfaces as the service'sConfigError(HTTP 400CONFIG_ERROR) rather thanINVALID_ARGUMENT— the docs scope theINVALID_ARGUMENT/exit-2 claim to the CLI.context.pyAGENT_CONTEXT,commands-reference.md,gotchas.md,keboola-expert.md, SKILL.md decision table — all tagged(since vNEXT). No version bump, no changelog entry (feature PR per docs: version bumps move out of feature PRs into dedicated release PRs #648 process).One deliberate deviation from the issue sketch
The issue sketches
replace-recipientas get → delete → recreate. This PR orders it create-new-first, then delete-old: if the second step fails, the caller is left with a recoverable duplicate (surfaced asold_deleted: falseplus a warning naming the old id) instead of a silently lost alert. A newsubscription_idis always minted either way — the output carries bothold_subscription_idandnew_subscription_idso scripts never cache the stale one. The old subscription'sevent/filters/expiresAtare passed through verbatim; only the recipient changes.Testing
mock_calls, real permission-engine exit-6 tests, serve route kwarg passthrough).make checkgreen.tests/test_e2e.py: create → detail → replace-recipient → delete → list round trip added to the existing notification E2E class (env-guarded, self-cleaning).~/kbagent/e2e/notification_write_e2e.py, config-dir pattern, no token handling) ran the full round trip against the dedicated E2E project 5946 — all steps passed, including leftover-subscription sweeps before and after.Non-goals (per the issue)
POST /notifications(push a one-off notification) — different endpoint, needs a Manage application token withnotifications:push-eventscope; untouched.updatecommand — the API has no update primitive, and the CLI does not pretend otherwise.