Skip to content

test(amazonq): address review findings on the access-blocked observer - #2801

Merged
ashishrp-aws merged 1 commit into
Amazon-Q-Developer:feature/qdev-signup-messagefrom
ashishrp-aws:fix/qdev-review-followups
Aug 13, 2026
Merged

test(amazonq): address review findings on the access-blocked observer#2801
ashishrp-aws merged 1 commit into
Amazon-Q-Developer:feature/qdev-signup-messagefrom
ashishrp-aws:fix/qdev-review-followups

Conversation

@ashishrp-aws

Copy link
Copy Markdown
Collaborator

Targets feature/qdev-signup-message, so #2800 picks these up automatically.

Follow-ups from the review of #2800. No behaviour change for users — one wiring fix and two test improvements.

1. Name the streaming client's middleware

The token client registers its observer with name: 'detectQDevPluginAccessBlocked'; the streaming client did not. Without a name, a second registration stacks another observer instead of replacing the first, which would report the same block twice, and the middleware is anonymous in SDK stack introspection.

2. Assert name uniqueness against the real servers, not the constants

The previous test compared AMAZON_Q_SERVICE_SERVER_IAM_NAME and ..._TOKEN_NAME. That guards the constants but cannot catch the same name being passed to both factory calls — which is exactly the mistake that shipped and caused Duplicate servers defined.

The test now invokes both real exported servers' initializers and asserts the names they actually report are distinct. Verified it bites: making the two names identical fails it (7 passing / 2 failing, versus 8 / 1 with the fix).

Reviewers asked for a composition-level test that walks every server registered by agent-standalone. That is the stronger form and I agree with it, but it belongs in the runtimes app package rather than here, since that is where the composition lives. Raising it separately rather than stretching this PR.

3. Two tests for the streaming observer

These assert the wiring, deliberately not the callback: the existing harness stubs CodeWhispererStreaming.prototype.sendMessage, which bypasses the middleware stack entirely, so a behavioural test written there would pass even if the middleware did not exist. Rather than write a test that proves nothing, they assert what is actually verifiable at this level — that exactly one named middleware is registered on the initialize step, and that the observer property is settable after construction (it must be, since the service manager assigns it later).

Testing

shared group: 337 passing / 45 failing, against 334 / 45 before — the three new tests, no new failures. The 45 are pre-existing and unchanged (utils.test.ts and the onUpdateConfiguration case). tsc and prettier clean.

Review findings deliberately not addressed here

  • Observer errors are logged at debug. Two reviewers suggested warn. Left alone for symmetry with the token client, which has shipped that way; changing one and not the other would be worse. Worth doing to both, separately.
  • showNotification logs at info when it drops a notification for want of a router. This is in the runtimes package and was one of the three silent failures behind fix(amazonq): pass notification feature to the Q service manager #2796/fix(amazonq): declare serverInfo so notifications reach the client #2797. Belongs in a runtimes change.
  • A snapshot assertion pinning the exact server-name strings. The names are embedded in notification followup ids, so a rename strands in-flight followups. Reasonable, but the JSDoc already states the constraint and the value is now asserted via the real servers.

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.
@ashishrp-aws
ashishrp-aws requested a review from a team as a code owner August 13, 2026 02:14
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.97%. Comparing base (f529673) to head (5f6f270).

Additional details and impacted files
@@                       Coverage Diff                       @@
##           feature/qdev-signup-message    #2801      +/-   ##
===============================================================
- Coverage                        60.25%   59.97%   -0.28%     
===============================================================
  Files                              282      282              
  Lines                            71550    71554       +4     
  Branches                          4558     4534      -24     
===============================================================
- Hits                             43109    42913     -196     
- Misses                           28356    28553     +197     
- Partials                            85       88       +3     
Flag Coverage Δ
unittests 59.97% <100.00%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ashishrp-aws
ashishrp-aws merged commit 60a3491 into Amazon-Q-Developer:feature/qdev-signup-message Aug 13, 2026
11 of 13 checks passed
ashishrp-aws added a commit that referenced this pull request Aug 13, 2026
… client (#2800)

* fix(amazonq): pass notification feature to the Q service manager (#2796)

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.

* fix(amazonq): declare serverInfo so notifications reach the client (#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.

* fix(amazonq): give the IAM and token servers distinct serverInfo names (#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".

* test(amazonq): address review findings on the access-blocked observer (#2801)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants