diff --git a/.changeset/rate-limit-config-dual-source-c9.md b/.changeset/rate-limit-config-dual-source-c9.md new file mode 100644 index 0000000000..41aaf6e3ff --- /dev/null +++ b/.changeset/rate-limit-config-dual-source-c9.md @@ -0,0 +1,70 @@ +--- +"@objectstack/spec": major +--- + +BREAKING(spec): `@objectstack/spec/integration` renames `RateLimitConfig` → +`ConnectorRateLimitConfig` (#4684, C9) + +Two entry points exported `RateLimitConfig` for **two different declarations**, +so which one you got depended only on the import path — the #4411 trap. They are +not variants of one concept; they describe opposite directions of traffic: + +| | `@objectstack/spec/shared` (unchanged) | `@objectstack/spec/integration` (renamed) | +|:--|:--|:--| +| what it limits | **inbound** — calls others make to our API | **outbound** — calls we make to an external system | +| written at | `apis[].rateLimit`, `httpServer.security.rateLimit` | `connectors[].rateLimitConfig` | +| window | `windowMs` (ms), defaults to 60000 | `windowSeconds` (s), **required**, min 1 | +| quota | `maxRequests`, defaults to 100 | `maxRequests`, **required**, min 1 | +| extras | `enabled` (default `false`) | `strategy`, `burstCapacity`, `respectUpstreamLimits`, `rateLimitHeaders` | + +Neither schema is `.strict()`, so a snippet copied from one side to the other +parsed **clean** with its foreign keys silently stripped — `RateLimitConfigSchema +.parse({ windowSeconds: 60, strategy: 'token_bucket' })` returned +`{ enabled: false, windowMs: 60000, maxRequests: 100 }` and nothing said a word. +Per ADR-0112 D9(a) — the same ruling that produced `ConnectorErrorCategory` and +`ConnectorRetryStrategy` in the same file — the **connector side is renamed** so +one name means one thing. + +## FROM → TO + +```ts +// before +import { RateLimitConfigSchema, type RateLimitConfig } from '@objectstack/spec/integration'; + +// after +import { + ConnectorRateLimitConfigSchema, + type ConnectorRateLimitConfig, +} from '@objectstack/spec/integration'; +``` + +No deprecated alias is kept: re-exporting the old name would be a third +declaration of it and would re-open the trap this change closes. + +**Importing from `@objectstack/spec/shared` (or `/api`, `/system`)? Nothing +changes** — that `RateLimitConfig` keeps its name, its keys and its defaults. + +## Authored metadata needs no migration + +This renames a TypeScript export and an internal JSON Schema `$def`, not an +authorable key. Every one of the six keys an author can write under +`connectors[].rateLimitConfig` — `strategy`, `maxRequests`, `windowSeconds`, +`burstCapacity`, `respectUpstreamLimits`, `rateLimitHeaders` — parses exactly as +before. Existing stack metadata, stored `sys_metadata` rows and published apps +are byte-for-byte unaffected, which is why this change ships with **no ADR-0087 +conversion and no tombstone**: nothing was retired. + +The only edit an upgrade needs is the import above, in TypeScript that named the +type. The published JSON Schema `$id` moves with it: +`…/integration/RateLimitConfig.json` → `…/integration/ConnectorRateLimitConfig.json`. + +## Gate change riding along + +`scripts/build-schemas.ts` learns a declarative `RENAMED_DEFS` table +(`scripts/lib/renamed-defs.ts`). Its two ratchets measure in `$def` units, so a +def rename previously read as six authorable keys vanishing at once. The table +carries the old snapshot forward under the new name and enforces the rule a +rename must obey: **every key under the old def must exist under the new one, or +the build fails** — plus the target must be emitted and the source must not (a +def that is still published is a copy, not a rename). This is stricter than the +hand-edited baseline it replaces, which could drop any line without a trace. diff --git a/content/docs/references/integration/connector.mdx b/content/docs/references/integration/connector.mdx index 7fbde4e520..9ec6c4e787 100644 --- a/content/docs/references/integration/connector.mdx +++ b/content/docs/references/integration/connector.mdx @@ -132,8 +132,8 @@ with simple `auth` — or by `[automation/sync.zod.ts](/docs/references/automati ## TypeScript Usage ```typescript -import { CircuitBreakerConfigSchema, ConnectorSchema, ConnectorActionSchema, ConnectorErrorCategorySchema, ConnectorHealthSchema, ConnectorRetryStrategySchema, ConnectorStatusSchema, ConnectorTriggerSchema, ConnectorTypeSchema, DataSyncConfigSchema, DeclarativeConnectorEntrySchema, ErrorMappingConfigSchema, ErrorMappingRuleSchema, HealthCheckConfigSchema, RateLimitStrategySchema, RetryConfigSchema, SyncStrategySchema, WebhookConfigSchema, WebhookEventSchema, WebhookSignatureAlgorithmSchema } from '@objectstack/spec/integration'; -import type { CircuitBreakerConfig, Connector, ConnectorErrorCategory, ConnectorHealth, ConnectorRetryStrategy, ConnectorStatus, ConnectorType, DataSyncConfig, DeclarativeConnectorEntry, ErrorMappingConfig, ErrorMappingRule, HealthCheckConfig, RateLimitStrategy, RetryConfig, SyncStrategy, WebhookConfig, WebhookEvent, WebhookSignatureAlgorithm } from '@objectstack/spec/integration'; +import { CircuitBreakerConfigSchema, ConnectorSchema, ConnectorActionSchema, ConnectorErrorCategorySchema, ConnectorHealthSchema, ConnectorRateLimitConfigSchema, ConnectorRetryStrategySchema, ConnectorStatusSchema, ConnectorTriggerSchema, ConnectorTypeSchema, DataSyncConfigSchema, DeclarativeConnectorEntrySchema, ErrorMappingConfigSchema, ErrorMappingRuleSchema, HealthCheckConfigSchema, RateLimitStrategySchema, RetryConfigSchema, SyncStrategySchema, WebhookConfigSchema, WebhookEventSchema, WebhookSignatureAlgorithmSchema } from '@objectstack/spec/integration'; +import type { CircuitBreakerConfig, Connector, ConnectorErrorCategory, ConnectorHealth, ConnectorRateLimitConfig, ConnectorRetryStrategy, ConnectorStatus, ConnectorType, DataSyncConfig, DeclarativeConnectorEntry, ErrorMappingConfig, ErrorMappingRule, HealthCheckConfig, RateLimitStrategy, RetryConfig, SyncStrategy, WebhookConfig, WebhookEvent, WebhookSignatureAlgorithm } from '@objectstack/spec/integration'; // Validate data const result = CircuitBreakerConfigSchema.parse(data); @@ -237,6 +237,22 @@ Connector health configuration | **circuitBreaker** | `{ enabled: boolean; failureThreshold: number; resetTimeoutMs: number; halfOpenMaxRequests: number; … }` | optional | Circuit breaker configuration | +--- + +## ConnectorRateLimitConfig + +### Properties + +| Property | Type | Required | Description | +| :--- | :--- | :--- | :--- | +| **strategy** | `Enum<'fixed_window' \| 'sliding_window' \| 'token_bucket' \| 'leaky_bucket'>` | ✅ | Rate limiting strategy | +| **maxRequests** | `number` | ✅ | Maximum requests per window | +| **windowSeconds** | `number` | ✅ | Time window in seconds | +| **burstCapacity** | `number` | optional | Burst capacity | +| **respectUpstreamLimits** | `boolean` | ✅ | Respect external rate limit headers | +| **rateLimitHeaders** | `{ remaining: string; limit: string; reset: string }` | optional | Custom rate limit headers | + + --- ## ConnectorRetryStrategy diff --git a/content/docs/references/integration/http.mdx b/content/docs/references/integration/http.mdx deleted file mode 100644 index b4ab04bbab..0000000000 --- a/content/docs/references/integration/http.mdx +++ /dev/null @@ -1,35 +0,0 @@ ---- -title: Http -description: Http protocol schemas ---- - -{/* ⚠️ AUTO-GENERATED — DO NOT EDIT. Run build-docs.ts to regenerate. Hand-written docs live in the module folders under content/docs/. */} - -## TypeScript Usage - -```typescript -import { RateLimitConfigSchema } from '@objectstack/spec/integration'; -import type { RateLimitConfig } from '@objectstack/spec/integration'; - -// Validate data -const result = RateLimitConfigSchema.parse(data); -``` - ---- - -## RateLimitConfig - -### Properties - -| Property | Type | Required | Description | -| :--- | :--- | :--- | :--- | -| **strategy** | `Enum<'fixed_window' \| 'sliding_window' \| 'token_bucket' \| 'leaky_bucket'>` | ✅ | Rate limiting strategy | -| **maxRequests** | `number` | ✅ | Maximum requests per window | -| **windowSeconds** | `number` | ✅ | Time window in seconds | -| **burstCapacity** | `number` | optional | Burst capacity | -| **respectUpstreamLimits** | `boolean` | ✅ | Respect external rate limit headers | -| **rateLimitHeaders** | `{ remaining: string; limit: string; reset: string }` | optional | Custom rate limit headers | - - ---- - diff --git a/content/docs/references/integration/meta.json b/content/docs/references/integration/meta.json index b125c0f212..d49912f689 100644 --- a/content/docs/references/integration/meta.json +++ b/content/docs/references/integration/meta.json @@ -6,7 +6,6 @@ "connector-auth", "mapping", "---Transport & Storage---", - "http", "offline" ] } \ No newline at end of file diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 5fb94ae4a8..b020a6c891 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -3939,6 +3939,8 @@ "ConnectorOrigin (type)", "ConnectorProviderContext (interface)", "ConnectorProviderFactory (type)", + "ConnectorRateLimitConfig (type)", + "ConnectorRateLimitConfigSchema (const)", "ConnectorRetryStrategy (type)", "ConnectorRetryStrategySchema (const)", "ConnectorSchema (const)", @@ -3961,8 +3963,6 @@ "FieldMappingSchema (const)", "HealthCheckConfig (type)", "HealthCheckConfigSchema (const)", - "RateLimitConfig (type)", - "RateLimitConfigSchema (const)", "RateLimitStrategy (type)", "RateLimitStrategySchema (const)", "ResolvedConnectorAuth (type)", diff --git a/packages/spec/authorable-surface.json b/packages/spec/authorable-surface.json index 8c2e2872fa..7f1a05eba9 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -4156,6 +4156,12 @@ "integration/ConnectorInstanceBearerAuth:credentialRef", "integration/ConnectorInstanceBearerAuth:type", "integration/ConnectorInstanceNoAuth:type", + "integration/ConnectorRateLimitConfig:burstCapacity", + "integration/ConnectorRateLimitConfig:maxRequests", + "integration/ConnectorRateLimitConfig:rateLimitHeaders", + "integration/ConnectorRateLimitConfig:respectUpstreamLimits", + "integration/ConnectorRateLimitConfig:strategy", + "integration/ConnectorRateLimitConfig:windowSeconds", "integration/ConnectorTrigger:description", "integration/ConnectorTrigger:interval", "integration/ConnectorTrigger:key", @@ -4219,12 +4225,6 @@ "integration/HealthCheckConfig:method", "integration/HealthCheckConfig:timeoutMs", "integration/HealthCheckConfig:unhealthyThreshold", - "integration/RateLimitConfig:burstCapacity", - "integration/RateLimitConfig:maxRequests", - "integration/RateLimitConfig:rateLimitHeaders", - "integration/RateLimitConfig:respectUpstreamLimits", - "integration/RateLimitConfig:strategy", - "integration/RateLimitConfig:windowSeconds", "integration/RetryConfig:backoffMultiplier", "integration/RetryConfig:initialDelayMs", "integration/RetryConfig:jitter", diff --git a/packages/spec/dual-source-exports.baseline.json b/packages/spec/dual-source-exports.baseline.json index dee39afe8e..0b5c76ffe5 100644 --- a/packages/spec/dual-source-exports.baseline.json +++ b/packages/spec/dual-source-exports.baseline.json @@ -15,8 +15,6 @@ "HttpMethod — [./api, ./shared (type)] ≠ [./ui (type)]", "PackageDependency — [./cloud (type)] ≠ [./kernel (type)]", "PackageDependencySchema — [./cloud (const)] ≠ [./kernel (const)]", - "RateLimitConfig — [./integration (type)] ≠ [./shared (type)]", - "RateLimitConfigSchema — [./integration (const)] ≠ [./shared (const)]", "TenantPlan — [./cloud (type)] ≠ [./system (type)]", "TenantPlanSchema — [./cloud (const)] ≠ [./system (const)]" ] diff --git a/packages/spec/json-schema.manifest.json b/packages/spec/json-schema.manifest.json index e12df638c1..6d2691aac8 100644 --- a/packages/spec/json-schema.manifest.json +++ b/packages/spec/json-schema.manifest.json @@ -879,6 +879,7 @@ "integration/ConnectorInstanceBasicAuth", "integration/ConnectorInstanceBearerAuth", "integration/ConnectorInstanceNoAuth", + "integration/ConnectorRateLimitConfig", "integration/ConnectorRetryStrategy", "integration/ConnectorStatus", "integration/ConnectorTrigger", @@ -889,7 +890,6 @@ "integration/ErrorMappingRule", "integration/FieldMapping", "integration/HealthCheckConfig", - "integration/RateLimitConfig", "integration/RateLimitStrategy", "integration/RetryConfig", "integration/SyncStrategy", diff --git a/packages/spec/scripts/build-schemas.ts b/packages/spec/scripts/build-schemas.ts index 82871782c8..9c1d4dd494 100644 --- a/packages/spec/scripts/build-schemas.ts +++ b/packages/spec/scripts/build-schemas.ts @@ -9,6 +9,7 @@ import fs from 'fs'; import path from 'path'; import { z } from 'zod'; import { schemaNameFromExportKey } from './lib/schema-name'; +import { RENAMED_DEFS, carryAuthorableKey, checkRenameTable } from './lib/renamed-defs'; import { CONVERSIONS_BY_MAJOR } from '../src/conversions/registry'; import { MIGRATIONS_BY_MAJOR } from '../src/migrations/registry'; import * as AI from '../src/ai'; @@ -301,7 +302,24 @@ try { } const generatedKeys = new Set(generatedSchemas.keys()); -const missing = (manifest?.schemas ?? []).filter((key) => !generatedKeys.has(key)); + +// ─── Declared def renames must describe THIS build ──────────────────── +// Both ratchets below consult RENAMED_DEFS, so an entry that no longer matches +// reality (target never emitted, or source still emitted alongside it) would +// weaken them silently. Fail before either one runs. See lib/renamed-defs.ts. +const renameProblems = checkRenameTable(generatedKeys); +if (renameProblems.length > 0) { + console.error(`\n❌ ${renameProblems.length} problem(s) in RENAMED_DEFS (scripts/lib/renamed-defs.ts):`); + for (const p of renameProblems) console.error(` - ${p}`); + process.exit(1); +} + +const missing = (manifest?.schemas ?? []).filter( + // A def listed as renamed is not missing — it is published under the new + // name, which `checkRenameTable` just proved this build emits. The manifest + // rewrite below drops the old key, so the entry self-clears on regeneration. + (key) => !generatedKeys.has(key) && !(key in RENAMED_DEFS), +); if (missing.length > 0) { console.error(`\n❌ ${missing.length} previously published schema(s) disappeared from this build:`); for (const key of missing) { @@ -319,7 +337,12 @@ if (missing.length > 0) { } const added = [...generatedKeys].filter((key) => !(manifest?.schemas ?? []).includes(key)); -if (!manifest || added.length > 0) { +// A renamed-away source key must be dropped from the manifest even in the (rare) +// case where the new name adds nothing — e.g. a rename onto a def that already +// existed. Without this the stale key would sit in the manifest forever, kept +// alive only by its RENAMED_DEFS entry. +const renamedAway = (manifest?.schemas ?? []).filter((key) => key in RENAMED_DEFS); +if (!manifest || added.length > 0 || renamedAway.length > 0) { const updated: SchemaManifest = { description: 'Ratchet manifest of every JSON Schema emitted by scripts/build-schemas.ts. ' + @@ -401,10 +424,46 @@ if (fs.existsSync(AUTHORABLE_SURFACE_PATH)) { } if (surfaceDoc) { - const prev = new Map( + const snapshot = new Map( surfaceDoc.keys.map((e) => [e.replace(RETIRED_MARK, ''), e.endsWith(RETIRED_MARK)]), ); + // Carry the snapshot through any declared def rename FIRST, so every check + // below compares like with like. A rename moves keys between defs; it must + // never be able to drop one, and it must never launder a retirement past + // check (b) either — which is why the carried key keeps the OLD key's + // retired state. See scripts/lib/renamed-defs.ts (#4684). + const prev = new Map(); + const carriedFrom = new Map(); // new key -> old key + for (const [key, retired] of snapshot) { + const carried = carryAuthorableKey(key); + if (carried !== key) carriedFrom.set(carried, key); + prev.set(carried, retired); + } + + // (a0) A declared rename that did not carry one of its keys. Reported apart + // from (a) because the remedy is the opposite one: the key did not leave + // the contract by accident of a deletion, it failed to arrive under the + // new def — restore it there, or stop calling this a rename. + const notCarried = [...carriedFrom.entries()].filter(([to]) => !currentKeys.has(to)); + if (notCarried.length > 0) { + console.error( + `\n❌ ${notCarried.length} authorable key(s) were lost by a declared def rename:`, + ); + for (const [to, from] of notCarried) console.error(` - ${from} → ${to} (absent)`); + console.error( + `\n RENAMED_DEFS (scripts/lib/renamed-defs.ts) declares that these defs were renamed,\n` + + ` and a rename must carry EVERY key: the author-facing contract is unchanged, only\n` + + ` an internal schema name moved. A key missing under the new name is a real removal\n` + + ` wearing a rename's clothes — and these schemas are NOT .strict(), so Zod would\n` + + ` silently strip whatever the author kept writing (#3733, ADR-0104).\n\n` + + ` Either re-add the key under the new def, or — if it is genuinely being retired —\n` + + ` tombstone it there with \`retiredKey()\` plus its registered D2 conversion, exactly\n` + + ` as a retirement without a rename would require.`, + ); + process.exit(1); + } + // (a) A key that vanished outright. The silent-strip class — always fatal. const vanished = [...prev.keys()].filter((k) => !currentKeys.has(k)); if (vanished.length > 0) { diff --git a/packages/spec/scripts/lib/renamed-defs.ts b/packages/spec/scripts/lib/renamed-defs.ts new file mode 100644 index 0000000000..b90752390c --- /dev/null +++ b/packages/spec/scripts/lib/renamed-defs.ts @@ -0,0 +1,111 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Declarative carry-over table for JSON Schema **def renames** (#4684). + * + * ## The gap this closes + * + * `build-schemas.ts` runs two ratchets, and both measure in units of the def + * key (`/`): + * + * - `json-schema.manifest.json` — every schema ever published; + * - `authorable-surface.json` — every `:` an author may write. + * + * Renaming an exported schema const renames its def, and to both ratchets that + * is indistinguishable from a **deletion**: the manifest sees a published + * schema disappear, and the authorable surface sees every key under the old def + * vanish. Yet in a pure rename *nothing leaves the author-facing contract* — + * `connectors[].rateLimitConfig.windowSeconds` still parses byte-for-byte the + * same. Only an internal schema name moved. + * + * The three remedies the ratchets suggest are all wrong for a rename: + * + * 1. hand-edit `authorable-surface.json` — forbidden (#4650): the snapshot is + * generated, and editing it is exactly how a real deletion would hide; + * 2. `retiredKey()` + an ADR-0087 D2 conversion — semantically false. Nothing + * is retired, so the tombstone has no live def to hang on and the + * conversion would have to name an author path that never changed. + * Registering it would pollute the ADR-0087 registries with a migration + * consumers must not run (the "green gate, wrong ledger" class of #4659); + * 3. delete the manifest line as a "deliberate removal" — right mechanism, + * wrong claim, and it says nothing about the keys underneath. + * + * So the ratchets learn renames instead, from this table. + * + * ## The rule + * + * > Every key under the OLD def must exist under the NEW def. Otherwise: red. + * + * This is strictly **stronger** than the status quo it replaces. Hand-editing + * the baseline (the practice #4650 banned) can silently drop any line at all; + * a declared rename cannot drop even one, and a key that is genuinely being + * retired during a rename still has to carry its tombstone and its registered + * migration (`build-schemas.ts` re-runs check (b) against the carried key's + * previous state). The table also fails on its own decay: a target that this + * build does not emit, or a source that it still emits — that is a copy, not a + * rename — is rejected before either ratchet runs. + * + * ## Adding an entry + * + * A rename is a breaking change for anyone importing the type by name, so an + * entry here rides with a `major` changeset spelling FROM → TO. Entries stay + * after the surface snapshot has been regenerated (they are then inert against + * the snapshot but still enforce the hygiene invariants above), and are pruned + * only when the old name has aged out — same discipline as a tombstone. + */ +export const RENAMED_DEFS: Readonly> = { + // #4684 / ADR-0112 D9a — the connector-side (outbound throttling) config no + // longer shares a name with `shared/RateLimitConfig` (inbound API limiting). + 'integration/RateLimitConfig': 'integration/ConnectorRateLimitConfig', +}; + +/** + * Rewrite an authorable-surface key (`:`) through the rename table. + * Returns the key unchanged when its def is not declared renamed. + * + * Only the def part is rewritten — a rename moves keys, it never renames them. + */ +export function carryAuthorableKey( + key: string, + renames: Readonly> = RENAMED_DEFS, +): string { + const sep = key.indexOf(':'); + if (sep < 0) return key; + const to = renames[key.slice(0, sep)]; + return to === undefined ? key : to + key.slice(sep); +} + +/** + * Validate the table against the defs a build actually emitted. + * + * Returns one human-readable problem line per broken entry; an empty array + * means the table is honest about this build. + */ +export function checkRenameTable( + emittedDefs: ReadonlySet, + renames: Readonly> = RENAMED_DEFS, +): string[] { + const problems: string[] = []; + for (const [from, to] of Object.entries(renames)) { + if (from === to) { + problems.push(`${from} → ${to}: source and target are the same def.`); + continue; + } + if (emittedDefs.has(from)) { + problems.push( + `${from} → ${to}: the SOURCE def is still emitted by this build. ` + + `That is a copy, not a rename — and a copy is precisely the dual-source ` + + `shape this table must never be able to launder (#4411, #4446).`, + ); + } + if (!emittedDefs.has(to)) { + problems.push( + `${from} → ${to}: the TARGET def is not emitted by this build. ` + + `Either the new name is misspelled here, or the renamed schema was ` + + `since deleted — in which case its keys really did leave the contract ` + + `and need the tombstone route, not this table.`, + ); + } + } + return problems; +} diff --git a/packages/spec/scripts/renamed-defs.test.ts b/packages/spec/scripts/renamed-defs.test.ts new file mode 100644 index 0000000000..c25df24d25 --- /dev/null +++ b/packages/spec/scripts/renamed-defs.test.ts @@ -0,0 +1,106 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Unit pins for the def-rename carry-over table (#4684). + * + * `build-schemas.ts` is a top-level script — importing it runs a full schema + * build — so the rules it enforces live in `lib/renamed-defs.ts` where they can + * be exercised directly. These tests pin the *rules*; the wiring is pinned by + * the gate itself, which the PR's sabotage runs exercised (drop one key from + * the renamed def → red; leave the old def emitted → red; misspell the target + * → red). + * + * The point of every assertion below is the same: a rename entry must be able + * to explain a def key moving, and must NOT be able to explain a key going + * away. That asymmetry is the whole reason the table is safe to add. + */ +import { describe, it, expect } from 'vitest'; +import { + RENAMED_DEFS, + carryAuthorableKey, + checkRenameTable, +} from './lib/renamed-defs'; + +describe('carryAuthorableKey', () => { + const renames = { 'integration/Old': 'integration/New' } as const; + + it('moves a key to the renamed def, leaving the property name alone', () => { + expect(carryAuthorableKey('integration/Old:windowSeconds', renames)).toBe( + 'integration/New:windowSeconds', + ); + }); + + it('leaves keys of undeclared defs untouched', () => { + expect(carryAuthorableKey('shared/RateLimitConfig:windowMs', renames)).toBe( + 'shared/RateLimitConfig:windowMs', + ); + }); + + it('matches the def exactly — a prefix is not a rename', () => { + // `integration/OldThing` merely starts with a renamed def's name. Rewriting + // it would silently retarget an unrelated schema's keys. + expect(carryAuthorableKey('integration/OldThing:x', renames)).toBe( + 'integration/OldThing:x', + ); + }); + + it('rewrites only the first separator, so a property containing ":" survives', () => { + expect(carryAuthorableKey('integration/Old:a:b', renames)).toBe('integration/New:a:b'); + }); + + it('returns a bare def key (no property) unchanged', () => { + expect(carryAuthorableKey('integration/Old', renames)).toBe('integration/Old'); + }); +}); + +describe('checkRenameTable', () => { + const renames = { 'integration/Old': 'integration/New' } as const; + + it('accepts a rename the build actually performed', () => { + expect(checkRenameTable(new Set(['integration/New']), renames)).toEqual([]); + }); + + it('rejects a rename whose source def is STILL emitted (a copy, not a rename)', () => { + const problems = checkRenameTable( + new Set(['integration/Old', 'integration/New']), + renames, + ); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain('SOURCE def is still emitted'); + }); + + it('rejects a rename whose target def does not exist (typo, or the def was deleted)', () => { + const problems = checkRenameTable(new Set(['integration/Other']), renames); + // Source absent + target absent → only the target complaint fires. + expect(problems).toHaveLength(1); + expect(problems[0]).toContain('TARGET def is not emitted'); + }); + + it('rejects a no-op entry', () => { + const problems = checkRenameTable(new Set(['integration/Old']), { + 'integration/Old': 'integration/Old', + }); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain('source and target are the same def'); + }); +}); + +describe('the committed RENAMED_DEFS table', () => { + it('records the #4684 connector rate-limit rename', () => { + expect(RENAMED_DEFS['integration/RateLimitConfig']).toBe( + 'integration/ConnectorRateLimitConfig', + ); + }); + + it('leaves the shared (inbound) declaration alone — only the connector side moved', () => { + // ADR-0112 D9a renames the CONNECTOR side so one name means one thing; + // `shared/RateLimitConfig` is the incumbent and keeps its name and its keys. + expect(RENAMED_DEFS['shared/RateLimitConfig']).toBeUndefined(); + }); + + it('is well-formed: no self-renames, no two defs claiming one target', () => { + const targets = Object.values(RENAMED_DEFS); + for (const [from, to] of Object.entries(RENAMED_DEFS)) expect(from).not.toBe(to); + expect(new Set(targets).size).toBe(targets.length); + }); +}); diff --git a/packages/spec/src/integration/connector.test.ts b/packages/spec/src/integration/connector.test.ts index 60005699fc..bf7bb4852f 100644 --- a/packages/spec/src/integration/connector.test.ts +++ b/packages/spec/src/integration/connector.test.ts @@ -14,7 +14,7 @@ import { WebhookEventSchema, // Rate Limiting & Retry - RateLimitConfigSchema, + ConnectorRateLimitConfigSchema, RetryConfigSchema, // Base Connector @@ -269,7 +269,7 @@ describe('WebhookConfigSchema', () => { // Rate Limiting & Retry Tests // ============================================================================ -describe('RateLimitConfigSchema', () => { +describe('ConnectorRateLimitConfigSchema', () => { it('should accept valid rate limit configuration', () => { const config = { strategy: 'token_bucket', @@ -279,7 +279,7 @@ describe('RateLimitConfigSchema', () => { respectUpstreamLimits: true, }; - expect(() => RateLimitConfigSchema.parse(config)).not.toThrow(); + expect(() => ConnectorRateLimitConfigSchema.parse(config)).not.toThrow(); }); it('should use default values', () => { @@ -288,7 +288,7 @@ describe('RateLimitConfigSchema', () => { windowSeconds: 60, }; - const parsed = RateLimitConfigSchema.parse(config); + const parsed = ConnectorRateLimitConfigSchema.parse(config); expect(parsed.strategy).toBe('token_bucket'); expect(parsed.respectUpstreamLimits).toBe(true); }); @@ -654,3 +654,143 @@ describe('ConnectorHealthSchema', () => { expect(connector.errorMapping?.rules).toHaveLength(1); }); }); + +// ─── [#4684] Dual-source regression pin ────────────────────────────── +// +// RUNTIME + compiler-API assertions, deliberately. #4642 established that a +// compile-time pin in `packages/spec` is a no-op: `tsconfig.json` excludes +// `**/*.test.ts` and `vitest.config.ts` never enables `typecheck`, so an +// `Assert< Equal< … > >` here would be dead text. The third test below is the +// only shape in this repo that actually pins a TYPE. +// +// What these defend: `RateLimitConfig` naming exactly ONE declaration across +// the published entries. `./shared` limits INBOUND API traffic (`enabled` / +// `windowMs` / `maxRequests`, every key defaulted); `./integration` throttles +// OUTBOUND connector calls (`strategy` / `maxRequests` / `windowSeconds` +// required, plus the upstream `X-RateLimit-*` header names). Neither is a +// superset of the other and neither schema is `.strict()`, so before #4684 a +// snippet copied from one side to the other PARSED CLEAN with its foreign keys +// silently stripped — ADR-0104's silent-strip class, in the export surface +// rather than in stored metadata. ADR-0112 D9a's remedy applies: the +// connector-side name gets a `Connector` prefix so one name means one thing. +describe('[#4684] RateLimitConfig no longer names two declarations', () => { + it('./integration exposes the connector shape only under its prefixed name', async () => { + const integrationEntry = await import('./index'); + + expect(integrationEntry.ConnectorRateLimitConfigSchema).toBeDefined(); + // The bare name must be gone from this entry — an alias re-export kept "for + // compatibility" would be a THIRD declaration of the same name and would + // re-open the trap this issue closed. + expect('RateLimitConfigSchema' in integrationEntry).toBe(false); + }); + + it('./shared keeps the inbound declaration untouched', async () => { + const sharedEntry = await import('../shared/index'); + + // Same object identity as before the rename, same three defaulted keys. + expect(sharedEntry.RateLimitConfigSchema.parse({})).toEqual({ + enabled: false, + windowMs: 60000, + maxRequests: 100, + }); + }); + + it('the two schemas remain distinct declarations that reject each other’s shape', async () => { + const integrationEntry = await import('./index'); + const sharedEntry = await import('../shared/index'); + + expect(integrationEntry.ConnectorRateLimitConfigSchema).not.toBe( + sharedEntry.RateLimitConfigSchema, + ); + + // The outbound shape demands what the inbound shape defaults away. + expect(integrationEntry.ConnectorRateLimitConfigSchema.safeParse({}).success).toBe(false); + // And the inbound shape still strips outbound keys — which is exactly why + // the two must not answer to one name. Pinned, not fixed: it is correct + // behaviour for a non-strict schema; the defect was the shared NAME. + expect(sharedEntry.RateLimitConfigSchema.parse({ windowSeconds: 60, strategy: 'token_bucket' })) + .toEqual({ enabled: false, windowMs: 60000, maxRequests: 100 }); + }); + + // The load-bearing one. `RateLimitConfig` / `ConnectorRateLimitConfig` are + // TYPES — erased before any runtime assertion can see them — so every test + // above would stay green if `./integration` re-added `export type + // RateLimitConfig = z.infer<…>`, which is precisely the defect. This resolves + // each entry's exports through their alias chains to the ORIGINAL + // declaration: the same symbol-identity measurement + // `check:dual-source-exports` makes, but over `src/` so it runs in `pnpm test` + // without a build. + it('no name resolves to two declarations across ./shared and ./integration (types included)', async () => { + const ts = (await import('typescript')).default; + const { resolve, relative, dirname } = await import('node:path'); + const { fileURLToPath } = await import('node:url'); + + const specDir = resolve(dirname(fileURLToPath(import.meta.url)), '../..'); + const entries = { + './shared': resolve(specDir, 'src/shared/index.ts'), + './integration': resolve(specDir, 'src/integration/index.ts'), + }; + const program = ts.createProgram(Object.values(entries), { + module: ts.ModuleKind.ESNext, + moduleResolution: ts.ModuleResolutionKind.Bundler, + skipLibCheck: true, + noEmit: true, + }); + const checker = program.getTypeChecker(); + const unalias = (s: import('typescript').Symbol) => + s.getFlags() & ts.SymbolFlags.Alias ? checker.getAliasedSymbol(s) : s; + + /** entry → exported name → `file:line` of the ORIGINAL declaration. */ + const originsByEntry = new Map>(); + for (const [sub, file] of Object.entries(entries)) { + const sf = program.getSourceFile(file); + const moduleSym = sf && checker.getSymbolAtLocation(sf); + // Without this, a resolution failure would make every assertion below + // pass vacuously — the exact way a gate goes dormant (#4642). + expect(moduleSym, `${sub} module symbol must resolve`).toBeTruthy(); + + const origins = new Map(); + for (const exported of checker.getExportsOfModule(moduleSym!)) { + const decl = unalias(exported).declarations?.[0]; + if (!decl) continue; + const declFile = decl.getSourceFile(); + origins.set( + exported.getName(), + `${relative(specDir, declFile.fileName)}:${ + declFile.getLineAndCharacterOfPosition(decl.getStart()).line + 1 + }`, + ); + } + // Guard #2: an entry that resolved to nothing would also pass vacuously. + expect(origins.size, `${sub} must export something`).toBeGreaterThan(20); + originsByEntry.set(sub, origins); + } + + const shared = originsByEntry.get('./shared')!; + const integration = originsByEntry.get('./integration')!; + + // The names this issue is about: present on the right entry, absent from + // the wrong one, and each resolving to its own file. + expect(integration.get('ConnectorRateLimitConfig')).toMatch( + /^src\/integration\/connector\.zod\.ts:\d+$/, + ); + expect(integration.get('ConnectorRateLimitConfigSchema')).toMatch( + /^src\/integration\/connector\.zod\.ts:\d+$/, + ); + expect(integration.get('RateLimitConfig')).toBeUndefined(); + expect(integration.get('RateLimitConfigSchema')).toBeUndefined(); + expect(shared.get('RateLimitConfig')).toMatch(/^src\/shared\/http\.zod\.ts:\d+$/); + expect(shared.get('RateLimitConfigSchema')).toMatch(/^src\/shared\/http\.zod\.ts:\d+$/); + + // And the general invariant for this pair of entries: any name they BOTH + // export must resolve to one and the same declaration. `FieldMapping` / + // `FieldMappingSchema` are the remaining known offenders (#4535 C12) — they + // stay listed here so this pin fails the moment a NEW one appears, instead + // of being written as a blanket "no shared names" that never held. + const KNOWN_STILL_DUAL_SOURCE = ['FieldMapping', 'FieldMappingSchema']; + const conflicts = [...shared.keys()] + .filter((name) => integration.has(name) && integration.get(name) !== shared.get(name)) + .sort(); + expect(conflicts).toEqual(KNOWN_STILL_DUAL_SOURCE); + }); +}); diff --git a/packages/spec/src/integration/connector.zod.ts b/packages/spec/src/integration/connector.zod.ts index 988ce9124d..0c2587ecb8 100644 --- a/packages/spec/src/integration/connector.zod.ts +++ b/packages/spec/src/integration/connector.zod.ts @@ -295,9 +295,21 @@ export const RateLimitStrategySchema = lazySchema(() => z.enum([ export type RateLimitStrategy = z.infer; /** - * Rate Limiting Configuration + * Rate Limiting Configuration — connector-side (OUTBOUND throttling). + * + * `Connector`-prefixed because `shared/http.zod.ts` exports a different + * `RateLimitConfig` for INBOUND API rate limiting (`enabled` / `windowMs` / + * `maxRequests`, all defaulted). The two describe opposite directions and are + * not interchangeable: this one throttles the calls *we* make to an external + * system (token bucket, burst capacity, and the upstream `X-RateLimit-*` + * response headers we read back), while the shared one limits the calls + * *others* make to us — where `respectUpstreamLimits` / `rateLimitHeaders` are + * structurally meaningless. Same name, different units (`windowSeconds` vs + * `windowMs`) and different requiredness, so an author copying a snippet from + * one side to the other got a clean parse with the foreign keys silently + * stripped (#4684, ADR-0104). They may not share a name (ADR-0112 D9a). */ -export const RateLimitConfigSchema = lazySchema(() => z.object({ +export const ConnectorRateLimitConfigSchema = lazySchema(() => z.object({ /** * Rate limiting strategy */ @@ -333,7 +345,7 @@ export const RateLimitConfigSchema = lazySchema(() => z.object({ }).optional().describe('Custom rate limit headers'), })); -export type RateLimitConfig = z.infer; +export type ConnectorRateLimitConfig = z.infer; /** * Retry Strategy — connector-side. @@ -671,7 +683,7 @@ export const ConnectorSchema = lazySchema(() => z.object({ /** * Rate limiting configuration */ - rateLimitConfig: RateLimitConfigSchema.optional().describe('Rate limiting configuration'), + rateLimitConfig: ConnectorRateLimitConfigSchema.optional().describe('Rate limiting configuration'), /** * Retry configuration