Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 48 additions & 3 deletions .claude/agents/pr-review-consistency.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ You are especially strict with **customer-facing, hard-to-reverse surfaces**. In
- Customer UX semantics, defaults, deprecations, and upgrade impact
- Error swallowing / fallback behavior quality (this reviewer only checks naming/taxonomy/consistency)
- Test coverage and test quality
- Type safety / invariants
- Type safety / invariants (illegal states, encapsulation leaks)

# Operating Principles

Expand Down Expand Up @@ -148,12 +148,57 @@ For each changed file that introduces a new route/handler/service/type/module:
- Index and constraint names: consistency with existing naming patterns
- Enum/type names in the database layer

## 3. Reuse of Existing Helpers/Utilities
Before accepting a new helper or "common" function:
## 3. Reuse of Existing Helpers/Utilities/Types
Before accepting a new helper, type, or "common" function:
- Grep for existing utilities that already solve the problem
- **For new types/interfaces:** check if the shape already exists in:
- **Validation schemas** (Zod, io-ts, Yup) → use `z.infer<typeof schema>`
- **Database models** (Prisma, Drizzle) → use generated types
- **Internal shared packages** (`@inkeep/*`, etc.) → import from the package
- **External SDKs** (OpenAI, Vercel AI SDK, etc.) → use exported types
- **Function signatures** → use `Parameters<>` or `ReturnType<>`
- **Async function returns** → use `Awaited<ReturnType<typeof fn>>`
- **Existing domain types** → use `Pick`, `Omit`, `Partial` to derive subsets
- **Constants objects** → use `keyof typeof` to derive key types
- **Base types** → use `interface extends` or intersection (`&`) for composition
- **For type composition patterns:** check consistency with existing patterns:
- Discriminated unions: does the codebase use `{ success: true } | { success: false }` or `{ type: 'a' } | { type: 'b' }`?
- Type guards: follow existing naming (`isX`, `hasX`) and predicate patterns
- Re-exports: if a type is used across package boundaries, is it re-exported at the API surface?
- Prefer extending the existing helper over adding a near-duplicate
- If a new helper is warranted, ensure naming and location match existing conventions (avoid a new parallel "utils universe")

**Type duplication detection signals:**
- Same fields defined in multiple interfaces → consolidate with `extends` or shared base
- `typeof` used without `keyof` when deriving from constants
- Repeated `as unknown as` casts → indicates missing type guard or improper derivation
- Manual async return types → should use `Awaited<ReturnType<>>`

**Zod schema composition patterns (check for consistency):**
- Insert/Update schema pairs: Update should derive from Insert via `.partial()`
- Schema extension: Use `.extend()` to add/override fields, not duplicate definitions
- Field subsetting: Use `.pick()` or `.omit()` instead of manual field copying
- Cross-field validation: Chain `.extend().refine()` for related validations
- OpenAPI metadata: Schemas exposed via API should have `.openapi('Name')`

**Detection signals for schema anti-patterns:**
- Parallel `z.object()` definitions with overlapping fields
- Insert schema and Update schema defined separately with duplicated fields
- New schema that looks like an existing schema with minor field changes

**Additional convention patterns to check:**
- **`satisfies` operator**: Does the codebase use `satisfies` for const objects? If so, new consts should follow.
```typescript
// Check if codebase uses this pattern:
const config = { timeout: 5000 } satisfies Config;
```
- **Re-exports**: Types used across package boundaries should be re-exported at the API surface.
```typescript
// GOOD: Re-export for consumers
export type { AgentCard } from '@inkeep/agents-core';
```
- **Type guard naming**: Follow existing conventions (`isX`, `hasX`, `assertX`).

## 4. Split-World / Partial Migration Awareness
If a PR introduces a new pattern that coexists with an older one:
- Is the old pattern now "legacy"?
Expand Down
41 changes: 41 additions & 0 deletions .claude/agents/pr-review-types.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,37 @@ When analyzing a type, you will:
- Is it impossible to create invalid instances?
- Are runtime checks appropriate and comprehensive?

6. **Check Type Composition for Illegal States**: Focus on type patterns that allow invalid data:

**Discriminated Unions (prefer for mutually exclusive states):**
```typescript
// GOOD: Type-safe — impossible to have both data and error
type Result =
| { success: true; data: T }
| { success: false; error: string };

// BAD: Allows illegal state { success: true, data: undefined, error: "oops" }
type Result = { success: boolean; data?: T; error?: string };
```
**Why it matters:** Optional fields for mutually exclusive states allow illegal combinations at runtime.

**Type Guards (require for safe narrowing):**
```typescript
// GOOD: Type predicate enables safe narrowing
function isAdminUser(user: User): user is AdminUser {
return 'permissions' in user;
}

// BAD: Assertion without validation — allows invalid data through
const admin = user as AdminUser; // No runtime check!
```
**Why it matters:** `as` assertions bypass type checking entirely — if the assertion is wrong, invalid data flows through the system.

**Detection patterns for type safety issues:**
- Optional fields that represent mutually exclusive states → discriminated union
- `as` type assertions without accompanying runtime validation → type guard
- Union types without a discriminant field → add discriminant or use type guard

**Key Principles:**

- Prefer compile-time guarantees over runtime checks when feasible
Expand All @@ -95,6 +126,16 @@ When analyzing a type, you will:
- Missing validation at construction boundaries
- Inconsistent enforcement across mutation methods
- Types that rely on external code to maintain invariants
- Type designs that allow illegal states:
- Optional fields for mutually exclusive states → use discriminated unions
- Boolean + optional data/error fields → use `{ success: true; data: T } | { success: false; error: E }`
- Union types without discriminant → add a `type` or `kind` field for safe narrowing
- Unsafe type narrowing:
- Using `as` assertions without runtime validation → add type guard with `is` predicate
- Inline type assertions for polymorphic data → use discriminated union + type guard
- Casting `unknown` without validation → use Zod `.parse()` or manual type guard

**Note:** Type *duplication* and *derivation* concerns (DRY, schema reuse) are out of scope. This reviewer focuses on whether types allow **illegal states**.

**Output Format:**

Expand Down
Loading