From 14c045f04f67f02491a37788076a8ec890d0bf85 Mon Sep 17 00:00:00 2001 From: Yifan Yang Date: Tue, 23 Jun 2026 19:00:23 +0800 Subject: [PATCH] Windows robustness for claude/codex backends (+ hardened JSON fallback) (#79) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Robustness for the claude/codex backends on Windows: argv overflow, subprocess encoding, tolerant JSON, test-eval dirs Fixes surfaced running SkillOpt end-to-end on the bundled `claude` backend (local Claude CLI) on Windows. None changes the OpenAI/GPT happy path. 1. skillopt/engine/trainer.py — the final test-eval directory (test_eval_final/) is written to before being created; add os.makedirs(..., exist_ok=True), matching the two sibling test-eval dirs. Without it, summary.json raises FileNotFoundError when a rollout yields zero predictions. 2. skillopt/model/claude_backend.py a. Pass the prompt via stdin (not argv): on Windows the whole command line is capped at ~32 KB and a large optimizer prompt (the success-analyst minibatch carrying several report trajectories) overflows it with [WinError 206], killing the run after retries. b. Pass the system prompt via --append-system-prompt-file (a temp file), not argv. The system prompt here is the skill being optimized, which SkillOpt grows over training; since the ~32 KB cap applies to the SUM of all argv, a grown skill would re-hit [WinError 206] even with the prompt on stdin. c. Pin the subprocess encoding to utf-8 (errors="replace"). With text=True and no encoding=, stdin is encoded with the system codepage; on a zh-CN box (cp936/GBK) a prompt containing an emoji or some Latin-1 characters raises UnicodeEncodeError before the CLI even starts, failing every retry. 3. skillopt/model/codex_backend.py — the same utf-8 encoding pin on its subprocess.run(input=...) call (identical unpinned-encoding pattern). 4. skillopt/utils/json_utils.py — extract_json() returned None for valid- looking JSON that strict json.loads rejects (unescaped ASCII quotes inside CJK string values, trailing commas), silently dropping the analyst's edits on non-schema backends (Claude/Qwen): reflect produces N edits, 0 applied. Add a json_repair fallback, but only on a single unambiguous object — a balanced-brace extractor plus a refuse-on-multiple-objects guard — so a chain-of-thought "scratch + final" response can't make repair silently return the wrong (discarded) object, which would be worse than None (None is detectable and retryable; a wrong-but-valid edit is applied blind). Declare json_repair in requirements.txt and the claude/qwen optional extras so the fallback is actually present (it otherwise no-ops, dropping edits silently). Co-Authored-By: Claude Opus 4.8 (1M context) (cherry picked from commit dca74a683e9063b81f61d2967bed8f2df57d900a) * fix(json_utils): harden tolerant JSON fallback from PR #77 Follow-up fixes on top of the cherry-picked Windows-robustness change: 1. Make _top_level_brace_objects() fully string-aware in its OUTER scan, not just inside an object. A '{' inside quoted prose (e.g. '"set it to {x}"') no longer starts a candidate object, so extract_json() returns None for prose pseudo-JSON instead of repairing it into a bogus dict — which would be strictly worse than dropping the edit, since extract_json feeds the optimizer's skill edits. 2. Pick the repair candidate BEFORE importing json_repair, so the missing- dependency RuntimeWarning only fires when there is genuinely a single malformed object that could have been repaired. Ordinary no-JSON / prose replies (the common case) now return None silently instead of warning on every call. 3. Resolve dependency-metadata inconsistency: json_repair is optional, so add it to the `all` extra (it was already in `claude`/`qwen`) and demote it from a hard requirement to an optional/commented entry in requirements.txt, matching the project's convention for backend-specific deps. Adds regression tests for prose-with-braces (-> None), no-warning-on-plain- text, single-object repair, and multi-object ambiguity. Existing 22 json tests still pass with and without json_repair installed. Co-Authored-By: Claude --------- Co-authored-by: samuelgoofus-boop <260247789+samuelgoofus-boop@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) --- pyproject.toml | 5 +- requirements.txt | 6 ++ skillopt/engine/trainer.py | 3 + skillopt/model/claude_backend.py | 16 ++++- skillopt/model/codex_backend.py | 2 + skillopt/utils/json_utils.py | 105 +++++++++++++++++++++++++++++++ tests/test_json_utils.py | 52 ++++++++++++++- 7 files changed, 184 insertions(+), 5 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 9a0020e..e9dfa71 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,9 +37,9 @@ dependencies = [ # Benchmark-specific dependencies alfworld = ["alfworld>=0.4.0", "gymnasium>=0.29.0"] # Claude model backend -claude = ["claude-agent-sdk>=0.1.0"] +claude = ["claude-agent-sdk>=0.1.0", "json_repair>=0.61.0"] # Qwen local model backend (via vLLM) -qwen = ["vllm>=0.4.0"] +qwen = ["vllm>=0.4.0", "json_repair>=0.61.0"] # SearchQA data materialization searchqa = ["datasets>=2.18.0"] # Documentation site @@ -53,6 +53,7 @@ all = [ "alfworld>=0.4.0", "gymnasium>=0.29.0", "claude-agent-sdk>=0.1.0", + "json_repair>=0.61.0", ] [project.scripts] diff --git a/requirements.txt b/requirements.txt index 29d1eb7..5db9e70 100644 --- a/requirements.txt +++ b/requirements.txt @@ -17,6 +17,12 @@ httpx>=0.27.0 # ── Optional: Qwen local model (via vLLM) ──────── # vllm>=0.4.0 +# ── Optional: tolerant JSON repair for free-form output from non-OpenAI +# backends (Claude/Qwen). Without it extract_json() falls back safely and +# drops a malformed analyst edit instead of repairing it. Installed by the +# `claude`, `qwen`, and `all` extras in pyproject.toml. +# json_repair>=0.61.0 + # ── Optional: WebUI dashboard ──────────────────── # gradio>=4.0.0 diff --git a/skillopt/engine/trainer.py b/skillopt/engine/trainer.py index 5fbe90f..85aae53 100644 --- a/skillopt/engine/trainer.py +++ b/skillopt/engine/trainer.py @@ -2133,6 +2133,7 @@ class ReflACTTrainer: ) print(f" Test items: {test_n}") baseline_test_dir = os.path.join(out_root, "test_eval_baseline") + os.makedirs(baseline_test_dir, exist_ok=True) baseline_test_results = adapter.rollout(test_env, skill_init, baseline_test_dir) baseline_test_hard, baseline_test_soft = compute_score(baseline_test_results) baseline_buckets = _compute_task_type_buckets(baseline_test_results, task_types) @@ -2167,6 +2168,7 @@ class ReflACTTrainer: ) print(f" Test items: {test_n2}") test_dir = os.path.join(out_root, "test_eval") + os.makedirs(test_dir, exist_ok=True) test_results = adapter.rollout(test_env2, best_skill, test_dir) test_hard, test_soft = compute_score(test_results) best_buckets = _compute_task_type_buckets(test_results, task_types) @@ -2230,6 +2232,7 @@ class ReflACTTrainer: ) print(f" Test items: {test_n3}") final_test_dir = os.path.join(out_root, "test_eval_final") + os.makedirs(final_test_dir, exist_ok=True) final_test_results = adapter.rollout(test_env3, current_skill, final_test_dir) final_test_hard, final_test_soft = compute_score(final_test_results) final_buckets = _compute_task_type_buckets(final_test_results, task_types) diff --git a/skillopt/model/claude_backend.py b/skillopt/model/claude_backend.py index 04a17a3..b2d0e94 100644 --- a/skillopt/model/claude_backend.py +++ b/skillopt/model/claude_backend.py @@ -252,13 +252,25 @@ def _run_claude_print(*, system: str, prompt: str, model: str, tools: list[dict[ if CLAUDE_SETTING_SOURCES: cmd.extend(["--setting-sources", CLAUDE_SETTING_SOURCES]) if system: - cmd.extend(["--append-system-prompt", system]) + # Write the system prompt to a file, not argv: here the skill being + # optimized IS the system prompt, and SkillOpt grows it over training, + # so past ~30 KB it would re-hit the Windows argv cap (WinError 206). + # The CLI reads it via --append-system-prompt-file. + system_path = os.path.join(temp_dir, "system_prompt.txt") + with open(system_path, "w", encoding="utf-8") as system_fh: + system_fh.write(system) + cmd.extend(["--append-system-prompt-file", system_path]) if effort: cmd.extend(["--effort", effort]) structured_output = bool(return_message) if structured_output: cmd.extend(["--schema", _assistant_message_schema_wrapper()]) - proc = subprocess.run(cmd + [prompt_for_cli], capture_output=True, text=True, timeout=timeout or 300, cwd=temp_dir) + # Feed the prompt via stdin (and the system prompt via a file, above), not + # argv: on Windows the whole command line is capped at ~32 KB and large + # optimizer prompts / grown skills overflow it → [WinError 206]. Pin UTF-8 + # so a zh-CN default codepage (cp936) can't raise UnicodeEncodeError on + # emoji / non-GBK glyphs before the CLI even starts. + proc = subprocess.run(cmd, input=prompt_for_cli, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=timeout or 300, cwd=temp_dir) stderr_text = (proc.stderr or "").strip() if proc.returncode != 0: _check_claude_error(stderr_text, model) diff --git a/skillopt/model/codex_backend.py b/skillopt/model/codex_backend.py index d9ab615..64b6f35 100644 --- a/skillopt/model/codex_backend.py +++ b/skillopt/model/codex_backend.py @@ -328,6 +328,8 @@ def _run_codex_exec( command, input=prompt, text=True, + encoding="utf-8", + errors="replace", capture_output=True, timeout=timeout, check=False, diff --git a/skillopt/utils/json_utils.py b/skillopt/utils/json_utils.py index 011241b..0fcc4a0 100644 --- a/skillopt/utils/json_utils.py +++ b/skillopt/utils/json_utils.py @@ -3,6 +3,72 @@ from __future__ import annotations import json import re +import warnings + + +def _top_level_brace_objects(text: str) -> list[str]: + """Return every balanced *top-level* ``{...}`` span in ``text``. + + Fully string/escape aware: braces inside quoted strings are ignored both + when scanning for an object start AND while tracking depth inside one, so a + ``{`` that appears in prose (e.g. ``'set it to {x}'``) is never mistaken for + the start of a JSON object. Used to detect ambiguity: when a response carries + more than one top-level object we must not let a repair pass silently pick + one — it may pick the wrong (discarded) edit, strictly worse than None. + """ + spans: list[str] = [] + i, n = 0, len(text) + outer_in_str = False + outer_esc = False + while i < n: + ch = text[i] + # Skip over braces that live *inside* a quoted string before any object + # has started — otherwise a `{` in prose like '"set it to {x}"' is wrongly + # treated as an object start, and the repair pass below turns non-JSON + # prose into a bogus dict (strictly worse than returning None). + if outer_in_str: + if outer_esc: + outer_esc = False + elif ch == "\\": + outer_esc = True + elif ch == '"': + outer_in_str = False + i += 1 + continue + if ch == '"': + outer_in_str = True + i += 1 + continue + if ch != "{": + i += 1 + continue + depth = 0 + in_str = False + esc = False + start = i + while i < n: + ch = text[i] + if in_str: + if esc: + esc = False + elif ch == "\\": + esc = True + elif ch == '"': + in_str = False + elif ch == '"': + in_str = True + elif ch == "{": + depth += 1 + elif ch == "}": + depth -= 1 + if depth == 0: + spans.append(text[start:i + 1]) + i += 1 + break + i += 1 + else: + break # unterminated final object + return spans def extract_json(text: str) -> dict | None: @@ -22,6 +88,45 @@ def extract_json(text: str) -> dict | None: return json.loads(m.group(0)) except json.JSONDecodeError: pass + # Tolerant fallback for non-OpenAI backends (Claude/Qwen, …) whose free-form + # JSON strict json.loads rejects — unescaped ASCII quotes inside CJK string + # values, trailing commas, etc. Repair so the analyst's edits aren't silently + # dropped, but ONLY a single unambiguous object: never feed the greedy `{.*}` + # span or the raw text, or json_repair would quietly return one of several + # objects (empirically the wrong/last one) — strictly worse than None, which + # the caller can detect and retry/skip. + # + # Pick the candidate FIRST, before importing json_repair, so the optional + # dependency only matters (and only warns) when there is genuinely a single + # malformed object we could have repaired. Ordinary no-JSON / prose replies + # have no candidate and return None silently. + candidate = None + fenced = re.search(r"```json\s*(.*?)```", text, re.DOTALL) + if fenced and len(_top_level_brace_objects(fenced.group(1))) == 1: + candidate = fenced.group(1) + else: + objs = _top_level_brace_objects(text) + if len(objs) == 1: + candidate = objs[0] + # 0 or >1 top-level objects → too ambiguous to repair safely → None + if not candidate: + return None + try: + from json_repair import repair_json + except ModuleNotFoundError: + warnings.warn( + "json_repair not installed; malformed-JSON recovery disabled — " + "a non-OpenAI analyst edit may be silently dropped. pip install json_repair", + RuntimeWarning, + stacklevel=2, + ) + return None + try: + repaired = repair_json(candidate, return_objects=True) + if isinstance(repaired, dict) and repaired: + return repaired + except Exception: # noqa: BLE001 — repair is best-effort + pass return None diff --git a/tests/test_json_utils.py b/tests/test_json_utils.py index d9a4b06..1fa98c5 100644 --- a/tests/test_json_utils.py +++ b/tests/test_json_utils.py @@ -3,7 +3,11 @@ from __future__ import annotations import pytest -from skillopt.utils.json_utils import extract_json, extract_json_array +from skillopt.utils.json_utils import ( + _top_level_brace_objects, + extract_json, + extract_json_array, +) class TestExtractJson: @@ -61,6 +65,52 @@ class TestExtractJson: assert extract_json(text) is None +class TestTopLevelBraceObjects: + """_top_level_brace_objects — string/escape-aware top-level object scan.""" + + def test_single_clean_object(self) -> None: + assert _top_level_brace_objects('{"a": 1}') == ['{"a": 1}'] + + def test_two_top_level_objects(self) -> None: + assert _top_level_brace_objects('{"a":1}\n{"b":2}') == ['{"a":1}', '{"b":2}'] + + def test_brace_inside_quoted_prose_is_ignored(self) -> None: + """A '{' inside a quoted string must NOT start an object (the bug).""" + # Brace-shaped content inside a string, with no real object → no spans. + assert _top_level_brace_objects('label is "set it to {x: 1}" done') == [] + + def test_real_object_after_quoted_brace(self) -> None: + """Quoted-prose braces are skipped; a later real object is still found.""" + text = 'note "{wrong: 1}" then actual {"edit": "right"}' + assert _top_level_brace_objects(text) == ['{"edit": "right"}'] + + +class TestExtractJsonTolerantFallback: + """extract_json — json_repair fallback for malformed non-OpenAI output.""" + + def test_prose_pseudo_json_returns_none(self) -> None: + """Regression: brace-shaped prose inside quotes must not be 'repaired' + into a bogus dict. It returned {'op': 'delete'} before the fix.""" + text = 'The literal string "{op: delete}" appears in prose, not as JSON.' + assert extract_json(text) is None + + def test_no_warning_on_plain_text(self, recwarn: pytest.WarningsRecorder) -> None: + """No json_repair warning for ordinary no-JSON replies (no candidate).""" + assert extract_json("Just plain text without JSON.") is None + assert extract_json("") is None + assert [w for w in recwarn.list if issubclass(w.category, RuntimeWarning)] == [] + + def test_trailing_comma_repaired_when_available(self) -> None: + """With json_repair installed, a single malformed object is repaired.""" + pytest.importorskip("json_repair") + assert extract_json('{"edit": "add", "text": "x",}') == {"edit": "add", "text": "x"} + + def test_two_malformed_objects_too_ambiguous(self) -> None: + """Multiple top-level objects are ambiguous → None, never guess.""" + pytest.importorskip("json_repair") + assert extract_json('{"first": true,} noise {"second": true,}') is None + + class TestExtractJsonArray: """extract_json_array — extract a JSON array from LLM response text."""