From 1466d3a082887fac5bbab5a21347aac4aa78b2f6 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Sun, 24 May 2026 14:06:21 +1000 Subject: [PATCH] =?UTF-8?q?fix(providers):=20D21=20=E2=80=94=20validatePro?= =?UTF-8?q?vider=20enforces=20maxSpawnTimeMs=20contract=20field=20(round-2?= =?UTF-8?q?=20F1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- lib/providers/base.mjs | 8 +++++++- test-features.mjs | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/lib/providers/base.mjs b/lib/providers/base.mjs index 749b6a1..c4bacfd 100644 --- a/lib/providers/base.mjs +++ b/lib/providers/base.mjs @@ -24,6 +24,7 @@ * @property {boolean} requiresTTY * @property {boolean} concurrentSpawnSafe * @property {number} maxConcurrent + * @property {number} [maxSpawnTimeMs] - optional integer milliseconds, default 600000 */ /** @@ -110,13 +111,18 @@ export function validateProvider(p) { } if (!p.hints || typeof p.hints !== 'object') { - errors.push('hints must be an object with { requiresTTY, concurrentSpawnSafe, maxConcurrent }'); + 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 }; diff --git a/test-features.mjs b/test-features.mjs index 2e8325c..556fd17 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -521,6 +521,42 @@ describe('Provider contract validation', () => { assert.ok(r.errors.some(e => e.includes('maxConcurrent'))); }); + it('validateProvider rejects negative maxSpawnTimeMs', () => { + const r = validateProvider(makeProvider({ hints: { requiresTTY: false, concurrentSpawnSafe: true, maxConcurrent: 4, maxSpawnTimeMs: -1 } })); + assert.equal(r.valid, false); + assert.ok(r.errors.some(e => e.includes('maxSpawnTimeMs'))); + }); + + it('validateProvider rejects zero maxSpawnTimeMs', () => { + const r = validateProvider(makeProvider({ hints: { requiresTTY: false, concurrentSpawnSafe: true, maxConcurrent: 4, maxSpawnTimeMs: 0 } })); + assert.equal(r.valid, false); + assert.ok(r.errors.some(e => e.includes('maxSpawnTimeMs'))); + }); + + it('validateProvider rejects non-integer maxSpawnTimeMs (e.g., 100.5)', () => { + const r = validateProvider(makeProvider({ hints: { requiresTTY: false, concurrentSpawnSafe: true, maxConcurrent: 4, maxSpawnTimeMs: 100.5 } })); + assert.equal(r.valid, false); + assert.ok(r.errors.some(e => e.includes('maxSpawnTimeMs'))); + }); + + it('validateProvider rejects non-number maxSpawnTimeMs (e.g., \'600\')', () => { + const r = validateProvider(makeProvider({ hints: { requiresTTY: false, concurrentSpawnSafe: true, maxConcurrent: 4, maxSpawnTimeMs: '600' } })); + assert.equal(r.valid, false); + assert.ok(r.errors.some(e => e.includes('maxSpawnTimeMs'))); + }); + + it('validateProvider accepts omitted maxSpawnTimeMs (optional field)', () => { + const r = validateProvider(makeProvider({ hints: { requiresTTY: false, concurrentSpawnSafe: true, maxConcurrent: 4 } })); + assert.equal(r.valid, true); + assert.deepEqual(r.errors, []); + }); + + it('validateProvider accepts positive integer maxSpawnTimeMs (e.g., 60000)', () => { + const r = validateProvider(makeProvider({ hints: { requiresTTY: false, concurrentSpawnSafe: true, maxConcurrent: 4, maxSpawnTimeMs: 60000 } })); + assert.equal(r.valid, true); + assert.deepEqual(r.errors, []); + }); + it('rejects non-object input', () => { const r = validateProvider(null); assert.equal(r.valid, false);