From 37e68848e511c3ffc659b71e34e2ec572b7dd8ab Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Mon, 13 Jul 2026 10:05:11 +0000 Subject: [PATCH] docs: update claude docs from PR review analysis (2026-07-06 to 2026-07-13) Co-Authored-By: Claude Sonnet 4.6 --- agent_docs/conventions.md | 2 +- agent_docs/rules.md | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/agent_docs/conventions.md b/agent_docs/conventions.md index c9eb7ad97..dba54f90f 100644 --- a/agent_docs/conventions.md +++ b/agent_docs/conventions.md @@ -11,7 +11,7 @@ - No `any` type — use `unknown` if truly unknown, then validate. - **NEVER** use `as unknown as` type casts — refactor to make types flow naturally. Casts hide real type errors and break when upstream types change. **One accepted exception**: `transformData()` returns `Record`, so `transformData(data, EntityMap) as unknown as TargetType` is permitted when no typed overload is available. Every other `as unknown as` must be refactored. **Refinement**: when `pascalToCamelCaseKeys()` precedes `transformData()` in the pipeline, `pascalToCamelCaseKeys()` returns `any`, which makes `transformData(camelCased, EntityMap)` also return `any`. In that case a single `as TargetType` cast is sufficient — drop the `as unknown` intermediate. - **Generic constraints on utility functions: use `T extends object`, not `T extends Record`** — interface types (e.g., `JobGetAllOptions`) do not structurally satisfy `Record` because they lack an index signature, causing TS2345 at call sites. Use `T extends object` (matching the `transformRequest` pattern) and cast internally: `const result: Record = { ...options as Record }`. **NEVER** add a redundant `as T` cast when the function signature already declares the return type as `T` — the compiler infers it and the extra cast is noise. -- Mark optional fields as optional in type interfaces — over-requiring causes runtime `undefined` access on fields the API didn't return. **Equally, do not mark fields as optional when the API always returns them** — misleading optional markers force callers to add unnecessary `undefined` checks for fields that are always present, and mask real type errors when the field is guaranteed. +- Mark optional fields as optional in type interfaces — over-requiring causes runtime `undefined` access on fields the API didn't return. **Equally, do not mark fields as optional when the API always returns them** — misleading optional markers force callers to add unnecessary `undefined` checks for fields that are always present, and mask real type errors when the field is guaranteed. **This rule applies to input types too**: for structured input types (join specifications, nested filter objects, inline configurations), fields that are structurally required for the server to execute the operation must be typed as required. Only use `?` when the server provides a meaningful default for that field. Example: in a join spec, the join key fields (`joinFieldName`, `relatedEntityName`, `relatedFieldName`) must be required because the server cannot execute any join without them; the entity alias field may be optional because the server defaults it to the queried entity. - Use enums for fixed value sets — **NEVER** leave raw strings/numbers. Raw values lose type safety and autocomplete. - **NEVER** write `param || {}` for required parameters — this hides bugs by silently accepting missing required data at call sites. diff --git a/agent_docs/rules.md b/agent_docs/rules.md index 38d1b5e78..fec88fce6 100644 --- a/agent_docs/rules.md +++ b/agent_docs/rules.md @@ -38,6 +38,7 @@ Every new method must also have an integration test in `tests/integration/shared - **`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. - **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. - **Extract shared data lookups to `beforeAll`** in integration tests — when multiple tests in a `describe` block need the same setup data (e.g., fetching an existing resource to run further operations against it), fetch it once in `beforeAll` and store in a `let variable!: Type` variable. Repeating `getAll` or equivalent calls inside each `it` block wastes API quota and slows the suite. - **Include a transform validation test** for new methods with a transform pipeline. This test should verify: (a) transformed camelCase fields exist and have values (`job.createdTime`, `job.processName`), AND (b) original PascalCase API fields are absent (`(job as any).CreationTime` is `undefined`, `(job as any).ReleaseName` is `undefined`). This is a separate test from the basic "should retrieve by ID" test — it validates the SDK transform layer against the live API. Note: existing integration tests don't yet follow this pattern, but unit tests do (Assets, Queues, ChoiceSets). Extending it to integration tests catches mismatches between the Swagger spec assumptions and the live API response.