fix(amazonq): deliver Q Developer access-blocked notifications to the client - #2800
Open
ashishrp-aws wants to merge 3 commits into
Open
fix(amazonq): deliver Q Developer access-blocked notifications to the client#2800ashishrp-aws wants to merge 3 commits into
ashishrp-aws wants to merge 3 commits into
Conversation
The access-blocked notification added in #2794 never reached the client. AmazonQServiceServerFactory destructures the features it forwards to the service manager, and notification was not among them, so features.notification was always undefined, the guard in serviceFactory never passed, onAccessBlocked was never assigned, and the notifier could not run. notification is optional on QServiceManagerFeatures so that existing constructions and test fixtures keep compiling. That is also why omitting it here did not fail the build -- it silently disabled client-facing reporting instead. Noted at the call site so the next person adding a feature there does not repeat it. Also set a stable id on the notification. Clients need to recognise it without inspecting its text: the message is the service's own copy and is expected to change, and FEATURE_NOT_SUPPORTED is reused across several RTS gates so the reason alone does not identify this one. Both IDE clients already prefer the id when present and fall back to matching the title only because the released server does not send one yet. Verified: tsc clean, prettier clean, 6/6 notifier tests pass, and a server bundle built from this branch contains the wiring where a bundle from the previous head did not.
…2797) * fix(amazonq): declare serverInfo so notifications reach the client The access-blocked notification still never reached the client after #2796. The runtime only constructs a notification router for servers that declare serverInfo: if (initializeResult?.serverInfo) { this.notificationRouter = new RouterByServerName(initializeResult.serverInfo.name, ...) } AmazonQServiceServer returned only capabilities and awsServerCapabilities, so the router was never built and notification.showNotification() logged "Notifications are not supported: serverInfo is not defined" and dropped the notification. Observed in VS Code: the block was detected and logged, then silently discarded. This is the last piece. With #2794 (detect), #2796 (wire) and this change (deliver), a blocked identity produces a notification the client can act on. Added a regression test, because the failure mode is silent: nothing throws and only a debug line marks the loss. The test asserts the exact name, which is deliberate -- the name is encoded into the id of every notification the client echoes back, so renaming it strands followups for notifications already on screen. Note: amazonQServer.test.ts has one pre-existing failure on this branch, "hooks onUpdateConfiguration handler to LSP server", present before this change (6 passing/1 failing before, 7 passing/1 failing after). Left alone as unrelated. * fix(amazonq): observe access-blocked on the streaming client too The observer added in #2794 was only on the token client. Chat runs through the streaming client, so the one surface where a blocked identity actually shows up to the user was the one place nothing was watching. Detection happened to work anyway because the gate denies every operation and the A/B config fetch goes through the token client moments after credentials arrive -- but that is incidental, not a guarantee. Mirrors the token client exactly: middleware on the outermost initialize step so it fires once per operation after retries are exhausted, the observer is called inside its own try/catch, and the error is always rethrown so callers behave as before. The notifier is now created once per service generation and shared by both clients rather than created per client. The notifier dedupes per instance, so sharing is what keeps a blocked identity to a single notification no matter which client sees it first. It is cleared by resetCodewhispererService, so signing out and back in with another blocked identity notifies again instead of being suppressed. Scoped to StreamingClientServiceToken. The IAM variant serves a different surface and the gate only denies Builder ID, which is bearer-token only. Pre-existing failures on this branch, unchanged by this commit: utils.test.ts 11 failing (89 passing) and amazonQServer.test.ts 1 failing, both identical before and after.
#2799) The serverInfo added in #2797 used one hardcoded name, but AmazonQServiceServerFactory is instantiated twice -- AmazonQServiceServerIAM and AmazonQServiceServerToken -- and runtimes including agent-standalone register both. Two servers reporting the same name makes lspRouter reject initialize outright: Duplicate servers defined: AWS Language Server for Amazon Q Developer That fails the whole language server, not just the duplicate. Observed in VS Code as: Failed to start downloaded LSP, falling back to bundled LSP: Duplicate servers defined: AWS Language Server for Amazon Q Developer The client then silently ran its bundled server instead, so Q appeared to work while none of the access-blocked reporting existed, with only a client-side warning to show for it. serverName is now a required parameter rather than a shared constant, since a default is precisely what let two instantiations collide. The two names are exported so the uniqueness is assertable, and they must stay stable: the name is encoded into the id of every notification the client echoes back. Added a regression test on the distinctness. Verified it bites -- reintroducing the collision gives 7 passing/2 failing, the fix gives 8 passing/1 failing. No existing test registers two servers from one runtime, which is why this reached a release. Pre-existing failure on this branch, unchanged: amazonQServer.test.ts "hooks onUpdateConfiguration handler to LSP server".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2800 +/- ##
==========================================
- Coverage 61.23% 60.25% -0.99%
==========================================
Files 282 282
Lines 71453 71550 +97
Branches 4686 4558 -128
==========================================
- Hits 43757 43109 -648
- Misses 27611 28356 +745
Partials 85 85
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 participants
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.
Merges
feature/qdev-signup-messageintomain.#2794 (the detection half) is already in
main. This brings across the three fixes that make it actually reach the client — without them the feature inmaintoday is inert, and one of them prevents a hard failure.What this contains
notificationfeature to the Q service managerfeatures.notificationisundefined,onAccessBlockedis never assigned, the notifier never runs. Nothing is reported.serverInfo, and observe access-blocked on the streaming client tooshowNotificationis a silent no-op. Chat — the surface where the block actually shows up — was also unobserved.serverInfonameslspRouterrejectedinitializewithDuplicate servers definedand clients silently fell back to their bundled server.Net effect: a Builder ID identity that RTS has blocked now produces one
aws/window/showNotificationcarrying the service's own message, which the IDE clients act on.Why it took three follow-ups
Each layer failed silently and independently — nothing threw at any point:
serverInfo→ router never built,showNotificationlogged a debug line and dropped the notificationserverInfoname →initializerejected, client fell back to its bundled server while appearing to workWorth considering separately:
showNotificationcurrentlylogs rather thanwarns when it discards a notification, and a server author has no way to notice. That is a runtimes-side change, not raised here.Testing
tsc --noEmit, prettier, eslint clean.server/aws-lsp-codewhisperer:shared334 passing / 45 failing vs 332 / 45 on base — the same 45 failures, compared by test name not just count, plus the 2 new tests.chat,inline-completion,configuration,securityScan,workspaceContext,netTransformall identical to base.Pre-existing failures on this branch, unchanged (identical before and after, verified by stashing):
utils.test.ts11 failing / 89 passing,amazonQServer.test.ts1 failing (hooks onUpdateConfiguration handler to LSP server).agenticChatcannot be run as a group locally — it hangs on base too; the three files touching this change were run individually (20, 8, 97 passing, zero failures).Backwards compatibility
The runtime gates
showNotificationon the client advertisingwindow.notifications, so plugin versions already in the market receive nothing and are unaffected. The client-side work ships separately (aws/amazon-q-vscode#159, aws/amazon-q-jetbrains#120).Known follow-ups, not in this PR
idon the notification is rewritten byRouterByServerNameinto base64 of{"serverName":...,"id":...}, so clients must decode it to identify the notification. Both clients now do. A runtimes-side change to expose the server's id directly would remove that coupling.