Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 "<path>": 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

Expand Down
24 changes: 24 additions & 0 deletions src/filter.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<FilterRelevantResult>}
* @throws {TypeError} when an option uses the Python snake_case name (`on_binary`, `max_bytes`, ...)
*/
export function filterRelevant(files: Record<string, string | Uint8Array>, task: string, opts?: {
model?: string;
Expand Down Expand Up @@ -285,6 +286,29 @@ export class RelevanceCache {
/** @returns {Promise<void>} */
_load(): Promise<void>;
}
/**
* 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;
/**
Expand Down
13 changes: 13 additions & 0 deletions src/gptdiff.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Record<string, string>>}
* @throws {SmartApplyError} when a model rewrite is refused (see above).
* Provider and network errors propagate unchanged.
*/
export function smartapply(diffText: string, files: Record<string, string>, opts?: {
model?: string;
Expand All @@ -186,3 +188,14 @@ export function smartapply(diffText: string, files: Record<string, string>, opts
onToken?: (text: string, path: string) => void;
callLlmForApply?: Function;
}): Promise<Record<string, string>>;
/**
* 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);
}
19 changes: 18 additions & 1 deletion src/gptdiff.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<Record<string, string>>}
* @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);
Expand Down Expand Up @@ -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`,
);
}
Expand Down
2 changes: 1 addition & 1 deletion src/index.d.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down
2 changes: 1 addition & 1 deletion src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
106 changes: 106 additions & 0 deletions test/smartapply_error.test.js
Original file line number Diff line number Diff line change
@@ -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');
});
Loading