docs: update Claude docs from PR review analysis

agent_docs/conventions.md

@@ -17,11 +17,12 @@
 
 ## General conventions
 
-- Services follow the pattern: extend `BaseService`, call `super(uiPath)`, use `this.get()` / `this.post()` etc.
+- Services follow the pattern: extend `BaseService`, call `super(uiPath)`, use `this.get()` / `this.post()` etc. **NEVER** bypass `BaseService` even for passthrough/proxy methods that use raw `fetch()` — extend `BaseService`, get the token via `this.getValidAuthToken()`, and use raw `fetch()` only within the specific method body (e.g., when the method must not throw on non-2xx responses). Services that bypass `BaseService` lose standard auth-flow integration and make tests harder to set up without `as unknown as` casts.
 - Types live in `src/models/{domain}/{domain}.types.ts`. Internal-only types go in `*.internal-types.ts`.
 - Constants live in `src/utils/constants/`. Endpoints are split per domain in `src/utils/constants/endpoints/` (e.g., `data-fabric.ts`, `maestro.ts`, `orchestrator.ts`).
 - Subpath exports: when adding a new service module, add entries to `package.json` `exports` and `rollup.config.js`.
-- Every public service method that makes an HTTP API call must be decorated with `@track('ServiceName.MethodName')` for telemetry — gaps are invisible until production debugging, when they're expensive. **Exception**: methods that are event handlers, window/DOM interactions, or UI-only operations (no HTTP calls) must NOT be tracked — tracking non-API methods causes excessive log ingestion with no diagnostic value. **Delegation anti-pattern**: when a public service method delegates to another service's public `@track`-decorated method, both `@track` decorators fire, causing double telemetry. Fix: extract the shared logic into an internal helper function (no `@track`) and have both public service methods call the helper directly.
+- Every public service method that makes an HTTP API call must be decorated with `@track('ServiceName.MethodName')` for telemetry — gaps are invisible until production debugging, when they're expensive. **Both segments must be PascalCase**: `@track('ConversationalAgent.DownloadCitationSource')`, not `@track('ConversationalAgent.downloadCitationSource')`. Using camelCase for the method name breaks dashboard queries that pattern-match on `ServiceName.*`. **Exception**: methods that are event handlers, window/DOM interactions, or UI-only operations (no HTTP calls) must NOT be tracked — tracking non-API methods causes excessive log ingestion with no diagnostic value. **Delegation anti-pattern**: when a public service method delegates to another service's public `@track`-decorated method, both `@track` decorators fire, causing double telemetry. Fix: extract the shared logic into an internal helper function (no `@track`) and have both public service methods call the helper directly.
+- **Service methods that bypass `ApiClient.request()` and use raw `fetch()` must manually add distributed-tracing headers** — `ApiClient` normally injects `traceparent` and `x-uipath-traceparent-id` on every request; raw `fetch()` callers are invisible to the platform's tracing infrastructure without them. Pattern: generate trace IDs with `crypto.randomUUID()` and set both `TRACEPARENT` and `UIPATH_TRACEPARENT_ID` headers (constants already exported from `src/utils/constants/headers.ts`).
 - Use named imports/exports (avoid default exports). Use barrel exports (`index.ts`) for public API. Never export internal types from barrel exports.
 - **Barrel files must use `export * from`**, not `export type * from`. Using `export type *` silently drops runtime values (classes, enums), causing `undefined` errors for SDK consumers. Note: individual `export type { Name }` for specific type-only re-exports is fine — the prohibition is on the wildcard form.
 - When a service method makes multiple independent API calls (e.g., chunk-based key resolution), parallelize them with `Promise.all` — sequential calls compound latency unnecessarily.

agent_docs/rules.md

@@ -11,11 +11,14 @@
 - **Use domain-specific error constants for entity-specific method tests** (e.g., `getById` → `JOB_TEST_CONSTANTS.ERROR_JOB_NOT_FOUND`, `ASSET_TEST_CONSTANTS.ERROR_ASSET_NOT_FOUND`). Generic `TEST_CONSTANTS.ERROR_MESSAGE` is acceptable for collection methods like `getAll`. Existing pattern: Assets, Queues, Jobs all follow this split.
 - Verify bound methods exist on response objects when `getById` returns entities with attached methods.
 - **Validate transform completeness** — for any method with a transform pipeline, verify both: (a) transformed fields have correct values (`result.createdTime`), AND (b) original PascalCase fields are absent (`(result as any).CreationTime` is `undefined`). This catches transform regressions that value-only assertions miss. Existing pattern: Assets (`assets.test.ts:94`), Queues (`queues.test.ts:88`), ChoiceSets (`choicesets.test.ts:243`). **For semantic rename tests, use a distinctive non-null value in the mock override** (e.g., a specific timestamp string) — a `null` default doesn't verify value preservation: `expect(result.renamedField).toBe(null)` passes whether the rename correctly transferred the null or the field was never populated. A distinctive value like `'2000-01-01T00:00:00.000Z'` confirms the specific value traveled through the rename intact.
+- **Read-modify-write patterns need a dedicated cast test** — when a service method (e.g., `updateById`) reads a renamed field from a `getById` response using a type cast back to the raw wire shape (e.g., accessing `isInsightsEnabled` from a response typed as having `isAnalyticsEnabled`), write a test that specifically validates the internal cast reads the correct wire field. Transform completeness tests only cover the public response shape; they don't verify downstream methods that cast back to access unmapped wire fields. Without this test, a drift in the cast (e.g., `.isAnalyticsEnabled` instead of `.isInsightsEnabled`) silently sends the wrong value on every write with no failing test.
 - **Every service with bound methods must have a model test file** — `tests/unit/models/{domain}/{entity}.test.ts` testing bound method delegation. This is separate from service tests and verifies that `create{Entity}WithMethods()` correctly binds methods. Existing pattern: Tasks, CaseInstances, ProcessInstances, Entities, Conversations.
 - **Mock factories must return `Raw{Entity}GetResponse`**, not `{Entity}GetResponse`** — mock factories like `createBasicJob()` produce plain data without bound methods. Methods are attached by the service layer via `create{Entity}WithMethods()`, not by mocks. Using the combined type causes compile errors for missing method properties.
 - **Shared mock setup belongs in `beforeEach`**, not inline in individual tests** — mock creation like `mockApiClient.getValidToken = vi.fn()` must be in the shared setup block, not duplicated inside each test.
 - **NEVER** leave unused mock methods in mock objects — dead mocks obscure what the test actually exercises and accumulate as the API evolves.
 - **NEVER** access private methods via `as any` in tests (e.g., `(service as any)._privateMethod()`). This violates the no-`any` rule and creates brittle tests tied to implementation internals. Test behaviour through the public API instead, or extract the logic to a module-level pure function that can be imported and tested directly.
+- **Unit test file paths must mirror the `src/` directory structure exactly** — a helper at `src/services/conversational-agent/helpers/citation.ts` belongs at `tests/unit/services/conversational-agent/helpers/citation.test.ts`, not `tests/unit/helpers/conversational-agent/citation.test.ts`. Misplaced test files are invisible to coverage tools and make the test suite hard to navigate.
+- **Use `.rejects.toBeInstanceOf(ErrorType)` consistently for async error assertions** — when all sibling error tests in a block use `.rejects.toBeInstanceOf(ValidationError)`, do not switch to `.toThrow(/pattern/)` for one test. The `toBeInstanceOf` form is more precise (it tests the error class, not just the message) and matches the SDK-wide pattern.
 - Use `let variable!: Type` (definite assignment assertion) for variables initialized in `beforeAll`, not `let variable: Type | undefined`. The `!` signals TypeScript that the value is guaranteed before any test runs, eliminating null-checks throughout the test bodies. The `afterAll` guard (`if (!variable) return`) still works at runtime.
 - **NEVER** wrap integration test API calls in try/catch — let errors propagate naturally. Silent catches mask real failures and make tests pass when they should fail.
 - **NEVER** create a separate `afterAll` per describe block if the file already has one — reuse the existing cleanup block by pushing to the shared `createdRecordIds` array.
@@ -36,7 +39,7 @@ Every new method must also have an integration test in `tests/integration/shared
 - Tests run in both `v0` and `v1` init modes via `describe.each(modes)` — **only if the service is registered in both modes in `unified-setup.ts`**. New services that only support `v1` init should use `['v1']` only.
 - **Always `throw new Error()` when test preconditions are not met** — whether it's missing config (e.g., no `folderId`) or missing test data (e.g., no running jobs). Never use `console.warn()` + `return` to silently skip — silent skips hide unrunnable tests and make CI green when tests aren't actually exercised.
 - **`describe.skip` is permitted only when the service does not support PAT auth** — e.g., `insightsrtm_` endpoints reject PAT tokens entirely (returns 401 regardless of scopes) and require OAuth. In this case, write the full test body as if it will eventually run, add a comment explaining the limitation (e.g., `// skip: insightsrtm_ endpoints do not support PAT auth — requires OAuth`), and use `describe.skip` rather than omitting the test entirely. This preserves the test structure for when PAT support is added. Do **not** use `describe.skip` for missing test data, missing config, or flakiness — those require a `beforeAll` guard or a `throw`. Equivalently, **NEVER** exclude integration test files via `vitest.integration.config.ts` using env vars or file exclusion patterns — that is functionally equivalent to `describe.skip` across an entire file and has the same problem: tests appear to pass but are never actually exercised. Guard with `beforeAll` + `throw` inside the test file instead.
-- **Use snapshot+restore for integration tests that mutate shared state** — when an operation has wide-reaching, hard-to-undo side effects (e.g., marking all notifications as read, clearing a queue), read the current state before the test, perform the operation, then restore the previous state in cleanup. This prevents one test from permanently altering the shared environment and corrupting subsequent tests.
+- **Use snapshot+restore for integration tests that mutate shared state** — when an operation has wide-reaching, hard-to-undo side effects (e.g., marking all notifications as read, clearing a queue), read the current state before the test, perform the operation, then restore the previous state in cleanup. This prevents one test from permanently altering the shared environment and corrupting subsequent tests. **The restore value must come from the snapshotted state — never hardcode an assumed original value** (e.g., `isSubscribed: true`). If the environment's real state differed from your assumption, a hardcoded restore mutates the environment instead of restoring it, silently corrupting subsequent tests.
 - **NEVER** write redundant integration tests — each test must cover a distinct code path, error scenario, or response shape aspect.
 - **When a new parameter is added to an existing method, update the integration test to exercise that parameter** — a passing integration test that never sends the new parameter does not validate that the API actually accepts it. Add a dedicated `it` block (or extend the existing one) that explicitly passes the new parameter and asserts the expected behavior.
 - **NEVER** test client-side `ValidationError` in integration tests — a call that throws before making any HTTP request (e.g., `updateValueById('', ...)` failing on empty ID) belongs in unit tests, not integration tests. Integration tests must only exercise code paths that reach the actual API.
@@ -73,6 +76,8 @@ JSDoc comments in `src/models/{domain}/*.models.ts` are the **source of truth fo
 - **Propagate `@internal` and `@experimental` to every public layer that exposes the same API** — if an underlying service method is tagged `@internal` or `@experimental`, the corresponding wrapper on the `UiPath` class (or any other public-facing class) must also carry the same tag. TypeDoc runs directly on public class methods; a missing tag on one layer surfaces the method in generated docs even if the service layer correctly marks it. This applies equally to `@experimental` — marking a service model `@experimental` without also marking the service class leaves the class-level JSDoc inconsistent with the published API status.
 - **Do not duplicate backend validation SDK-side** — when an API parameter has backend-enforced constraints (e.g., character limits, allowed formats), document the constraint in JSDoc (`@param search - Search string (1–100 chars)`) rather than adding matching SDK-side validation. SDK-side checks risk drifting out of sync with backend rules as the API evolves, producing confusing double-error scenarios.
 - **NEVER** expose internal implementation details in user-facing JSDoc — remove references to `OData`, internal API protocols, or internal endpoint names (e.g., "via the OData GetFiles endpoint", "with OData query options"). End users don't need to know which protocol or internal API mechanism the SDK uses; such details belong in code comments or internal docs, not in generated API documentation.
+- **Do NOT use `@throws` in `{Entity}ServiceModel` method JSDoc** — the tag is not rendered by TypeDoc for the SDK's documentation pattern and adds noise with no benefit. Error behavior should be described in prose in the method's `@returns` or description if necessary. (`@throws` may appear on internal base service methods where it aids IDE navigation, but not on public model interfaces.)
+- **NEVER** embed PR-specific notes, in-progress implementation comments, or references to follow-up PRs in class or method JSDoc — these appear in IDE tooltips indefinitely and become stale or incorrect once the referenced work ships. JSDoc describes the current, stable public contract only. Move such notes to the PR description or a code comment inside the method body.
 
 ### Samples & Template Gallery