Improve test coverage to ~100% across all core libraries

pr-37993-review-feedback.md

@@ -0,0 +1,207 @@
+# PR #37993 — Review Agent Feedback
+
+> **Context:** These findings were generated by the Architecture Review (Archie) and
+> Security Review (Sentinel) agentic workflows on
+> [PR #37993](https://github.com/Azure/azure-sdk-for-js/pull/37993)
+> (`[Projects] v2.1.0RC1`) but **could not be posted** due to
+> [MCP server blocking (gh-aw#25550)](https://github.com/github/gh-aw/issues/25550).
+>
+> - **Architecture Review run:**
+>   [24220062323](https://github.com/Azure/azure-sdk-for-js/actions/runs/24220062323)
+>   — 66 turns, 2.94M tokens, zero output
+> - **Security Review run:**
+>   [24220062345](https://github.com/Azure/azure-sdk-for-js/actions/runs/24220062345)
+>   — failed after 18 min, zero output
+
+---
+
+## 🏗️ Architecture Review (Archie)
+
+**7 findings** — 4 🔴 breaking changes, 2 🟡 design concerns, 1 🔵 documentation suggestion
+
+### 🔴 Finding 1 — `TextResponseFormatConfiguration` type renames (breaking)
+
+**File:** `sdk/ai/ai-projects/review/ai-projects-node.api.md`, line 2565
+
+The following types existed in `@azure/ai-projects_2.0.2` (GA) and are now removed:
+
+| Old name (GA) | New name |
+|---|---|
+| `TextResponseFormatConfiguration` | `TextResponseFormat` |
+| `TextResponseFormatConfigurationResponseFormatText` | `TextResponseFormatText` |
+| `TextResponseFormatConfigurationResponseFormatJsonObject` | `TextResponseFormatJsonObject` |
+| `TextResponseFormatConfigurationUnion` | `TextResponseFormatUnion` |
+
+Any caller that imported or annotated these types by name will get TypeScript
+compilation errors after upgrading. Per the Azure SDK guidelines, renaming a
+GA-stable exported type is a breaking change requiring a major version bump.
+
+**Fix:** Restore the old names as deprecated type aliases pointing to the new
+names, or bump the major version to `3.0.0` and document the migration in the
+changelog.
+
+---
+
+### 🔴 Finding 2 — `BetaEvaluatorsListLatestVersionsOptionalParams` renamed (breaking)
+
+**File:** `sdk/ai/ai-projects/review/ai-projects-node.api.md`, line 541
+
+`BetaEvaluatorsListLatestVersionsOptionalParams` was renamed to
+`BetaEvaluatorsListOptionalParams`, removing the GA-stable type. Users who
+reference the old name in type annotations will get a compilation error.
+
+**Fix:** Keep the old name as a deprecated alias:
+
+```ts
+/** @deprecated Use BetaEvaluatorsListOptionalParams instead. */
+export type BetaEvaluatorsListLatestVersionsOptionalParams = BetaEvaluatorsListOptionalParams;
+```
+
+---
+
+### 🔴 Finding 3 — `CodeBasedEvaluatorDefinition.code_text` required → optional (breaking)
+
+**File:** `sdk/ai/ai-projects/review/ai-projects-node.api.md`, line 942
+
+In `@azure/ai-projects_2.0.2`, `code_text` was `string` (required). It is now
+`string | undefined` (optional). Code reading this property without an
+`undefined` check will have a type error in strict TypeScript.
+
+**Fix:** Either keep `code_text` required (and add a separate optional
+`blob_uri` alongside), or document this as a breaking change requiring a major
+version bump.
+
+---
+
+### 🔴 Finding 4 — `HostedAgentDefinition.container_protocol_versions` required → optional (breaking)
+
+**File:** `sdk/ai/ai-projects/review/ai-projects-node.api.md`, line 1714
+
+In `@azure/ai-projects_2.0.2`, `container_protocol_versions` was
+`ProtocolVersionRecord[]` (required). It is now optional. Existing code that
+reads the value without checking for `undefined` will have a type error in
+strict mode.
+
+**Fix:** Keep it required, or document this as a breaking change requiring a
+major version bump.
+
+---
+
+### 🟡 Finding 5 — `listManagedIdentityBlueprints` returns wrong paging type
+
+**File:** `sdk/ai/ai-projects/review/ai-projects-node.api.md`, line 470
+
+`BetaAgentsOperations.listManagedIdentityBlueprints` returns
+`Promise<PagedManagedAgentIdentityBlueprint>` instead of
+`PagedAsyncIterableIterator<ManagedAgentIdentityBlueprint>`.
+
+Per the Azure SDK guidelines (§4 Async method requirements): *"Every `list*`
+method must return `PagedAsyncIterableIterator<T>`, not a plain array or
+generic `AsyncIterableIterator`."* Returning a raw Promise wrapping the paged
+result is inconsistent with the rest of the package (e.g., `listSessions`
+correctly returns `PagedAsyncIterableIterator<AgentSessionResource>`).
+
+**Fix:** Change the return type to
+`PagedAsyncIterableIterator<ManagedAgentIdentityBlueprint>` using
+`@azure/core-paging`.
+
+---
+
+### 🟡 Finding 6 — Generic `*VersionOptionalParams` naming inconsistency
+
+**File:** `sdk/ai/ai-projects/review/ai-projects-node.api.md`, line 1128
+
+`CreateVersionOptionalParams`, `DeleteVersionOptionalParams`,
+`GetVersionOptionalParams`, and `ListVersionsOptionalParams` are
+inconsistently named.
+
+The established naming pattern in this package prefixes options types with
+their operation group:
+
+- `AgentsCreateVersionOptionalParams`
+- `BetaEvaluatorsCreateVersionOptionalParams`
+- `DatasetsListVersionsOptionalParams`
+
+The new Toolboxes operations use generic names without a prefix, which is
+ambiguous and inconsistent.
+
+**Fix:** Rename to `BetaToolboxesCreateVersionOptionalParams`,
+`BetaToolboxesDeleteVersionOptionalParams`,
+`BetaToolboxesGetVersionOptionalParams`,
+`BetaToolboxesListVersionsOptionalParams`.
+
+---
+
+### 🔵 Finding 7 — Undocumented response types
+
+**File:** `sdk/ai/ai-projects/review/ai-projects-node.api.md`, line 434
+
+`BetaAgentsDownloadSessionFileResponse` and `BetaSkillsDownloadResponse` are
+`@public (undocumented)`. Per the Azure SDK guidelines (§8 Documentation):
+*"Add TSDoc comments to every public-facing type, property, method, and
+parameter."*
+
+**Fix:** Add TSDoc comments describing the response shape (blob body for
+browser, readable stream for Node.js).
+
+---
+
+### Architecture Review Summary
+
+> **7 API design findings** (4 🔴 breaking, 2 🟡 design concerns, 1 🔵
+> documentation). The most critical issue is that the package version was
+> bumped from `2.0.2` to `2.1.0` despite containing multiple breaking changes
+> against the GA-stable API surface: four `TextResponseFormat*` type renames
+> and one `BetaEvaluatorsListLatestVersionsOptionalParams` rename. Per the
+> Azure SDK guidelines, breaking changes to a stable package require a major
+> version bump (e.g., `3.0.0`). Additionally, two required properties were
+> made optional in GA-stable interfaces.
+
+---
+
+## 🛡️ Security Review (Sentinel)
+
+**1 finding** — 1 🔵 low-severity
+
+### 🔵 Finding 1 — Header injection via `foundryFeatures` (CWE-113, Low)
+
+**File:** `sdk/ai/ai-projects/src/api/agents/operations.ts`, line 275
+(also lines 556 and 617)
+
+User-supplied `foundryFeatures` option is placed directly in the
+`foundry-features` HTTP header without runtime validation.
+
+TypeScript's union type (`AgentDefinitionOptInKeys`) constrains valid values to
+specific string literals at compile time, but provides no runtime guard for
+JavaScript consumers. A JS caller could pass an arbitrary string — including
+one containing `\r\n` — to inject additional headers. In practice, modern
+Node.js and the Fetch API both reject CRLF sequences in header values, so
+exploitability is very low.
+
+**Fix:** Add an explicit allowlist check before writing the header, or —
+consistent with the beta agents operations in
+`src/api/beta/agents/operations.ts` where `foundryFeatures` is a hardcoded
+constant — consider internalizing these constants rather than accepting them
+from callers:
+
+```typescript
+const ALLOWED_FOUNDRY_FEATURES: ReadonlySet<string> = new Set([
+  "HostedAgents=V1Preview",
+  "WorkflowAgents=V1Preview",
+  "ContainerAgents=V1Preview",
+  "AgentEndpoints=V1Preview",
+]);
+
+if (
+  options?.foundryFeatures !== undefined &&
+  !ALLOWED_FOUNDRY_FEATURES.has(options.foundryFeatures)
+) {
+  throw new Error(`Invalid foundryFeatures value: ${options.foundryFeatures}`);
+}
+```
+
+### Security Review Summary
+
+> **1 low-severity finding.** One header injection risk (CWE-113) was
+> identified. No critical or medium findings. The PR's removal of
+> `KeyCredential` support is a positive security improvement.

sdk/core/core-amqp/test/internal/browser/checkNetworkConnection.spec.ts

@@ -0,0 +1,14 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+import { describe, it, assert } from "vitest";
+import { checkNetworkConnection } from "../../../src/util/checkNetworkConnection.common.js";
+
+describe("checkNetworkConnection (browser)", function () {
+  it("returns a boolean reflecting navigator.onLine", async function () {
+    const result = await checkNetworkConnection("hostname.example.com");
+    assert.isBoolean(result);
+    // In a browser test environment, navigator.onLine should be true
+    assert.equal(result, self.navigator.onLine);
+  });
+});

sdk/core/core-amqp/test/internal/browser/errors.spec.ts

@@ -0,0 +1,28 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+import { describe, it, assert } from "vitest";
+import { translate, MessagingError } from "../../../src/errors.js";
+
+describe("translate - isBrowserWebsocketError (browser)", function () {
+  it("translates a WebSocket error event into a MessagingError", function () {
+    const ws = Object.create(WebSocket.prototype);
+    const errorEvent = new Event("error");
+    Object.defineProperty(errorEvent, "target", { value: ws, writable: false });
+
+    const result = translate(errorEvent);
+
+    assert.instanceOf(result, MessagingError);
+    assert.equal((result as MessagingError).code, "ServiceCommunicationError");
+    assert.isFalse((result as MessagingError).retryable);
+    assert.include(result.message, "Websocket");
+  });
+
+  it("does not treat a plain error as a browser websocket error", function () {
+    const plainError = new Error("not a websocket error");
+    const result = translate(plainError);
+
+    // A plain Error should be returned as-is, not wrapped as ServiceCommunicationError
+    assert.equal(result, plainError);
+  });
+});

sdk/core/core-amqp/test/internal/browser/hmacSha256.spec.ts

@@ -0,0 +1,36 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+import { describe, it, assert } from "vitest";
+import { signString } from "../../../src/util/hmacSha256.common.js";
+
+describe("signString (browser - Web Crypto)", function () {
+  it("produces a URL-encoded base64 HMAC-SHA256 signature", async function () {
+    const signature = await signString("testKey", "testMessage");
+    assert.isOk(signature);
+    assert.isString(signature);
+    // The result should be URL-encoded (no +, /, = unencoded)
+    assert.notMatch(signature, /[+/=]/);
+    assert.isOk(decodeURIComponent(signature));
+  });
+
+  it("returns consistent results for the same inputs", async function () {
+    const sig1 = await signString("key", "data");
+    const sig2 = await signString("key", "data");
+    assert.equal(sig1, sig2);
+  });
+
+  it("returns different results for different keys", async function () {
+    const sig1 = await signString("key1", "data");
+    const sig2 = await signString("key2", "data");
+    assert.notEqual(sig1, sig2);
+  });
+});
+
+describe("hmacSha256.common (Web Crypto API)", () => {
+  it("signString produces a valid HMAC-SHA256 signature", async () => {
+    const result = await signString("testkey", "testdata");
+    assert.isString(result);
+    assert.isAbove(result.length, 0);
+  });
+});

sdk/core/core-amqp/test/internal/browser/runtimeInfo.spec.ts

@@ -0,0 +1,18 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+import { describe, it, assert } from "vitest";
+import { getPlatformInfo, getFrameworkInfo } from "../../../src/util/runtimeInfo-browser.mjs";
+
+describe("runtimeInfo (browser)", function () {
+  it("getPlatformInfo returns a string containing 'javascript-Browser'", function () {
+    const info = getPlatformInfo();
+    assert.include(info, "javascript-Browser");
+    assert.match(info, /^\(javascript-Browser-.+\)$/);
+  });
+
+  it("getFrameworkInfo returns a string starting with 'Browser/'", function () {
+    const info = getFrameworkInfo();
+    assert.match(info, /^Browser\/.+/);
+  });
+});

sdk/core/core-amqp/test/internal/errors.spec.ts

@@ -223,3 +223,42 @@ describe("Errors", function () {
     });
   });
 });
+
+describe("errors.ts - additional coverage", () => {
+  it("translate maps AMQP error with status-code: 404 in description to MessagingEntityNotFoundError", () => {
+    const err: any = {
+      name: "AmqpProtocolError",
+      condition: "amqp:not-found",
+      description: "The messaging entity blah could not be found. status-code: 404",
+    };
+    const translated = Errors.translate(err) as Errors.MessagingError;
+    assert.equal(translated.code, "MessagingEntityNotFoundError");
+  });
+
+  it("translate maps AMQP error with 'messaging entity could not be found' to MessagingEntityNotFoundError", () => {
+    const err: any = {
+      name: "AmqpProtocolError",
+      condition: "amqp:not-found",
+      description: "The messaging entity 'myentity' could not be found.",
+    };
+    const translated = Errors.translate(err) as Errors.MessagingError;
+    assert.equal(translated.code, "MessagingEntityNotFoundError");
+  });
+
+  it("translate handles already-translated MessagingError", () => {
+    const err = new Errors.MessagingError("already translated");
+    const translated = Errors.translate(err);
+    assert.strictEqual(translated, err);
+  });
+
+  it("translate handles MessageWaitTimeout condition", () => {
+    const err: any = {
+      name: "AmqpProtocolError",
+      condition: "com.microsoft:message-wait-timeout",
+      description: "No messages available",
+    };
+    const translated = Errors.translate(err) as Errors.MessagingError;
+    assert.equal(translated.name, "MessagingError");
+    assert.equal(translated.code, "MessageWaitTimeout");
+  });
+});