Files
olp/lib/providers/base.mjs
T
taodengandClaude Opus 4.7 1466d3a082 fix(providers): D21 — validateProvider enforces maxSpawnTimeMs contract field (round-2 F1)
cold-audit catch from 2026-05-24

Round-2 cold-audit Finding 1 (P2 contract drift). ADR 0002 Amendment 1
(D11, commit f659e29) added `maxSpawnTimeMs` to the Provider contract
v1.0 hints set alongside requiresTTY/concurrentSpawnSafe/maxConcurrent.
But `lib/providers/base.mjs` validateProvider was never updated —
ProviderHints typedef listed only the original 3 fields, and the
validator's error message + per-field checks ignored maxSpawnTimeMs
entirely. A plugin that omitted the field would still pass validation;
each plugin's `?? 600_000` runtime default masked the omission at the
spawn site. Round-1 cold audit + D11 diff review both missed this.

Changes (lib/providers/base.mjs +6 / -1, test-features.mjs +36):

1. ProviderHints typedef extended:
   `@property {number} [maxSpawnTimeMs] - optional integer milliseconds, default 600000`
   The `[name]` JSDoc marker correctly indicates optional.

2. Hints existence error message updated to include the new field:
   `'hints must be an object with { requiresTTY, concurrentSpawnSafe, maxConcurrent, maxSpawnTimeMs }'`

3. Per-field validation added inside the hints-object branch:
   ```
   if (p.hints.maxSpawnTimeMs !== undefined) {
     if (typeof !== 'number' || !Number.isInteger(...) || <= 0) push error
   }
   ```
   The `!== undefined` outer guard correctly short-circuits omission to
   "valid" (the field is optional per ADR). The inner check rejects:
   negative, zero, non-integer (catches floats, NaN, Infinity since
   Number.isInteger handles all non-finite cases), and non-number types.

4. test-features.mjs Suite 4: 6 new tests covering all rejection axes
   + both positive-path cases (omitted-is-valid, positive-integer-is-valid):
   - rejects negative
   - rejects zero
   - rejects non-integer (100.5)
   - rejects non-number ('600' string)
   - accepts omitted (optional field)
   - accepts positive integer (60000)

Tests: 328 → 334 (+6). All three shipped plugins (anthropic / codex /
mistral) declare maxSpawnTimeMs: 600_000 (positive integer) and continue
to pass validation. Suite 16 spawn-timeout tests confirm the existing
positive integer flows through to the runtime enforcement loop unchanged.

Authority:
- ADR 0002 § Amendments § Amendment 1 (D11, 2026-05-23) — establishes
  maxSpawnTimeMs as part of the v1.0 hints contract
  https://github.com/dtzp555-max/olp/blob/main/docs/adr/0002-plugin-architecture.md
- ADR 0004 § Trigger taxonomy — Hard triggers bullet 4 — the contract
  field's use site (per-plugin spawn-timeout enforcement → SPAWN_TIMEOUT
  hard trigger)
- CC 开发铁律 v1.6 § 10.x — Round-2 Cold Audit caught this

Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent
of drafter): APPROVE. Verified ADR Amendment 1 wording matches validator
behavior; verified all 4 rejection axes plus 2 positive axes covered
by tests; verified existing plugin declarations continue to pass via
334/334 (Suite 16 spawn-timeout suite included). No blocking issues.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-24 14:06:21 +10:00

226 lines
8.4 KiB
JavaScript

/**
* lib/providers/base.mjs — Provider contract definition and shared helpers
*
* Authority: ADR 0002 § "Provider contract (v1.0 interface)"
*
* This module does NOT implement the Provider contract itself.
* Provider plugins compose the helpers exported here; they do not inherit
* from a base class (per ADR 0002 § Consequences/Mitigations: "compose helpers,
* do not inherit").
*/
// ── Contract typedef ──────────────────────────────────────────────────────
/**
* @typedef {Object} ProviderAuth
* @property {string} type - e.g. 'subscription', 'api-key', 'oauth'
* @property {string} storage - e.g. 'cli-managed', 'env', 'keychain'
* @property {string} path - artifact location hint
* @property {string|null} refresh - refresh mechanism or null if not applicable
*/
/**
* @typedef {Object} ProviderHints
* @property {boolean} requiresTTY
* @property {boolean} concurrentSpawnSafe
* @property {number} maxConcurrent
* @property {number} [maxSpawnTimeMs] - optional integer milliseconds, default 600000
*/
/**
* @typedef {Object} ProviderContractV1
* @property {string} name - unique lowercase key
* @property {string} displayName - human-readable name
* @property {'1.0'} contractVersion - must be '1.0' for v1.0 plugins (D4 fold-in per reviewer F3)
* @property {string[]} models - model strings this provider serves
* @property {ProviderAuth} auth
* @property {function} spawn - async (irRequest, authContext) => AsyncIterator<IRResponseChunk>
* @property {function} estimateCost - (request) => {inputTokens, outputTokensEstimate, currency, usd}|null
* @property {function} quotaStatus - async (authContext) => {available, percentUsed, resetsAt, pool}|null
* @property {function} healthCheck - async () => {ok: boolean, latencyMs: number, error?: string}
* @property {ProviderHints} hints
*/
// ── Contract validator ────────────────────────────────────────────────────
/**
* Validates that a plugin object satisfies the v1.0 Provider contract.
* Per ADR 0002 § "Loading model", the registry calls this at startup for
* every registered provider; an invalid provider throws rather than silently
* degrading.
*
* @param {*} p
* @returns {{ valid: boolean, errors: string[] }}
*/
export function validateProvider(p) {
const errors = [];
if (!p || typeof p !== 'object') {
errors.push('provider must be an object');
return { valid: false, errors };
}
if (typeof p.name !== 'string' || p.name.trim() === '') {
errors.push('name must be a non-empty string');
} else if (!/^[a-z][a-z0-9_-]*$/.test(p.name)) {
errors.push('name must be lowercase alphanumeric (with _ or -) starting with a letter');
}
if (typeof p.displayName !== 'string' || p.displayName.trim() === '') {
errors.push('displayName must be a non-empty string');
}
// contractVersion: required to be exactly '1.0' for v1.0 plugins (D4 fold-in per reviewer F3)
// Per ADR 0002 § Mitigations: "The contract is versioned. v1.0 is the subset in this ADR;
// future additions require ADR amendment plus a contract-version bump."
if (p.contractVersion !== '1.0') {
errors.push(`contractVersion must be '1.0', got ${JSON.stringify(p.contractVersion)}`);
}
if (!Array.isArray(p.models)) {
errors.push('models must be an array of strings');
} else if (p.models.some(m => typeof m !== 'string')) {
errors.push('every entry in models must be a string');
}
if (!p.auth || typeof p.auth !== 'object') {
errors.push('auth must be an object with { type, storage, path, refresh }');
} else {
if (typeof p.auth.type !== 'string') errors.push('auth.type must be a string');
if (typeof p.auth.storage !== 'string') errors.push('auth.storage must be a string');
if (typeof p.auth.path !== 'string') errors.push('auth.path must be a string');
if (p.auth.refresh !== null && typeof p.auth.refresh !== 'string') {
errors.push('auth.refresh must be a string or null');
}
}
if (typeof p.spawn !== 'function') {
errors.push('spawn must be a function');
}
if (typeof p.estimateCost !== 'function') {
errors.push('estimateCost must be a function');
}
if (typeof p.quotaStatus !== 'function') {
errors.push('quotaStatus must be a function');
}
if (typeof p.healthCheck !== 'function') {
errors.push('healthCheck must be a function');
}
if (!p.hints || typeof p.hints !== 'object') {
errors.push('hints must be an object with { requiresTTY, concurrentSpawnSafe, maxConcurrent, maxSpawnTimeMs }');
} else {
if (typeof p.hints.requiresTTY !== 'boolean') errors.push('hints.requiresTTY must be a boolean');
if (typeof p.hints.concurrentSpawnSafe !== 'boolean') errors.push('hints.concurrentSpawnSafe must be a boolean');
if (typeof p.hints.maxConcurrent !== 'number' || !Number.isInteger(p.hints.maxConcurrent) || p.hints.maxConcurrent < 0) {
errors.push('hints.maxConcurrent must be a non-negative integer');
}
if (p.hints.maxSpawnTimeMs !== undefined) {
if (typeof p.hints.maxSpawnTimeMs !== 'number' || !Number.isInteger(p.hints.maxSpawnTimeMs) || p.hints.maxSpawnTimeMs <= 0) {
errors.push('hints.maxSpawnTimeMs must be a positive integer (milliseconds) or omitted');
}
}
}
return { valid: errors.length === 0, errors };
}
// ── Error class ───────────────────────────────────────────────────────────
/** Error codes surfaced by provider plugins */
export const PROVIDER_ERROR_CODES = /** @type {const} */ ([
'AUTH_MISSING',
'QUOTA_EXHAUSTED',
'RATE_LIMITED',
'CLI_NOT_FOUND',
'SPAWN_FAILED',
'OUTPUT_PARSE_ERROR',
'SPAWN_TIMEOUT', // ADR 0004 § Trigger taxonomy bullet 4: spawn timeout is a hard trigger
]);
export class ProviderError extends Error {
/**
* @param {string} message
* @param {typeof PROVIDER_ERROR_CODES[number]} code
*/
constructor(message, code) {
super(message);
this.name = 'ProviderError';
this.code = code;
}
}
// ── Shared helpers ────────────────────────────────────────────────────────
/**
* Wraps a promise with a timeout. Rejects with a ProviderError if the promise
* does not settle within `ms` milliseconds.
*
* @template T
* @param {Promise<T>} promise
* @param {number} ms
* @param {typeof PROVIDER_ERROR_CODES[number]} errorCode
* @returns {Promise<T>}
*/
export function withTimeout(promise, ms, errorCode) {
return new Promise((resolve, reject) => {
const timer = setTimeout(() => {
reject(new ProviderError(`Operation timed out after ${ms}ms`, errorCode));
}, ms);
promise.then(
v => { clearTimeout(timer); resolve(v); },
e => { clearTimeout(timer); reject(e); },
);
});
}
/**
* Merges two AsyncIterators into a single ordered stream.
* Items from whichever source yields first are emitted first.
* Useful when a provider plugin wants to interleave two internal streams.
*
* Not used at D3 (no providers yet) but provided as infrastructure so
* provider authors don't each implement their own fan-in.
*
* @template T
* @param {AsyncIterator<T>} iter1
* @param {AsyncIterator<T>} iter2
* @returns {AsyncGenerator<T>}
*/
export async function* mergeStreams(iter1, iter2) {
// Convert each iterator to a pull-based promise queue
const done1 = { done: true };
const done2 = { done: true };
let p1 = iter1.next();
let p2 = iter2.next();
while (true) {
const winner = await Promise.race([
p1.then(r => ({ r, which: 1 })),
p2.then(r => ({ r, which: 2 })),
]);
if (winner.which === 1) {
if (winner.r.done) {
// iter1 exhausted — drain iter2
for await (const v of { [Symbol.asyncIterator]: () => iter2 }) yield v;
return;
}
yield winner.r.value;
p1 = iter1.next();
} else {
if (winner.r.done) {
// iter2 exhausted — drain iter1
for await (const v of { [Symbol.asyncIterator]: () => iter1 }) yield v;
return;
}
yield winner.r.value;
p2 = iter2.next();
}
}
}