From 5f6f270c26e160dc5b1fc30bf1f9dac4dec916d1 Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Wed, 12 Aug 2026 19:13:41 -0700 Subject: [PATCH] test(amazonq): address review findings on the access-blocked observer Three review follow-ups, no behaviour change for users. Name the streaming client's middleware, matching the token client. Without a name a second registration stacks another observer rather than replacing the first, which would report the same block twice, and the middleware is anonymous in SDK stack introspection. Assert the server-name uniqueness against the real exported servers rather than the two constants. Comparing constants cannot catch the same name being passed to both factory calls, which is the mistake that actually shipped. Verified the test bites: making the names identical fails it (7 passing/2 failing vs 8/1). Add two tests for the streaming observer. They assert the wiring rather than the callback because the existing harness stubs CodeWhispererStreaming.prototype.sendMessage, which bypasses the middleware stack entirely -- a behavioural test there would pass even if the middleware did not exist. shared group: 337 passing / 45 failing, against 334 / 45 before, so the 3 new tests and no new failures. --- .../src/shared/amazonQServer.test.ts | 44 ++++++++++++++----- .../src/shared/streamingClientService.test.ts | 26 +++++++++++ .../src/shared/streamingClientService.ts | 4 ++ 3 files changed, 64 insertions(+), 10 deletions(-) diff --git a/server/aws-lsp-codewhisperer/src/shared/amazonQServer.test.ts b/server/aws-lsp-codewhisperer/src/shared/amazonQServer.test.ts index 3edcc4c097..5ad0522a0b 100644 --- a/server/aws-lsp-codewhisperer/src/shared/amazonQServer.test.ts +++ b/server/aws-lsp-codewhisperer/src/shared/amazonQServer.test.ts @@ -14,6 +14,8 @@ import { AMAZON_Q_SERVICE_SERVER_IAM_NAME, AMAZON_Q_SERVICE_SERVER_TOKEN_NAME, AmazonQServiceServerFactory, + AmazonQServiceServerIAM, + AmazonQServiceServerToken, } from './amazonQServer' import { BaseAmazonQServiceManager } from './amazonQServiceManager/BaseAmazonQServiceManager' @@ -63,17 +65,39 @@ describe('AmazonQServiceServer', () => { expect(result.serverInfo?.name).to.equal(TEST_SERVER_NAME) }) - it('gives the IAM and token servers distinct serverInfo names', () => { - // Regression guard. Runtimes such as agent-standalone register BOTH of these servers, and the - // runtime rejects initialize with `Duplicate servers defined` when two servers report the same - // name -- which fails the entire language server, not just the duplicate. A shared name here - // made every such runtime fall back to whatever server the client had bundled, visible only as - // a client-side warning, so Q kept working while silently running a different server. - const names = [AMAZON_Q_SERVICE_SERVER_IAM_NAME, AMAZON_Q_SERVICE_SERVER_TOKEN_NAME] - - for (const name of names) { - expect(name).to.be.a('string').and.not.empty + it('gives the IAM and token servers distinct serverInfo names', async () => { + // Regression guard, asserted against the real exported servers rather than the constants, so + // it also catches the same name being passed to both factory calls. + // + // Runtimes such as agent-standalone register BOTH of these servers, and the runtime rejects + // initialize with `Duplicate servers defined` when two servers report the same name -- which + // fails the entire language server, not just the duplicate. A shared name here made every such + // runtime fall back to whatever server the client had bundled, visible only as a client-side + // warning, so Q kept working while silently running a different server. + const names: (string | undefined)[] = [] + + for (const qServer of [AmazonQServiceServerIAM, AmazonQServiceServerToken]) { + const serverFeatures = new TestFeatures() + try { + // The service managers refuse to initialize before the LSP connection has, so the + // client params have to be in place before the initializer runs. + serverFeatures.setClientParams({} as InitializeParams) + qServer(serverFeatures) + + const initializer = serverFeatures.lsp.addInitializer.args[0]?.[0] + const result = (await initializer( + {} as InitializeParams, + {} as CancellationToken + )) as PartialInitializeResult + + names.push(result.serverInfo?.name) + } finally { + serverFeatures.dispose() + TestAmazonQServiceManager.resetInstance() + } } + + expect(names).to.deep.equal([AMAZON_Q_SERVICE_SERVER_IAM_NAME, AMAZON_Q_SERVICE_SERVER_TOKEN_NAME]) expect(new Set(names).size, `server names must be unique: ${names.join(', ')}`).to.equal(names.length) }) diff --git a/server/aws-lsp-codewhisperer/src/shared/streamingClientService.test.ts b/server/aws-lsp-codewhisperer/src/shared/streamingClientService.test.ts index 5b1f08d851..382052cce7 100644 --- a/server/aws-lsp-codewhisperer/src/shared/streamingClientService.test.ts +++ b/server/aws-lsp-codewhisperer/src/shared/streamingClientService.test.ts @@ -142,6 +142,32 @@ describe('StreamingClientServiceToken', () => { expect(streamingClientServiceDefault['shareCodeWhispererContentWithAWS']).to.be.undefined }) + describe('access-blocked observer', () => { + it('registers exactly one named middleware on the initialize step', () => { + // Guards the wiring rather than the callback: the harness stubs + // CodeWhispererStreaming.prototype.sendMessage, which bypasses the middleware stack, so a + // behavioural test here would pass without the middleware existing at all. + // + // The name matters. Without it a second registration stacks another observer instead of + // replacing the first, which would report the same block twice. + const registered = streamingClientService.client.middlewareStack + .identify() + .filter(entry => entry.includes('detectQDevPluginAccessBlocked')) + + expect(registered).to.have.lengthOf(1) + expect(registered[0]).to.contain('initialize') + }) + + it('exposes a settable observer for the service manager to assign', () => { + // Assigned after construction, so the middleware has to read it at call time. If this + // became readonly or were dropped, chat-time blocks would go unobserved. + const observer = () => {} + streamingClientService.onAccessBlocked = observer + + expect(streamingClientService.onAccessBlocked).to.equal(observer) + }) + }) + describe('generateAssistantResponse', () => { const MOCKED_GENERATE_RESPONSE_REQUEST = { conversationState: { diff --git a/server/aws-lsp-codewhisperer/src/shared/streamingClientService.ts b/server/aws-lsp-codewhisperer/src/shared/streamingClientService.ts index d14c24fecf..dbb6aa98c3 100644 --- a/server/aws-lsp-codewhisperer/src/shared/streamingClientService.ts +++ b/server/aws-lsp-codewhisperer/src/shared/streamingClientService.ts @@ -163,6 +163,10 @@ export class StreamingClientServiceToken extends StreamingClientServiceBase { }, { step: 'initialize', + // Named so a second registration replaces this one rather than stacking another + // observer, and so the middleware is identifiable in SDK stack introspection. Matches + // the token client's registration. + name: 'detectQDevPluginAccessBlocked', } ) }