From dfd6a667b14f5b76c1f4e410c01bcef58e5e80b3 Mon Sep 17 00:00:00 2001 From: ntc-ai Date: Tue, 13 Oct 2026 14:11:18 -0600 Subject: [PATCH 1/2] filterRelevant: reject Python snake_case option names { on_binary: 'include' } used to be ignored silently, so the default exclude applied. Snake_case names from patchling-py now throw a TypeError naming the camelCase option, before any model call. --- README.md | 1 + landing/lib/filter.js | 32 ++++++++++++++++++++++++++++++++ src/filter.js | 32 ++++++++++++++++++++++++++++++++ test/filter_options.test.js | 28 ++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+) create mode 100644 test/filter_options.test.js diff --git a/README.md b/README.md index 9d00ad5..eaf47db 100644 --- a/README.md +++ b/README.md @@ -144,6 +144,7 @@ const next = await smartapply(diff, files); Each decision is `{ path, decision, reason, raw, tokens, promptTokens, completionTokens, cached, skippedReason, error }`. - `decision` is `"yes"` or `"no"`. After `` / `` blocks are stripped, the decision is a leading standalone `yes` or `no` (quotes, markdown bold, backticks, and an `Answer:` or `Decision:` prefix are wrappers, including when a reason follows, as in `**no** unrelated`). Any other wording, including a negated "no" later in the sentence, is malformed: the file is included, `failedOpen` is true, and the judgment is not cached. An empty answer, a non-string message content, and a thrown call do the same. `error` is set when the call throws or the answer is empty. `result.failedOpen` counts those files, and `formatFilterSummary` includes that count. +- Options are camelCase. A Python-style name such as `on_binary`, `max_bytes`, or `reasoning_effort` throws a `TypeError` that names the camelCase spelling, instead of being ignored and leaving the default in place. - `skippedReason` is `"binary"`, `"oversize"`, `"truncated"`, `"ignored"`, or `null`. - `tokens`, `promptTokens`, and `completionTokens` are `0` when no model call was made. `reason` is a string. `failedOpen` is true only when the file was included without a definitive yes/no. - `result.billedTokens` sums token counts for decisions that were not cache hits. Identical contents at different paths are separate calls. diff --git a/landing/lib/filter.js b/landing/lib/filter.js index c821004..4d87d28 100644 --- a/landing/lib/filter.js +++ b/landing/lib/filter.js @@ -1063,6 +1063,36 @@ async function cachePut(store, key, record) { else await store.set(key, record); } +/** + * Python-style option names that filterRelevant would otherwise ignore + * silently, mapped to the camelCase name it reads. + */ +const SNAKE_CASE_OPTIONS = { + max_bytes: 'maxBytes', + max_tokens: 'maxTokens', + max_output_tokens: 'maxOutputTokens', + on_oversize: 'onOversize', + on_binary: 'onBinary', + on_decision: 'onDecision', + reasoning_effort: 'reasoningEffort', + retry_base_ms: 'retryBaseMs', + api_key: 'apiKey', + base_url: 'baseUrl', + call_llm: 'callLlm', +}; + +/** + * Reject snake_case option names (the patchling-py spelling). Without this, + * `{ on_binary: 'include' }` is dropped and the default `exclude` applies. + * @param {object} opts + */ +function rejectSnakeCaseOptions(opts) { + const wrong = Object.keys(opts).filter((key) => Object.prototype.hasOwnProperty.call(SNAKE_CASE_OPTIONS, key)); + if (!wrong.length) return; + const fixes = wrong.map((key) => `${key} -> ${SNAKE_CASE_OPTIONS[key]}`).join(', '); + throw new TypeError(`filterRelevant options are camelCase in JavaScript: ${fixes}`); +} + /** * Ask a cheap model which files are relevant to `task`. * @@ -1107,8 +1137,10 @@ async function cachePut(store, key, record) { * @param {string} [opts.baseUrl] `GPTDIFF_LLM_BASE_URL` when omitted * @param {Function} [opts.callLlm] injectable client (tests) * @returns {Promise} + * @throws {TypeError} when an option uses the Python snake_case name (`on_binary`, `max_bytes`, ...) */ export async function filterRelevant(files, task, opts = {}) { + if (opts != null && typeof opts === 'object') rejectSnakeCaseOptions(opts); if (files == null || typeof files !== 'object' || Array.isArray(files)) { throw new TypeError('files must be an object of path -> content'); } diff --git a/src/filter.js b/src/filter.js index c821004..4d87d28 100644 --- a/src/filter.js +++ b/src/filter.js @@ -1063,6 +1063,36 @@ async function cachePut(store, key, record) { else await store.set(key, record); } +/** + * Python-style option names that filterRelevant would otherwise ignore + * silently, mapped to the camelCase name it reads. + */ +const SNAKE_CASE_OPTIONS = { + max_bytes: 'maxBytes', + max_tokens: 'maxTokens', + max_output_tokens: 'maxOutputTokens', + on_oversize: 'onOversize', + on_binary: 'onBinary', + on_decision: 'onDecision', + reasoning_effort: 'reasoningEffort', + retry_base_ms: 'retryBaseMs', + api_key: 'apiKey', + base_url: 'baseUrl', + call_llm: 'callLlm', +}; + +/** + * Reject snake_case option names (the patchling-py spelling). Without this, + * `{ on_binary: 'include' }` is dropped and the default `exclude` applies. + * @param {object} opts + */ +function rejectSnakeCaseOptions(opts) { + const wrong = Object.keys(opts).filter((key) => Object.prototype.hasOwnProperty.call(SNAKE_CASE_OPTIONS, key)); + if (!wrong.length) return; + const fixes = wrong.map((key) => `${key} -> ${SNAKE_CASE_OPTIONS[key]}`).join(', '); + throw new TypeError(`filterRelevant options are camelCase in JavaScript: ${fixes}`); +} + /** * Ask a cheap model which files are relevant to `task`. * @@ -1107,8 +1137,10 @@ async function cachePut(store, key, record) { * @param {string} [opts.baseUrl] `GPTDIFF_LLM_BASE_URL` when omitted * @param {Function} [opts.callLlm] injectable client (tests) * @returns {Promise} + * @throws {TypeError} when an option uses the Python snake_case name (`on_binary`, `max_bytes`, ...) */ export async function filterRelevant(files, task, opts = {}) { + if (opts != null && typeof opts === 'object') rejectSnakeCaseOptions(opts); if (files == null || typeof files !== 'object' || Array.isArray(files)) { throw new TypeError('files must be an object of path -> content'); } diff --git a/test/filter_options.test.js b/test/filter_options.test.js new file mode 100644 index 0000000..0672c27 --- /dev/null +++ b/test/filter_options.test.js @@ -0,0 +1,28 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { filterRelevant } from '../src/filter.js'; + +const yes = async () => ({ choices: [{ message: { content: 'yes' } }], usage: {} }); + +test('snake_case options from patchling-py throw with the camelCase name', async () => { + let calls = 0; + const callLlm = async () => { calls += 1; return yes(); }; + await assert.rejects( + filterRelevant({ 'a.bin': new Uint8Array([0, 1]) }, 'task', { callLlm, apiKey: 'k', on_binary: 'include' }), + (err) => err instanceof TypeError && /on_binary -> onBinary/.test(err.message), + ); + await assert.rejects( + filterRelevant({ 'a.js': 'x' }, 'task', { callLlm, apiKey: 'k', max_bytes: 10, reasoning_effort: null }), + /max_bytes -> maxBytes, reasoning_effort -> reasoningEffort/, + ); + assert.equal(calls, 0, 'no model call is made before the options are rejected'); +}); + +test('camelCase options and unrelated extra keys still work', async () => { + const result = await filterRelevant( + { 'a.bin': new Uint8Array([0, 1]), 'a.js': 'x' }, + 'task', + { callLlm: yes, apiKey: 'k', onBinary: 'include', cache: false, label: 'mine' }, + ); + assert.deepEqual(result.decisions.map((d) => d.decision).sort(), ['yes', 'yes']); +}); From 0a9c6fe1850c42070bfe81456c1931ea50a91ab4 Mon Sep 17 00:00:00 2001 From: ntc-ai Date: Tue, 13 Oct 2026 17:32:50 -0600 Subject: [PATCH 2/2] filterRelevant: reject every patchling-py snake_case keyword Audited filter_relevant() in patchling-py (relevance.py @ bf9c8f0). Adds skip_patterns (use ignore), max_attempts (retries = max_attempts - 1), retry_backoff (retryBaseMs = seconds * 1000) and sleep (not supported in JS; use retryBaseMs: 0 in tests). The error names the JS option and the conversion, and is thrown before any model call. Tests run against both src/ and landing/lib/. --- README.md | 2 +- landing/lib/filter.js | 40 +++++++++++-------- src/filter.js | 40 +++++++++++-------- test/filter_options.test.js | 77 ++++++++++++++++++++++++++++++++----- 4 files changed, 114 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index eaf47db..26d17df 100644 --- a/README.md +++ b/README.md @@ -144,7 +144,7 @@ const next = await smartapply(diff, files); Each decision is `{ path, decision, reason, raw, tokens, promptTokens, completionTokens, cached, skippedReason, error }`. - `decision` is `"yes"` or `"no"`. After `` / `` blocks are stripped, the decision is a leading standalone `yes` or `no` (quotes, markdown bold, backticks, and an `Answer:` or `Decision:` prefix are wrappers, including when a reason follows, as in `**no** unrelated`). Any other wording, including a negated "no" later in the sentence, is malformed: the file is included, `failedOpen` is true, and the judgment is not cached. An empty answer, a non-string message content, and a thrown call do the same. `error` is set when the call throws or the answer is empty. `result.failedOpen` counts those files, and `formatFilterSummary` includes that count. -- Options are camelCase. A Python-style name such as `on_binary`, `max_bytes`, or `reasoning_effort` throws a `TypeError` that names the camelCase spelling, instead of being ignored and leaving the default in place. +- Options are camelCase. Every patchling-py keyword in snake_case (`on_binary`, `max_bytes`, `skip_patterns`, `max_attempts`, `retry_backoff`, `sleep`, ...) throws a `TypeError` before any model call, naming the JS option: `skip_patterns` is `ignore`, `max_attempts: n` is `retries: n - 1`, `retry_backoff: s` (seconds) is `retryBaseMs: s * 1000`, and `sleep` is not supported in JS (pass `retryBaseMs: 0` in tests). - `skippedReason` is `"binary"`, `"oversize"`, `"truncated"`, `"ignored"`, or `null`. - `tokens`, `promptTokens`, and `completionTokens` are `0` when no model call was made. `reason` is a string. `failedOpen` is true only when the file was included without a definitive yes/no. - `result.billedTokens` sums token counts for decisions that were not cache hits. Identical contents at different paths are separate calls. diff --git a/landing/lib/filter.js b/landing/lib/filter.js index 4d87d28..a8f38b0 100644 --- a/landing/lib/filter.js +++ b/landing/lib/filter.js @@ -1064,22 +1064,28 @@ async function cachePut(store, key, record) { } /** - * Python-style option names that filterRelevant would otherwise ignore - * silently, mapped to the camelCase name it reads. + * Snake_case option names, mapped to what to pass in JavaScript. Covers + * every keyword of patchling-py `filter_relevant(...)` (files, task, model, + * concurrency and cache share their names), plus snake_case spellings of + * JS-only options. Without this guard they are ignored silently. */ -const SNAKE_CASE_OPTIONS = { - max_bytes: 'maxBytes', - max_tokens: 'maxTokens', - max_output_tokens: 'maxOutputTokens', - on_oversize: 'onOversize', - on_binary: 'onBinary', - on_decision: 'onDecision', - reasoning_effort: 'reasoningEffort', - retry_base_ms: 'retryBaseMs', - api_key: 'apiKey', - base_url: 'baseUrl', - call_llm: 'callLlm', -}; +export const SNAKE_CASE_OPTIONS = Object.freeze({ + api_key: 'use apiKey', + base_url: 'use baseUrl', + max_bytes: 'use maxBytes', + max_tokens: 'use maxTokens', + on_oversize: 'use onOversize', + on_binary: 'use onBinary', + max_output_tokens: 'use maxOutputTokens', + skip_patterns: 'use ignore (an array replaces DEFAULT_IGNORE_PATTERNS; [] or false classifies every path)', + max_attempts: 'use retries, which counts extra attempts: retries = max_attempts - 1', + retry_backoff: 'use retryBaseMs, in milliseconds: retryBaseMs = retry_backoff * 1000', + sleep: 'not supported in JS; backoff uses timers. In tests pass retryBaseMs: 0 to skip the waits', + call_llm: 'use callLlm', + on_decision: 'use onDecision', + reasoning_effort: 'use reasoningEffort', + retry_base_ms: 'use retryBaseMs', +}); /** * Reject snake_case option names (the patchling-py spelling). Without this, @@ -1089,8 +1095,8 @@ const SNAKE_CASE_OPTIONS = { function rejectSnakeCaseOptions(opts) { const wrong = Object.keys(opts).filter((key) => Object.prototype.hasOwnProperty.call(SNAKE_CASE_OPTIONS, key)); if (!wrong.length) return; - const fixes = wrong.map((key) => `${key} -> ${SNAKE_CASE_OPTIONS[key]}`).join(', '); - throw new TypeError(`filterRelevant options are camelCase in JavaScript: ${fixes}`); + const fixes = wrong.map((key) => `${key}: ${SNAKE_CASE_OPTIONS[key]}`).join('; '); + throw new TypeError(`filterRelevant options are camelCase in JavaScript. ${fixes}`); } /** diff --git a/src/filter.js b/src/filter.js index 4d87d28..a8f38b0 100644 --- a/src/filter.js +++ b/src/filter.js @@ -1064,22 +1064,28 @@ async function cachePut(store, key, record) { } /** - * Python-style option names that filterRelevant would otherwise ignore - * silently, mapped to the camelCase name it reads. + * Snake_case option names, mapped to what to pass in JavaScript. Covers + * every keyword of patchling-py `filter_relevant(...)` (files, task, model, + * concurrency and cache share their names), plus snake_case spellings of + * JS-only options. Without this guard they are ignored silently. */ -const SNAKE_CASE_OPTIONS = { - max_bytes: 'maxBytes', - max_tokens: 'maxTokens', - max_output_tokens: 'maxOutputTokens', - on_oversize: 'onOversize', - on_binary: 'onBinary', - on_decision: 'onDecision', - reasoning_effort: 'reasoningEffort', - retry_base_ms: 'retryBaseMs', - api_key: 'apiKey', - base_url: 'baseUrl', - call_llm: 'callLlm', -}; +export const SNAKE_CASE_OPTIONS = Object.freeze({ + api_key: 'use apiKey', + base_url: 'use baseUrl', + max_bytes: 'use maxBytes', + max_tokens: 'use maxTokens', + on_oversize: 'use onOversize', + on_binary: 'use onBinary', + max_output_tokens: 'use maxOutputTokens', + skip_patterns: 'use ignore (an array replaces DEFAULT_IGNORE_PATTERNS; [] or false classifies every path)', + max_attempts: 'use retries, which counts extra attempts: retries = max_attempts - 1', + retry_backoff: 'use retryBaseMs, in milliseconds: retryBaseMs = retry_backoff * 1000', + sleep: 'not supported in JS; backoff uses timers. In tests pass retryBaseMs: 0 to skip the waits', + call_llm: 'use callLlm', + on_decision: 'use onDecision', + reasoning_effort: 'use reasoningEffort', + retry_base_ms: 'use retryBaseMs', +}); /** * Reject snake_case option names (the patchling-py spelling). Without this, @@ -1089,8 +1095,8 @@ const SNAKE_CASE_OPTIONS = { function rejectSnakeCaseOptions(opts) { const wrong = Object.keys(opts).filter((key) => Object.prototype.hasOwnProperty.call(SNAKE_CASE_OPTIONS, key)); if (!wrong.length) return; - const fixes = wrong.map((key) => `${key} -> ${SNAKE_CASE_OPTIONS[key]}`).join(', '); - throw new TypeError(`filterRelevant options are camelCase in JavaScript: ${fixes}`); + const fixes = wrong.map((key) => `${key}: ${SNAKE_CASE_OPTIONS[key]}`).join('; '); + throw new TypeError(`filterRelevant options are camelCase in JavaScript. ${fixes}`); } /** diff --git a/test/filter_options.test.js b/test/filter_options.test.js index 0672c27..ebe20ad 100644 --- a/test/filter_options.test.js +++ b/test/filter_options.test.js @@ -1,25 +1,82 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import { filterRelevant } from '../src/filter.js'; +import * as src from '../src/filter.js'; +import * as landing from '../landing/lib/filter.js'; const yes = async () => ({ choices: [{ message: { content: 'yes' } }], usage: {} }); -test('snake_case options from patchling-py throw with the camelCase name', async () => { - let calls = 0; - const callLlm = async () => { calls += 1; return yes(); }; +/** Every keyword of patchling-py filter_relevant() (patchling/relevance.py @ bf9c8f0). */ +const PY_KEYWORDS = [ + 'files', 'task', 'model', 'concurrency', 'cache', + 'api_key', 'base_url', 'max_bytes', 'max_tokens', 'on_oversize', 'on_binary', + 'max_output_tokens', 'skip_patterns', 'max_attempts', 'retry_backoff', 'sleep', 'call_llm', +]; +const SHARED_NAMES = new Set(['files', 'task', 'model', 'concurrency', 'cache']); + +function counting() { + const state = { calls: 0 }; + state.callLlm = async () => { state.calls += 1; throw Object.assign(new Error('HTTP 503'), { status: 503 }); }; + return state; +} + +for (const [label, mod] of [['src', src], ['landing', landing]]) { + test(`${label}: skip_patterns: [] throws instead of still excluding a generated file`, async () => { + const llm = counting(); + await assert.rejects( + mod.filterRelevant({ 'dist/app.js': 'x' }, 'task', { callLlm: llm.callLlm, apiKey: 'k', cache: false, skip_patterns: [] }), + (err) => err instanceof TypeError && /skip_patterns: use ignore/.test(err.message), + ); + assert.equal(llm.calls, 0); + }); + + test(`${label}: max_attempts: 1 throws before any attempt`, async () => { + const llm = counting(); + await assert.rejects( + mod.filterRelevant({ 'a.js': 'x' }, 'task', { callLlm: llm.callLlm, apiKey: 'k', cache: false, retryBaseMs: 0, max_attempts: 1 }), + /max_attempts: use retries, which counts extra attempts: retries = max_attempts - 1/, + ); + assert.equal(llm.calls, 0, 'no mocked attempt is made'); + }); + + test(`${label}: retry_backoff: 0 throws with the retryBaseMs conversion`, async () => { + const llm = counting(); + await assert.rejects( + mod.filterRelevant({ 'a.js': 'x' }, 'task', { callLlm: llm.callLlm, apiKey: 'k', retry_backoff: 0 }), + /retry_backoff: use retryBaseMs, in milliseconds: retryBaseMs = retry_backoff \* 1000/, + ); + assert.equal(llm.calls, 0); + }); + + test(`${label}: every snake_case patchling-py keyword is rejected`, async () => { + for (const key of PY_KEYWORDS.filter((k) => !SHARED_NAMES.has(k))) { + assert.ok(Object.hasOwn(mod.SNAKE_CASE_OPTIONS, key), `${key} is mapped`); + const llm = counting(); + await assert.rejects( + mod.filterRelevant({ 'a.js': 'x' }, 'task', { callLlm: llm.callLlm, apiKey: 'k', cache: false, [key]: undefined }), + (err) => err instanceof TypeError && err.message.includes(`${key}: `), + key, + ); + assert.equal(llm.calls, 0, key); + } + }); +} + +test('sleep is reported as not supported in JS', async () => { await assert.rejects( - filterRelevant({ 'a.bin': new Uint8Array([0, 1]) }, 'task', { callLlm, apiKey: 'k', on_binary: 'include' }), - (err) => err instanceof TypeError && /on_binary -> onBinary/.test(err.message), + src.filterRelevant({ 'a.js': 'x' }, 'task', { callLlm: yes, apiKey: 'k', sleep: () => {} }), + /sleep: not supported in JS/, ); +}); + +test('several snake_case options are all named in one error', async () => { await assert.rejects( - filterRelevant({ 'a.js': 'x' }, 'task', { callLlm, apiKey: 'k', max_bytes: 10, reasoning_effort: null }), - /max_bytes -> maxBytes, reasoning_effort -> reasoningEffort/, + src.filterRelevant({ 'a.js': 'x' }, 'task', { callLlm: yes, apiKey: 'k', on_binary: 'include', max_bytes: 10 }), + /on_binary: use onBinary; max_bytes: use maxBytes/, ); - assert.equal(calls, 0, 'no model call is made before the options are rejected'); }); test('camelCase options and unrelated extra keys still work', async () => { - const result = await filterRelevant( + const result = await src.filterRelevant( { 'a.bin': new Uint8Array([0, 1]), 'a.js': 'x' }, 'task', { callLlm: yes, apiKey: 'k', onBinary: 'include', cache: false, label: 'mine' },