fix(notification): narrow --event client-side; the service ignores ?event= - #618
Merged
Conversation
…vent= 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.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…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.
padak
added a commit
that referenced
this pull request
Aug 20, 2026
…recognised prefix
Two findings from Devin's review of this PR, plus one they could not see.
The serve route for `token list` was cited as `GET /tokens/{project}`. Both
halves are wrong: the router carries `prefix="/token"` (singular) and the
operation is registered at `/{project}/list`, so the real path is
`GET /token/{project}/list` -- confirmed against the runtime OpenAPI schema,
not the source, because that is what a caller actually hits. Worth noting the
review's proposed correction (`/tokens/{project}/list`) is itself wrong on the
prefix; taking it verbatim would have swapped one 404 for another.
`CI:` is not a recognised note prefix. `_PREFIX_STYLES` / `_PREFIX_RE` in
commands/changelog.py define the set, the module docstring states the contract,
and an unrecognised label renders unhighlighted. Retitled to `Note:`, which
also reads better: the winget job being disabled has a user-facing consequence
(WinGet users stay on the last published version), so burying it under a dim
`Internal:` would understate it.
The finding Devin could not report: the four notification notes carried by
#615/#618 have no prefix at all. They were outside this PR's diff, so no
reviewer looking at the diff would flag them -- but they ship in the same
release block and break the same contract, leaving half of v0.86.0 rendering
flat. Prefixed `New:` / `Note:` with no change of meaning. Every 0.86.0 note
now matches `_PREFIX_RE`, verified by asserting over the live CHANGELOG rather
than by reading.
Each replacement is written to disk on its own. Running several in one script
means a later failed assert discards the earlier successful writes, which is
precisely how #618's stale "server-side ?event=" claim survived its own fix
pass.
padak
added a commit
that referenced
this pull request
Aug 20, 2026
…85.1 (#619) * chore(release): complete the 0.86.0 changelog and drop the phantom 0.85.1 The version bump to 0.86.0 already landed on main (#615), but the release notes it produces were incomplete in two ways. Missing entries. PR #616 (`token list`, plus the retry-policy and exceptionId changes) carried no changelog note at all -- its commit message says "No version bump: this lands in a stack of PRs released as one version. The (since v0.86.0) doc tags assume 0.86.0 and the bump PR must confirm that", and the bump PR did not. `make changelog-check` cannot catch this: it verifies every published GitHub release has an entry, not that every merged PR has a note. #556/#606 (merge-request endpoints, Layer 3) and #610 (winget job disabled) were likewise unannounced. All four are added. Phantom 0.85.1. pyproject went 0.85.0 -> 0.85.1 (#614) -> 0.86.0 (#615) without a tag in between, so 0.85.1 exists only as a changelog bucket -- no release, no artifact, nobody running it. `format_whats_new` shows the notes of the *target* version only, so every user upgrading 0.85.0 -> 0.86.0 would have silently missed those four fixes (Azure ciphertext prefix, the `parameters` wrapper, GCP/Azure sync ciphertext, the encrypt-values docs). The bucket is folded into 0.86.0 verbatim. The same phantom leaked into the agent-facing version gates, which is the worse half: `keboola-expert.md` told users to "upgrade to 0.85.1+" and four gotchas.md entries were tagged `(since v0.85.1)` -- a version nobody can install. Retagged to 0.86.0, along with two source comments. Three of the new notes had to lead with a shorter sentence to satisfy `test_newest_release_notes_are_not_truncated` (the headline is the note's first sentence, capped at 160 chars). No behaviour change; documentation and release metadata only. * fix(changelog): correct the serve route and give every 0.86.0 note a recognised prefix Two findings from Devin's review of this PR, plus one they could not see. The serve route for `token list` was cited as `GET /tokens/{project}`. Both halves are wrong: the router carries `prefix="/token"` (singular) and the operation is registered at `/{project}/list`, so the real path is `GET /token/{project}/list` -- confirmed against the runtime OpenAPI schema, not the source, because that is what a caller actually hits. Worth noting the review's proposed correction (`/tokens/{project}/list`) is itself wrong on the prefix; taking it verbatim would have swapped one 404 for another. `CI:` is not a recognised note prefix. `_PREFIX_STYLES` / `_PREFIX_RE` in commands/changelog.py define the set, the module docstring states the contract, and an unrecognised label renders unhighlighted. Retitled to `Note:`, which also reads better: the winget job being disabled has a user-facing consequence (WinGet users stay on the last published version), so burying it under a dim `Internal:` would understate it. The finding Devin could not report: the four notification notes carried by #615/#618 have no prefix at all. They were outside this PR's diff, so no reviewer looking at the diff would flag them -- but they ship in the same release block and break the same contract, leaving half of v0.86.0 rendering flat. Prefixed `New:` / `Note:` with no change of meaning. Every 0.86.0 note now matches `_PREFIX_RE`, verified by asserting over the live CHANGELOG rather than by reading. Each replacement is written to disk on its own. Running several in one script means a later failed assert discards the earlier successful writes, which is precisely how #618's stale "server-side ?event=" claim survived its own fix pass.
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.
Follow-up to #615, found by exercising the merged feature against a real project that actually has Notifications-tab subscriptions.
0.86.0is onmainbut not yet released (latest release isv0.85.0), so both fixes fold into that entry rather than shipping a0.86.1for a bug no user ever saw.🔴 The service ignores its own
?event=filter…and the body contains every subscription in the project, including
job-succeededandjob-processing-longones. Verified live oneurope-west3.gcpagainst a project with four subscriptions across three events; the parameter really is on the wire (checked via--verbose).So
--event job-failedwas answering "who gets paged when this flow breaks" with a superset that also lists success and processing-delay recipients — for an audit command that is worse than an error, because it looks like an answer. A camelCase typo (jobFailed) returned everything rather than nothing.This is the same trap issue #600 itself documents one paragraph in, for Storage's
list_files: "unknown params are silently ignored". I trusted the swagger, which documents?event=on this endpoint. A documented query parameter is a statement of intent, not of behavior.Fix: keep sending the parameter (a server-side fix would then cost nothing) and narrow the rows client-side, alongside
--component-id/--config-idwhich were already client-side.--eventjob-failedjob-failedjob-succeededjob-processing-longjobFailed(typo)branch_idis populated on every row, production includedThe Flow Builder writes a
branch.idfilter on every subscription. For a production one the value is the default branch's own numeric id (390737=Mainin the project I tested); the dev-branch subscription carried401464=Padak Test.The docs shipped in #615 said a
branch.idfilter marked a subscription as branch-specific. That is false, and it is the kind of false that matters: an agent readinggotchas.mdwould classify a live production alert as dev-branch-only and drop it from an audit. Corrected ingotchas.md,CLAUDE.md,context.pyandcommands-reference.md, with the cross-check (kbagent branch list) spelled out.This also settles the open question #615 could not answer — the schema supported
branch.id, but whether the UI wrote it was unverified. It does, always.✅ Confirmed correct against the same live data
Everything else held up: kebab-case event names, dotted filter paths (
job.component.id,job.configuration.id,branch.id), webhook recipients arriving asurland normalizing into the singleaddresscolumn,durationOvertimePercentage >= 0.7surviving verbatim intonotification detail, and the config-name join resolvingMarts Refresh.Testing — the E2E gap is the actual root cause
The assertion that should have caught this already existed:
It passed vacuously, because the E2E project has no subscriptions and
all()over an empty list is true. A green vacuous test is invisible; that is how this shipped. So E2E is strengthened rather than merely patched:pytest.skipwith an explicit reason instead of passing silently — a skipped test in the report is a visible gap.test_event_filter_actually_narrows_a_mixed_projectgives theall(...)check teeth: it requires ≥ 2 distinct events and asserts the filtered count is both exact and strictly smaller.test_api_side_event_filter_is_still_ignoredis a canary — if the service ever honors the parameter, it flips to failing, which is the notification we want (the client-side filter becomes redundant and the gotchas entry needs retiring).Unit coverage pins the behavior deterministically in CI (E2E needs credentials): a mock client that ignores the
eventkwarg exactly like the live service, asserting the service narrows anyway — plus one test that the parameter is still sent, and one that a typo narrows to nothing.make check: 5691 passed.