diff --git a/CHANGELOG.md b/CHANGELOG.md index d961dac..167d884 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,13 @@ ## [0.3.1] — Unreleased -Stricter UTF-8 tail handling and a guard against Python-style option names in `filterRelevant`. No change to `generateDiff`, `smartapply`, or the apply path. +Stricter UTF-8 tail handling, a guard against Python-style option names in `filterRelevant`, and a typed `SmartApplyError` for smartapply refusals. No change to `generateDiff` or to what `smartapply` writes. ### Behavior changes - **`filterRelevant` now throws on snake_case options.** Passing any patchling-py keyword spelling (`on_binary`, `on_oversize`, `max_bytes`, `max_tokens`, `max_output_tokens`, `api_key`, `base_url`, `call_llm`, `on_decision`, `reasoning_effort`, `retry_base_ms`, `skip_patterns`, `max_attempts`, `retry_backoff`, `sleep`) throws a `TypeError` before any model call, naming the camelCase option to use (`skip_patterns` → `ignore`, `max_attempts: n` → `retries: n - 1`, `retry_backoff: s` → `retryBaseMs: s * 1000`; `sleep` is not supported). In 0.3.0 these keys were silently ignored, so `{ on_binary: 'include' }` quietly fell back to the default `exclude`. Code that passed such keys and relied on the defaults will now fail loudly. The guard is not configurable (the `SNAKE_CASE_OPTIONS` map lives in `src/filter.js` and is not re-exported from the package root). - **A truncated UTF-8 tail keeps its valid prefix at any file length.** In 0.3.0 an incomplete trailing sequence was tolerated only when the buffer was exactly 8192 bytes; any other length took the binary path. Now a file that ends in a legal partial character is text with the partial character dropped (`0x61 0xC3` and `0x61 0xE0 0xA0` are `"a"`). Tails that can never become a legal character stay binary (`0x61 0xE0 0x80`, `0x61 0xED 0xA0`, `0x61 0xF0 0x80`, `0x61 0xF4 0x90`; leads C0, C1, F5-FF). Some short files that 0.3.0 skipped as binary are now sent to the decision model. +- **`smartapply` refusals throw `SmartApplyError` instead of a plain `Error`.** When smartapply refuses a model rewrite (it changed lines the diff did not add or remove, and the edit could not be replayed from the `+`/`-` lines), the thrown object is now a `SmartApplyError`, exported from the package root and matching patchling-py. What changed: the error's class and constructor (`err.constructor === SmartApplyError`, `instanceof SmartApplyError` is true; it is still `instanceof Error`), `err.name` (`'SmartApplyError'`, was `'Error'`), and therefore how it prints (`String(err)` and the first line of `err.stack` read `SmartApplyError: smartapply: refusing to write …` instead of `Error: …`). Code that checks `err.name === 'Error'`, `err.constructor === Error`, or snapshots the printed error will see the difference. What did not change: the message text (`smartapply: refusing to write "": the model changed lines the diff did not add or remove`, so matching on `err.message` keeps working), when a refusal happens, and provider/LLM errors, which still propagate as the original error object and are never wrapped. ### Docs and tests diff --git a/src/filter.d.ts b/src/filter.d.ts index 99151ca..2af5d8e 100644 --- a/src/filter.d.ts +++ b/src/filter.d.ts @@ -94,6 +94,7 @@ export function isIgnoredPath(filePath: string, patterns?: string[]): boolean; * @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 function filterRelevant(files: Record, task: string, opts?: { model?: string; @@ -285,6 +286,29 @@ export class RelevanceCache { /** @returns {Promise} */ _load(): Promise; } +/** + * 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. + */ +export const SNAKE_CASE_OPTIONS: Readonly<{ + 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"; +}>; export type RelevanceDecision = { path: string; /** diff --git a/src/gptdiff.d.ts b/src/gptdiff.d.ts index 00c3d4c..64be599 100644 --- a/src/gptdiff.d.ts +++ b/src/gptdiff.d.ts @@ -176,6 +176,8 @@ export function callLlmForApplyWithThink(filePath: string, originalContent: stri * by the deterministic fast path emit no tokens (no LLM call is made). * @param {Function} [opts.callLlmForApply] injectable single-file applier * @returns {Promise>} + * @throws {SmartApplyError} when a model rewrite is refused (see above). + * Provider and network errors propagate unchanged. */ export function smartapply(diffText: string, files: Record, opts?: { model?: string; @@ -186,3 +188,14 @@ export function smartapply(diffText: string, files: Record, opts onToken?: (text: string, path: string) => void; callLlmForApply?: Function; }): Promise>; +/** + * Thrown by {@link smartapply} when it refuses to write a file: the model's + * rewrite changed lines the diff did not add or remove, and the edit could + * not be replayed from the diff's `+`/`-` lines without guessing. Mirrors + * `SmartApplyError` in the Python package. Provider and network failures are + * not wrapped; they propagate as the original error. + */ +export class SmartApplyError extends Error { + /** @param {string} message */ + constructor(message: string); +} diff --git a/src/gptdiff.js b/src/gptdiff.js index 373efb7..0fda65a 100644 --- a/src/gptdiff.js +++ b/src/gptdiff.js @@ -26,6 +26,21 @@ import { } from './llm.js'; import { getEnv, DEFAULT_MODEL } from './env.js'; +/** + * Thrown by {@link smartapply} when it refuses to write a file: the model's + * rewrite changed lines the diff did not add or remove, and the edit could + * not be replayed from the diff's `+`/`-` lines without guessing. Mirrors + * `SmartApplyError` in the Python package. Provider and network failures are + * not wrapped; they propagate as the original error. + */ +export class SmartApplyError extends Error { + /** @param {string} message */ + constructor(message) { + super(message); + this.name = 'SmartApplyError'; + } +} + const DIFF_TOOL_PROMPT = `Save the calculated diff as used in 'git apply'. Should include the file and line number. For example: \`\`\`diff a/file.py b/file.py @@ -360,6 +375,8 @@ export async function callLlmForApplyWithThink(filePath, originalContent, fileDi * by the deterministic fast path emit no tokens (no LLM call is made). * @param {Function} [opts.callLlmForApply] injectable single-file applier * @returns {Promise>} + * @throws {SmartApplyError} when a model rewrite is refused (see above). + * Provider and network errors propagate unchanged. */ export async function smartapply(diffText, files, opts = {}) { const model = resolveModel(opts.model); @@ -408,7 +425,7 @@ export async function smartapply(diffText, files, opts = {}) { // add or remove. Replay only the +/- lines, or refuse the write. const intent = applyIntentIgnoringBadContext(original, patch); if (intent == null || diffOutputViolatesIntent(original, intent, patch)) { - throw new Error( + throw new SmartApplyError( `smartapply: refusing to write ${JSON.stringify(path)}: the model changed lines the diff did not add or remove`, ); } diff --git a/src/index.d.ts b/src/index.d.ts index 6a71f24..525d8f4 100644 --- a/src/index.d.ts +++ b/src/index.d.ts @@ -1,7 +1,7 @@ export * as oauth from "./oauth.js"; export type RelevanceDecision = import("./filter.js").RelevanceDecision; export type FilterRelevantResult = import("./filter.js").FilterRelevantResult; -export { generateDiff, smartapply, buildEnvironment, callLlmForDiff, callLlmForApply, callLlmForApplyWithThink } from "./gptdiff.js"; +export { generateDiff, smartapply, buildEnvironment, callLlmForDiff, callLlmForApply, callLlmForApplyWithThink, SmartApplyError } from "./gptdiff.js"; export { filterRelevant, RelevanceCache, createMemoryCache, parseRelevanceAnswer, isBinaryContent, isIgnoredPath, countTokens, formatFilterSummary, dumpDecisions, DEFAULT_FILTER_MODEL, DEFAULT_FILTER_MAX_BYTES, DEFAULT_FILTER_MAX_TOKENS, DEFAULT_FILTER_MAX_OUTPUT_TOKENS, DEFAULT_FILTER_REASONING_EFFORT, DEFAULT_FILTER_CONCURRENCY, FILTER_PROMPT_VERSION, PROMPT_VERSION, DEFAULT_IGNORE_PATTERNS } from "./filter.js"; export { parseDiffPerFile, applyDiff, applyPatchToFile } from "./applydiff.js"; export { colorCodeDiff, swallowReasoning, stripBadOutput, extractDiffBlocks, stripThinkTags } from "./text.js"; diff --git a/src/index.js b/src/index.js index f9a4c32..d49c793 100644 --- a/src/index.js +++ b/src/index.js @@ -5,7 +5,7 @@ * smartapply and their building blocks. */ -export { generateDiff, smartapply, buildEnvironment, callLlmForDiff, callLlmForApply, callLlmForApplyWithThink } from './gptdiff.js'; +export { generateDiff, smartapply, buildEnvironment, callLlmForDiff, callLlmForApply, callLlmForApplyWithThink, SmartApplyError } from './gptdiff.js'; export { filterRelevant, RelevanceCache, diff --git a/test/smartapply_error.test.js b/test/smartapply_error.test.js new file mode 100644 index 0000000..3ad7f99 --- /dev/null +++ b/test/smartapply_error.test.js @@ -0,0 +1,106 @@ +/** + * SmartApplyError: smartapply's refusal to write a corrupted file is a typed + * error (parity with patchling-py's SmartApplyError), while provider/LLM + * failures still propagate as the original error. All LLM calls are mocked. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { smartapply, callLlmForApply, SmartApplyError } from '../src/index.js'; +import { SmartApplyError as SmartApplyErrorFromModule } from '../src/gptdiff.js'; + +// Context and '-' line are not in the file, so the deterministic paths fail +// and the (mocked) model is consulted. +const UNANCHORED_DIFF = [ + '--- a/f.py', + '+++ b/f.py', + '@@ -1,2 +1,2 @@', + ' NOT_A_REAL_LINE', + '-no_such_line', + '+added', +].join('\n'); + +const REFUSAL_MESSAGE = + 'smartapply: refusing to write "f.py": the model changed lines the diff did not add or remove'; + +test('SmartApplyError is exported from the package entry and is an Error subclass', () => { + assert.equal(SmartApplyError, SmartApplyErrorFromModule); + const err = new SmartApplyError('boom'); + assert.ok(err instanceof Error); + assert.ok(err instanceof SmartApplyError); + assert.equal(err.name, 'SmartApplyError'); + assert.equal(err.message, 'boom'); + assert.equal(String(err), 'SmartApplyError: boom'); +}); + +test('a refused model rewrite rejects with SmartApplyError and keeps its message', async () => { + const files = { 'f.py': 'alpha\nbeta\n' }; + let calls = 0; + await assert.rejects( + () => smartapply(UNANCHORED_DIFF, files, { + callLlmForApply: async () => { + calls += 1; + return 'NOT_A_REAL_LINE\nadded\n'; + }, + }), + (err) => { + assert.ok(err instanceof SmartApplyError, `expected SmartApplyError, got ${err && err.name}`); + assert.ok(err instanceof Error); + assert.equal(err.name, 'SmartApplyError'); + assert.equal(err.message, REFUSAL_MESSAGE); + return true; + }, + ); + assert.equal(calls, 1); + assert.deepEqual(files, { 'f.py': 'alpha\nbeta\n' }, 'input map is not mutated'); +}); + +test('an empty model rewrite is refused with SmartApplyError too', async () => { + await assert.rejects( + () => smartapply(UNANCHORED_DIFF, { 'f.py': 'alpha\nbeta\n' }, { + callLlmForApply: async () => '', + }), + (err) => err instanceof SmartApplyError && err.message === REFUSAL_MESSAGE, + ); +}); + +test('a provider error from the applier propagates unchanged, not as SmartApplyError', async () => { + class ProviderError extends Error { + constructor(message, status) { + super(message); + this.name = 'ProviderError'; + this.status = status; + } + } + const providerError = new ProviderError('LLM API error 402: insufficient balance', 402); + await assert.rejects( + () => smartapply(UNANCHORED_DIFF, { 'f.py': 'alpha\nbeta\n' }, { + callLlmForApply: async () => { throw providerError; }, + }), + (err) => { + assert.equal(err, providerError, 'same error object, not wrapped'); + assert.ok(!(err instanceof SmartApplyError)); + assert.equal(err.name, 'ProviderError'); + assert.equal(err.status, 402); + return true; + }, + ); +}); + +test('a plain Error from the LLM client propagates unchanged through callLlmForApply', async () => { + const networkError = new TypeError('fetch failed'); + await assert.rejects( + () => callLlmForApply('f.py', 'alpha\n', '@@ -1 +1 @@\n-alpha\n+beta\n', 'test-model', { + apiKey: 'sk-test', + callLlm: async () => { throw networkError; }, + }), + (err) => err === networkError && !(err instanceof SmartApplyError), + ); +}); + +test('the deterministic fast path never throws SmartApplyError or calls the model', async () => { + const diff = '--- a/f.py\n+++ b/f.py\n@@ -1,2 +1,2 @@\n alpha\n-beta\n+gamma\n'; + const result = await smartapply(diff, { 'f.py': 'alpha\nbeta\n' }, { + callLlmForApply: async () => { throw new Error('model must not be called'); }, + }); + assert.equal(result['f.py'], 'alpha\ngamma\n'); +});