From 79447e311807a03639eded8fadf8d7467eb9035e Mon Sep 17 00:00:00 2001 From: Dan Baciu Date: Fri, 21 Aug 2026 18:49:37 +0200 Subject: [PATCH 1/3] feat(sleep): paired A/B evalkit with McNemar and bootstrap CIs Add a stdlib evalkit so Sleep comparisons share one instrument: one fixed task manifest, McNemar on paired binary outcomes, percentile bootstrap CIs on the success-rate delta, and multi-seed variance bands. Cross-manifest id mismatches are refused. The nightly gate is unchanged. Related: #108 --- CHANGELOG.md | 4 + docs/reference/cli.md | 7 +- docs/sleep/README.md | 14 + docs/sleep/evalkit.md | 53 ++ skillopt_sleep/__main__.py | 26 + skillopt_sleep/evalkit.py | 479 ++++++++++++++++++ tests/fixtures/evalkit/aa_manifest.json | 8 + tests/fixtures/evalkit/aa_outcomes.json | 9 + tests/fixtures/evalkit/mcnemar_textbook.json | 10 + .../evalkit/results_searchqa_nano_gated.json | 9 + tests/test_evalkit.py | 214 ++++++++ 11 files changed, 831 insertions(+), 2 deletions(-) create mode 100644 docs/sleep/evalkit.md create mode 100644 skillopt_sleep/evalkit.py create mode 100644 tests/fixtures/evalkit/aa_manifest.json create mode 100644 tests/fixtures/evalkit/aa_outcomes.json create mode 100644 tests/fixtures/evalkit/mcnemar_textbook.json create mode 100644 tests/fixtures/evalkit/results_searchqa_nano_gated.json create mode 100644 tests/test_evalkit.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 513840a9..3da89ab6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ All notable changes to SkillOpt are documented here. This project adheres to ## [Unreleased] ### Added +- **SkillOpt-Sleep paired A/B evalkit** (`python -m skillopt_sleep.evalkit`): + McNemar plus percentile-bootstrap CIs on a fixed task manifest, with + multi-seed variance bands and an A/A calibration. The nightly gate is + unchanged (thanks @bogdanbaciu21). - **SkillOpt-Sleep multi-skill fan-out and reviewed subset adoption**: each hinted skill is consolidated from its own pinned live baseline, staged as an independent proposal with per-skill gate evidence, and promoted only through diff --git a/docs/reference/cli.md b/docs/reference/cli.md index f0ea40a5..6ed591c2 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -123,8 +123,11 @@ skillopt-sleep [options] python -m skillopt_sleep [options] ``` -Actions are `run`, `dry-run`, `status`, `adopt`, `harvest`, `schedule`, and -`unschedule`. Common options include: +Actions are `run`, `dry-run`, `status`, `adopt`, `harvest`, `schedule`, +`unschedule`, and `evalkit`. `evalkit` is also available as +`python -m skillopt_sleep.evalkit` and compares two conditions on one fixed +task manifest (McNemar + bootstrap CI). See `docs/sleep/evalkit.md`. Common +options for the nightly actions include: | Argument | Description | |---|---| diff --git a/docs/sleep/README.md b/docs/sleep/README.md index 2576c127..c3c00db2 100644 --- a/docs/sleep/README.md +++ b/docs/sleep/README.md @@ -339,6 +339,20 @@ correctness signal; the validation gate still governs what ships. | `recall_k` | `0` | Associative recall โ€” pull the K most-similar past tasks (from a persisted archive) into tonight's dream. | | `dream_factor` | `0` | Add N lightweight synthetic variants of each task. | +### Paired A/B evalkit + +Reports and PRs that claim "B beats A" should go through the shared evalkit +rather than quoting a single-run cell. One command pairs two conditions on one +fixed task manifest, runs McNemar's test, and reports a bootstrap CI on the +success-rate delta. The nightly gate is unchanged. + +```text +python -m skillopt_sleep.evalkit --manifest tasks.json --a cond_a.json --b cond_b.json +``` + +See [`evalkit.md`](evalkit.md) for the id-set contract, multi-seed bands, the +A/A calibration, and the published RESULTS cell replay. + ## Results > ๐Ÿ“Š **More results & analysis โ€” the gate-safety stress test, experience-replay diff --git a/docs/sleep/evalkit.md b/docs/sleep/evalkit.md new file mode 100644 index 00000000..ce6e9c61 --- /dev/null +++ b/docs/sleep/evalkit.md @@ -0,0 +1,53 @@ +# Paired A/B evalkit + +Sleep contributors have a shared instrument for "condition B beats condition A": + +```text +python -m skillopt_sleep.evalkit --manifest tasks.json --a cond_a.json --b cond_b.json +``` + +The kit pairs outcomes by task id, runs McNemar's test on binary successes, and +reports a percentile-bootstrap confidence interval on the success-rate delta. +It does not change the nightly gate. + +## Inputs + +- `--manifest`: JSON list of task ids, or `{"ids": [...]}` / `{"tasks": [{"id": ...}]}`. +- `--a` / `--b`: JSON objects mapping those same ids to `0`/`1` (or a list of + per-seed `0`/`1` values). A wrapper `{"outcomes": {...}}` is also accepted. +- `--aa`: A/A calibration (reuses `--a` as both conditions). Must not reject. +- `--allow-graded`: permit non-binary scores. McNemar is omitted; bootstrap only. +- `--boot`, `--seed`, `--alpha`, `--json`. + +The id sets of the manifest, A, and B must be identical. Cross-manifest +comparisons are refused. + +## Multi-seed + +When each task maps to a same-length list of seed repeats, the kit: + +1. averages per task across seeds for the headline delta and bootstrap CI +2. pools `(task, seed)` pairs for McNemar +3. publishes the per-seed deltas plus their mean and sample sd + +That is the house answer to single-seed noise (see issue #108 and the +single-seed warning in `RESULTS.md`). + +## RESULTS cell replay + +`tests/fixtures/evalkit/results_searchqa_nano_gated.json` replays the published +SearchQA / GPT-5.4-nano / gated / cumulative nights=5 cell (baseline 0.560, +after 0.679, ฮ” +11.9 on n=1400). Per-task pairs were not published, so the +replay uses a documented maximum-concordance reconstruction: the first +`round(n * rate)` tasks succeed in each condition. The harness recovers the +published delta; it does not claim to recover the original microdata. + +## A/A check + +```text +python -m skillopt_sleep.evalkit --manifest tests/fixtures/evalkit/aa_manifest.json \ + --a tests/fixtures/evalkit/aa_outcomes.json --aa +``` + +Identical conditions must report delta 0, McNemar p_exact = 1, and a CI that +includes 0. If they do not, the statistics are miscoded. diff --git a/skillopt_sleep/__main__.py b/skillopt_sleep/__main__.py index 6875ad21..259de9d4 100644 --- a/skillopt_sleep/__main__.py +++ b/skillopt_sleep/__main__.py @@ -882,6 +882,19 @@ def main(argv=None) -> int: p_unsched = sub.add_parser("unschedule", help="remove the nightly cron entry") _add_common(p_unsched) p_unsched.add_argument("--all", action="store_true", help="remove all managed entries") + p_eval = sub.add_parser( + "evalkit", + help="paired A/B comparison (McNemar + bootstrap CI)", + ) + p_eval.add_argument("--manifest", required=True) + p_eval.add_argument("--a", required=True) + p_eval.add_argument("--b", default="") + p_eval.add_argument("--aa", action="store_true") + p_eval.add_argument("--alpha", type=float, default=0.05) + p_eval.add_argument("--boot", type=int, default=10000) + p_eval.add_argument("--seed", type=int, default=42) + p_eval.add_argument("--allow-graded", action="store_true") + p_eval.add_argument("--json", action="store_true") args = parser.parse_args(argv) if args.cmd == "run": @@ -898,6 +911,19 @@ def main(argv=None) -> int: return cmd_schedule(args) if args.cmd == "unschedule": return cmd_unschedule(args) + if args.cmd == "evalkit": + from skillopt_sleep.evalkit import main as evalkit_main + argv = ["--manifest", args.manifest, "--a", args.a] + if args.b: + argv.extend(["--b", args.b]) + if args.aa: + argv.append("--aa") + argv.extend(["--alpha", str(args.alpha), "--boot", str(args.boot), "--seed", str(args.seed)]) + if args.allow_graded: + argv.append("--allow-graded") + if args.json: + argv.append("--json") + return evalkit_main(argv) parser.print_help() return 2 diff --git a/skillopt_sleep/evalkit.py b/skillopt_sleep/evalkit.py new file mode 100644 index 00000000..8a09dd72 --- /dev/null +++ b/skillopt_sleep/evalkit.py @@ -0,0 +1,479 @@ +"""Paired A/B evaluation kit for SkillOpt-Sleep. + +Sleep reports (and many PRs) quote single-run success rates with no +uncertainty and no guarantee that the two conditions saw the same tasks. +This module is the shared instrument for those comparisons: + + * one fixed task manifest, paired by task id + * McNemar's test on per-task binary outcomes + * percentile-bootstrap confidence intervals on the success-rate delta + * optional multi-seed repeats (per-seed deltas + a pooled pair test) + +It does not change the nightly gate. It standardizes the evidence that +reports and PRs cite. Pure stdlib; no numpy / scipy. + +Refuse comparisons whose task-id sets differ. Graded (non-binary) scores +are bootstrap-only: McNemar is not defined for them. + +CLI:: + + python -m skillopt_sleep.evalkit --manifest M.json --a A.json --b B.json +""" +from __future__ import annotations + +import argparse +import json +import math +import random +import sys +from dataclasses import asdict, dataclass, field +from typing import Any, Dict, Iterable, List, Mapping, Optional, Sequence, Tuple + + +# โ”€โ”€ errors โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ + +class EvalkitError(ValueError): + """User-facing contract failure (mismatched ids, empty, etc.).""" + + +# โ”€โ”€ results โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ + +@dataclass +class McNemarResult: + both_success: int + a_only: int # A success, B fail (c in the usual 2x2) + b_only: int # A fail, B success (b) + both_fail: int + n: int + chi2: float # uncorrected (b-c)^2 / (b+c); nan if no discordants + p_chi2: float + p_exact: float # two-sided exact binomial on discordants + significant: bool + + def to_dict(self) -> Dict[str, Any]: + return asdict(self) + + +@dataclass +class BootstrapCI: + n_boot: int + seed: int + alpha: float + low: float + high: float + mean: float + + def to_dict(self) -> Dict[str, Any]: + return asdict(self) + + +@dataclass +class EvalReport: + n_tasks: int + rate_a: float + rate_b: float + delta: float + mcnemar: Optional[McNemarResult] + bootstrap: BootstrapCI + per_seed: List[Dict[str, float]] = field(default_factory=list) + seed_mean_delta: Optional[float] = None + seed_sd_delta: Optional[float] = None + notes: List[str] = field(default_factory=list) + refused: bool = False + refuse_reason: str = "" + + def to_dict(self) -> Dict[str, Any]: + d = asdict(self) + if self.mcnemar is not None: + d["mcnemar"] = self.mcnemar.to_dict() + d["bootstrap"] = self.bootstrap.to_dict() + return d + + +# โ”€โ”€ statistics โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ + +def _chi2_sf_df1(x: float) -> float: + """Survival function of chi-square with 1 df: P(X > x) = erfc(sqrt(x/2)).""" + if x < 0.0 or math.isnan(x): + return float("nan") + if x == 0.0: + return 1.0 + return math.erfc(math.sqrt(x / 2.0)) + + +def _binom_pmf(k: int, n: int, p: float = 0.5) -> float: + if k < 0 or k > n: + return 0.0 + # nCk * p^k * (1-p)^(n-k). For p=0.5 this is nCk / 2^n. + if p == 0.5: + return math.comb(n, k) / float(1 << n) if n < 1024 else math.comb(n, k) * (0.5 ** n) + return math.comb(n, k) * (p ** k) * ((1.0 - p) ** (n - k)) + + +def exact_mcnemar_p(b: int, c: int) -> float: + """Two-sided exact McNemar p-value (binomial test of discordants, p=0.5).""" + n = b + c + if n == 0: + return 1.0 + k = min(b, c) + tail = sum(_binom_pmf(i, n, 0.5) for i in range(0, k + 1)) + return min(1.0, 2.0 * tail) + + +def mcnemar_from_counts( + both_success: int, + a_only: int, + b_only: int, + both_fail: int, + *, + alpha: float = 0.05, +) -> McNemarResult: + n = both_success + a_only + b_only + both_fail + disc = a_only + b_only + if disc == 0: + chi2 = 0.0 + p_chi2 = 1.0 + else: + chi2 = (b_only - a_only) ** 2 / float(disc) + p_chi2 = _chi2_sf_df1(chi2) + p_exact = exact_mcnemar_p(b_only, a_only) + return McNemarResult( + both_success=both_success, + a_only=a_only, + b_only=b_only, + both_fail=both_fail, + n=n, + chi2=chi2, + p_chi2=p_chi2, + p_exact=p_exact, + significant=p_exact < alpha, + ) + + +def mcnemar_paired(a: Sequence[int], b: Sequence[int], *, alpha: float = 0.05) -> McNemarResult: + if len(a) != len(b): + raise EvalkitError("McNemar requires equal-length paired outcomes") + bs = ao = bo = bf = 0 + for x, y in zip(a, b): + if x and y: + bs += 1 + elif x and not y: + ao += 1 + elif (not x) and y: + bo += 1 + else: + bf += 1 + return mcnemar_from_counts(bs, ao, bo, bf, alpha=alpha) + + +def bootstrap_delta_ci( + a: Sequence[float], + b: Sequence[float], + *, + n_boot: int = 10000, + seed: int = 42, + alpha: float = 0.05, +) -> BootstrapCI: + if len(a) != len(b) or not a: + raise EvalkitError("bootstrap requires a non-empty paired sample") + if n_boot < 1: + raise EvalkitError("n_boot must be >= 1") + rng = random.Random(seed) + n = len(a) + deltas: List[float] = [] + for _ in range(n_boot): + idx = [rng.randrange(n) for _ in range(n)] + da = sum(a[i] for i in idx) / n + db = sum(b[i] for i in idx) / n + deltas.append(db - da) + deltas.sort() + # Inclusive percentile on the sorted sample. + lo_i = int(math.floor((alpha / 2.0) * (n_boot - 1))) + hi_i = int(math.ceil((1.0 - alpha / 2.0) * (n_boot - 1))) + lo_i = max(0, min(n_boot - 1, lo_i)) + hi_i = max(0, min(n_boot - 1, hi_i)) + return BootstrapCI( + n_boot=n_boot, + seed=seed, + alpha=alpha, + low=deltas[lo_i], + high=deltas[hi_i], + mean=sum(deltas) / n_boot, + ) + + +# โ”€โ”€ pairing / loading โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ + +def _as_binary(value: Any) -> Optional[int]: + if value is True or value == 1 or value == 1.0: + return 1 + if value is False or value == 0 or value == 0.0: + return 0 + return None + + +def _normalize_outcomes(raw: Mapping[str, Any]) -> Dict[str, List[float]]: + """Map task id -> list of per-seed scores (length 1 if unseeded).""" + out: Dict[str, List[float]] = {} + for tid, val in raw.items(): + key = str(tid) + if isinstance(val, Mapping) and "seeds" in val: + val = val["seeds"] + if isinstance(val, (list, tuple)): + out[key] = [float(x) for x in val] + else: + out[key] = [float(val)] + return out + + +def align_pairs( + manifest_ids: Sequence[str], + outcomes_a: Mapping[str, Any], + outcomes_b: Mapping[str, Any], +) -> Tuple[List[str], List[List[float]], List[List[float]]]: + """Align A and B onto the manifest. Refuse any id-set mismatch.""" + ids = [str(i) for i in manifest_ids] + if not ids: + raise EvalkitError("manifest is empty") + if len(ids) != len(set(ids)): + raise EvalkitError("manifest has duplicate task ids") + a = _normalize_outcomes(outcomes_a) + b = _normalize_outcomes(outcomes_b) + a_ids, b_ids = set(a), set(b) + want = set(ids) + if a_ids != want or b_ids != want: + missing_a = sorted(want - a_ids) + missing_b = sorted(want - b_ids) + extra_a = sorted(a_ids - want) + extra_b = sorted(b_ids - want) + raise EvalkitError( + "outcome task ids must equal the manifest " + f"(missing_a={missing_a[:8]}, missing_b={missing_b[:8]}, " + f"extra_a={extra_a[:8]}, extra_b={extra_b[:8]})" + ) + n_seed_a = {len(a[i]) for i in ids} + n_seed_b = {len(b[i]) for i in ids} + if len(n_seed_a) != 1 or n_seed_a != n_seed_b: + raise EvalkitError("every task must have the same number of seed repeats in A and B") + return ids, [a[i] for i in ids], [b[i] for i in ids] + + +def _is_binary_matrix(rows: Sequence[Sequence[float]]) -> bool: + for row in rows: + for x in row: + if _as_binary(x) is None: + return False + return True + + +def _mean(xs: Iterable[float]) -> float: + seq = list(xs) + return sum(seq) / len(seq) if seq else float("nan") + + +def _sd(xs: Sequence[float]) -> float: + if len(xs) < 2: + return 0.0 + m = _mean(xs) + return math.sqrt(sum((x - m) ** 2 for x in xs) / (len(xs) - 1)) + + +def reconstruct_paired_from_rates(n: int, rate_a: float, rate_b: float) -> Tuple[List[int], List[int]]: + """Deterministic maximum-concordance reconstruction of paired binaries. + + First ``round(n * rate)`` tasks succeed in each condition, same id order. + This is a published-rate replay convention, not original microdata. + """ + if n < 1: + raise EvalkitError("n must be >= 1") + ka = int(round(n * rate_a)) + kb = int(round(n * rate_b)) + a = [1 if i < ka else 0 for i in range(n)] + b = [1 if i < kb else 0 for i in range(n)] + return a, b + + +def compare( + manifest_ids: Sequence[str], + outcomes_a: Mapping[str, Any], + outcomes_b: Mapping[str, Any], + *, + alpha: float = 0.05, + n_boot: int = 10000, + seed: int = 42, + allow_graded: bool = False, +) -> EvalReport: + ids, a_rows, b_rows = align_pairs(manifest_ids, outcomes_a, outcomes_b) + n_seed = len(a_rows[0]) + notes: List[str] = [] + + # Per-task mean across seeds (the headline paired sample). + a_mean = [_mean(row) for row in a_rows] + b_mean = [_mean(row) for row in b_rows] + rate_a = _mean(a_mean) + rate_b = _mean(b_mean) + delta = rate_b - rate_a + boot = bootstrap_delta_ci(a_mean, b_mean, n_boot=n_boot, seed=seed, alpha=alpha) + + binary = _is_binary_matrix(a_rows) and _is_binary_matrix(b_rows) + mcnemar: Optional[McNemarResult] = None + if binary: + # Pool (task, seed) as paired observations when seeds align. + flat_a = [int(_as_binary(x) or 0) for row in a_rows for x in row] + flat_b = [int(_as_binary(x) or 0) for row in b_rows for x in row] + mcnemar = mcnemar_paired(flat_a, flat_b, alpha=alpha) + elif allow_graded: + notes.append("graded scores: McNemar omitted; bootstrap CI only") + else: + raise EvalkitError( + "non-binary scores require --allow-graded (McNemar is undefined)" + ) + + per_seed: List[Dict[str, float]] = [] + seed_mean = seed_sd = None + if n_seed > 1: + for s in range(n_seed): + da = _mean(row[s] for row in a_rows) + db = _mean(row[s] for row in b_rows) + per_seed.append({"seed": float(s), "rate_a": da, "rate_b": db, "delta": db - da}) + deltas = [row["delta"] for row in per_seed] + seed_mean = _mean(deltas) + seed_sd = _sd(deltas) + notes.append( + f"multi-seed: {n_seed} repeats; seed-mean delta={seed_mean:.6f} " + f"sd={seed_sd:.6f}" + ) + + return EvalReport( + n_tasks=len(ids), + rate_a=rate_a, + rate_b=rate_b, + delta=delta, + mcnemar=mcnemar, + bootstrap=boot, + per_seed=per_seed, + seed_mean_delta=seed_mean, + seed_sd_delta=seed_sd, + notes=notes, + ) + + +def compare_aa( + manifest_ids: Sequence[str], + outcomes: Mapping[str, Any], + **kwargs: Any, +) -> EvalReport: + """A/A calibration: identical conditions must not reject at alpha.""" + report = compare(manifest_ids, outcomes, outcomes, **kwargs) + report.notes.append("A/A calibration (identical conditions)") + return report + + +# โ”€โ”€ I/O โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ + +def _load_json(path: str) -> Any: + with open(path, encoding="utf-8") as f: + return json.load(f) + + +def _manifest_ids(obj: Any) -> List[str]: + if isinstance(obj, list): + return [str(x) for x in obj] + if isinstance(obj, Mapping): + if "ids" in obj: + return [str(x) for x in obj["ids"]] + if "tasks" in obj: + return [str(t["id"] if isinstance(t, Mapping) else t) for t in obj["tasks"]] + if "outcomes" in obj: + return [str(k) for k in obj["outcomes"]] + raise EvalkitError("manifest must be a list of ids or an object with ids/tasks") + + +def _outcomes(obj: Any) -> Dict[str, Any]: + if isinstance(obj, Mapping) and "outcomes" in obj: + return dict(obj["outcomes"]) + if isinstance(obj, Mapping): + return dict(obj) + raise EvalkitError("outcomes file must be an object mapping task id to score") + + +def format_markdown(report: EvalReport) -> str: + lines = [ + "# Paired A/B evalkit report", + "", + f"- n_tasks: {report.n_tasks}", + f"- rate_a: {report.rate_a:.6f}", + f"- rate_b: {report.rate_b:.6f}", + f"- delta (B-A): {report.delta:+.6f}", + ( + f"- bootstrap {int((1 - report.bootstrap.alpha) * 100)}% CI: " + f"[{report.bootstrap.low:+.6f}, {report.bootstrap.high:+.6f}] " + f"(n_boot={report.bootstrap.n_boot}, seed={report.bootstrap.seed})" + ), + ] + if report.mcnemar is not None: + m = report.mcnemar + lines.append( + f"- McNemar 2x2: both+={m.both_success} a_only={m.a_only} " + f"b_only={m.b_only} both-={m.both_fail}" + ) + lines.append( + f"- McNemar chi2={m.chi2:.4f} p_chi2={m.p_chi2:.6g} " + f"p_exact={m.p_exact:.6g} significant={m.significant}" + ) + if report.seed_mean_delta is not None: + lines.append( + f"- multi-seed mean delta: {report.seed_mean_delta:+.6f} " + f"(sd {report.seed_sd_delta:.6f}, k={len(report.per_seed)})" + ) + for note in report.notes: + lines.append(f"- note: {note}") + return "\n".join(lines) + "\n" + + +def main(argv: Optional[Sequence[str]] = None) -> int: + p = argparse.ArgumentParser( + prog="skillopt_sleep.evalkit", + description="Paired A/B comparison with McNemar and bootstrap CIs", + ) + p.add_argument("--manifest", required=True, help="JSON list of task ids (or {ids,tasks})") + p.add_argument("--a", required=True, help="JSON outcomes for condition A") + p.add_argument("--b", default="", help="JSON outcomes for condition B (omit for A/A)") + p.add_argument("--aa", action="store_true", help="A/A calibration (ignore --b, reuse --a)") + p.add_argument("--alpha", type=float, default=0.05) + p.add_argument("--boot", type=int, default=10000) + p.add_argument("--seed", type=int, default=42) + p.add_argument("--allow-graded", action="store_true") + p.add_argument("--json", action="store_true") + args = p.parse_args(list(argv) if argv is not None else None) + + try: + ids = _manifest_ids(_load_json(args.manifest)) + a = _outcomes(_load_json(args.a)) + if args.aa or not args.b: + report = compare_aa( + ids, a, alpha=args.alpha, n_boot=args.boot, + seed=args.seed, allow_graded=args.allow_graded, + ) + else: + b = _outcomes(_load_json(args.b)) + report = compare( + ids, a, b, alpha=args.alpha, n_boot=args.boot, + seed=args.seed, allow_graded=args.allow_graded, + ) + except EvalkitError as exc: + print(f"ERR_EVALKIT {exc}", file=sys.stderr) + return 2 + except OSError as exc: + print(f"ERR_EVALKIT {exc}", file=sys.stderr) + return 1 + + if args.json: + print(json.dumps(report.to_dict(), indent=2, sort_keys=True)) + else: + print(format_markdown(report), end="") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/fixtures/evalkit/aa_manifest.json b/tests/fixtures/evalkit/aa_manifest.json new file mode 100644 index 00000000..05448f5b --- /dev/null +++ b/tests/fixtures/evalkit/aa_manifest.json @@ -0,0 +1,8 @@ +{ + "ids": [ + "t00", "t01", "t02", "t03", "t04", "t05", "t06", "t07", "t08", "t09", + "t10", "t11", "t12", "t13", "t14", "t15", "t16", "t17", "t18", "t19", + "t20", "t21", "t22", "t23", "t24", "t25", "t26", "t27", "t28", "t29", + "t30", "t31", "t32", "t33", "t34", "t35", "t36", "t37", "t38", "t39" + ] +} diff --git a/tests/fixtures/evalkit/aa_outcomes.json b/tests/fixtures/evalkit/aa_outcomes.json new file mode 100644 index 00000000..444ad43f --- /dev/null +++ b/tests/fixtures/evalkit/aa_outcomes.json @@ -0,0 +1,9 @@ +{ + "outcomes": { + "t00": 1, "t01": 1, "t02": 1, "t03": 1, "t04": 1, "t05": 1, "t06": 1, "t07": 1, + "t08": 1, "t09": 1, "t10": 1, "t11": 1, "t12": 1, "t13": 1, "t14": 1, "t15": 1, + "t16": 1, "t17": 1, "t18": 1, "t19": 1, "t20": 0, "t21": 0, "t22": 0, "t23": 0, + "t24": 0, "t25": 0, "t26": 0, "t27": 0, "t28": 0, "t29": 0, "t30": 0, "t31": 0, + "t32": 0, "t33": 0, "t34": 0, "t35": 0, "t36": 0, "t37": 0, "t38": 0, "t39": 0 + } +} diff --git a/tests/fixtures/evalkit/mcnemar_textbook.json b/tests/fixtures/evalkit/mcnemar_textbook.json new file mode 100644 index 00000000..f6019ccb --- /dev/null +++ b/tests/fixtures/evalkit/mcnemar_textbook.json @@ -0,0 +1,10 @@ +{ + "name": "textbook-2x2", + "both_success": 40, + "a_only": 2, + "b_only": 12, + "both_fail": 46, + "chi2": 7.142857142857143, + "p_chi2": 0.007526315166457887, + "p_exact": 0.012939453125 +} diff --git a/tests/fixtures/evalkit/results_searchqa_nano_gated.json b/tests/fixtures/evalkit/results_searchqa_nano_gated.json new file mode 100644 index 00000000..5df05905 --- /dev/null +++ b/tests/fixtures/evalkit/results_searchqa_nano_gated.json @@ -0,0 +1,9 @@ +{ + "cell_id": "results-searchqa-nano-gated-cumulative-nights5", + "source": "docs/sleep/RESULTS.md section 2", + "n": 1400, + "baseline": 0.560, + "after": 0.679, + "published_delta": 0.119, + "reconstruction": "maximum-concordance: first round(n*rate) tasks succeed in each condition" +} diff --git a/tests/test_evalkit.py b/tests/test_evalkit.py new file mode 100644 index 00000000..e09c6338 --- /dev/null +++ b/tests/test_evalkit.py @@ -0,0 +1,214 @@ +"""Paired A/B evalkit: known-answer stats, A/A calibration, RESULTS replay.""" +from __future__ import annotations + +import json +import math +import os +import subprocess +import sys +import tempfile +import unittest + +from skillopt_sleep.evalkit import ( + EvalkitError, + bootstrap_delta_ci, + compare, + compare_aa, + exact_mcnemar_p, + format_markdown, + main as evalkit_main, + mcnemar_from_counts, + mcnemar_paired, + reconstruct_paired_from_rates, +) + + +FIXTURE_DIR = os.path.join(os.path.dirname(__file__), "fixtures", "evalkit") + + +def _load(name: str): + with open(os.path.join(FIXTURE_DIR, name), encoding="utf-8") as f: + return json.load(f) + + +class TestMcNemarKnownAnswer(unittest.TestCase): + def test_textbook_2x2_chi2_and_exact(self): + fx = _load("mcnemar_textbook.json") + res = mcnemar_from_counts( + fx["both_success"], fx["a_only"], fx["b_only"], fx["both_fail"], + ) + self.assertAlmostEqual(res.chi2, fx["chi2"], places=12) + self.assertAlmostEqual(res.p_chi2, fx["p_chi2"], places=12) + self.assertAlmostEqual(res.p_exact, fx["p_exact"], places=12) + self.assertTrue(res.significant) + self.assertEqual(res.n, 100) + + def test_zero_discordants_is_not_significant(self): + res = mcnemar_from_counts(20, 0, 0, 5) + self.assertEqual(res.chi2, 0.0) + self.assertEqual(res.p_chi2, 1.0) + self.assertEqual(res.p_exact, 1.0) + self.assertFalse(res.significant) + + def test_paired_vectors_match_counts(self): + a = [1, 1, 1, 0, 0] + b = [1, 0, 1, 1, 0] + res = mcnemar_paired(a, b) + self.assertEqual(res.both_success, 2) + self.assertEqual(res.a_only, 1) + self.assertEqual(res.b_only, 1) + self.assertEqual(res.both_fail, 1) + self.assertAlmostEqual(res.p_exact, exact_mcnemar_p(1, 1)) + + +class TestBootstrapCoverage(unittest.TestCase): + def test_identical_series_ci_collapses_to_zero(self): + a = [1, 0, 1, 0, 1, 0, 1, 0] + ci = bootstrap_delta_ci(a, a, n_boot=2000, seed=7) + self.assertEqual(ci.low, 0.0) + self.assertEqual(ci.high, 0.0) + self.assertEqual(ci.mean, 0.0) + + def test_known_shift_ci_excludes_zero(self): + # A always 0, B always 1: delta = 1 exactly, CI is [1, 1]. + a = [0] * 30 + b = [1] * 30 + ci = bootstrap_delta_ci(a, b, n_boot=1000, seed=1) + self.assertEqual(ci.low, 1.0) + self.assertEqual(ci.high, 1.0) + + def test_seed_is_deterministic(self): + a = [1, 0, 1, 1, 0, 0, 1, 0, 1, 0] + b = [1, 1, 1, 0, 0, 1, 1, 0, 0, 1] + x = bootstrap_delta_ci(a, b, n_boot=500, seed=99) + y = bootstrap_delta_ci(a, b, n_boot=500, seed=99) + self.assertEqual((x.low, x.high, x.mean), (y.low, y.high, y.mean)) + + +class TestAACalibration(unittest.TestCase): + def test_aa_does_not_reject(self): + man = _load("aa_manifest.json") + out = _load("aa_outcomes.json") + report = compare_aa(man["ids"], out["outcomes"], n_boot=2000, seed=42) + self.assertEqual(report.delta, 0.0) + self.assertIsNotNone(report.mcnemar) + self.assertFalse(report.mcnemar.significant) + self.assertEqual(report.mcnemar.p_exact, 1.0) + self.assertLessEqual(report.bootstrap.low, 0.0) + self.assertGreaterEqual(report.bootstrap.high, 0.0) + + +class TestCompareContracts(unittest.TestCase): + def test_mismatched_ids_are_refused(self): + with self.assertRaises(EvalkitError) as ctx: + compare(["t1", "t2"], {"t1": 1, "t2": 0}, {"t1": 1, "t3": 0}) + self.assertIn("must equal the manifest", str(ctx.exception)) + + def test_duplicate_manifest_ids_refused(self): + with self.assertRaises(EvalkitError): + compare(["t1", "t1"], {"t1": 1}, {"t1": 0}) + + def test_empty_manifest_refused(self): + with self.assertRaises(EvalkitError): + compare([], {}, {}) + + def test_graded_refused_without_flag(self): + with self.assertRaises(EvalkitError) as ctx: + compare(["t1", "t2"], {"t1": 0.4, "t2": 0.9}, {"t1": 0.5, "t2": 0.8}) + self.assertIn("allow-graded", str(ctx.exception)) + + def test_graded_bootstrap_only(self): + report = compare( + ["t1", "t2"], + {"t1": 0.4, "t2": 0.9}, + {"t1": 0.5, "t2": 0.8}, + allow_graded=True, + n_boot=500, + seed=3, + ) + self.assertIsNone(report.mcnemar) + self.assertTrue(any("graded" in n for n in report.notes)) + self.assertAlmostEqual(report.delta, 0.0, places=12) + + def test_multi_seed_variance_band(self): + report = compare( + ["t1", "t2"], + {"t1": [1, 0, 1], "t2": [0, 0, 1]}, + {"t1": [1, 1, 1], "t2": [1, 0, 1]}, + n_boot=400, + seed=2, + ) + self.assertEqual(len(report.per_seed), 3) + self.assertIsNotNone(report.seed_mean_delta) + self.assertGreaterEqual(report.seed_sd_delta, 0.0) + self.assertAlmostEqual(report.rate_a, (2 / 3 + 1 / 3) / 2) + self.assertAlmostEqual(report.rate_b, (1.0 + 2 / 3) / 2) + + +class TestResultsCellReplay(unittest.TestCase): + def test_published_searchqa_nano_gated_delta(self): + cell = _load("results_searchqa_nano_gated.json") + a, b = reconstruct_paired_from_rates(cell["n"], cell["baseline"], cell["after"]) + self.assertEqual(len(a), cell["n"]) + self.assertAlmostEqual(sum(a) / cell["n"], cell["baseline"], places=3) + self.assertAlmostEqual(sum(b) / cell["n"], cell["after"], places=3) + ids = [f"q{i:04d}" for i in range(cell["n"])] + report = compare( + ids, + dict(zip(ids, a)), + dict(zip(ids, b)), + n_boot=800, + seed=42, + ) + self.assertAlmostEqual(report.delta, cell["published_delta"], places=3) + self.assertGreater(report.bootstrap.low, 0.0) + self.assertTrue(report.mcnemar.significant) + md = format_markdown(report) + self.assertIn("delta (B-A)", md) + self.assertIn("McNemar", md) + + +class TestCLI(unittest.TestCase): + def test_aa_cli_exit_zero(self): + rc = evalkit_main([ + "--manifest", os.path.join(FIXTURE_DIR, "aa_manifest.json"), + "--a", os.path.join(FIXTURE_DIR, "aa_outcomes.json"), + "--aa", + "--boot", "300", + "--json", + ]) + self.assertEqual(rc, 0) + + def test_mismatch_cli_exit_two(self): + with tempfile.TemporaryDirectory() as td: + man = os.path.join(td, "m.json") + a = os.path.join(td, "a.json") + b = os.path.join(td, "b.json") + with open(man, "w", encoding="utf-8") as f: + json.dump(["t1", "t2"], f) + with open(a, "w", encoding="utf-8") as f: + json.dump({"t1": 1, "t2": 0}, f) + with open(b, "w", encoding="utf-8") as f: + json.dump({"t1": 1, "t3": 0}, f) + rc = evalkit_main(["--manifest", man, "--a", a, "--b", b]) + self.assertEqual(rc, 2) + + def test_module_entrypoint(self): + proc = subprocess.run( + [ + sys.executable, "-m", "skillopt_sleep.evalkit", + "--manifest", os.path.join(FIXTURE_DIR, "aa_manifest.json"), + "--a", os.path.join(FIXTURE_DIR, "aa_outcomes.json"), + "--aa", + "--boot", "200", + ], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("delta (B-A): +0.000000", proc.stdout) + + +if __name__ == "__main__": + unittest.main() From eb845b6bc13e868308faa39c837722657fd09b76 Mon Sep 17 00:00:00 2001 From: "Bogdan (Dan) Baciu" Date: Wed, 26 Aug 2026 14:37:46 +0400 Subject: [PATCH 2/3] fix(evalkit): harden paired statistical inference --- .gitignore | 3 + CHANGELOG.md | 2 +- configs/_base_/default.yaml | 3 +- docs/reference/config.md | 2 + docs/sleep/README.md | 4 +- docs/sleep/evalkit.md | 20 +- mkdocs.yml | 1 + plugins/dsh/scripts/audit-control-chars.mjs | 42 +- plugins/dsh/scripts/audit-injection.mjs | 44 +- plugins/dsh/scripts/canary.mjs | 12 +- plugins/dsh/src/index.js | 38 +- scripts/eval_only.py | 9 + scripts/train.py | 9 + skillopt/config.py | 1 + skillopt/engine/trainer.py | 32 +- skillopt/gradient/reflect.py | 13 + skillopt/model/__init__.py | 41 ++ skillopt/model/backend_config.py | 4 +- skillopt/model/claude_code_backend.py | 313 ++++++++++++++ skillopt/model/codex_harness.py | 377 +++++++++++++++- skillopt/model/common.py | 2 +- skillopt/model/minimax_backend.py | 32 +- skillopt_sleep/backend.py | 11 + skillopt_sleep/evalkit.py | 204 +++++++-- tests/test_claude_code_backend.py | 453 ++++++++++++++++++++ tests/test_claude_code_exec_resolution.py | 141 ++++++ tests/test_copilot_tool_scope.py | 80 ++++ tests/test_evalkit.py | 143 +++++- tests/test_minimax_backend.py | 171 ++++++++ tests/test_role_backend_resolution.py | 9 + 30 files changed, 2128 insertions(+), 88 deletions(-) create mode 100644 skillopt/model/claude_code_backend.py create mode 100644 tests/test_claude_code_backend.py create mode 100644 tests/test_claude_code_exec_resolution.py create mode 100644 tests/test_copilot_tool_scope.py create mode 100644 tests/test_minimax_backend.py diff --git a/.gitignore b/.gitignore index 7c5950d8..e8179f86 100644 --- a/.gitignore +++ b/.gitignore @@ -43,6 +43,9 @@ configs/**/*.local.yaml .secrets/ .codex_azure*/ +# Local MCP server config โ€” references the machine's GITHUB_PAT, never commit +.mcp.json + # Internal docs (not for open-source release) docs/ablation_plan.md docs/ablation_paper_tables.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 3da89ab6..cf538fb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ All notable changes to SkillOpt are documented here. This project adheres to ### Added - **SkillOpt-Sleep paired A/B evalkit** (`python -m skillopt_sleep.evalkit`): McNemar plus percentile-bootstrap CIs on a fixed task manifest, with - multi-seed variance bands and an A/A calibration. The nightly gate is + task-cluster multi-seed inference and calibrated A/A coverage. The nightly gate is unchanged (thanks @bogdanbaciu21). - **SkillOpt-Sleep multi-skill fan-out and reviewed subset adoption**: each hinted skill is consolidated from its own pinned live baseline, staged as an diff --git a/configs/_base_/default.yaml b/configs/_base_/default.yaml index 09405994..4fe64088 100644 --- a/configs/_base_/default.yaml +++ b/configs/_base_/default.yaml @@ -35,6 +35,7 @@ model: copilot_chat_target_model: "" copilot_chat_timeout: null # preserves COPILOT_CHAT_TIMEOUT or the built-in default codex_trace_to_optimizer: true + claude_trace_to_optimizer: true azure_openai_endpoint: "" # e.g. "https://your-resource.openai.azure.com/" azure_openai_api_version: "2024-12-01-preview" azure_openai_api_key: "" # Fill locally if you do not export AZURE_OPENAI_API_KEY @@ -66,7 +67,7 @@ model: minimax_region: "" # global_en (default) or cn_zh; selects the base URL minimax_base_url: "" # region base URL if blank minimax_api_key: "" - minimax_model: "MiniMax-M2.7" + minimax_model: "MiniMax-M3" minimax_temperature: "0.7" minimax_max_tokens: "8000" minimax_enable_thinking: "false" diff --git a/docs/reference/config.md b/docs/reference/config.md index ff79c740..e7111430 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -100,6 +100,8 @@ winner. Prefer `thinking_mode`; use `disabled` when you need the field sent. | `model.minimax_*` | MiniMax `region`, `base_url`, `api_key`, shared `minimax_model`, `temperature`, `max_tokens`, and `enable_thinking`; `minimax_model` applies when MiniMax is the target | | `model.codex_exec_*` | Codex path, sandbox, profile, SDK mode, reasoning, network/search, and approval policy; see compatibility notes below | | `model.claude_code_exec_*` | Claude path, profile, SDK mode, effort, and thinking-token cap | +| `model.codex_trace_to_optimizer` | When `true` (default) and target is `codex_exec`, inject the agent's codex trace steps into the reflection prompt | +| `model.claude_trace_to_optimizer` | When `true` (default) and target is `claude_code_exec`, inject the agent's claude trace steps into the reflection prompt | | `model.cursor_exec_path` | Cursor Agent executable path; default `cursor-agent` | | `model.cursor_exec_sandbox` | Cursor sandbox mode: `enabled` (default) or `disabled`; file-edit rollouts require `enabled` | | `model.copilot_exec_path` | GitHub Copilot CLI executable path; default `copilot` | diff --git a/docs/sleep/README.md b/docs/sleep/README.md index c3c00db2..86fc02ff 100644 --- a/docs/sleep/README.md +++ b/docs/sleep/README.md @@ -350,8 +350,8 @@ success-rate delta. The nightly gate is unchanged. python -m skillopt_sleep.evalkit --manifest tasks.json --a cond_a.json --b cond_b.json ``` -See [`evalkit.md`](evalkit.md) for the id-set contract, multi-seed bands, the -A/A calibration, and the published RESULTS cell replay. +See [`evalkit.md`](evalkit.md) for the id-set contract, task-cluster multi-seed +inference, A/A checks, and the published RESULTS cell replay. ## Results diff --git a/docs/sleep/evalkit.md b/docs/sleep/evalkit.md index ce6e9c61..c8797423 100644 --- a/docs/sleep/evalkit.md +++ b/docs/sleep/evalkit.md @@ -15,20 +15,23 @@ It does not change the nightly gate. - `--manifest`: JSON list of task ids, or `{"ids": [...]}` / `{"tasks": [{"id": ...}]}`. - `--a` / `--b`: JSON objects mapping those same ids to `0`/`1` (or a list of per-seed `0`/`1` values). A wrapper `{"outcomes": {...}}` is also accepted. -- `--aa`: A/A calibration (reuses `--a` as both conditions). Must not reject. +- `--aa`: A/A identity smoke check (reuses `--a` as both conditions). Must not reject. - `--allow-graded`: permit non-binary scores. McNemar is omitted; bootstrap only. - `--boot`, `--seed`, `--alpha`, `--json`. The id sets of the manifest, A, and B must be identical. Cross-manifest -comparisons are refused. +comparisons are refused. Seed lists must be non-empty, scores must be finite +and in `[0, 1]`, `alpha` must be strictly between 0 and 1, and `--boot` must be +between 1 and 1,000,000. JSON output is strict and never emits NaN/Infinity. ## Multi-seed When each task maps to a same-length list of seed repeats, the kit: 1. averages per task across seeds for the headline delta and bootstrap CI -2. pools `(task, seed)` pairs for McNemar -3. publishes the per-seed deltas plus their mean and sample sd +2. resamples whole tasks, preserving the task as the independent cluster +3. omits McNemar rather than treating repeated seeds as independent samples +4. publishes the per-seed deltas plus their mean and sample sd That is the house answer to single-seed noise (see issue #108 and the single-seed warning in `RESULTS.md`). @@ -42,12 +45,15 @@ replay uses a documented maximum-concordance reconstruction: the first `round(n * rate)` tasks succeed in each condition. The harness recovers the published delta; it does not claim to recover the original microdata. -## A/A check +## A/A checks ```text python -m skillopt_sleep.evalkit --manifest tests/fixtures/evalkit/aa_manifest.json \ --a tests/fixtures/evalkit/aa_outcomes.json --aa ``` -Identical conditions must report delta 0, McNemar p_exact = 1, and a CI that -includes 0. If they do not, the statistics are miscoded. +The command above is an identity smoke check: identical conditions must report +delta 0, McNemar p_exact = 1, and a CI that includes 0. The test suite separately +runs a seeded null simulation with genuine discordant pairs and bounds the +empirical type-I-error rate; comparing one array with itself is not presented as +a statistical calibration. diff --git a/mkdocs.yml b/mkdocs.yml index 26c9a4c9..5d01170c 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -50,6 +50,7 @@ nav: - Deep Learning Analogy: guide/dl-analogy.md - SkillOpt-Sleep: - Overview: sleep/README.md + - Paired A/B Evalkit: sleep/evalkit.md - Multi-skill Staging: sleep/multi-skill-staging.md - OpenAI-compatible Endpoints: sleep/openai-compatible-endpoints.md - Results: sleep/RESULTS.md diff --git a/plugins/dsh/scripts/audit-control-chars.mjs b/plugins/dsh/scripts/audit-control-chars.mjs index 233c8b8e..c7dbd0b3 100644 --- a/plugins/dsh/scripts/audit-control-chars.mjs +++ b/plugins/dsh/scripts/audit-control-chars.mjs @@ -1,10 +1,13 @@ -// ๅฎ‰ๅ…จๅฎกๆ ธ๏ผšๆŽงๅˆถๅญ—็ฌฆ/ๆข่กŒไธ่ƒฝ้€ƒ้€ธๅ•ๅผ•ๅท่พน็•Œ๏ผŒไธ”ไธไบง็”Ÿๅ‰ฏไฝœ็”จใ€‚ -// ไฟฎๅคๅŽ็š„ๆœŸๆœ›๏ผšๆŽงๅˆถๅญ—็ฌฆ่ขซๅ‰ฅ็ฆปไธบ็ฉบๆ ผ๏ผŒๅ€ผไปไฝœไธบๅ•ไธชๅ‚ๆ•ฐๅˆฐ่พพ๏ผŒๆ— ๆ–‡ไปถ/ๅ‘ฝไปคๆ‰ง่กŒใ€‚ +// Security audit: control characters/newlines cannot escape the quoting +// boundary of the HOST shell, and produce no side effects. Expected after the +// fix: control chars are folded to spaces, the value still arrives as a single +// argument, no file or command runs. import { execFileSync } from 'node:child_process' import { existsSync } from 'node:fs' +import { join } from 'node:path' const m = await import('../src/index.js') -const { quoteArgv } = m -const BASH = process.env.BASH_PATH || 'C:/Program Files/Git/bin/bash.exe' +const { quoteArgv, IS_WINDOWS } = m + const payloads = [ 'x\n touch /tmp/nl-pwned', 'x\r echo PWNED', @@ -14,20 +17,39 @@ const payloads = [ "'; touch /tmp/semi-pwned;'", 'normal\r\ntouch /tmp/crnl-pwned', ] + +function expandArgs(quoted) { + if (IS_WINDOWS) { + const candidates = [ + process.env.PWSH_PATH, + process.env.ProgramFiles ? join(process.env.ProgramFiles, 'PowerShell', '7', 'pwsh.exe') : '', + join(process.env.SystemRoot || 'C:\\Windows', 'System32', 'WindowsPowerShell', 'v1.0', 'powershell.exe'), + ].filter(Boolean) + const shell = candidates.find((p) => existsSync(p)) + if (!shell) throw new Error('no PowerShell executable found for control-char audit') + const args = quoted.replace(/^& /, '') + const script = `Write-Output ${args} | ForEach-Object { "[$_]" }` + const out = execFileSync(shell, ['-NoProfile', '-Command', script], { encoding: 'utf8' }) + // PowerShell emits CRLF; strip \r before slicing the [..] markers. + return out.trim().split('\n').map((l) => l.replace(/\r$/, '').slice(1, -1)) + } + const script = `for a in ${quoted}; do printf '[%s]\\n' "$a"; done` + const out = execFileSync(process.env.BASH_PATH || '/bin/bash', ['-c', script], { encoding: 'utf8' }) + return out.trim().split('\n').map((l) => l.slice(1, -1)) +} + let fail = 0 for (const p of payloads) { const argv = ['python', '-m', 'skillopt_sleep', 'run', '--preferences', p] const quoted = quoteArgv(argv) - const script = `for a in ${quoted}; do printf '[%s]\\n' "$a"; done` - const out = execFileSync(BASH, ['-c', script], { encoding: 'utf8' }) - const args = out.trim().split('\n').map((l) => l.slice(1, -1)) - // ๅฎ‰ๅ…จๆ–ญ่จ€ 1๏ผš--preferences ๅŽๆฐๅฅฝไธ€ไธชๅ‚ๆ•ฐ๏ผˆๆ— ๅ‚ๆ•ฐๅˆ†่ฃ‚๏ผ‰ + const args = expandArgs(quoted) + // Safety assertion 1: exactly one argument after --preferences (no split). const idx = args.indexOf('--preferences') const singleArg = idx >= 0 && idx + 1 < args.length && idx + 2 === args.length - // ๅฎ‰ๅ…จๆ–ญ่จ€ 2๏ผšๅˆฐ่พพ็š„ๅ€ผไธๅซๅŽŸๅง‹ๆŽงๅˆถๅญ—็ฌฆ๏ผˆๅทฒ่ขซๅ‰ฅ็ฆปไธบ็ฉบๆ ผ๏ผ‰ + // Safety assertion 2: the arrived value has no raw control chars (folded to space). const arrived = idx >= 0 ? args[idx + 1] : '' const stripped = !/[\r\n\t\x00]/.test(arrived) - // ๅฎ‰ๅ…จๆ–ญ่จ€ 3๏ผšๆฒกๆœ‰ๆณจๅ…ฅๅ‘ฝไปคๅ‡บ็Žฐๅœจๅ‚ๆ•ฐๅˆ—่กจๅค– + // Safety assertion 3: no injected command outside the argument list. const noInjection = !args.some((a) => /touch|PWNED|rm\s/.test(a) && a !== arrived) if (!singleArg || !stripped || !noInjection) { fail++ diff --git a/plugins/dsh/scripts/audit-injection.mjs b/plugins/dsh/scripts/audit-injection.mjs index c53602d2..ed873ec2 100644 --- a/plugins/dsh/scripts/audit-injection.mjs +++ b/plugins/dsh/scripts/audit-injection.mjs @@ -1,9 +1,13 @@ -// ็‹ฌ็ซ‹ๆณจๅ…ฅๅฎก่ฎก๏ผšๅ„็งๆถๆ„ payload ่ฟ‡ quoteArgv โ†’ ็œŸๅฎž bash โ†’ ้ชŒ่ฏไธ้€ƒ้€ธ +// Injection audit: malicious payloads through quoteArgv -> the HOST shell -> +// verify none escape. On win32 the host shell is PowerShell (5.1 or pwsh 7); +// on POSIX it is bash. Each payload must arrive as exactly one argument, and +// no command may run. import { execFileSync } from 'node:child_process' import { existsSync } from 'node:fs' +import { join } from 'node:path' const m = await import('../src/index.js') -const { quoteArgv } = m -const BASH = process.env.BASH_PATH || 'C:/Program Files/Git/bin/bash.exe' +const { quoteArgv, IS_WINDOWS } = m + const payloads = [ 'x; touch /tmp/pwned', 'x$(touch /tmp/pwned2)', @@ -13,13 +17,41 @@ const payloads = [ "' OR 1=1 --", 'x > /tmp/redirected', ] + +// Render `quoted` as one argument per line and return the array. The command +// is passed to the host shell as ONE string; each argv element arrives quoted, +// so the script only needs to echo every received argument back verbatim. +function expandArgs(quoted) { + if (IS_WINDOWS) { + // Windows PowerShell (5.1 or pwsh 7): single-quoted args parse the same on + // both. Write-Output pipes each argument through ForEach-Object as one + // pipeline object, so the round-trip is the real parse. + const candidates = [ + process.env.PWSH_PATH, + process.env.ProgramFiles ? join(process.env.ProgramFiles, 'PowerShell', '7', 'pwsh.exe') : '', + join(process.env.SystemRoot || 'C:\\Windows', 'System32', 'WindowsPowerShell', 'v1.0', 'powershell.exe'), + ].filter(Boolean) + const shell = candidates.find((p) => existsSync(p)) + if (!shell) throw new Error('no PowerShell executable found for injection audit') + // quoteArgv now prepends "& " (the PS call operator) on win32. Strip it + // before echoing the argument array; PowerShell would reject `&` in the + // middle of a pipeline expression. + const args = quoted.replace(/^& /, '') + const script = `Write-Output ${args} | ForEach-Object { "[$_]" }` + const out = execFileSync(shell, ['-NoProfile', '-Command', script], { encoding: 'utf8' }) + // PowerShell emits CRLF; strip \r before slicing the [..] markers. + return out.trim().split('\n').map((l) => l.replace(/\r$/, '').slice(1, -1)) + } + const script = `for a in ${quoted}; do printf '[%s]\\n' "$a"; done` + const out = execFileSync(process.env.BASH_PATH || '/bin/bash', ['-c', script], { encoding: 'utf8' }) + return out.trim().split('\n').map((l) => l.slice(1, -1)) +} + let fail = 0 for (const p of payloads) { const argv = ['python', '-m', 'skillopt_sleep', 'run', '--preferences', p] const quoted = quoteArgv(argv) - const script = `for a in ${quoted}; do printf '[%s]\\n' "$a"; done` - const out = execFileSync(BASH, ['-c', script], { encoding: 'utf8' }) - const args = out.trim().split('\n').map((l) => l.slice(1, -1)) + const args = expandArgs(quoted) const pref = args[args.indexOf('--preferences') + 1] const ok = pref === p if (!ok) { fail++; console.log('FAIL:', JSON.stringify(p), '->', JSON.stringify(pref)) } diff --git a/plugins/dsh/scripts/canary.mjs b/plugins/dsh/scripts/canary.mjs index 66a87d96..c22a9b8a 100644 --- a/plugins/dsh/scripts/canary.mjs +++ b/plugins/dsh/scripts/canary.mjs @@ -196,16 +196,20 @@ check('spill path present', trig.includes('C:/spill/stdout.log')) // --------------------------------------------------------------------------- // 6. argv quoting: spaces and metacharacters cannot break out // --------------------------------------------------------------------------- -console.log('6. argv quoting is shell-safe') -const { buildArgv, quoteArgv } = await import(pathToFileURL(join(packedRoot, 'src/index.js')).href) +console.log('6. argv quoting is shell-safe (platform-aware)') +const { buildArgv, quoteArgv, q, IS_WINDOWS } = await import(pathToFileURL(join(packedRoot, 'src/index.js')).href) // Verify quoting directly: a preference with spaces and metacharacters must stay -// inside one argument (single-quoted, embedded quotes doubled). +// inside one argument (single-quoted, embedded quotes escaped per platform). const argv = buildArgv({}, 'run', { preferences: "never ' rm -rf /" }) const quoted = quoteArgv(argv) const prefArg = argv[argv.indexOf('--preferences') + 1] check('preference stays one argv element', argv.includes('--preferences') && argv[argv.indexOf('--preferences') + 1] === "never ' rm -rf /") -check('quoted form uses bash-safe escape', quoted.includes("'never '\\'' rm -rf /'")) +// pwsh escapes an embedded quote by doubling it (''); bash closes/reopens ('\''). +const expected = IS_WINDOWS ? "'never '' rm -rf /'" : "'never '\\'' rm -rf /'" +check('quoted form uses the host shell escape', quoted.includes(expected), `expected ${expected} got ...${quoted.slice(-40)}`) +check('embedded quote escaped exactly once', (IS_WINDOWS ? quoted.split("''").length - 1 : quoted.split("'\\''").length - 1) === 1) check('no unquoted shell metacharacters', !/;\s*rm\s+-rf/.test(quoted)) +check('q() control chars folded to single spaces', q("a\r\nb\x00c") === "'a b c'") // --------------------------------------------------------------------------- // 7. auto-adopt is OPERATOR-ONLY: the model cannot set it diff --git a/plugins/dsh/src/index.js b/plugins/dsh/src/index.js index 7e07a6e9..bcc7753c 100644 --- a/plugins/dsh/src/index.js +++ b/plugins/dsh/src/index.js @@ -18,7 +18,7 @@ export const name = 'skillopt' const PLUGIN_DIR = dirname(fileURLToPath(import.meta.url)) + '/..' // Exported for the canary test (scripts/canary.mjs). -export { buildArgv, quoteArgv } +export { buildArgv, quoteArgv, q, IS_WINDOWS } // Wait for the tool registry and the shell executor before applying. export const inject = ['tools', 'shell'] @@ -66,20 +66,31 @@ export const Config = Schema.object({ // Helpers // --------------------------------------------------------------------------- -// Quote one argv element for a POSIX shell (bash). Single quotes are literal; -// an embedded single quote is expressed as '\'' (close quote, escaped quote, -// reopen quote) โ€” the only portable POSIX spelling. PowerShell is not a target -// here: dsh's ctx.shell executes via `bash -c` (LocalBashExecutor), so the -// quoting only needs to be bash-correct. +// Quote one argv element for the HOST's shell. dsh's ctx.shell is the +// platform executor: on win32 the bash stack is disabled and ctx.shell is a +// pwsh executor (`pwsh -Command `), on POSIX it is bash (`bash -c +// `). PowerShell and POSIX shell quoting differ, so the quoting +// follows the platform: +// +// - POSIX (bash): single quotes are literal; an embedded single quote is +// expressed as '\'' (close quote, escaped quote, reopen quote) โ€” the only +// portable POSIX spelling. +// - Windows (pwsh): single-quoted strings are literal too, but an embedded +// single quote is expressed as '' (doubled quote). // // Control characters are stripped as defense in depth: \r and \r\n inside a -// single-quoted word would otherwise split the value into multiple argv words +// quoted word would otherwise split the value into multiple argv words // (broken command, not RCE โ€” quotes never execute), and \n would corrupt the // engine's own arg parsing. Model-controlled values must arrive as exactly -// one argument. +// one argument under either shell. +const IS_WINDOWS = process.platform === 'win32' + function q(value) { - const s = String(value).replace(/[\r\n\u0000-\u001f\u007f]/g, ' ') - return `'${s.replace(/'/g, "'\\''")}'` + // Fold \r\n and lone \r into ONE space (not two), then strip remaining C0. + const s = String(value) + .replace(/\r\n?/g, ' ') + .replace(/[\n\u0000-\u001f\u007f]/g, ' ') + return IS_WINDOWS ? `'${s.replace(/'/g, "''")}'` : `'${s.replace(/'/g, "'\\''")}'` } /** @@ -136,7 +147,12 @@ function buildArgv(config, action, explicit = {}, extras = [], allowed = null) { /** Join argv with safe quoting for the platform shell. */ function quoteArgv(argv) { - return argv.map(q).join(' ') + const quoted = argv.map(q).join(' ') + // Windows PowerShell: a bare string is not a command invocation โ€” `'python' + // 'args'` parses as a string-array expression and errors. Prepend the `&` + // call operator so the quoted argv runs as a command (same as bash, where + // the quote is the whole word and the first word is the command). + return IS_WINDOWS ? `& ${quoted}` : quoted } // Pick exactly the parameters a tool declares. dsh's parameter schema does diff --git a/scripts/eval_only.py b/scripts/eval_only.py index 2b14bce9..abe44905 100644 --- a/scripts/eval_only.py +++ b/scripts/eval_only.py @@ -384,6 +384,9 @@ def _set_role(key: str, value: str) -> None: _set_role("optimizer_backend", "codex_exec") _set_role("target_backend", "codex_exec") elif backend == "claude_code_exec": + # Only the target defaults to Claude Code (it produces the SDK trace + # the reflector consumes); the optimizer keeps its configured + # backend so an explicit --optimizer_backend is never clobbered. _set_role("optimizer_backend", "openai_chat") _set_role("target_backend", "claude_code_exec") elif backend == "cursor_exec": @@ -415,6 +418,12 @@ def _set_role(key: str, value: str) -> None: and not _has_model_override("model.optimizer", "optimizer_model") ): cfg["optimizer_model"] = default_model_for_backend("claude_chat") + if cfg.get("optimizer_backend") == "claude_code_exec": + if ( + str(cfg.get("optimizer_model", "") or "").strip() in _OPENAI_DEFAULT_MODEL_SENTINELS + and not _has_model_override("model.optimizer", "optimizer_model") + ): + cfg["optimizer_model"] = default_model_for_backend("claude_code_exec") if cfg.get("target_backend") == "claude_chat": if ( str(cfg.get("target_model", "") or "").strip() in _OPENAI_DEFAULT_MODEL_SENTINELS diff --git a/scripts/train.py b/scripts/train.py index 4d6e79a3..f8f6cbf5 100644 --- a/scripts/train.py +++ b/scripts/train.py @@ -656,6 +656,9 @@ def _set_role(key: str, value: str) -> None: _set_role("optimizer_backend", "codex_exec") _set_role("target_backend", "codex_exec") elif backend == "claude_code_exec": + # Only the target defaults to Claude Code (it produces the SDK trace + # the reflector consumes); the optimizer keeps its configured + # backend so an explicit --optimizer_backend is never clobbered. _set_role("optimizer_backend", "openai_chat") _set_role("target_backend", "claude_code_exec") elif backend == "cursor_exec": @@ -689,6 +692,12 @@ def _set_role(key: str, value: str) -> None: and not _has_model_override("model.optimizer", "optimizer_model") ): flat["optimizer_model"] = default_model_for_backend("claude_chat") + if flat.get("optimizer_backend") == "claude_code_exec": + if ( + str(flat.get("optimizer_model", "") or "").strip() in _OPENAI_DEFAULT_MODEL_SENTINELS + and not _has_model_override("model.optimizer", "optimizer_model") + ): + flat["optimizer_model"] = default_model_for_backend("claude_code_exec") if flat.get("optimizer_backend") == "qwen_chat": if ( str(flat.get("optimizer_model", "") or "").strip() in _OPENAI_DEFAULT_MODEL_SENTINELS diff --git a/skillopt/config.py b/skillopt/config.py index 69e4125a..b73573ae 100644 --- a/skillopt/config.py +++ b/skillopt/config.py @@ -73,6 +73,7 @@ "model.copilot_chat_target_model": "copilot_chat_target_model", "model.copilot_chat_timeout": "copilot_chat_timeout", "model.codex_trace_to_optimizer": "codex_trace_to_optimizer", + "model.claude_trace_to_optimizer": "claude_trace_to_optimizer", "model.azure_endpoint": "azure_endpoint", "model.azure_api_version": "azure_api_version", "model.azure_api_key": "azure_api_key", diff --git a/skillopt/engine/trainer.py b/skillopt/engine/trainer.py index 64e2dfad..520a90eb 100644 --- a/skillopt/engine/trainer.py +++ b/skillopt/engine/trainer.py @@ -453,6 +453,27 @@ def _resolve_train_size(cfg: dict, dataloader) -> int: _ROLE_BACKEND_DEFAULTS = (None, "", "openai_chat") +def _configure_trace_to_optimizer_gates(target_backend: str, cfg: dict) -> None: + """Turn on trace-to-optimizer gates for the exec target's trace artifact. + + Sets ``REFLACT_CODEX_TRACE_TO_OPTIMIZER`` (codex) and + ``REFLACT_CLAUDE_TRACE_TO_OPTIMIZER`` (claude) to ``"1"`` only when the + target actually runs on that exec backend and the matching config knob is + on. ``skillopt.gradient.reflect.fmt_minibatch_trajectories`` reads these + env vars, so a non-exec target never pays the injection. + """ + os.environ["REFLACT_CODEX_TRACE_TO_OPTIMIZER"] = ( + "1" + if target_backend == "codex_exec" and cfg.get("codex_trace_to_optimizer", False) + else "0" + ) + os.environ["REFLACT_CLAUDE_TRACE_TO_OPTIMIZER"] = ( + "1" + if target_backend == "claude_code_exec" and cfg.get("claude_trace_to_optimizer", False) + else "0" + ) + + def _resolve_role_backends( backend: str, optimizer_backend: str | None, target_backend: str | None ) -> tuple[str, str]: @@ -479,6 +500,11 @@ def _resolve_role_backends( if target_backend in _ROLE_BACKEND_DEFAULTS: target_backend = "codex_exec" elif backend == "claude_code_exec": + # Only the *target* defaults to Claude Code (that is what produces the + # SDK trace the reflector consumes). The optimizer keeps its configured + # backend (openai_chat by default) so an explicit --optimizer_backend is + # never silently overridden and existing users' cost profile is + # unchanged. Opt in with --optimizer_backend claude_code_exec. optimizer_backend = optimizer_backend or "openai_chat" if target_backend in _ROLE_BACKEND_DEFAULTS: target_backend = "claude_code_exec" @@ -806,11 +832,7 @@ def _build_eval_env(split: str, env_num: int, seed: int): minimax_model_cfg = cfg.get("minimax_model") if minimax_model_cfg and cfg.get("target_backend") == "minimax_chat": set_target_deployment(str(minimax_model_cfg)) - os.environ["REFLACT_CODEX_TRACE_TO_OPTIMIZER"] = ( - "1" - if target_backend == "codex_exec" and cfg.get("codex_trace_to_optimizer", False) - else "0" - ) + _configure_trace_to_optimizer_gates(target_backend, cfg) reasoning = cfg.get("reasoning_effort", "") or None set_reasoning_effort(reasoning) print( diff --git a/skillopt/gradient/reflect.py b/skillopt/gradient/reflect.py index 8078f852..df68c9b7 100644 --- a/skillopt/gradient/reflect.py +++ b/skillopt/gradient/reflect.py @@ -209,6 +209,19 @@ def fmt_minibatch_trajectories( f"{codex_probe_trace_steps}\n" ) + # Claude Code exec backend (issue #233): the SDK's full session trace is + # persisted as claude_trace_steps.txt; surface it so the analyst sees the + # agent's actual tool activity instead of only the collapsed final answer. + # Gated like the codex summary above: only the trainer turns it on, and + # only when the target actually runs on claude_code_exec. + if os.environ.get("REFLACT_CLAUDE_TRACE_TO_OPTIMIZER", "0") == "1": + claude_steps_path = os.path.join(prediction_dir, tid, "claude_trace_steps.txt") + if os.path.exists(claude_steps_path): + with open(claude_steps_path, encoding="utf-8") as f: + claude_steps = f.read().strip() + if claude_steps: + header += f"\n#### Claude Trace Steps\n{claude_steps}\n" + preview = item.get("spreadsheet_preview", "") if not preview: preview_path = os.path.join(prediction_dir, tid, "spreadsheet_preview.txt") diff --git a/skillopt/model/__init__.py b/skillopt/model/__init__.py index 08aa073d..9033bc98 100644 --- a/skillopt/model/__init__.py +++ b/skillopt/model/__init__.py @@ -6,6 +6,7 @@ from skillopt.model import azure_openai as _openai from skillopt.model import claude_backend as _claude +from skillopt.model import claude_code_backend as _claude_code from skillopt.model import codex_backend as _codex from skillopt.model import copilot_backend as _copilot from skillopt.model import minimax_backend as _minimax @@ -55,6 +56,9 @@ def set_backend(name: str | None) -> str: set_target_backend("codex_exec") return normalized if normalized == "claude_code_exec": + # Only the target defaults to Claude Code (it produces the SDK trace the + # reflector consumes); the optimizer keeps its configured backend unless + # explicitly selected via --optimizer_backend claude_code_exec. set_optimizer_backend("openai_chat") set_target_backend(normalized) return normalized @@ -181,6 +185,16 @@ def chat_optimizer( stage=stage, timeout=timeout, ) + if get_optimizer_backend() == "claude_code_exec": + return _claude_code.chat_optimizer( + system=system, + user=user, + max_completion_tokens=max_completion_tokens, + retries=retries, + stage=stage, + timeout=timeout, + reasoning_effort=reasoning_effort, + ) return _openai.chat_optimizer( system=system, user=user, @@ -346,6 +360,18 @@ def chat_optimizer_messages( return_message=return_message, timeout=timeout, ) + if get_optimizer_backend() == "claude_code_exec": + return _claude_code.chat_optimizer_messages( + messages=messages, + max_completion_tokens=max_completion_tokens, + retries=retries, + stage=stage, + tools=tools, + tool_choice=tool_choice, + return_message=return_message, + timeout=timeout, + reasoning_effort=reasoning_effort, + ) return _openai.chat_optimizer_messages( messages=messages, max_completion_tokens=max_completion_tokens, @@ -509,6 +535,17 @@ def get_token_summary() -> dict: summary[stage]["prompt_tokens"] += values["prompt_tokens"] summary[stage]["completion_tokens"] += values["completion_tokens"] summary[stage]["total_tokens"] += values["total_tokens"] + claude_code_summary = _claude_code.get_token_summary() + for stage, values in claude_code_summary.items(): + if stage == "_total": + continue + if stage not in summary: + summary[stage] = values + continue + summary[stage]["calls"] += values["calls"] + summary[stage]["prompt_tokens"] += values["prompt_tokens"] + summary[stage]["completion_tokens"] += values["completion_tokens"] + summary[stage]["total_tokens"] += values["total_tokens"] qwen_summary = _qwen.get_token_summary() for stage, values in qwen_summary.items(): if stage == "_total": @@ -584,6 +621,7 @@ def get_token_summary() -> dict: def reset_token_tracker() -> None: _openai.reset_token_tracker() _claude.reset_token_tracker() + _claude_code.reset_token_tracker() _qwen.reset_token_tracker() _minimax.reset_token_tracker() _openai_compat.reset_token_tracker() @@ -749,6 +787,7 @@ def configure_openai_compatible( def set_reasoning_effort(effort: str | None) -> None: _openai.set_reasoning_effort(effort) _claude.set_reasoning_effort(effort) + _claude_code.set_reasoning_effort(effort) _qwen.set_reasoning_effort(effort) _minimax.set_reasoning_effort(effort) _openai_compat.set_reasoning_effort(effort) @@ -758,6 +797,7 @@ def set_reasoning_effort(effort: str | None) -> None: def set_target_deployment(deployment: str) -> None: _openai.set_target_deployment(deployment) _claude.set_target_deployment(deployment) + _claude_code.set_target_deployment(deployment) _qwen.set_target_deployment(deployment) _minimax.set_target_deployment(deployment) _openai_compat.set_target_deployment(deployment) @@ -767,6 +807,7 @@ def set_target_deployment(deployment: str) -> None: def set_optimizer_deployment(deployment: str) -> None: _openai.set_optimizer_deployment(deployment) _claude.set_optimizer_deployment(deployment) + _claude_code.set_optimizer_deployment(deployment) _qwen.set_optimizer_deployment(deployment) _openai_compat.set_optimizer_deployment(deployment) _codex.set_optimizer_deployment(deployment) diff --git a/skillopt/model/backend_config.py b/skillopt/model/backend_config.py index 5db7d9e8..8b84d213 100644 --- a/skillopt/model/backend_config.py +++ b/skillopt/model/backend_config.py @@ -133,11 +133,12 @@ def set_optimizer_backend(backend: str) -> None: "openai_compatible", "copilot_chat", "codex_exec", + "claude_code_exec", }: raise ValueError( f"Unsupported optimizer backend: {OPTIMIZER_BACKEND!r}. " "Supported values are 'openai_chat', 'claude_chat', 'qwen_chat', 'minimax_chat', " - "'openai_compatible', 'copilot_chat', and 'codex_exec'." + "'openai_compatible', 'copilot_chat', 'codex_exec', and 'claude_code_exec'." ) os.environ["OPTIMIZER_BACKEND"] = OPTIMIZER_BACKEND @@ -176,6 +177,7 @@ def is_optimizer_chat_backend() -> bool: "openai_compatible", "copilot_chat", "codex_exec", + "claude_code_exec", } diff --git a/skillopt/model/claude_code_backend.py b/skillopt/model/claude_code_backend.py new file mode 100644 index 00000000..7cc626d2 --- /dev/null +++ b/skillopt/model/claude_code_backend.py @@ -0,0 +1,313 @@ +"""Claude Code CLI/SDK chat backend for ReflACT (optimizer role). + +Runs Claude Code (the same CLI/SDK that powers the ``claude_code_exec`` target +backend) as a plain chat model for reflection. This gives the optimizer access +to Claude's full context window, so minibatch trajectories are not truncated by +a narrower chat backend. +""" +from __future__ import annotations + +import json +import os +import time +from typing import Any + +from skillopt.model import codex_backend as _codex +from skillopt.model.claude_backend import _build_prompt_from_messages +from skillopt.model.codex_harness import run_claude_code_chat +from skillopt.model.common import TokenTracker + +OPTIMIZER_DEPLOYMENT = os.environ.get("OPTIMIZER_DEPLOYMENT", "claude-sonnet-4-6") +TARGET_DEPLOYMENT = os.environ.get("TARGET_DEPLOYMENT", "claude-sonnet-4-6") +REASONING_EFFORT: str | None = None +tracker = TokenTracker() + + +def _assistant_message_schema() -> dict[str, Any]: + return _codex._assistant_message_schema() + + +def _compat_message_from_payload( + payload: dict[str, Any], + *, + tool_choice: str | dict[str, Any] | None = None, +): + return _codex._compat_message_from_payload(payload, tool_choice=tool_choice) + + +def _chat_messages_impl( + model: str, + messages: list[dict[str, Any]], + max_completion_tokens: int, + retries: int, + stage: str, + *, + tools: list[dict[str, Any]] | None = None, + tool_choice: str | dict[str, Any] | None = None, + return_message: bool = False, + timeout: int | None = None, + reasoning_effort: str | None = None, +) -> tuple[Any, dict[str, int]]: + del max_completion_tokens # Claude Code does not expose a completion-token cap + last_err = None + structured_output = bool(tools) or return_message + schema = _assistant_message_schema() if structured_output else None + # An explicit per-call effort wins; otherwise fall back to the value set via + # set_reasoning_effort (the `--reasoning_effort` / config path). + effort = reasoning_effort if reasoning_effort is not None else REASONING_EFFORT + + for attempt in range(retries): + try: + system, prompt, attachments = _build_prompt_from_messages( + messages, + tools=tools, + tool_choice=tool_choice, + structured_output=structured_output, + ) + if attachments: + raise RuntimeError( + "claude_code_exec backend does not support image attachments" + ) + raw_text, usage_info = run_claude_code_chat( + system=system, + prompt=prompt, + model=model, + timeout=timeout, + schema=schema, + effort=effort, + ) + tracker.record( + stage, + usage_info["prompt_tokens"], + usage_info["completion_tokens"], + ) + if not structured_output: + return raw_text, usage_info + payload = json.loads(raw_text) + compat = _compat_message_from_payload(payload, tool_choice=tool_choice) + return (compat if return_message else compat.content), usage_info + except Exception as exc: # noqa: BLE001 + last_err = exc + time.sleep(min(2 ** attempt, 30)) + + raise RuntimeError(f"Claude Code call failed after {retries} retries: {last_err}") + + +def chat_with_model( + model: str, + system: str, + user: str, + max_completion_tokens: int = 16384, + retries: int = 5, + stage: str = "custom", + timeout: int | None = None, + reasoning_effort: str | None = None, +) -> tuple[str, dict[str, int]]: + messages = [ + {"role": "system", "content": system}, + {"role": "user", "content": user}, + ] + return _chat_messages_impl( + model, + messages, + max_completion_tokens, + retries, + stage, + timeout=timeout, + reasoning_effort=reasoning_effort, + ) + + +def chat_messages_with_model( + model: str, + messages: list[dict[str, Any]], + max_completion_tokens: int = 16384, + retries: int = 5, + stage: str = "custom", + *, + tools: list[dict[str, Any]] | None = None, + tool_choice: str | dict[str, Any] | None = None, + return_message: bool = False, + timeout: int | None = None, + reasoning_effort: str | None = None, +) -> tuple[Any, dict[str, int]]: + return _chat_messages_impl( + model, + messages, + max_completion_tokens, + retries, + stage, + tools=tools, + tool_choice=tool_choice, + return_message=return_message, + timeout=timeout, + reasoning_effort=reasoning_effort, + ) + + +def chat_optimizer( + system: str, + user: str, + max_completion_tokens: int = 16384, + retries: int = 5, + stage: str = "optimizer", + timeout: int | None = None, + reasoning_effort: str | None = None, +) -> tuple[str, dict[str, int]]: + return chat_with_model( + model=OPTIMIZER_DEPLOYMENT, + system=system, + user=user, + max_completion_tokens=max_completion_tokens, + retries=retries, + stage=stage, + timeout=timeout, + reasoning_effort=reasoning_effort, + ) + + +def chat_target( + system: str, + user: str, + max_completion_tokens: int = 16384, + retries: int = 5, + stage: str = "target", + timeout: int | None = None, + reasoning_effort: str | None = None, +) -> tuple[str, dict[str, int]]: + return chat_with_model( + model=TARGET_DEPLOYMENT, + system=system, + user=user, + max_completion_tokens=max_completion_tokens, + retries=retries, + stage=stage, + timeout=timeout, + reasoning_effort=reasoning_effort, + ) + + +def chat_with_deployment( + deployment: str, + system: str, + user: str, + max_completion_tokens: int = 16384, + retries: int = 5, + stage: str = "custom", + timeout: int | None = None, + reasoning_effort: str | None = None, +) -> tuple[str, dict[str, int]]: + return chat_with_model( + model=deployment, + system=system, + user=user, + max_completion_tokens=max_completion_tokens, + retries=retries, + stage=stage, + timeout=timeout, + reasoning_effort=reasoning_effort, + ) + + +def chat_optimizer_messages( + messages: list[dict[str, Any]], + max_completion_tokens: int = 16384, + retries: int = 5, + stage: str = "optimizer", + *, + tools: list[dict[str, Any]] | None = None, + tool_choice: str | dict[str, Any] | None = None, + return_message: bool = False, + timeout: int | None = None, + reasoning_effort: str | None = None, +) -> tuple[Any, dict[str, int]]: + return _chat_messages_impl( + OPTIMIZER_DEPLOYMENT, + messages, + max_completion_tokens, + retries, + stage, + tools=tools, + tool_choice=tool_choice, + return_message=return_message, + timeout=timeout, + reasoning_effort=reasoning_effort, + ) + + +def chat_target_messages( + messages: list[dict[str, Any]], + max_completion_tokens: int = 16384, + retries: int = 5, + stage: str = "target", + *, + tools: list[dict[str, Any]] | None = None, + tool_choice: str | dict[str, Any] | None = None, + return_message: bool = False, + timeout: int | None = None, + reasoning_effort: str | None = None, +) -> tuple[Any, dict[str, int]]: + return _chat_messages_impl( + TARGET_DEPLOYMENT, + messages, + max_completion_tokens, + retries, + stage, + tools=tools, + tool_choice=tool_choice, + return_message=return_message, + timeout=timeout, + reasoning_effort=reasoning_effort, + ) + + +def chat_messages_with_deployment( + deployment: str, + messages: list[dict[str, Any]], + max_completion_tokens: int = 16384, + retries: int = 5, + stage: str = "custom", + *, + tools: list[dict[str, Any]] | None = None, + tool_choice: str | dict[str, Any] | None = None, + return_message: bool = False, + timeout: int | None = None, + reasoning_effort: str | None = None, +) -> tuple[Any, dict[str, int]]: + return _chat_messages_impl( + deployment, + messages, + max_completion_tokens, + retries, + stage, + tools=tools, + tool_choice=tool_choice, + return_message=return_message, + timeout=timeout, + reasoning_effort=reasoning_effort, + ) + + +def get_token_summary() -> dict[str, dict[str, int]]: + return tracker.summary() + + +def reset_token_tracker() -> None: + tracker.reset() + + +def set_reasoning_effort(effort: str | None) -> None: + global REASONING_EFFORT + REASONING_EFFORT = effort if effort else None + + +def set_target_deployment(deployment: str) -> None: + global TARGET_DEPLOYMENT + TARGET_DEPLOYMENT = deployment + os.environ["TARGET_DEPLOYMENT"] = deployment + + +def set_optimizer_deployment(deployment: str) -> None: + global OPTIMIZER_DEPLOYMENT + OPTIMIZER_DEPLOYMENT = deployment + os.environ["OPTIMIZER_DEPLOYMENT"] = deployment diff --git a/skillopt/model/codex_harness.py b/skillopt/model/codex_harness.py index 056539bc..70bb7151 100644 --- a/skillopt/model/codex_harness.py +++ b/skillopt/model/codex_harness.py @@ -7,6 +7,7 @@ import re import shutil import subprocess +import tempfile import threading import traceback import warnings @@ -287,7 +288,7 @@ def _persist_artifacts( response: str, prefix: str, summary_builder, -) -> None: +) -> str: pred_dir = os.path.dirname(work_dir.rstrip(os.sep)) raw_path = os.path.join(pred_dir, f"{prefix}_raw.txt") summary_path = os.path.join(pred_dir, f"{prefix}_trace_summary.txt") @@ -302,6 +303,7 @@ def _persist_artifacts( f.write(combined_raw) with open(summary_path, "w", encoding="utf-8") as f: f.write(summary_builder(combined_raw, response)) + return combined_raw def _persist_codex_artifacts(work_dir: str, raw: str, response: str) -> None: @@ -315,13 +317,22 @@ def _persist_codex_artifacts(work_dir: str, raw: str, response: str) -> None: def _persist_claude_artifacts(work_dir: str, raw: str, response: str) -> None: - _persist_artifacts( + combined_raw = _persist_artifacts( work_dir=work_dir, raw=raw, response=response, prefix="claude", summary_builder=_build_claude_trace_summary, ) + # Structured trace steps for the reflector (issue #233): expose what the + # agent actually did, not just the collapsed final answer. Format from the + # *combined* raw (across turns) and write unconditionally so a turn that + # parses to no steps never leaves the previous turn's stale file behind. + steps_text = format_claude_trace_steps(combined_raw) + pred_dir = os.path.dirname(work_dir.rstrip(os.sep)) + steps_path = os.path.join(pred_dir, "claude_trace_steps.txt") + with open(steps_path, "w", encoding="utf-8") as f: + f.write(steps_text) def _persist_cursor_artifacts(work_dir: str, raw: str, response: str) -> None: @@ -431,6 +442,129 @@ def extract_codex_trace_prefix(raw: str, *, after_step: int) -> str: return "\n".join(lines[:end_line]).strip() +# โ”€โ”€ Claude Code trace steps (SDK messages โ†’ compact steps) โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ +# The Claude Code SDK serializes its full session into ``messages``: most +# entries are bookkeeping (init / thinking_tokens), the rest are assistant text, +# tool calls, and tool results. Flatten those into numbered steps so the +# reflector can see what the agent actually did without paying the full raw +# payload size. + + +def _claude_step_truncate(text: str, limit: int) -> str: + text = str(text or "").strip() + if len(text) <= limit: + return text + return text[:limit] + f"...[+{len(text) - limit} chars]" + + +def _summarize_claude_tool_call(name: str, input_data: Any) -> str: + name = str(name or "") + if isinstance(input_data, dict): + if name == "Read": + return f"Read {input_data.get('file_path', '')}" + if name == "Glob": + return f"Glob {input_data.get('pattern', '')}" + if name == "Grep": + return f"Grep {input_data.get('pattern', '')}" + if name == "Bash": + return f"Bash {input_data.get('command', '')}" + return _claude_step_truncate(f"{name} {json.dumps(input_data, ensure_ascii=False)}", 500) + + +def _iter_claude_json_blocks(raw: str): + """Yield each JSON object embedded in ``raw``. + + ``run_claude_code_exec`` prefixes every attempt with a + ``===== CLAUDE ... ATTEMPT n =====`` header, so the persisted payload is not + a single JSON document. Split on those headers and parse each block. + """ + for chunk in re.split(r"(?m)^={5,}.*={5,}\s*$", raw or ""): + chunk = chunk.strip() + if not chunk: + continue + try: + yield json.loads(chunk) + except json.JSONDecodeError: + continue + + +def parse_claude_trace_steps(raw: str) -> list[dict]: + """Parse serialized Claude Code SDK messages into ordered, compact steps. + + Returns a list of ``{"index", "type", "summary"}`` dicts where ``type`` is + one of ``text`` / ``tool_call`` / ``tool_result``. System bookkeeping + events (init, thinking tokens) are dropped; tool results are truncated to + keep the trace small. + """ + steps: list[dict] = [] + for block in _iter_claude_json_blocks(raw): + messages = block.get("messages") if isinstance(block, dict) else None + if not isinstance(messages, list): + continue + for message in messages: + if not isinstance(message, dict): + continue + if message.get("subtype") in {"init", "thinking_tokens"}: + continue + data = message.get("data") + if isinstance(data, dict) and data.get("type") == "system": + continue + content = message.get("content") + if not isinstance(content, list): + # Terminal result message carries the final text. + text = str(message.get("result") or "").strip() + if text: + steps.append({"type": "text", "summary": _claude_step_truncate(text, 500)}) + continue + for item in content: + if not isinstance(item, dict): + continue + if "name" in item and "input" in item: + steps.append({ + "type": "tool_call", + "summary": _summarize_claude_tool_call(item.get("name"), item.get("input")), + }) + elif "tool_use_id" in item: + body = item.get("content") + if isinstance(body, list): + text_parts: list[str] = [] + for part in body: + if isinstance(part, dict): + # Anthropic content blocks carry their payload + # under ``text`` (tool_result content is + # ``[{"type": "text", "text": "..."}]``), not + # ``content``. + part_text = part.get("text") + if isinstance(part_text, str): + text_parts.append(part_text) + elif isinstance(part, str): + text_parts.append(part) + body = "\n".join(text_parts) + summary = _claude_step_truncate(body, 200) + if item.get("is_error"): + summary = f"[error] {summary}" + steps.append({"type": "tool_result", "summary": summary}) + else: + text = item.get("text") + if isinstance(text, str) and text.strip(): + steps.append({"type": "text", "summary": _claude_step_truncate(text, 500)}) + for index, step in enumerate(steps, 1): + step["index"] = index + return steps + + +def format_claude_trace_steps(raw: str, *, max_chars: int = 4000) -> str: + """Render parsed Claude Code SDK trace into numbered compact steps.""" + steps = parse_claude_trace_steps(raw) + if not steps: + return "" + rendered = [f"[{step['index']}] {step['type']}: {step['summary']}" for step in steps] + text = "\n".join(rendered) + if len(text) > max_chars: + text = text[:max_chars] + "\n...[claude trace steps truncated]..." + return text + + _DENIED_DATA_DIR_NAMES = {"officeqa_split", "sealqa_split"} @@ -859,6 +993,245 @@ def run_claude_code_exec( return last_response, combined +# โ”€โ”€ Claude Code *chat* mode (optimizer role) โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ +# The functions above run Claude Code as the *target* exec backend: they embed +# the target preamble, force the ANSWER_SCHEMA structured output, and read from +# a prepared workspace. When Claude Code is instead selected as the optimizer +# backend (claude_code_exec), reflection calls need a plain-text model call with +# the analyst's own system prompt and no tooling โ€” mirroring claude_backend's +# chat path but driven through the same Claude Code CLI/SDK as the target. + + +def _claude_chat_text_from_messages(messages: list[Any]) -> str: + """Extract the final assistant text from SDK chat-mode messages. + + With ``output_format={"type": "text"}`` the SDK ends with a result message + whose ``result`` holds the final text; fall back to the last text block of + the final assistant message. + """ + for msg in reversed(messages): + result = getattr(msg, "result", None) + if isinstance(result, str) and result.strip(): + return result + content = getattr(msg, "content", None) + if content is None and isinstance(msg, dict): + content = msg.get("content") + if not isinstance(content, list): + continue + for item in content: + text = item.get("text") if isinstance(item, dict) else getattr(item, "text", None) + if isinstance(text, str) and text.strip(): + return text + return "" + + +def _claude_chat_usage_from_event(event: Any) -> dict[str, int]: + """Convert an SDK/CLI result usage payload into the shared usage shape.""" + usage = getattr(event, "usage", {}) if not isinstance(event, dict) else (event or {}).get("usage", {}) + if isinstance(usage, dict): + input_tokens = int(usage.get("input_tokens", 0) or 0) + output_tokens = int(usage.get("output_tokens", 0) or 0) + else: + input_tokens = int(getattr(usage, "input_tokens", 0) or 0) + output_tokens = int(getattr(usage, "output_tokens", 0) or 0) + return { + "prompt_tokens": input_tokens, + "completion_tokens": output_tokens, + "total_tokens": input_tokens + output_tokens, + } + + +def _run_claude_code_sdk_chat_exec( + *, + system: str, + prompt: str, + model: str, + timeout: int, + schema: dict[str, Any] | None = None, + effort: str | None = None, +) -> tuple[str, dict]: + from claude_agent_sdk import ClaudeAgentOptions, ClaudeSDKClient + + async def _query() -> tuple[str, dict]: + # Optimizer chat call: no workspace, no tools, optional schema. + with tempfile.TemporaryDirectory(prefix="skillopt_claude_code_chat_") as tmp: + system_prompt: dict[str, Any] = { + "type": "preset", + "preset": "claude_code", + "append": system or "", + } + kwargs: dict[str, Any] = { + "system_prompt": system_prompt, + "output_format": ( + {"type": "json_schema", "schema": schema} + if schema is not None + else {"type": "text"} + ), + "tools": [], + "cwd": tmp, + "permission_mode": "bypassPermissions", + } + config = get_claude_code_exec_config() + effort_value = _claude_effort(effort if effort is not None else config.get("effort")) + if effort_value: + kwargs["effort"] = effort_value + max_thinking_tokens = int(config.get("max_thinking_tokens", 0) or 0) + if max_thinking_tokens > 0: + kwargs["max_thinking_tokens"] = max_thinking_tokens + options = ClaudeAgentOptions(**kwargs) + if model: + options.model = model.split("/", 1)[1] if model.startswith("anthropic/") else model + + messages = [] + async with ClaudeSDKClient(options) as client: + await client.query(prompt) + messages = [msg async for msg in client.receive_response()] + last = messages[-1] if messages else None + if schema is not None: + payload = _extract_claude_structured_output(messages) + text = _json_dumps(payload) if isinstance(payload, dict) else "" + if not text: + result = getattr(last, "result", None) + if isinstance(result, str) and result.strip(): + text = result + else: + text = _claude_chat_text_from_messages(messages) + usage_info = _claude_chat_usage_from_event(last) + return text, usage_info + + return _run_async(asyncio.wait_for(_query(), timeout=timeout)) + + +def _run_claude_code_cli_chat_exec( + *, + system: str, + prompt: str, + model: str, + timeout: int, + schema: dict[str, Any] | None = None, + effort: str | None = None, +) -> tuple[str, dict]: + config = get_claude_code_exec_config() + cmd = [ + str(config["path"]), + "-p", + "--output-format", + "json", + "--permission-mode", + "dontAsk", + "--tools", + "", + ] + if model: + cmd.extend(["--model", model]) + if schema is not None: + cmd.extend(["--json-schema", json.dumps(schema, ensure_ascii=False)]) + if config.get("profile"): + cmd.extend(["--settings", '{"env":{"CLAUDE_CODE_USE_BEDROCK":"0"}}']) + cmd.extend(["--append-system-prompt", f"Profile: {config['profile']}"]) + effort_value = _claude_effort(effort if effort is not None else config.get("effort")) + if effort_value: + cmd.extend(["--effort", effort_value]) + + with tempfile.TemporaryDirectory(prefix="skillopt_claude_code_chat_") as tmp: + # System prompt via file, not argv, to avoid the Windows argv cap. + system_path = os.path.join(tmp, "system_prompt.txt") + with open(system_path, "w", encoding="utf-8") as system_fh: + system_fh.write(system or "") + cmd.extend(["--append-system-prompt-file", system_path]) + proc = subprocess.run( + cmd, + input=prompt, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=timeout or 300, + cwd=tmp, + ) + + stderr_text = (proc.stderr or "").strip() + if proc.returncode != 0: + raise RuntimeError(stderr_text or f"Claude Code CLI exited with code {proc.returncode}") + stream = [] + for raw_line in (proc.stdout or "").splitlines(): + raw_line = raw_line.strip() + if not raw_line: + continue + try: + stream.append(json.loads(raw_line)) + except json.JSONDecodeError: + continue + result_event = None + for event in reversed(stream): + if event.get("type") == "result": + result_event = event + break + if result_event is None: + raise RuntimeError("Claude Code CLI did not return a result event.") + text = str(result_event.get("result") or result_event.get("content") or "") + usage_info = _claude_chat_usage_from_event(result_event) + return text, usage_info + + +def run_claude_code_chat( + *, + system: str, + prompt: str, + model: str, + timeout: int, + schema: dict[str, Any] | None = None, + effort: str | None = None, +) -> tuple[str, dict]: + """Run Claude Code as a plain chat model (optimizer role). + + ``effort`` overrides the configured ``claude_code_exec_effort``; when ``None`` + the config value (default "medium") is used, matching the target-exec path. + """ + config = get_claude_code_exec_config() + mode = _sdk_mode(config.get("use_sdk")) + retries = int(config.get("empty_response_retries", 0) or 0) + last_text = "" + last_usage: dict[str, int] = {"prompt_tokens": 0, "completion_tokens": 0, "total_tokens": 0} + + for _attempt in range(retries + 1): + if mode != "cli": + try: + text, usage_info = _run_claude_code_sdk_chat_exec( + system=system, + prompt=prompt, + model=model, + timeout=timeout, + schema=schema, + effort=effort, + ) + last_text = text + last_usage = usage_info + if text.strip(): + return text, usage_info + except (ImportError, ModuleNotFoundError): + if mode == "sdk": + raise + except Exception: # noqa: BLE001 + if mode == "sdk": + raise + if mode != "sdk": + text, usage_info = _run_claude_code_cli_chat_exec( + system=system, + prompt=prompt, + model=model, + timeout=timeout, + schema=schema, + effort=effort, + ) + last_text = text + last_usage = usage_info + if text.strip(): + return text, usage_info + + return last_text, last_usage + + def _run_codex_sdk_exec( *, work_dir: str, diff --git a/skillopt/model/common.py b/skillopt/model/common.py index d3f4df52..cb91f415 100644 --- a/skillopt/model/common.py +++ b/skillopt/model/common.py @@ -29,7 +29,7 @@ # deployments (not the CLI model, which uses the copilot_chat model keys) # and keeps the openai_chat optimizer for copilot_exec non-empty. "qwen_chat": "Qwen/Qwen3.5-4B", - "minimax_chat": "MiniMax-M2.7", + "minimax_chat": "MiniMax-M3", "openai_compatible": "gpt-4o-mini", } diff --git a/skillopt/model/minimax_backend.py b/skillopt/model/minimax_backend.py index 596924c6..9736a533 100644 --- a/skillopt/model/minimax_backend.py +++ b/skillopt/model/minimax_backend.py @@ -70,6 +70,34 @@ def base_url_for_region(region: str | None) -> str: default_model_for_backend("minimax_chat"), ) +# Models whose thinking cannot actually be turned off. Per MiniMax's +# OpenAI-compatible docs the M2.x family accepts ``{"type": "disabled"}`` but +# keeps thinking on regardless, so sending "disabled" there is a lie we would +# then have to reason about downstream. Send the honest value instead. +_ALWAYS_THINKING_PREFIXES: tuple[str, ...] = ("MiniMax-M2",) + + +def _thinking_is_forced(deployment: str) -> bool: + """True when ``deployment`` cannot honor ``thinking: {"type": "disabled"}``.""" + name = str(deployment or "").strip() + return any(name.startswith(prefix) for prefix in _ALWAYS_THINKING_PREFIXES) + + +def _resolve_thinking_type(deployment: str) -> str: + """Return the documented top-level ``thinking.type`` for ``deployment``. + + MiniMax documents thinking control as a top-level ``thinking`` object -- + ``{"thinking": {"type": "adaptive"}}`` or ``{"thinking": {"type": + "disabled"}}`` -- NOT as ``chat_template_kwargs.enable_thinking``, which is + a Qwen/HuggingFace-serving convention that this endpoint simply ignores. + Unknown deployments are treated as capable of adaptive thinking, matching + the API default (thinking on when the parameter is omitted). + """ + if _thinking_is_forced(deployment): + return "adaptive" + return "adaptive" if ENABLE_THINKING else "disabled" + + _config_lock = threading.Lock() tracker = TokenTracker() @@ -177,7 +205,9 @@ def _chat_messages_impl( "messages": _json_safe(messages), "max_tokens": min(max_completion_tokens, MAX_TOKENS), } - payload["chat_template_kwargs"] = {"enable_thinking": ENABLE_THINKING} + payload["thinking"] = { + "type": _resolve_thinking_type(deployment or TARGET_DEPLOYMENT) + } if TEMPERATURE is not None: payload["temperature"] = TEMPERATURE if tools: diff --git a/skillopt_sleep/backend.py b/skillopt_sleep/backend.py index d2ca2260..99e41f41 100644 --- a/skillopt_sleep/backend.py +++ b/skillopt_sleep/backend.py @@ -1735,7 +1735,12 @@ def _call(self, prompt: str, *, max_tokens: int = 1024) -> str: "--stream", "off", "--no-color", "--log-level", "none", + # ``--allow-all-tools`` is REQUIRED for non-interactive mode (it waives + # the approval prompt); it is the permission axis. Tool *visibility* is + # a separate axis: ``--available-tools`` restricts which tools the model + # can see at all. Scoping happens there, so we keep both. "--allow-all-tools", + "--available-tools", os.environ.get("COPILOT_AVAILABLE_TOOLS", "bash"), "-C", clean_cwd, ] if not self.full_env: @@ -1873,7 +1878,13 @@ def attempt_with_tools(self, task, skill, memory, tools): "--stream", "off", "--no-color", "--log-level", "none", + # ``--allow-all-tools`` is REQUIRED for non-interactive mode (it + # waives the approval prompt); it is the permission axis. Tool + # *visibility* is a separate axis: ``--available-tools`` restricts + # which tools the model can see at all. Scoping happens there, so + # we keep both. "--allow-all-tools", + "--available-tools", os.environ.get("COPILOT_AVAILABLE_TOOLS", "bash"), "-C", work, ] if not self.full_env: diff --git a/skillopt_sleep/evalkit.py b/skillopt_sleep/evalkit.py index 8a09dd72..753990e7 100644 --- a/skillopt_sleep/evalkit.py +++ b/skillopt_sleep/evalkit.py @@ -7,7 +7,7 @@ * one fixed task manifest, paired by task id * McNemar's test on per-task binary outcomes * percentile-bootstrap confidence intervals on the success-rate delta - * optional multi-seed repeats (per-seed deltas + a pooled pair test) + * optional multi-seed repeats with task-cluster inference It does not change the nightly gate. It standardizes the evidence that reports and PRs cite. Pure stdlib; no numpy / scipy. @@ -29,13 +29,47 @@ from dataclasses import asdict, dataclass, field from typing import Any, Dict, Iterable, List, Mapping, Optional, Sequence, Tuple - # โ”€โ”€ errors โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ class EvalkitError(ValueError): """User-facing contract failure (mismatched ids, empty, etc.).""" +MAX_BOOTSTRAPS = 1_000_000 + + +def _validate_alpha(alpha: float) -> float: + if isinstance(alpha, bool): + raise EvalkitError("alpha must be a finite number strictly between 0 and 1") + try: + value = float(alpha) + except (TypeError, ValueError, OverflowError): + raise EvalkitError("alpha must be a finite number strictly between 0 and 1") from None + if not math.isfinite(value) or not 0.0 < value < 1.0: + raise EvalkitError("alpha must be a finite number strictly between 0 and 1") + return value + + +def _validate_bootstraps(n_boot: int) -> int: + if isinstance(n_boot, bool) or not isinstance(n_boot, int): + raise EvalkitError("n_boot must be an integer") + if not 1 <= n_boot <= MAX_BOOTSTRAPS: + raise EvalkitError(f"n_boot must be between 1 and {MAX_BOOTSTRAPS}") + return n_boot + + +def _validate_seed(seed: int) -> int: + if isinstance(seed, bool) or not isinstance(seed, int): + raise EvalkitError("seed must be an integer") + return seed + + +def _validate_count(name: str, value: int) -> int: + if isinstance(value, bool) or not isinstance(value, int) or value < 0: + raise EvalkitError(f"{name} must be a non-negative integer") + return value + + # โ”€โ”€ results โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ @dataclass @@ -104,19 +138,45 @@ def _chi2_sf_df1(x: float) -> float: def _binom_pmf(k: int, n: int, p: float = 0.5) -> float: if k < 0 or k > n: return 0.0 - # nCk * p^k * (1-p)^(n-k). For p=0.5 this is nCk / 2^n. + # Retained for callers/tests that need one PMF value. Use log-gamma so the + # integer binomial coefficient is never coerced to an overflowing float. if p == 0.5: - return math.comb(n, k) / float(1 << n) if n < 1024 else math.comb(n, k) * (0.5 ** n) - return math.comb(n, k) * (p ** k) * ((1.0 - p) ** (n - k)) + log_pmf = ( + math.lgamma(n + 1) + - math.lgamma(k + 1) + - math.lgamma(n - k + 1) + - n * math.log(2.0) + ) + return math.exp(log_pmf) + if not 0.0 < p < 1.0: + raise EvalkitError("binomial p must be strictly between 0 and 1") + log_pmf = ( + math.lgamma(n + 1) + - math.lgamma(k + 1) + - math.lgamma(n - k + 1) + + k * math.log(p) + + (n - k) * math.log1p(-p) + ) + return math.exp(log_pmf) def exact_mcnemar_p(b: int, c: int) -> float: """Two-sided exact McNemar p-value (binomial test of discordants, p=0.5).""" + b = _validate_count("b", b) + c = _validate_count("c", c) n = b + c if n == 0: return 1.0 k = min(b, c) - tail = sum(_binom_pmf(i, n, 0.5) for i in range(0, k + 1)) + # Start at the largest term in the requested lower tail, then recur + # downward. This avoids both the enormous int-to-float conversion in + # comb(n, k) * 0.5**n and loss from starting at an underflowed 2**-n. + term = _binom_pmf(k, n, 0.5) + terms = [term] + for i in range(k, 0, -1): + term *= i / (n - i + 1) + terms.append(term) + tail = math.fsum(terms) return min(1.0, 2.0 * tail) @@ -128,6 +188,11 @@ def mcnemar_from_counts( *, alpha: float = 0.05, ) -> McNemarResult: + alpha = _validate_alpha(alpha) + both_success = _validate_count("both_success", both_success) + a_only = _validate_count("a_only", a_only) + b_only = _validate_count("b_only", b_only) + both_fail = _validate_count("both_fail", both_fail) n = both_success + a_only + b_only + both_fail disc = a_only + b_only if disc == 0: @@ -151,10 +216,13 @@ def mcnemar_from_counts( def mcnemar_paired(a: Sequence[int], b: Sequence[int], *, alpha: float = 0.05) -> McNemarResult: - if len(a) != len(b): - raise EvalkitError("McNemar requires equal-length paired outcomes") + alpha = _validate_alpha(alpha) + if len(a) != len(b) or not a: + raise EvalkitError("McNemar requires a non-empty, equal-length paired sample") bs = ao = bo = bf = 0 for x, y in zip(a, b): + if _as_binary(x) is None or _as_binary(y) is None: + raise EvalkitError("McNemar outcomes must be binary 0/1 values") if x and y: bs += 1 elif x and not y: @@ -174,17 +242,27 @@ def bootstrap_delta_ci( seed: int = 42, alpha: float = 0.05, ) -> BootstrapCI: + alpha = _validate_alpha(alpha) + n_boot = _validate_bootstraps(n_boot) + seed = _validate_seed(seed) if len(a) != len(b) or not a: raise EvalkitError("bootstrap requires a non-empty paired sample") - if n_boot < 1: - raise EvalkitError("n_boot must be >= 1") + numeric_a: List[float] = [] + numeric_b: List[float] = [] + try: + numeric_a = [float(x) for x in a] + numeric_b = [float(x) for x in b] + except (TypeError, ValueError, OverflowError): + raise EvalkitError("bootstrap scores must be numeric and finite") from None + if any(not math.isfinite(x) for x in numeric_a + numeric_b): + raise EvalkitError("bootstrap scores must be numeric and finite") rng = random.Random(seed) n = len(a) deltas: List[float] = [] for _ in range(n_boot): idx = [rng.randrange(n) for _ in range(n)] - da = sum(a[i] for i in idx) / n - db = sum(b[i] for i in idx) / n + da = sum(numeric_a[i] for i in idx) / n + db = sum(numeric_b[i] for i in idx) / n deltas.append(db - da) deltas.sort() # Inclusive percentile on the sorted sample. @@ -214,15 +292,31 @@ def _as_binary(value: Any) -> Optional[int]: def _normalize_outcomes(raw: Mapping[str, Any]) -> Dict[str, List[float]]: """Map task id -> list of per-seed scores (length 1 if unseeded).""" + if not isinstance(raw, Mapping): + raise EvalkitError("outcomes must be a JSON object keyed by task id") out: Dict[str, List[float]] = {} for tid, val in raw.items(): key = str(tid) + if key in out: + raise EvalkitError(f"duplicate outcome task id after normalization: {key}") if isinstance(val, Mapping) and "seeds" in val: val = val["seeds"] if isinstance(val, (list, tuple)): - out[key] = [float(x) for x in val] + values = list(val) else: - out[key] = [float(val)] + values = [val] + if not values: + raise EvalkitError(f"task {key} has an empty seed list") + normalized: List[float] = [] + for item in values: + try: + score = float(item) + except (TypeError, ValueError, OverflowError): + raise EvalkitError(f"task {key} contains a non-numeric score") from None + if not math.isfinite(score) or not 0.0 <= score <= 1.0: + raise EvalkitError(f"task {key} scores must be finite and between 0 and 1") + normalized.append(score) + out[key] = normalized return out @@ -284,10 +378,16 @@ def reconstruct_paired_from_rates(n: int, rate_a: float, rate_b: float) -> Tuple First ``round(n * rate)`` tasks succeed in each condition, same id order. This is a published-rate replay convention, not original microdata. """ - if n < 1: + if isinstance(n, bool) or not isinstance(n, int) or n < 1: raise EvalkitError("n must be >= 1") - ka = int(round(n * rate_a)) - kb = int(round(n * rate_b)) + try: + numeric_a, numeric_b = float(rate_a), float(rate_b) + except (TypeError, ValueError, OverflowError): + raise EvalkitError("rates must be numeric, finite, and between 0 and 1") from None + if not all(math.isfinite(rate) and 0.0 <= rate <= 1.0 for rate in (numeric_a, numeric_b)): + raise EvalkitError("rates must be finite and between 0 and 1") + ka = int(round(n * numeric_a)) + kb = int(round(n * numeric_b)) a = [1 if i < ka else 0 for i in range(n)] b = [1 if i < kb else 0 for i in range(n)] return a, b @@ -303,6 +403,8 @@ def compare( seed: int = 42, allow_graded: bool = False, ) -> EvalReport: + alpha = _validate_alpha(alpha) + n_boot = _validate_bootstraps(n_boot) ids, a_rows, b_rows = align_pairs(manifest_ids, outcomes_a, outcomes_b) n_seed = len(a_rows[0]) notes: List[str] = [] @@ -317,11 +419,20 @@ def compare( binary = _is_binary_matrix(a_rows) and _is_binary_matrix(b_rows) mcnemar: Optional[McNemarResult] = None - if binary: - # Pool (task, seed) as paired observations when seeds align. - flat_a = [int(_as_binary(x) or 0) for row in a_rows for x in row] - flat_b = [int(_as_binary(x) or 0) for row in b_rows for x in row] - mcnemar = mcnemar_paired(flat_a, flat_b, alpha=alpha) + if binary and n_seed == 1: + # One independent binary observation per task: McNemar's intended unit. + mcnemar = mcnemar_paired( + [int(_as_binary(row[0]) or 0) for row in a_rows], + [int(_as_binary(row[0]) or 0) for row in b_rows], + alpha=alpha, + ) + elif binary: + # Repeated seeds within a task are clustered measurements, not + # independent observations. The task-level bootstrap above is the + # inferential result; pooling here would create pseudoreplication. + notes.append( + "multi-seed binary scores: McNemar omitted; task-cluster bootstrap CI is authoritative" + ) elif allow_graded: notes.append("graded scores: McNemar omitted; bootstrap CI only") else: @@ -335,7 +446,7 @@ def compare( for s in range(n_seed): da = _mean(row[s] for row in a_rows) db = _mean(row[s] for row in b_rows) - per_seed.append({"seed": float(s), "rate_a": da, "rate_b": db, "delta": db - da}) + per_seed.append({"seed": s, "rate_a": da, "rate_b": db, "delta": db - da}) deltas = [row["delta"] for row in per_seed] seed_mean = _mean(deltas) seed_sd = _sd(deltas) @@ -363,17 +474,20 @@ def compare_aa( outcomes: Mapping[str, Any], **kwargs: Any, ) -> EvalReport: - """A/A calibration: identical conditions must not reject at alpha.""" + """A/A identity smoke check: identical conditions must not reject.""" report = compare(manifest_ids, outcomes, outcomes, **kwargs) - report.notes.append("A/A calibration (identical conditions)") + report.notes.append("A/A identity smoke check (identical conditions)") return report # โ”€โ”€ I/O โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ def _load_json(path: str) -> Any: - with open(path, encoding="utf-8") as f: - return json.load(f) + try: + with open(path, encoding="utf-8") as f: + return json.load(f) + except (json.JSONDecodeError, UnicodeError) as exc: + raise EvalkitError(f"invalid JSON in {path}: {exc}") from None def _manifest_ids(obj: Any) -> List[str]: @@ -381,17 +495,37 @@ def _manifest_ids(obj: Any) -> List[str]: return [str(x) for x in obj] if isinstance(obj, Mapping): if "ids" in obj: - return [str(x) for x in obj["ids"]] + values = obj["ids"] + if not isinstance(values, list): + raise EvalkitError("manifest ids must be a JSON array") + return [str(x) for x in values] if "tasks" in obj: - return [str(t["id"] if isinstance(t, Mapping) else t) for t in obj["tasks"]] + tasks = obj["tasks"] + if not isinstance(tasks, list): + raise EvalkitError("manifest tasks must be a JSON array") + ids: List[str] = [] + for task in tasks: + if isinstance(task, Mapping): + if "id" not in task: + raise EvalkitError("every manifest task object must contain id") + ids.append(str(task["id"])) + else: + ids.append(str(task)) + return ids if "outcomes" in obj: - return [str(k) for k in obj["outcomes"]] + outcomes = obj["outcomes"] + if not isinstance(outcomes, Mapping): + raise EvalkitError("manifest outcomes must be a JSON object") + return [str(k) for k in outcomes] raise EvalkitError("manifest must be a list of ids or an object with ids/tasks") def _outcomes(obj: Any) -> Dict[str, Any]: if isinstance(obj, Mapping) and "outcomes" in obj: - return dict(obj["outcomes"]) + values = obj["outcomes"] + if not isinstance(values, Mapping): + raise EvalkitError("outcomes must be a JSON object keyed by task id") + return dict(values) if isinstance(obj, Mapping): return dict(obj) raise EvalkitError("outcomes file must be an object mapping task id to score") @@ -439,7 +573,7 @@ def main(argv: Optional[Sequence[str]] = None) -> int: p.add_argument("--manifest", required=True, help="JSON list of task ids (or {ids,tasks})") p.add_argument("--a", required=True, help="JSON outcomes for condition A") p.add_argument("--b", default="", help="JSON outcomes for condition B (omit for A/A)") - p.add_argument("--aa", action="store_true", help="A/A calibration (ignore --b, reuse --a)") + p.add_argument("--aa", action="store_true", help="A/A identity smoke check (ignore --b, reuse --a)") p.add_argument("--alpha", type=float, default=0.05) p.add_argument("--boot", type=int, default=10000) p.add_argument("--seed", type=int, default=42) @@ -469,7 +603,11 @@ def main(argv: Optional[Sequence[str]] = None) -> int: return 1 if args.json: - print(json.dumps(report.to_dict(), indent=2, sort_keys=True)) + try: + print(json.dumps(report.to_dict(), indent=2, sort_keys=True, allow_nan=False)) + except (TypeError, ValueError) as exc: + print(f"ERR_EVALKIT report is not strict JSON: {exc}", file=sys.stderr) + return 2 else: print(format_markdown(report), end="") return 0 diff --git a/tests/test_claude_code_backend.py b/tests/test_claude_code_backend.py new file mode 100644 index 00000000..0719f8e2 --- /dev/null +++ b/tests/test_claude_code_backend.py @@ -0,0 +1,453 @@ +"""claude_code_exec optimizer backend: trace parsing, dispatch, persistence, retries. + +Covers the four highest-risk, previously-untested points introduced with +``claude_code_backend`` (issue #233): + +- ``parse_claude_trace_steps`` extracts text / tool_call / tool_result and drops + init / thinking_tokens bookkeeping (``skillopt/model/codex_harness.py``). +- the dispatcher routes ``chat_optimizer`` to the claude_code branch when the + optimizer backend is ``claude_code_exec``. +- ``_persist_claude_artifacts`` writes ``claude_trace_steps.txt`` for the reflector. +- a non-JSON structured reply is retried and then surfaces as ``RuntimeError``. + +Plus a gating regression: ``fmt_minibatch_trajectories`` only injects +``#### Claude Trace Steps`` when ``REFLACT_CLAUDE_TRACE_TO_OPTIMIZER == "1"``. +""" +from __future__ import annotations + +import importlib.util +import json +import os +import sys +import types +from collections.abc import Iterator +from typing import Any + +import pytest + +from skillopt.gradient.reflect import fmt_minibatch_trajectories +from skillopt.model import codex_harness +from skillopt.model.codex_harness import ( + _json_dumps, + _persist_claude_artifacts, + format_claude_trace_steps, + parse_claude_trace_steps, +) + + +class _OpenAIClientStub: + def __init__(self, *args: Any, **kwargs: Any) -> None: + self.args = args + self.kwargs = kwargs + + +def _install_openai_stub() -> None: + if "openai" in sys.modules or importlib.util.find_spec("openai") is not None: + return + openai_stub = types.ModuleType("openai") + openai_stub.AzureOpenAI = _OpenAIClientStub + openai_stub.OpenAI = _OpenAIClientStub + sys.modules["openai"] = openai_stub + + +@pytest.fixture(autouse=True) +def isolate_backend_state() -> Iterator[None]: + _install_openai_stub() + from skillopt.model import backend_config + + optimizer_backend = backend_config.get_optimizer_backend() + target_backend = backend_config.get_target_backend() + env = { + key: os.environ.get(key) + for key in ( + "OPTIMIZER_BACKEND", + "TARGET_BACKEND", + "OPTIMIZER_DEPLOYMENT", + "TARGET_DEPLOYMENT", + ) + } + yield + backend_config.set_optimizer_backend(optimizer_backend) + backend_config.set_target_backend(target_backend) + for key, value in env.items(): + if value is None: + os.environ.pop(key, None) + else: + os.environ[key] = value + + +def _sdk_payload() -> str: + """Two SDK attempt blocks: bookkeeping noise + real steps + tool result.""" + block1 = { + "messages": [ + {"subtype": "init", "content": []}, + {"subtype": "thinking_tokens", "content": []}, + {"data": {"type": "system", "text": "system banner"}, "content": []}, + { + "content": [ + {"type": "text", "text": "Let me read the task."}, + {"type": "tool_use", "id": "tu_1", "name": "Read", "input": {"file_path": "task.md"}}, + ], + "data": {"type": "assistant"}, + }, + { + "content": [ + { + "type": "tool_result", + "tool_use_id": "tu_1", + "content": [{"type": "text", "text": "X" * 300}], + "is_error": False, + } + ], + "data": {"type": "user"}, + }, + {"result": "THE ANSWER", "data": {"type": "result"}}, + ] + } + block2 = { + "messages": [ + { + "content": [{"type": "text", "text": "Final answer body"}], + "data": {"type": "assistant"}, + } + ] + } + return ( + _json_dumps(block1) + + "\n===== CLAUDE SDK ATTEMPT 2 =====\n" + + _json_dumps(block2) + ) + + +def test_parse_claude_trace_steps_extracts_and_filters() -> None: + steps = parse_claude_trace_steps(_sdk_payload()) + + types_seen = [step["type"] for step in steps] + # init / thinking_tokens / system bookkeeping are dropped. + assert types_seen == ["text", "tool_call", "tool_result", "text", "text"] + + # Indices are renumbered sequentially across attempt blocks. + assert [step["index"] for step in steps] == [1, 2, 3, 4, 5] + + assert steps[0]["summary"] == "Let me read the task." + assert steps[1]["summary"] == "Read task.md" + # tool_result is truncated to 200 chars + a [+N chars] trailer. + assert steps[2]["summary"].startswith("X" * 200) + assert "[+100 chars]" in steps[2]["summary"] + assert steps[3]["summary"] == "THE ANSWER" + assert steps[4]["summary"] == "Final answer body" + + +def test_parse_claude_trace_steps_marks_errors() -> None: + block = { + "messages": [ + { + "content": [ + { + "type": "tool_result", + "tool_use_id": "tu_9", + "content": [{"type": "text", "text": "boom"}], + "is_error": True, + } + ] + } + ] + } + steps = parse_claude_trace_steps(_json_dumps(block)) + assert len(steps) == 1 + assert steps[0]["type"] == "tool_result" + assert steps[0]["summary"] == "[error] boom" + + +def test_format_claude_trace_steps_truncates_total() -> None: + text = format_claude_trace_steps(_sdk_payload(), max_chars=40) + trailer = "\n...[claude trace steps truncated]..." + assert text.endswith(trailer) + assert text == text[:40] + trailer + + +def test_persist_claude_artifacts_writes_trace_steps(tmp_path) -> None: + work_dir = tmp_path / "pred" / "work" + work_dir.mkdir(parents=True) + + _persist_claude_artifacts(str(work_dir), _sdk_payload(), "response") + + steps_path = tmp_path / "pred" / "claude_trace_steps.txt" + assert steps_path.exists() + content = steps_path.read_text(encoding="utf-8") + assert content.strip() + # text step is index 1, so the tool_call is index 2. + assert "[2] tool_call: Read task.md" in content + + +def test_chat_optimizer_routes_to_claude_code_backend( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from skillopt.model import backend_config, claude_code_backend + from skillopt.model import azure_openai + + claude_calls: list[dict[str, Any]] = [] + + def fake_claude_optimizer(**kwargs: Any) -> tuple[str, dict[str, int]]: + claude_calls.append(kwargs) + return "claude result", { + "prompt_tokens": 1, + "completion_tokens": 2, + "total_tokens": 3, + } + + def fail_openai_optimizer(**_kwargs: Any) -> tuple[str, dict[str, int]]: + raise AssertionError("openai optimizer should not be called for claude_code_exec") + + monkeypatch.setattr(claude_code_backend, "chat_optimizer", fake_claude_optimizer) + monkeypatch.setattr(azure_openai, "chat_optimizer", fail_openai_optimizer) + backend_config.set_optimizer_backend("claude_code_exec") + + from skillopt.model import chat_optimizer + + text, usage = chat_optimizer("system", "user", retries=1, timeout=5) + + assert text == "claude result" + assert usage["total_tokens"] == 3 + assert claude_calls[0]["system"] == "system" + assert claude_calls[0]["user"] == "user" + assert claude_calls[0]["timeout"] == 5 + + +def test_reasoning_effort_forwarded_to_run_claude_code_chat( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from skillopt.model import claude_code_backend + + calls: list[dict[str, Any]] = [] + + def fake_chat(**kwargs: Any) -> tuple[str, dict[str, int]]: + calls.append(kwargs) + return "plain reply", {"prompt_tokens": 1, "completion_tokens": 1} + + monkeypatch.setattr(claude_code_backend, "run_claude_code_chat", fake_chat) + claude_code_backend.set_reasoning_effort("high") + try: + text, _usage = claude_code_backend.chat_optimizer("s", "u", retries=1) + finally: + claude_code_backend.set_reasoning_effort(None) + + assert text == "plain reply" + assert calls[0]["effort"] == "high" + + +def test_reasoning_effort_param_beats_module_global( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from skillopt.model import claude_code_backend + + calls: list[dict[str, Any]] = [] + + def fake_chat(**kwargs: Any) -> tuple[str, dict[str, int]]: + calls.append(kwargs) + return "plain reply", {"prompt_tokens": 1, "completion_tokens": 1} + + monkeypatch.setattr(claude_code_backend, "run_claude_code_chat", fake_chat) + claude_code_backend.set_reasoning_effort("low") + try: + claude_code_backend.chat_optimizer( + "s", "u", retries=1, reasoning_effort="max" + ) + finally: + claude_code_backend.set_reasoning_effort(None) + + assert calls[0]["effort"] == "max" + + +def test_claude_code_backend_retry_on_bad_json( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from skillopt.model import claude_code_backend + + monkeypatch.setattr( + claude_code_backend, + "run_claude_code_chat", + lambda **kwargs: ("this is not json", {"prompt_tokens": 3, "completion_tokens": 4}), + ) + + claude_code_backend.reset_token_tracker() + try: + # structured output (tools set) forces a json.loads on the reply; a + # non-JSON reply must be retried, then surfaced as RuntimeError. + with pytest.raises(RuntimeError, match="failed after 2 retries"): + claude_code_backend.chat_optimizer_messages( + [{"role": "user", "content": "hi"}], + retries=2, + tools=[{"name": "lookup"}], + ) + + summary = claude_code_backend.get_token_summary() + optimizer = summary["optimizer"] + assert optimizer["calls"] == 2 + assert optimizer["prompt_tokens"] == 6 + assert optimizer["completion_tokens"] == 8 + finally: + claude_code_backend.reset_token_tracker() + + +@pytest.mark.parametrize( + ("target_backend", "config_on", "expect_codex", "expect_claude"), + [ + ("claude_code_exec", True, "0", "1"), + ("claude_code_exec", False, "0", "0"), + ("codex_exec", True, "1", "0"), + ("codex_exec", False, "0", "0"), + ("openai_chat", True, "0", "0"), + ], +) +def test_trainer_configures_trace_gates( + monkeypatch: pytest.MonkeyPatch, + target_backend: str, + config_on: bool, + expect_codex: str, + expect_claude: str, +) -> None: + from skillopt.engine.trainer import _configure_trace_to_optimizer_gates + + monkeypatch.delenv("REFLACT_CODEX_TRACE_TO_OPTIMIZER", raising=False) + monkeypatch.delenv("REFLACT_CLAUDE_TRACE_TO_OPTIMIZER", raising=False) + _configure_trace_to_optimizer_gates( + target_backend, + {"codex_trace_to_optimizer": config_on, "claude_trace_to_optimizer": config_on}, + ) + assert os.environ["REFLACT_CODEX_TRACE_TO_OPTIMIZER"] == expect_codex + assert os.environ["REFLACT_CLAUDE_TRACE_TO_OPTIMIZER"] == expect_claude + + +@pytest.mark.parametrize( + ("gate_value", "expect_injected"), + [("0", False), ("1", True)], +) +def test_claude_trace_steps_gated_in_fmt_minibatch( + monkeypatch: pytest.MonkeyPatch, + tmp_path, + gate_value: str, + expect_injected: bool, +) -> None: + tid = "tid0" + pred_dir = tmp_path / "predictions" + (pred_dir / tid).mkdir(parents=True) + (pred_dir / tid / "conversation.json").write_text( + json.dumps([{"role": "assistant", "content": "hi"}]), + encoding="utf-8", + ) + (pred_dir / tid / "claude_trace_steps.txt").write_text( + "[1] tool_call: Read task.md", + encoding="utf-8", + ) + + monkeypatch.setenv("REFLACT_CLAUDE_TRACE_TO_OPTIMIZER", gate_value) + + formatted = fmt_minibatch_trajectories( + [{"id": tid, "task_description": "t", "task_type": "q"}], + str(pred_dir), + ) + + assert ("#### Claude Trace Steps" in formatted) is expect_injected + + +def test_parse_claude_trace_steps_preserves_tool_result_payload() -> None: + # Regression for the #233 review: Anthropic content blocks carry their + # payload under ``text``, not ``content``. A realistic message stream must + # surface the tool_result observation, not collapse to an empty summary. + raw = _json_dumps({ + "messages": [ + { + "role": "user", + "content": [ + { + "type": "tool_result", + "tool_use_id": "tu_1", + "content": [{"type": "text", "text": "THE ACTUAL RESULT PAYLOAD"}], + } + ], + } + ] + }) + steps = parse_claude_trace_steps(raw) + assert steps == [ + {"type": "tool_result", "summary": "THE ACTUAL RESULT PAYLOAD", "index": 1} + ] + + +def test_cli_chat_uses_json_schema_and_disables_tools(monkeypatch) -> None: + # The CLI structured-output path must emit the real --json-schema flag and + # an explicit --tools "" (an empty tool list), not --schema or + # --permission-mode alone. + captured: dict[str, Any] = {} + + def fake_run(cmd, **kwargs): + captured["cmd"] = cmd + return types.SimpleNamespace( + returncode=0, + stdout='{"type": "result", "result": "ok"}\n', + stderr="", + ) + + monkeypatch.setattr(codex_harness.subprocess, "run", fake_run) + + text, _usage = codex_harness._run_claude_code_cli_chat_exec( + system="sys", + prompt="hi", + model="claude-sonnet-4-6", + timeout=10, + schema={"type": "object"}, + ) + + cmd = captured["cmd"] + assert "--json-schema" in cmd + assert "--schema" not in cmd + tools_idx = cmd.index("--tools") + assert cmd[tools_idx + 1] == "" + assert text == "ok" + + +def test_sdk_chat_disables_tools(monkeypatch) -> None: + # ``tools=[]`` (not ``allowed_tools=[]``) is what actually strips the + # optimizer's built-in tool access in the SDK path. + captured: dict[str, Any] = {} + + sdk = types.ModuleType("claude_agent_sdk") + + class _Options: + def __init__(self, **kwargs): + captured.update(kwargs) + + class _Client: + def __init__(self, options): + self._options = options + + async def __aenter__(self): + return self + + async def __aexit__(self, *exc): + return False + + async def query(self, prompt): + return None + + def receive_response(self): + async def gen(): + yield types.SimpleNamespace(result="ok", content=None, usage={}) + + return gen() + + sdk.ClaudeAgentOptions = _Options + sdk.ClaudeSDKClient = _Client + monkeypatch.setitem(sys.modules, "claude_agent_sdk", sdk) + + text, _usage = codex_harness._run_claude_code_sdk_chat_exec( + system="sys", + prompt="hi", + model="claude-sonnet-4-6", + timeout=10, + schema=None, + ) + + assert captured["tools"] == [] + assert text == "ok" diff --git a/tests/test_claude_code_exec_resolution.py b/tests/test_claude_code_exec_resolution.py new file mode 100644 index 00000000..b283b264 --- /dev/null +++ b/tests/test_claude_code_exec_resolution.py @@ -0,0 +1,141 @@ +"""Config-resolution regressions for the claude_code_exec backend (issue #233). + +Route B: ``--backend claude_code_exec`` defaults only the *target* to Claude +Code; the optimizer keeps its configured backend (openai_chat by default). +Opting the optimizer in via ``--optimizer_backend claude_code_exec`` must then +normalize its model to the Claude default rather than leaving ``gpt-5.5``. +""" + +from __future__ import annotations + +import sys +from pathlib import Path +from types import SimpleNamespace + +import pytest + +import scripts.train as train_script + +_ROOT = Path(__file__).resolve().parents[1] + + +def _train_cfg(monkeypatch, *extra_argv: str) -> dict: + monkeypatch.setattr( + sys, + "argv", + [ + "skillopt-train", + "--config", + str(_ROOT / "configs" / "searchqa" / "default.yaml"), + *extra_argv, + ], + ) + return train_script.load_config(train_script.parse_args()) + + +def test_train_claude_code_exec_defaults_target_only(monkeypatch) -> None: + cfg = _train_cfg(monkeypatch, "--backend", "claude_code_exec") + + assert cfg["optimizer_backend"] == "openai_chat" + assert cfg["optimizer_model"] == "gpt-5.5" + assert cfg["target_backend"] == "claude_code_exec" + assert cfg["target_model"] == "claude-sonnet-4-6" + + +def test_train_claude_code_exec_optimizer_opt_in_normalizes_model(monkeypatch) -> None: + cfg = _train_cfg( + monkeypatch, + "--backend", + "claude_code_exec", + "--optimizer_backend", + "claude_code_exec", + ) + + assert cfg["optimizer_backend"] == "claude_code_exec" + assert cfg["optimizer_model"] == "claude-sonnet-4-6" + assert cfg["target_backend"] == "claude_code_exec" + assert cfg["target_model"] == "claude-sonnet-4-6" + + +class _StopAfterResolution(Exception): + pass + + +def _run_eval_resolution( + monkeypatch, tmp_path, *, optimizer_backend: str | None +) -> dict: + import scripts.eval_only as eval_script + + skill_path = tmp_path / "skill.md" + skill_path.write_text("# Test skill\n", encoding="utf-8") + + cfg = { + "model": { + "backend": "azure_openai", + "optimizer": "gpt-5.5", + "target": "gpt-5.5", + "optimizer_backend": "openai_chat", + "target_backend": "openai_chat", + }, + "env": {"out_root": str(tmp_path / "out")}, + } + args = SimpleNamespace( + config="unused.yaml", + skill=str(skill_path), + split=None, + cfg_options=[], + backend="claude_code_exec", + optimizer_backend=optimizer_backend, + ) + monkeypatch.setattr(eval_script, "parse_args", lambda: args) + monkeypatch.setattr("skillopt.config.load_config", lambda *a, **kw: cfg) + + observed: dict[str, str] = {} + + def capture(name): + def _fn(value, *a, **k): + observed[name] = value + + return _fn + + monkeypatch.setattr(eval_script, "configure_azure_openai", lambda **kw: None) + monkeypatch.setattr(eval_script, "set_optimizer_backend", capture("optimizer_backend")) + monkeypatch.setattr(eval_script, "set_target_backend", capture("target_backend")) + monkeypatch.setattr(eval_script, "set_optimizer_deployment", capture("optimizer_model")) + monkeypatch.setattr(eval_script, "set_target_deployment", capture("target_model")) + + def stop(*a, **k): + raise _StopAfterResolution + + monkeypatch.setattr(eval_script, "configure_codex_exec_from_config", stop) + + with pytest.raises(_StopAfterResolution): + eval_script.main() + + return observed + + +def test_eval_claude_code_exec_defaults_target_only(monkeypatch, tmp_path) -> None: + observed = _run_eval_resolution(monkeypatch, tmp_path, optimizer_backend=None) + + assert observed == { + "optimizer_backend": "openai_chat", + "target_backend": "claude_code_exec", + "optimizer_model": "gpt-5.5", + "target_model": "claude-sonnet-4-6", + } + + +def test_eval_claude_code_exec_optimizer_opt_in_normalizes_model( + monkeypatch, tmp_path +) -> None: + observed = _run_eval_resolution( + monkeypatch, tmp_path, optimizer_backend="claude_code_exec" + ) + + assert observed == { + "optimizer_backend": "claude_code_exec", + "target_backend": "claude_code_exec", + "optimizer_model": "claude-sonnet-4-6", + "target_model": "claude-sonnet-4-6", + } diff --git a/tests/test_copilot_tool_scope.py b/tests/test_copilot_tool_scope.py new file mode 100644 index 00000000..f1eb6beb --- /dev/null +++ b/tests/test_copilot_tool_scope.py @@ -0,0 +1,80 @@ +"""Tests for Copilot CLI tool-scope reduction. + +The sleep engine's Copilot backend used to launch the CLI with bare +``--allow-all-tools``, granting the model every tool the CLI exposes. Scoping +now happens on the *visibility* axis via ``--available-tools`` (default +``bash``, overridable with ``COPILOT_AVAILABLE_TOOLS``). + +The two axes are independent and both are needed: + +* ``--allow-all-tools`` waives the interactive approval prompt. The CLI's own + help states it is "required for non-interactive mode", so dropping it makes + every headless call hang or fail -- it is not the flag to scope on. +* ``--available-tools`` is what actually narrows the surface: "Only these tools + will be available to the model". + +Verified against GitHub Copilot CLI 1.0.80: ``--allowed-tools`` does not exist +(``error: unknown option``), and the selector is the lowercase tool name +``bash`` -- ``--available-tools=Bash`` silently blocks the tool instead of +allowing it, since selectors are case-sensitive. + +These tests capture the constructed argv without spawning the CLI. +""" +from __future__ import annotations + +from skillopt_sleep import backend as backend_mod +from skillopt_sleep.backend import CopilotCliBackend + + +def _make_backend() -> CopilotCliBackend: + b = CopilotCliBackend.__new__(CopilotCliBackend) + b.copilot_path = "copilot" + b.full_env = False + b.model = "" + b.copilot_home = "" + b.timeout = 10 + return b + + +def _capture_argv(monkeypatch) -> list[str]: + captured: dict[str, list[str]] = {} + + def fake_run(cmd, *args, **kwargs): # noqa: ANN001 + captured["cmd"] = cmd + raise RuntimeError("stop before spawn") + + monkeypatch.setattr(backend_mod.subprocess, "run", fake_run) + _make_backend()._call("hello") + return captured["cmd"] + + +def test_nonexistent_allowed_tools_flag_is_never_sent(monkeypatch) -> None: + # Guards the original regression: `--allowed-tools` is not a Copilot CLI + # option, so sending it aborts the process before any work happens. + cmd = _capture_argv(monkeypatch) + assert "--allowed-tools" not in cmd + + +def test_non_interactive_permission_flag_is_kept(monkeypatch) -> None: + cmd = _capture_argv(monkeypatch) + assert "--allow-all-tools" in cmd + + +def test_visibility_is_scoped(monkeypatch) -> None: + cmd = _capture_argv(monkeypatch) + assert "--available-tools" in cmd + + +def test_default_scope_is_lowercase_bash(monkeypatch) -> None: + monkeypatch.delenv("COPILOT_AVAILABLE_TOOLS", raising=False) + cmd = _capture_argv(monkeypatch) + idx = cmd.index("--available-tools") + # Lowercase matters: selectors are case-sensitive and "Bash" matches nothing. + assert cmd[idx + 1] == "bash" + + +def test_env_var_overrides_scope(monkeypatch) -> None: + monkeypatch.setenv("COPILOT_AVAILABLE_TOOLS", "bash,write") + cmd = _capture_argv(monkeypatch) + idx = cmd.index("--available-tools") + assert cmd[idx + 1] == "bash,write" diff --git a/tests/test_evalkit.py b/tests/test_evalkit.py index e09c6338..fb8be99b 100644 --- a/tests/test_evalkit.py +++ b/tests/test_evalkit.py @@ -1,13 +1,16 @@ -"""Paired A/B evalkit: known-answer stats, A/A calibration, RESULTS replay.""" +"""Paired A/B evalkit: known-answer stats, seeded null calibration, RESULTS replay.""" from __future__ import annotations +import io import json import math import os +import random import subprocess import sys import tempfile import unittest +from contextlib import redirect_stderr, redirect_stdout from skillopt_sleep.evalkit import ( EvalkitError, @@ -16,12 +19,13 @@ compare_aa, exact_mcnemar_p, format_markdown, - main as evalkit_main, mcnemar_from_counts, mcnemar_paired, reconstruct_paired_from_rates, ) - +from skillopt_sleep.evalkit import ( + main as evalkit_main, +) FIXTURE_DIR = os.path.join(os.path.dirname(__file__), "fixtures", "evalkit") @@ -60,6 +64,24 @@ def test_paired_vectors_match_counts(self): self.assertEqual(res.both_fail, 1) self.assertAlmostEqual(res.p_exact, exact_mcnemar_p(1, 1)) + def test_exact_tail_is_stable_above_one_thousand_discordants(self): + self.assertEqual(exact_mcnemar_p(700, 700), 1.0) + value = exact_mcnemar_p(590, 611) + self.assertTrue(math.isfinite(value)) + self.assertGreater(value, 0.0) + self.assertLessEqual(value, 1.0) + + def test_invalid_counts_and_binary_vectors_are_refused(self): + for args in ((-1, 0), (True, 0), (1.5, 0)): + with self.subTest(args=args), self.assertRaises(EvalkitError): + exact_mcnemar_p(*args) + with self.assertRaises(EvalkitError): + mcnemar_from_counts(1, -1, 0, 1) + with self.assertRaises(EvalkitError): + mcnemar_paired([], []) + with self.assertRaises(EvalkitError): + mcnemar_paired([0, 2], [0, 1]) + class TestBootstrapCoverage(unittest.TestCase): def test_identical_series_ci_collapses_to_zero(self): @@ -84,6 +106,22 @@ def test_seed_is_deterministic(self): y = bootstrap_delta_ci(a, b, n_boot=500, seed=99) self.assertEqual((x.low, x.high, x.mean), (y.low, y.high, y.mean)) + def test_invalid_alpha_and_bootstrap_counts_are_refused(self): + for alpha in (0, 1, -0.1, 1.1, float("nan"), float("inf"), True): + with self.subTest(alpha=alpha), self.assertRaises(EvalkitError): + bootstrap_delta_ci([0], [1], alpha=alpha) + for n_boot in (0, -1, 1.5, True, 1_000_001): + with self.subTest(n_boot=n_boot), self.assertRaises(EvalkitError): + bootstrap_delta_ci([0], [1], n_boot=n_boot) + for seed in (True, 1.5, "7"): + with self.subTest(seed=seed), self.assertRaises(EvalkitError): + bootstrap_delta_ci([0], [1], n_boot=10, seed=seed) + + def test_malformed_direct_api_scores_are_contract_errors(self): + for value in ("not-a-number", None, object(), float("nan"), float("inf")): + with self.subTest(value=value), self.assertRaises(EvalkitError): + bootstrap_delta_ci([0], [value], n_boot=10) + class TestAACalibration(unittest.TestCase): def test_aa_does_not_reject(self): @@ -97,6 +135,25 @@ def test_aa_does_not_reject(self): self.assertLessEqual(report.bootstrap.low, 0.0) self.assertGreaterEqual(report.bootstrap.high, 0.0) + def test_exact_test_controls_type_one_error_under_a_seeded_null(self): + # Unlike comparing an array with itself, this exercises non-zero, + # symmetrically distributed discordance and can catch p-value inflation. + rng = random.Random(20260824) + trials = 500 + rejected = 0 + for _ in range(trials): + a = [] + b = [] + for _task in range(80): + left = int(rng.random() < 0.5) + right = 1 - left if rng.random() < 0.30 else left + a.append(left) + b.append(right) + rejected += int(mcnemar_paired(a, b, alpha=0.05).significant) + rate = rejected / trials + self.assertGreater(rejected, 0) + self.assertLessEqual(rate, 0.075) + class TestCompareContracts(unittest.TestCase): def test_mismatched_ids_are_refused(self): @@ -143,9 +200,46 @@ def test_multi_seed_variance_band(self): self.assertGreaterEqual(report.seed_sd_delta, 0.0) self.assertAlmostEqual(report.rate_a, (2 / 3 + 1 / 3) / 2) self.assertAlmostEqual(report.rate_b, (1.0 + 2 / 3) / 2) + self.assertIsNone(report.mcnemar) + self.assertTrue(any("task-cluster" in note for note in report.notes)) + + def test_empty_nonfinite_and_out_of_range_seed_scores_are_refused(self): + bad_values = ([], [float("nan")], [float("inf")], [-0.01], [1.01]) + for value in bad_values: + with self.subTest(value=value), self.assertRaises(EvalkitError): + compare(["t1"], {"t1": value}, {"t1": [1]}, allow_graded=True) + + def test_duplicating_seeds_within_tasks_does_not_inflate_inference(self): + ids = ["t1", "t2", "t3", "t4"] + a_two = {tid: [0, 0] for tid in ids} + b_two = {tid: [1, 1] for tid in ids} + a_many = {tid: [0] * 100 for tid in ids} + b_many = {tid: [1] * 100 for tid in ids} + two = compare(ids, a_two, b_two, n_boot=500, seed=8) + many = compare(ids, a_many, b_many, n_boot=500, seed=8) + self.assertIsNone(two.mcnemar) + self.assertIsNone(many.mcnemar) + self.assertEqual(two.delta, many.delta) + self.assertEqual(two.bootstrap.to_dict(), many.bootstrap.to_dict()) + + def test_invalid_compare_parameters_are_refused(self): + for alpha in (0, 1, float("nan")): + with self.subTest(alpha=alpha), self.assertRaises(EvalkitError): + compare(["t1"], {"t1": 0}, {"t1": 1}, alpha=alpha) + for n_boot in (0, -4, True): + with self.subTest(n_boot=n_boot), self.assertRaises(EvalkitError): + compare(["t1"], {"t1": 0}, {"t1": 1}, n_boot=n_boot) class TestResultsCellReplay(unittest.TestCase): + def test_malformed_reconstruction_inputs_are_contract_errors(self): + for rates in (("bad", 0.5), (None, 0.5), (float("nan"), 0.5), (0.5, 1.1)): + with self.subTest(rates=rates), self.assertRaises(EvalkitError): + reconstruct_paired_from_rates(10, *rates) + for n in (True, 0, 1.5): + with self.subTest(n=n), self.assertRaises(EvalkitError): + reconstruct_paired_from_rates(n, 0.5, 0.5) + def test_published_searchqa_nano_gated_delta(self): cell = _load("results_searchqa_nano_gated.json") a, b = reconstruct_paired_from_rates(cell["n"], cell["baseline"], cell["after"]) @@ -193,6 +287,27 @@ def test_mismatch_cli_exit_two(self): rc = evalkit_main(["--manifest", man, "--a", a, "--b", b]) self.assertEqual(rc, 2) + def test_malformed_json_and_shapes_are_clean_contract_errors(self): + cases = ( + ("{", '{"t1": 1}', '{"t1": 1}'), + ('{"ids": "t1"}', '{"t1": 1}', '{"t1": 1}'), + ('["t1"]', '{"outcomes": []}', '{"t1": 1}'), + ) + for manifest_text, a_text, b_text in cases: + with self.subTest(manifest=manifest_text), tempfile.TemporaryDirectory() as td: + paths = [] + for name, content in (("m.json", manifest_text), ("a.json", a_text), ("b.json", b_text)): + path = os.path.join(td, name) + with open(path, "w", encoding="utf-8") as handle: + handle.write(content) + paths.append(path) + stderr = io.StringIO() + with redirect_stderr(stderr): + rc = evalkit_main(["--manifest", paths[0], "--a", paths[1], "--b", paths[2]]) + self.assertEqual(rc, 2) + self.assertTrue(stderr.getvalue().startswith("ERR_EVALKIT ")) + self.assertNotIn("Traceback", stderr.getvalue()) + def test_module_entrypoint(self): proc = subprocess.run( [ @@ -209,6 +324,28 @@ def test_module_entrypoint(self): self.assertEqual(proc.returncode, 0, proc.stderr) self.assertIn("delta (B-A): +0.000000", proc.stdout) + def test_nonfinite_input_is_refused_without_nonstandard_json_output(self): + with tempfile.TemporaryDirectory() as td: + man = os.path.join(td, "m.json") + a = os.path.join(td, "a.json") + b = os.path.join(td, "b.json") + with open(man, "w", encoding="utf-8") as f: + json.dump(["t1"], f) + with open(a, "w", encoding="utf-8") as f: + f.write('{"t1": NaN}') + with open(b, "w", encoding="utf-8") as f: + json.dump({"t1": 1}, f) + stdout = io.StringIO() + stderr = io.StringIO() + with redirect_stdout(stdout), redirect_stderr(stderr): + rc = evalkit_main([ + "--manifest", man, "--a", a, "--b", b, "--json", + ]) + self.assertEqual(rc, 2) + self.assertEqual(stdout.getvalue(), "") + self.assertIn("finite", stderr.getvalue()) + self.assertNotIn("NaN", stdout.getvalue()) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_minimax_backend.py b/tests/test_minimax_backend.py new file mode 100644 index 00000000..ec049a21 --- /dev/null +++ b/tests/test_minimax_backend.py @@ -0,0 +1,171 @@ +"""Tests for the OpenAI-compatible MiniMax chat backend.""" + +from __future__ import annotations + +import importlib.util +import json +import sys +import types +from collections.abc import Iterator +from typing import Any + +import pytest + + +class _FakeResponse: + def __init__(self, payload: dict[str, Any]) -> None: + self._payload = payload + + def __enter__(self) -> "_FakeResponse": + return self + + def __exit__(self, exc_type: object, exc: object, traceback: object) -> None: + return None + + def read(self) -> bytes: + return json.dumps(self._payload).encode("utf-8") + + +class _UrlopenRecorder: + def __init__(self, content: str = "answer") -> None: + self.content = content + self.calls: list[dict[str, Any]] = [] + + def __call__(self, request: Any, timeout: float | None = None) -> _FakeResponse: + self.calls.append( + { + "payload": json.loads(request.data.decode("utf-8")), + "timeout": timeout, + } + ) + return _FakeResponse( + { + "choices": [ + {"message": {"content": self.content}, "finish_reason": "stop"} + ], + "usage": {"prompt_tokens": 2, "completion_tokens": 1, "total_tokens": 3}, + } + ) + + +class _OpenAIClientStub: + def __init__(self, *args: Any, **kwargs: Any) -> None: + self.args = args + self.kwargs = kwargs + + +def _install_openai_stub() -> None: + if "openai" in sys.modules or importlib.util.find_spec("openai") is not None: + return + openai_stub = types.ModuleType("openai") + openai_stub.AzureOpenAI = _OpenAIClientStub + openai_stub.OpenAI = _OpenAIClientStub + sys.modules["openai"] = openai_stub + + +@pytest.fixture() +def minimax_backend() -> Iterator[Any]: + _install_openai_stub() + from skillopt.model import minimax_backend as backend + + snapshot = { + "ENABLE_THINKING": backend.ENABLE_THINKING, + "TARGET_DEPLOYMENT": backend.TARGET_DEPLOYMENT, + "API_KEY": backend.API_KEY, + "BASE_URL": backend.BASE_URL, + } + backend.reset_token_tracker() + yield backend + backend.reset_token_tracker() + for key, value in snapshot.items(): + setattr(backend, key, value) + + +def _record_urlopen(monkeypatch: pytest.MonkeyPatch, backend: Any) -> _UrlopenRecorder: + recorder = _UrlopenRecorder() + monkeypatch.setattr(backend.urllib.request, "urlopen", recorder) + return recorder + + +def test_default_deployment_is_current_model(minimax_backend: Any) -> None: + from skillopt.model.common import default_model_for_backend + + assert default_model_for_backend("minimax_chat") == "MiniMax-M3" + + +def test_always_on_model_sends_adaptive_not_disabled( + monkeypatch: pytest.MonkeyPatch, minimax_backend: Any +) -> None: + """M2.x cannot turn thinking off, so never claim it is disabled. + + MiniMax documents that the M2 family accepts ``{"type": "disabled"}`` but + keeps thinking on anyway. Sending "disabled" would record a request that + does not match what the model actually does. + """ + minimax_backend.ENABLE_THINKING = False + minimax_backend.TARGET_DEPLOYMENT = "MiniMax-M2.7" + recorder = _record_urlopen(monkeypatch, minimax_backend) + + minimax_backend.chat_target("system", "user", retries=1) + + payload = recorder.calls[0]["payload"] + assert payload["model"] == "MiniMax-M2.7" + assert payload["thinking"] == {"type": "adaptive"} + + +def test_adaptive_model_respects_disabled_flag( + monkeypatch: pytest.MonkeyPatch, minimax_backend: Any +) -> None: + minimax_backend.ENABLE_THINKING = False + minimax_backend.TARGET_DEPLOYMENT = "MiniMax-M3" + recorder = _record_urlopen(monkeypatch, minimax_backend) + + minimax_backend.chat_target("system", "user", retries=1) + + payload = recorder.calls[0]["payload"] + assert payload["model"] == "MiniMax-M3" + assert payload["thinking"] == {"type": "disabled"} + + +def test_adaptive_model_respects_enabled_flag( + monkeypatch: pytest.MonkeyPatch, minimax_backend: Any +) -> None: + minimax_backend.ENABLE_THINKING = True + minimax_backend.TARGET_DEPLOYMENT = "MiniMax-M3" + recorder = _record_urlopen(monkeypatch, minimax_backend) + + minimax_backend.chat_target("system", "user", retries=1) + + assert recorder.calls[0]["payload"]["thinking"] == {"type": "adaptive"} + + +def test_unsupported_chat_template_kwargs_is_never_sent( + monkeypatch: pytest.MonkeyPatch, minimax_backend: Any +) -> None: + """Guards the original regression. + + ``chat_template_kwargs.enable_thinking`` is a Qwen/HuggingFace-serving + convention. It appears nowhere in MiniMax's OpenAI-compatible reference, so + the endpoint ignores it -- meaning thinking silently stayed at the server + default no matter what the flag said. + """ + minimax_backend.ENABLE_THINKING = False + minimax_backend.TARGET_DEPLOYMENT = "MiniMax-M3" + recorder = _record_urlopen(monkeypatch, minimax_backend) + + minimax_backend.chat_target("system", "user", retries=1) + + assert "chat_template_kwargs" not in recorder.calls[0]["payload"] + + +def test_unknown_deployment_defaults_to_adaptive( + monkeypatch: pytest.MonkeyPatch, minimax_backend: Any +) -> None: + """An unrecognized model follows the documented API default (thinking on).""" + minimax_backend.ENABLE_THINKING = True + minimax_backend.TARGET_DEPLOYMENT = "MiniMax-Future-9" + recorder = _record_urlopen(monkeypatch, minimax_backend) + + minimax_backend.chat_target("system", "user", retries=1) + + assert recorder.calls[0]["payload"]["thinking"] == {"type": "adaptive"} diff --git a/tests/test_role_backend_resolution.py b/tests/test_role_backend_resolution.py index afb92e5a..d5b1cd72 100644 --- a/tests/test_role_backend_resolution.py +++ b/tests/test_role_backend_resolution.py @@ -148,6 +148,15 @@ def test_explicit_target_is_preserved_when_optimizer_is_default() -> None: ) +def test_claude_code_exec_optimizer_opt_in_is_preserved() -> None: + # Route B: the target defaults to Claude Code, but an explicit + # --optimizer_backend claude_code_exec still drives both roles. + assert _resolve_role_backends("claude_code_exec", "claude_code_exec", "openai_chat") == ( + "claude_code_exec", + "claude_code_exec", + ) + + def test_copilot_maps_both_roles_to_the_cli_authenticated_backend() -> None: # No separate provider API key is needed because the CLI carries sign-in. assert _resolve_role_backends("copilot", *_BASE_CONFIG) == ("copilot_chat", "copilot_chat") From 76866294a8045c2fe80880ffdc1212df90f8883f Mon Sep 17 00:00:00 2001 From: "Bogdan (Dan) Baciu" Date: Wed, 26 Aug 2026 23:34:17 +0400 Subject: [PATCH 3/3] test: freeze pr242 native validation candidate --- CHANGELOG.md | 5 +- docs/contributing.md | 6 + docs/reference/cli.md | 5 +- docs/review_guidelines.md | 78 ++++ docs/sleep/README.md | 5 +- docs/sleep/evalkit.md | 42 +- mkdocs.yml | 1 + skillopt/config.py | 8 + skillopt_sleep/__main__.py | 6 +- skillopt_sleep/adapters/superpowers.py | 2 +- skillopt_sleep/evalkit.py | 245 +++++++---- skillopt_sleep/staging.py | 74 +++- .../evalkit/results_searchqa_nano_gated.json | 3 +- tests/test_devin_plugin.py | 1 + tests/test_env_section_survives_dedup.py | 80 ++++ tests/test_evalkit.py | 387 ++++++++++++++++-- tests/test_pi_integration.py | 2 +- tests/test_sleep_adopt_skill_subset.py | 74 ++-- tests/test_sleep_engine.py | 8 +- tests/test_sleep_scheduler_safety.py | 1 + tests/test_sleep_skill_resolver.py | 40 +- tests/test_sleep_staging_fanout.py | 128 +++++- tests/test_superpowers_scenarios.py | 21 +- 23 files changed, 1038 insertions(+), 184 deletions(-) create mode 100644 docs/review_guidelines.md create mode 100644 tests/test_env_section_survives_dedup.py diff --git a/CHANGELOG.md b/CHANGELOG.md index cf538fb9..c61a74dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,8 +9,9 @@ All notable changes to SkillOpt are documented here. This project adheres to ### Added - **SkillOpt-Sleep paired A/B evalkit** (`python -m skillopt_sleep.evalkit`): McNemar plus percentile-bootstrap CIs on a fixed task manifest, with - task-cluster multi-seed inference and calibrated A/A coverage. The nightly gate is - unchanged (thanks @bogdanbaciu21). + task-cluster multi-seed inference and seeded null calibration for exact-test + type-I error and bootstrap coverage. The nightly gate is unchanged + (thanks @bogdanbaciu21). - **SkillOpt-Sleep multi-skill fan-out and reviewed subset adoption**: each hinted skill is consolidated from its own pinned live baseline, staged as an independent proposal with per-skill gate evidence, and promoted only through diff --git a/docs/contributing.md b/docs/contributing.md index 89716938..229864d0 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -73,6 +73,12 @@ mkdocs serve # Preview at http://localhost:8000 4. Run focused tests, the full test suite, and `mkdocs build --strict` when docs change 5. Submit a PR with a clear description +## Code Review Guidelines + +See the [code review checklist](review_guidelines.md) for common considerations +that may help reviewers and contributors. Apply the items relevant to the scope +and risk of each change. + ## License By contributing, you agree that your contributions will be licensed under the MIT License. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 6ed591c2..6aefbb20 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -126,8 +126,9 @@ python -m skillopt_sleep [options] Actions are `run`, `dry-run`, `status`, `adopt`, `harvest`, `schedule`, `unschedule`, and `evalkit`. `evalkit` is also available as `python -m skillopt_sleep.evalkit` and compares two conditions on one fixed -task manifest (McNemar + bootstrap CI). See `docs/sleep/evalkit.md`. Common -options for the nightly actions include: +task manifest (McNemar + bootstrap CI). Exactly one of its `--b` comparison +input or `--aa` identity-check flag is required. See `docs/sleep/evalkit.md`. +Common options for the nightly actions include: | Argument | Description | |---|---| diff --git a/docs/review_guidelines.md b/docs/review_guidelines.md new file mode 100644 index 00000000..07414afd --- /dev/null +++ b/docs/review_guidelines.md @@ -0,0 +1,78 @@ +# Code Review & Contribution Guidelines + +> This checklist summarizes common considerations for contributors and +> reviewers. It is advisory, not an exhaustive merge policy: apply the items +> relevant to the scope and risk of a change, and use maintainer judgment for +> merge decisions. + +## Review approach + +- **Review the current PR HEAD** โ€” confirm the exact revision before probing or + testing, and use the target branch as the comparison baseline. +- **Reproduce the failure against the real version and real behavior**, not an + imagined input. +- **Check the real third-party contract** โ€” verify the actual CLI/SDK flags and + behavior a dependency supports, rather than assuming. +- **Run tests proportionate to the change** โ€” include focused regression tests + and, when practical, the relevant broader suite; report the commands and + results. +- **Separate blockers from suggestions** and make feedback actionable by citing + the relevant behavior or location and explaining the impact. + +## Pre-submission self-check + +- [ ] The declared dependency range matches supported behavior; test + representative boundary versions when compatibility differs or the range + changes. +- [ ] Error fallbacks fire only for the *expected* failure; other errors + re-raise; existing targets fail closed; no out-of-bounds mutation. +- [ ] Sensitive data is redacted at every relevant output boundary while + preserving useful diagnostics. Prefer structural redaction for structured + data, and test both secret-key variants and non-secret lookalikes such as + `token_count`. +- [ ] Shared mutable state is synchronized with a mechanism appropriate to the + implementation; failures and accounting remain isolated per operation, + and cache behavior is explicit. +- [ ] A bug fix considers all affected callers and subclasses and includes a + regression test that fails before the fix and verifies externally + observable behavior. +- [ ] Concurrency tests coordinate execution deterministically enough to + exercise the race (for example with barriers, events, or controlled hooks) + and cover relevant failure inputs. +- [ ] Third-party behavior is checked against supported versions. Use an + integration smoke test when practical, or a faithful offline contract test + when live calls are unsuitable. +- [ ] PR hygiene: focused scope, no unrelated commits, appropriate attribution, + and a sufficiently current base to assess conflicts and integration. +- [ ] Domain math (stats/research/numeric) is correct and the formula verified. +- [ ] Config/role routing is consistent (optimizer/target/judge are role-aware; + matching model fields accompany backend changes). +- [ ] Data isolation: test/eval data never leaks back into training; holdout/test + sets are explicitly excluded. +- [ ] External paths and identifiers are validated at the appropriate trust + boundary; invalid or untrusted inputs are rejected safely. +- [ ] Features wired end-to-end (not just primitives). +- [ ] Deliberate resilience/fallback contracts are respected, not "cleaned up"; + deprecated options retire cleanly rather than being re-purposed. +- [ ] Prefer existing infrastructure when it fits the requirement; introduce + new abstractions when they provide a clear benefit. +- [ ] Sensitive content is redacted **before** reaching downstream consumers; + resource operations are bounded. +- [ ] Mutating endpoints enforce appropriate authorization; browser endpoints + that rely on ambient credentials include CSRF protection or an equivalent + defense. +- [ ] Avoid unintended compatibility regressions across supported backends; + document and test intentional backend-specific differences. +- [ ] Filesystem boundaries handled (cross-drive paths, empty home, path + normalization, symlinks). +- [ ] Keep each PR a coherent, reviewable slice with tests appropriate to its + behavior; split unrelated follow-up work into separate PRs. + +## Notes + +- A recurring review failure mode is validating an assumed shape rather than + the behavior the real entry point produces. When practical, exercise the real + entry point with representative input before writing assertions. +- Fix failures at the narrowest layer that correctly covers the affected + callers; check neighboring callers and follow existing conventions to avoid + over- or under-correction. diff --git a/docs/sleep/README.md b/docs/sleep/README.md index 86fc02ff..f47556ee 100644 --- a/docs/sleep/README.md +++ b/docs/sleep/README.md @@ -350,8 +350,9 @@ success-rate delta. The nightly gate is unchanged. python -m skillopt_sleep.evalkit --manifest tasks.json --a cond_a.json --b cond_b.json ``` -See [`evalkit.md`](evalkit.md) for the id-set contract, task-cluster multi-seed -inference, A/A checks, and the published RESULTS cell replay. +See [`evalkit.md`](evalkit.md) for the id-set contract, task-cluster inference +over positional seed repeats, A/A checks, and the point-estimate-only published +RESULTS cell replay. ## Results diff --git a/docs/sleep/evalkit.md b/docs/sleep/evalkit.md index c8797423..20565c89 100644 --- a/docs/sleep/evalkit.md +++ b/docs/sleep/evalkit.md @@ -13,25 +13,40 @@ It does not change the nightly gate. ## Inputs - `--manifest`: JSON list of task ids, or `{"ids": [...]}` / `{"tasks": [{"id": ...}]}`. -- `--a` / `--b`: JSON objects mapping those same ids to `0`/`1` (or a list of - per-seed `0`/`1` values). A wrapper `{"outcomes": {...}}` is also accepted. -- `--aa`: A/A identity smoke check (reuses `--a` as both conditions). Must not reject. + Every id must be a non-empty JSON string; ids are never coerced from numbers, + booleans, nulls, arrays, or objects. +- `--a` / `--b`: JSON objects mapping those same ids to `0`/`1` (or an ordered + list of repeated-seed `0`/`1` values). A wrapper `{"outcomes": {...}}` is also + accepted. Direct keys that exactly match the manifest take precedence over + wrapper detection, so a task literally named `outcomes` remains unambiguous. +- `--aa`: A/A identity smoke check (reuses `--a` as both conditions). + Exactly one of `--b` or `--aa` is required. - `--allow-graded`: permit non-binary scores. McNemar is omitted; bootstrap only. - `--boot`, `--seed`, `--alpha`, `--json`. The id sets of the manifest, A, and B must be identical. Cross-manifest -comparisons are refused. Seed lists must be non-empty, scores must be finite -and in `[0, 1]`, `alpha` must be strictly between 0 and 1, and `--boot` must be -between 1 and 1,000,000. JSON output is strict and never emits NaN/Infinity. +comparisons and duplicate JSON object keys are refused. Seed lists must be +non-empty. Scores must be JSON numbers (never booleans or numeric strings), +finite, and in `[0, 1]`. `alpha` must be strictly between 0 and 1, and `--boot` +must be between 1 and 1,000,000. The total bootstrap workload is capped at +50,000,000 paired draws (`n_tasks * n_boot`). Exact McNemar evaluation is capped +at 1,000,000 discordant pairs and accumulates its tail from a bounded-memory +stream. JSON parsing is strict throughout the document, including metadata, and +rejects `NaN`/`Infinity` rather than silently accepting non-standard constants. ## Multi-seed -When each task maps to a same-length list of seed repeats, the kit: +When each task maps to a same-length list of seed repeats, the lists are +positional: A and B must use the same seed ordering. The JSON report calls each +position `seed_index`; it does not claim to verify the underlying RNG seed id. +The kit: 1. averages per task across seeds for the headline delta and bootstrap CI 2. resamples whole tasks, preserving the task as the independent cluster 3. omits McNemar rather than treating repeated seeds as independent samples -4. publishes the per-seed deltas plus their mean and sample sd +4. publishes the per-position deltas plus their mean and sample sd as a + descriptive diagnostic only; the sd is not a confidence interval, standard + error, or other inferential uncertainty estimate That is the house answer to single-seed noise (see issue #108 and the single-seed warning in `RESULTS.md`). @@ -43,7 +58,9 @@ SearchQA / GPT-5.4-nano / gated / cumulative nights=5 cell (baseline 0.560, after 0.679, ฮ” +11.9 on n=1400). Per-task pairs were not published, so the replay uses a documented maximum-concordance reconstruction: the first `round(n * rate)` tasks succeed in each condition. The harness recovers the -published delta; it does not claim to recover the original microdata. +published point delta only. It does not claim to recover the original microdata, +and p-values or confidence intervals from the reconstructed pairs must not be +cited as evidence for the published experiment. ## A/A checks @@ -54,6 +71,7 @@ python -m skillopt_sleep.evalkit --manifest tests/fixtures/evalkit/aa_manifest.j The command above is an identity smoke check: identical conditions must report delta 0, McNemar p_exact = 1, and a CI that includes 0. The test suite separately -runs a seeded null simulation with genuine discordant pairs and bounds the -empirical type-I-error rate; comparing one array with itself is not presented as -a statistical calibration. +runs seeded null simulations with genuine discordant pairs, bounds the empirical +McNemar type-I-error rate on both sides of its nominal level, and checks paired +bootstrap coverage. These are seeded null calibration checks. Comparing one +array with itself is not presented as a statistical calibration. diff --git a/mkdocs.yml b/mkdocs.yml index 5d01170c..b5934ad6 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -63,6 +63,7 @@ nav: - CLI Reference: reference/cli.md - API Reference: reference/api.md - Contributing: contributing.md + - Code Review Guidelines: review_guidelines.md markdown_extensions: - admonition diff --git a/skillopt/config.py b/skillopt/config.py index b73573ae..e1fdec04 100644 --- a/skillopt/config.py +++ b/skillopt/config.py @@ -221,12 +221,20 @@ def _resolve_layer_format_duplicates(cfg: dict) -> None: """Prefer canonical structured keys over equivalent flat keys in a layer.""" for dotted, flat_key in _FLATTEN_MAP.items(): if _nested_key_present(cfg, dotted): + # `env.name -> env` maps onto the section name itself: popping it + # would delete the whole env section. Skip that case. + if flat_key == dotted.split(".", 1)[0]: + continue cfg.pop(flat_key, None) def _drop_base_keys_overridden_by_layer(base: dict, override: dict) -> None: """Honor child precedence when inheritance mixes flat and structured YAML.""" for dotted, flat_key in _FLATTEN_MAP.items(): + if flat_key == dotted.split(".", 1)[0]: + # `env.name -> env` maps onto the section name itself: dropping + # it would delete the whole section instead of one key. + continue if flat_key in override or _nested_key_present(override, dotted): base.pop(flat_key, None) _remove_nested_key(base, dotted) diff --git a/skillopt_sleep/__main__.py b/skillopt_sleep/__main__.py index 259de9d4..e3b7794c 100644 --- a/skillopt_sleep/__main__.py +++ b/skillopt_sleep/__main__.py @@ -888,8 +888,8 @@ def main(argv=None) -> int: ) p_eval.add_argument("--manifest", required=True) p_eval.add_argument("--a", required=True) - p_eval.add_argument("--b", default="") - p_eval.add_argument("--aa", action="store_true") + p_eval.add_argument("--b", default=None, help="required unless --aa") + p_eval.add_argument("--aa", action="store_true", help="mutually exclusive with --b") p_eval.add_argument("--alpha", type=float, default=0.05) p_eval.add_argument("--boot", type=int, default=10000) p_eval.add_argument("--seed", type=int, default=42) @@ -914,7 +914,7 @@ def main(argv=None) -> int: if args.cmd == "evalkit": from skillopt_sleep.evalkit import main as evalkit_main argv = ["--manifest", args.manifest, "--a", args.a] - if args.b: + if args.b is not None: argv.extend(["--b", args.b]) if args.aa: argv.append("--aa") diff --git a/skillopt_sleep/adapters/superpowers.py b/skillopt_sleep/adapters/superpowers.py index a9ba168f..3ddf6868 100644 --- a/skillopt_sleep/adapters/superpowers.py +++ b/skillopt_sleep/adapters/superpowers.py @@ -717,7 +717,7 @@ def _run_scenario( if p.is_symlink(): raise ValueError(f"Refusing symlinked overlay path: {p}") skill_dir.mkdir(parents=True, exist_ok=True) - if not skill_dir.resolve().is_relative_to(workspace): + if not skill_dir.resolve().is_relative_to(workspace.resolve()): raise ValueError(f"Skill path {skill_dir} escapes workspace {workspace}") shutil.copy2(skill_overlay, skill_dest, follow_symlinks=False) diff --git a/skillopt_sleep/evalkit.py b/skillopt_sleep/evalkit.py index 753990e7..1c3ba313 100644 --- a/skillopt_sleep/evalkit.py +++ b/skillopt_sleep/evalkit.py @@ -36,14 +36,16 @@ class EvalkitError(ValueError): MAX_BOOTSTRAPS = 1_000_000 +MAX_BOOTSTRAP_DRAWS = 50_000_000 +MAX_MCNEMAR_DISCORDANTS = 1_000_000 def _validate_alpha(alpha: float) -> float: - if isinstance(alpha, bool): + if isinstance(alpha, bool) or not isinstance(alpha, (int, float)): raise EvalkitError("alpha must be a finite number strictly between 0 and 1") try: value = float(alpha) - except (TypeError, ValueError, OverflowError): + except OverflowError: raise EvalkitError("alpha must be a finite number strictly between 0 and 1") from None if not math.isfinite(value) or not 0.0 < value < 1.0: raise EvalkitError("alpha must be a finite number strictly between 0 and 1") @@ -167,16 +169,24 @@ def exact_mcnemar_p(b: int, c: int) -> float: n = b + c if n == 0: return 1.0 + if n > MAX_MCNEMAR_DISCORDANTS: + raise EvalkitError( + "exact McNemar workload exceeds the " + f"{MAX_MCNEMAR_DISCORDANTS} discordant-pair limit" + ) k = min(b, c) # Start at the largest term in the requested lower tail, then recur # downward. This avoids both the enormous int-to-float conversion in # comb(n, k) * 0.5**n and loss from starting at an underflowed 2**-n. - term = _binom_pmf(k, n, 0.5) - terms = [term] - for i in range(k, 0, -1): - term *= i / (n - i + 1) - terms.append(term) - tail = math.fsum(terms) + def lower_tail_terms() -> Iterable[float]: + # Feed fsum lazily so memory stays bounded independently of k. + term = _binom_pmf(k, n, 0.5) + yield term + for i in range(k, 0, -1): + term *= i / (n - i + 1) + yield term + + tail = math.fsum(lower_tail_terms()) return min(1.0, 2.0 * tail) @@ -194,7 +204,14 @@ def mcnemar_from_counts( b_only = _validate_count("b_only", b_only) both_fail = _validate_count("both_fail", both_fail) n = both_success + a_only + b_only + both_fail + if n == 0: + raise EvalkitError("McNemar requires at least one paired observation") disc = a_only + b_only + if disc > MAX_MCNEMAR_DISCORDANTS: + raise EvalkitError( + "exact McNemar workload exceeds the " + f"{MAX_MCNEMAR_DISCORDANTS} discordant-pair limit" + ) if disc == 0: chi2 = 0.0 p_chi2 = 1.0 @@ -247,23 +264,34 @@ def bootstrap_delta_ci( seed = _validate_seed(seed) if len(a) != len(b) or not a: raise EvalkitError("bootstrap requires a non-empty paired sample") - numeric_a: List[float] = [] - numeric_b: List[float] = [] + n = len(a) + if n_boot > MAX_BOOTSTRAP_DRAWS // n: + raise EvalkitError( + "bootstrap workload exceeds the " + f"{MAX_BOOTSTRAP_DRAWS} paired-draw limit (n_tasks * n_boot)" + ) + if any( + isinstance(x, bool) or not isinstance(x, (int, float)) + for values in (a, b) + for x in values + ): + raise EvalkitError("bootstrap scores must be JSON numbers, not booleans or strings") try: numeric_a = [float(x) for x in a] numeric_b = [float(x) for x in b] - except (TypeError, ValueError, OverflowError): + except OverflowError: raise EvalkitError("bootstrap scores must be numeric and finite") from None if any(not math.isfinite(x) for x in numeric_a + numeric_b): raise EvalkitError("bootstrap scores must be numeric and finite") + if any(not 0.0 <= x <= 1.0 for x in numeric_a + numeric_b): + raise EvalkitError("bootstrap scores must be between 0 and 1") rng = random.Random(seed) - n = len(a) + paired_deltas = [right - left for left, right in zip(numeric_a, numeric_b)] deltas: List[float] = [] for _ in range(n_boot): - idx = [rng.randrange(n) for _ in range(n)] - da = sum(numeric_a[i] for i in idx) / n - db = sum(numeric_b[i] for i in idx) / n - deltas.append(db - da) + deltas.append( + math.fsum(paired_deltas[rng.randrange(n)] for _ in range(n)) / n + ) deltas.sort() # Inclusive percentile on the sorted sample. lo_i = int(math.floor((alpha / 2.0) * (n_boot - 1))) @@ -283,40 +311,65 @@ def bootstrap_delta_ci( # โ”€โ”€ pairing / loading โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ def _as_binary(value: Any) -> Optional[int]: - if value is True or value == 1 or value == 1.0: + if isinstance(value, bool) or not isinstance(value, (int, float)): + return None + if value == 1 or value == 1.0: return 1 - if value is False or value == 0 or value == 0.0: + if value == 0 or value == 0.0: return 0 return None +def _task_id(value: Any, *, context: str) -> str: + if not isinstance(value, str) or not value.strip(): + raise EvalkitError(f"{context} task ids must be non-empty JSON strings") + return value + + +def _display_task_id(value: str) -> str: + display = value if len(value) <= 80 else value[:77] + "..." + return repr(display) + + +def _score(value: Any, *, task_id: str) -> float: + if isinstance(value, bool) or not isinstance(value, (int, float)): + raise EvalkitError( + f"task {_display_task_id(task_id)} scores must be JSON numbers, " + "not booleans or strings" + ) + try: + score = float(value) + except OverflowError: + raise EvalkitError( + f"task {_display_task_id(task_id)} scores must be finite and between 0 and 1" + ) from None + if not math.isfinite(score) or not 0.0 <= score <= 1.0: + raise EvalkitError( + f"task {_display_task_id(task_id)} scores must be finite and between 0 and 1" + ) + return score + + def _normalize_outcomes(raw: Mapping[str, Any]) -> Dict[str, List[float]]: - """Map task id -> list of per-seed scores (length 1 if unseeded).""" + """Map task id -> positional repeat scores (length 1 if unseeded).""" if not isinstance(raw, Mapping): raise EvalkitError("outcomes must be a JSON object keyed by task id") out: Dict[str, List[float]] = {} for tid, val in raw.items(): - key = str(tid) - if key in out: - raise EvalkitError(f"duplicate outcome task id after normalization: {key}") - if isinstance(val, Mapping) and "seeds" in val: + key = _task_id(tid, context="outcome") + if isinstance(val, Mapping): + if set(val) != {"seeds"} or not isinstance(val["seeds"], list): + raise EvalkitError( + f"task {_display_task_id(key)} must contain only a seeds array" + ) val = val["seeds"] if isinstance(val, (list, tuple)): values = list(val) else: values = [val] if not values: - raise EvalkitError(f"task {key} has an empty seed list") - normalized: List[float] = [] - for item in values: - try: - score = float(item) - except (TypeError, ValueError, OverflowError): - raise EvalkitError(f"task {key} contains a non-numeric score") from None - if not math.isfinite(score) or not 0.0 <= score <= 1.0: - raise EvalkitError(f"task {key} scores must be finite and between 0 and 1") - normalized.append(score) - out[key] = normalized + raise EvalkitError(f"task {_display_task_id(key)} has an empty seed list") + out[key] = [_score(item, task_id=key) for item in values] return out @@ -326,7 +379,9 @@ def align_pairs( outcomes_b: Mapping[str, Any], ) -> Tuple[List[str], List[List[float]], List[List[float]]]: """Align A and B onto the manifest. Refuse any id-set mismatch.""" - ids = [str(i) for i in manifest_ids] + if isinstance(manifest_ids, (str, bytes)) or not isinstance(manifest_ids, Sequence): + raise EvalkitError("manifest task ids must be a JSON array") + ids = [_task_id(item, context="manifest") for item in manifest_ids] if not ids: raise EvalkitError("manifest is empty") if len(ids) != len(set(ids)): @@ -380,10 +435,15 @@ def reconstruct_paired_from_rates(n: int, rate_a: float, rate_b: float) -> Tuple """ if isinstance(n, bool) or not isinstance(n, int) or n < 1: raise EvalkitError("n must be >= 1") + if any( + isinstance(rate, bool) or not isinstance(rate, (int, float)) + for rate in (rate_a, rate_b) + ): + raise EvalkitError("rates must be JSON numbers, finite, and between 0 and 1") try: numeric_a, numeric_b = float(rate_a), float(rate_b) - except (TypeError, ValueError, OverflowError): - raise EvalkitError("rates must be numeric, finite, and between 0 and 1") from None + except OverflowError: + raise EvalkitError("rates must be finite and between 0 and 1") from None if not all(math.isfinite(rate) and 0.0 <= rate <= 1.0 for rate in (numeric_a, numeric_b)): raise EvalkitError("rates must be finite and between 0 and 1") ka = int(round(n * numeric_a)) @@ -405,6 +465,8 @@ def compare( ) -> EvalReport: alpha = _validate_alpha(alpha) n_boot = _validate_bootstraps(n_boot) + if not isinstance(allow_graded, bool): + raise EvalkitError("allow_graded must be a boolean") ids, a_rows, b_rows = align_pairs(manifest_ids, outcomes_a, outcomes_b) n_seed = len(a_rows[0]) notes: List[str] = [] @@ -446,13 +508,18 @@ def compare( for s in range(n_seed): da = _mean(row[s] for row in a_rows) db = _mean(row[s] for row in b_rows) - per_seed.append({"seed": s, "rate_a": da, "rate_b": db, "delta": db - da}) + per_seed.append( + {"seed_index": s, "rate_a": da, "rate_b": db, "delta": db - da} + ) deltas = [row["delta"] for row in per_seed] seed_mean = _mean(deltas) seed_sd = _sd(deltas) notes.append( - f"multi-seed: {n_seed} repeats; seed-mean delta={seed_mean:.6f} " - f"sd={seed_sd:.6f}" + f"multi-seed positional repeats: {n_seed}; mean delta={seed_mean:.6f} " + f"sample sd={seed_sd:.6f}; descriptive only, not an uncertainty estimate" + ) + notes.append( + "seed lists are positional repeats; seed_index is not a verified RNG seed id" ) return EvalReport( @@ -482,53 +549,87 @@ def compare_aa( # โ”€โ”€ I/O โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ -def _load_json(path: str) -> Any: +def _reject_duplicate_object_keys(pairs: Sequence[Tuple[str, Any]]) -> Dict[str, Any]: + obj: Dict[str, Any] = {} + for key, value in pairs: + if key in obj: + display_key = key if len(key) <= 80 else key[:77] + "..." + raise EvalkitError(f"duplicate JSON object key: {display_key!r}") + obj[key] = value + return obj + + +def _reject_json_constant(value: str) -> Any: + raise EvalkitError(f"non-standard JSON constant {value} is not allowed") + + +def _load_json(path: str, *, label: str) -> Any: try: with open(path, encoding="utf-8") as f: - return json.load(f) - except (json.JSONDecodeError, UnicodeError) as exc: - raise EvalkitError(f"invalid JSON in {path}: {exc}") from None + return json.load( + f, + object_pairs_hook=_reject_duplicate_object_keys, + parse_constant=_reject_json_constant, + ) + except EvalkitError as exc: + raise EvalkitError(f"invalid JSON in {label}: {exc}") from None + except (json.JSONDecodeError, UnicodeError, ValueError, RecursionError) as exc: + if isinstance(exc, json.JSONDecodeError): + detail = exc.msg + elif isinstance(exc, UnicodeError): + detail = "invalid text encoding" + elif isinstance(exc, RecursionError): + detail = "nesting is too deep" + else: + detail = "invalid numeric literal" + raise EvalkitError(f"invalid JSON in {label}: {detail}") from None + except OSError as exc: + detail = exc.strerror or type(exc).__name__ + raise EvalkitError(f"could not read {label}: {detail}") from None def _manifest_ids(obj: Any) -> List[str]: if isinstance(obj, list): - return [str(x) for x in obj] + return [_task_id(item, context="manifest") for item in obj] if isinstance(obj, Mapping): + forms = [name for name in ("ids", "tasks", "outcomes") if name in obj] + if len(forms) > 1: + raise EvalkitError("manifest must use exactly one of ids, tasks, or outcomes") if "ids" in obj: values = obj["ids"] if not isinstance(values, list): raise EvalkitError("manifest ids must be a JSON array") - return [str(x) for x in values] + return [_task_id(item, context="manifest") for item in values] if "tasks" in obj: tasks = obj["tasks"] if not isinstance(tasks, list): raise EvalkitError("manifest tasks must be a JSON array") ids: List[str] = [] for task in tasks: - if isinstance(task, Mapping): - if "id" not in task: - raise EvalkitError("every manifest task object must contain id") - ids.append(str(task["id"])) - else: - ids.append(str(task)) + if not isinstance(task, Mapping) or "id" not in task: + raise EvalkitError("every manifest task must be an object containing id") + ids.append(_task_id(task["id"], context="manifest")) return ids if "outcomes" in obj: outcomes = obj["outcomes"] if not isinstance(outcomes, Mapping): raise EvalkitError("manifest outcomes must be a JSON object") - return [str(k) for k in outcomes] + return [_task_id(key, context="manifest") for key in outcomes] raise EvalkitError("manifest must be a list of ids or an object with ids/tasks") -def _outcomes(obj: Any) -> Dict[str, Any]: - if isinstance(obj, Mapping) and "outcomes" in obj: +def _outcomes(obj: Any, manifest_ids: Sequence[str]) -> Dict[str, Any]: + if not isinstance(obj, Mapping): + raise EvalkitError("outcomes file must be an object mapping task id to score") + # A direct mapping wins when its keys exactly match the manifest. This makes + # a task literally named ``outcomes`` unambiguous even when its score is a + # seeded object. Otherwise the one-key wrapper is recognized. + if set(obj) == set(manifest_ids): + return dict(obj) + if set(obj) == {"outcomes"} and isinstance(obj["outcomes"], Mapping): values = obj["outcomes"] - if not isinstance(values, Mapping): - raise EvalkitError("outcomes must be a JSON object keyed by task id") return dict(values) - if isinstance(obj, Mapping): - return dict(obj) - raise EvalkitError("outcomes file must be an object mapping task id to score") + return dict(obj) def format_markdown(report: EvalReport) -> str: @@ -540,7 +641,7 @@ def format_markdown(report: EvalReport) -> str: f"- rate_b: {report.rate_b:.6f}", f"- delta (B-A): {report.delta:+.6f}", ( - f"- bootstrap {int((1 - report.bootstrap.alpha) * 100)}% CI: " + f"- bootstrap {100 * (1 - report.bootstrap.alpha):g}% CI: " f"[{report.bootstrap.low:+.6f}, {report.bootstrap.high:+.6f}] " f"(n_boot={report.bootstrap.n_boot}, seed={report.bootstrap.seed})" ), @@ -557,8 +658,9 @@ def format_markdown(report: EvalReport) -> str: ) if report.seed_mean_delta is not None: lines.append( - f"- multi-seed mean delta: {report.seed_mean_delta:+.6f} " - f"(sd {report.seed_sd_delta:.6f}, k={len(report.per_seed)})" + f"- positional-repeat mean delta: {report.seed_mean_delta:+.6f} " + f"(descriptive sample sd {report.seed_sd_delta:.6f}, " + f"k={len(report.per_seed)}; not an uncertainty estimate)" ) for note in report.notes: lines.append(f"- note: {note}") @@ -572,8 +674,8 @@ def main(argv: Optional[Sequence[str]] = None) -> int: ) p.add_argument("--manifest", required=True, help="JSON list of task ids (or {ids,tasks})") p.add_argument("--a", required=True, help="JSON outcomes for condition A") - p.add_argument("--b", default="", help="JSON outcomes for condition B (omit for A/A)") - p.add_argument("--aa", action="store_true", help="A/A identity smoke check (ignore --b, reuse --a)") + p.add_argument("--b", default=None, help="JSON outcomes for condition B (required unless --aa)") + p.add_argument("--aa", action="store_true", help="A/A identity smoke check (mutually exclusive with --b)") p.add_argument("--alpha", type=float, default=0.05) p.add_argument("--boot", type=int, default=10000) p.add_argument("--seed", type=int, default=42) @@ -582,15 +684,17 @@ def main(argv: Optional[Sequence[str]] = None) -> int: args = p.parse_args(list(argv) if argv is not None else None) try: - ids = _manifest_ids(_load_json(args.manifest)) - a = _outcomes(_load_json(args.a)) - if args.aa or not args.b: + if (args.b is not None) == args.aa: + raise EvalkitError("exactly one of --b or --aa is required") + ids = _manifest_ids(_load_json(args.manifest, label="--manifest")) + a = _outcomes(_load_json(args.a, label="--a"), ids) + if args.aa: report = compare_aa( ids, a, alpha=args.alpha, n_boot=args.boot, seed=args.seed, allow_graded=args.allow_graded, ) else: - b = _outcomes(_load_json(args.b)) + b = _outcomes(_load_json(args.b, label="--b"), ids) report = compare( ids, a, b, alpha=args.alpha, n_boot=args.boot, seed=args.seed, allow_graded=args.allow_graded, @@ -598,9 +702,6 @@ def main(argv: Optional[Sequence[str]] = None) -> int: except EvalkitError as exc: print(f"ERR_EVALKIT {exc}", file=sys.stderr) return 2 - except OSError as exc: - print(f"ERR_EVALKIT {exc}", file=sys.stderr) - return 1 if args.json: try: diff --git a/skillopt_sleep/staging.py b/skillopt_sleep/staging.py index e2ecfbd5..615c5ac8 100644 --- a/skillopt_sleep/staging.py +++ b/skillopt_sleep/staging.py @@ -439,6 +439,7 @@ def _write_atomic_bytes( *, create_parents: bool = True, mode: Optional[int] = None, + replace_permission_retries: int = 0, ) -> None: """Write raw bytes atomically, optionally restoring an exact file mode.""" directory = os.path.dirname(path) or "." @@ -458,7 +459,18 @@ def _write_atomic_bytes( else: os.chmod(tmp, existing_mode) os.fsync(f.fileno()) - os.replace(tmp, path) + # A caller may explicitly tolerate a bounded transient Windows sharing + # violation. Live files, WALs, receipts, backups, and rollback always + # use the zero-retry default so a concurrent editor cannot be overwritten + # after the caller's compare-and-swap validation. + for attempt in range(replace_permission_retries + 1): + try: + os.replace(tmp, path) + break + except PermissionError: + if os.name != "nt" or attempt == replace_permission_retries: + raise + time.sleep(0.005 * (attempt + 1)) _fsync_parent(path) except BaseException: try: @@ -535,7 +547,10 @@ def _remove_private_temp_aliases(path: str) -> None: if not entry.name.startswith(".tmp-new-") or entry.path == path: continue try: - candidate = entry.stat(follow_symlinks=False) + # ``DirEntry.stat()`` reports zeroed file IDs/link counts on the + # Windows GitHub runner. A path-based lstat returns the actual + # NTFS identity and keeps the hard-link comparison meaningful. + candidate = os.lstat(entry.path) if ( stat.S_ISREG(candidate.st_mode) and (candidate.st_dev, candidate.st_ino) == file_id @@ -546,6 +561,18 @@ def _remove_private_temp_aliases(path: str) -> None: continue if removed: _fsync_directory(directory) + if os.name == "nt": + # NTFS can report the pre-unlink link count briefly after the + # directory entry is gone. Wait only for metadata convergence; + # any surviving hard link still leaves nlink > 1 and the caller + # will continue to fail closed. + for attempt in range(20): + try: + if os.lstat(path).st_nlink <= 1: + break + except OSError: + break + time.sleep(0.005 * (attempt + 1)) def _artifact_snapshot(path: str) -> Optional[tuple[bytes, int]]: @@ -915,7 +942,14 @@ def _publish_latest(root: str, out: str) -> None: or info.st_nlink != 1 ): raise StagingError(f"latest-staging pointer is unsafe: {pointer}") - _write_atomic_bytes(pointer, f"{name}\n".encode("utf-8"), mode=0o600) + # Concurrent staging publishers may briefly retain the replace destination + # on Windows. Retrying is safe only for this derived, last-writer-wins pointer. + _write_atomic_bytes( + pointer, + f"{name}\n".encode("utf-8"), + mode=0o600, + replace_permission_retries=20, + ) def _staging_order(path: str) -> tuple: @@ -1902,14 +1936,32 @@ def _existing_path_is_canonical_staging_descendant( The staging root itself may be supplied through a symlink, so compare the resolved candidate with the same relative path beneath the resolved root. """ - try: - relative = os.path.relpath(path, staging_dir) - except ValueError: - return False - if relative == os.pardir or relative.startswith(os.pardir + os.sep): - return False - expected_real = os.path.join(os.path.realpath(staging_dir), relative) - return _path_identity_key(os.path.realpath(path)) == _path_identity_key(expected_real) + candidate = os.path.abspath(path) + root = os.path.abspath(staging_dir) + # Compare actual directory identities while walking upward. This tolerates + # equivalent root spellings (/var vs /private/var and Windows 8.3 vs long + # names) without resolving away a symlink/junction *below* the root. The + # supplied root itself may be an alias, so test its identity before applying + # the descendant-link refusal. + current = candidate + while True: + try: + if os.path.samefile(current, root): + if ( + _path_identity_key(current) != _path_identity_key(root) + and _path_is_within(current, root) + and _is_link_or_junction(current) + ): + return False + return current != candidate + except OSError: + return False + if _is_link_or_junction(current): + return False + parent = os.path.dirname(current) + if parent == current: + return False + current = parent def _immutable_backup_snapshot( diff --git a/tests/fixtures/evalkit/results_searchqa_nano_gated.json b/tests/fixtures/evalkit/results_searchqa_nano_gated.json index 5df05905..3122c7fe 100644 --- a/tests/fixtures/evalkit/results_searchqa_nano_gated.json +++ b/tests/fixtures/evalkit/results_searchqa_nano_gated.json @@ -5,5 +5,6 @@ "baseline": 0.560, "after": 0.679, "published_delta": 0.119, - "reconstruction": "maximum-concordance: first round(n*rate) tasks succeed in each condition" + "reconstruction": "maximum-concordance: first round(n*rate) tasks succeed in each condition", + "inference": "unsupported: reconstructed pairs recover the point delta only" } diff --git a/tests/test_devin_plugin.py b/tests/test_devin_plugin.py index cc2fe637..0d449f7a 100644 --- a/tests/test_devin_plugin.py +++ b/tests/test_devin_plugin.py @@ -364,6 +364,7 @@ def test_env_tilde_is_expanded(self): importlib.reload(mcp_server) +@unittest.skipIf(os.name == "nt", "Devin installer and hook are POSIX shell scripts") class TestDevinInstaller(unittest.TestCase): def _run_installer(self, project, home, installer=INSTALLER): env = os.environ.copy() diff --git a/tests/test_env_section_survives_dedup.py b/tests/test_env_section_survives_dedup.py new file mode 100644 index 00000000..69151677 --- /dev/null +++ b/tests/test_env_section_survives_dedup.py @@ -0,0 +1,80 @@ +"""Regression: the ``env`` section must survive layer-format dedup. + +``_resolve_layer_format_duplicates`` pops a flat key whenever the +equivalent structured key is present. For the mapping ``env.name -> env`` +the flat key is the section name itself, so popping it deleted the entire +``env`` block from every structured config (base and child) before +inheritance โ€” ``env.name``, ``env.split_dir``, ``env.skill_init``, etc. +were silently lost, which broke ``load_config`` for every environment. + +The same section-name collision also applied to +``_drop_base_keys_overridden_by_layer``, which deleted a base's whole +``env`` section whenever a child overrode ``env.name``. +""" + +from __future__ import annotations + +from skillopt.config import _load_yaml, flatten_config, load_config + + +def test_env_section_survives_dedup_in_base(tmp_path): + base = tmp_path / "base.yaml" + base.write_text( + "env:\n" + " name: searchqa\n" + " split_mode: split_dir\n" + " split_dir: data/searchqa_split\n" + " workers: 24\n", + encoding="utf-8", + ) + cfg = load_config(str(base)) + assert cfg["env"]["name"] == "searchqa" + assert cfg["env"]["split_dir"] == "data/searchqa_split" + + +def test_env_section_survives_child_inheritance(tmp_path): + base = tmp_path / "base.yaml" + base.write_text( + "env:\n" + " name: base\n" + " split_mode: ratio\n" + " workers: 4\n", + encoding="utf-8", + ) + child = tmp_path / "child.yaml" + child.write_text( + "_base_: base.yaml\n" + "env:\n" + " name: pricewatch\n" + " split_mode: split_dir\n" + " split_dir: data/pricewatch_split\n", + encoding="utf-8", + ) + cfg = load_config(str(child)) + assert cfg["env"]["name"] == "pricewatch" + assert cfg["env"]["split_mode"] == "split_dir" + assert cfg["env"]["split_dir"] == "data/pricewatch_split" + # Inherited env key is preserved alongside the child overrides. + assert cfg["env"]["workers"] == 4 + + +def test_flatten_config_keeps_env_keys(tmp_path): + config = tmp_path / "c.yaml" + config.write_text( + "env:\n" + " name: pricewatch\n" + " split_dir: data/pricewatch_split\n" + " max_completion_tokens: 2048\n", + encoding="utf-8", + ) + flat = flatten_config(load_config(str(config))) + assert flat["env"] == "pricewatch" + assert flat["split_dir"] == "data/pricewatch_split" + assert flat["max_completion_tokens"] == 2048 + + +def test_shipped_searchqa_config_still_loads_env(): + # Guard against regressions in the repo's own environment configs. + cfg = _load_yaml("configs/searchqa/default.yaml") + assert cfg["env"]["name"] == "searchqa" + assert cfg["env"]["split_dir"] == "data/searchqa_split" diff --git a/tests/test_evalkit.py b/tests/test_evalkit.py index fb8be99b..556925e9 100644 --- a/tests/test_evalkit.py +++ b/tests/test_evalkit.py @@ -11,8 +11,11 @@ import tempfile import unittest from contextlib import redirect_stderr, redirect_stdout +from unittest.mock import patch from skillopt_sleep.evalkit import ( + MAX_BOOTSTRAP_DRAWS, + MAX_MCNEMAR_DISCORDANTS, EvalkitError, bootstrap_delta_ci, compare, @@ -67,9 +70,25 @@ def test_paired_vectors_match_counts(self): def test_exact_tail_is_stable_above_one_thousand_discordants(self): self.assertEqual(exact_mcnemar_p(700, 700), 1.0) value = exact_mcnemar_p(590, 611) - self.assertTrue(math.isfinite(value)) - self.assertGreater(value, 0.0) - self.assertLessEqual(value, 1.0) + # Independent reference from scipy.stats.binomtest(590, 1201, 0.5). + self.assertAlmostEqual(value, 0.563883319454372, places=10) + self.assertEqual(value, exact_mcnemar_p(611, 590)) + + def test_exact_tail_has_a_documented_resource_limit(self): + with self.assertRaisesRegex(EvalkitError, "discordant-pair limit"): + exact_mcnemar_p(MAX_MCNEMAR_DISCORDANTS + 1, 0) + with self.assertRaisesRegex(EvalkitError, "discordant-pair limit"): + mcnemar_from_counts(0, MAX_MCNEMAR_DISCORDANTS, 1, 0) + + def test_exact_tail_accumulates_from_a_lazy_stream(self): + real_fsum = math.fsum + + def consume(values): + self.assertNotIsInstance(values, (list, tuple)) + return real_fsum(values) + + with patch("skillopt_sleep.evalkit.math.fsum", side_effect=consume): + self.assertAlmostEqual(exact_mcnemar_p(590, 611), 0.563883319454372) def test_invalid_counts_and_binary_vectors_are_refused(self): for args in ((-1, 0), (True, 0), (1.5, 0)): @@ -107,7 +126,7 @@ def test_seed_is_deterministic(self): self.assertEqual((x.low, x.high, x.mean), (y.low, y.high, y.mean)) def test_invalid_alpha_and_bootstrap_counts_are_refused(self): - for alpha in (0, 1, -0.1, 1.1, float("nan"), float("inf"), True): + for alpha in (0, 1, -0.1, 1.1, float("nan"), float("inf"), True, "0.05"): with self.subTest(alpha=alpha), self.assertRaises(EvalkitError): bootstrap_delta_ci([0], [1], alpha=alpha) for n_boot in (0, -1, 1.5, True, 1_000_001): @@ -117,11 +136,31 @@ def test_invalid_alpha_and_bootstrap_counts_are_refused(self): with self.subTest(seed=seed), self.assertRaises(EvalkitError): bootstrap_delta_ci([0], [1], n_boot=10, seed=seed) + def test_total_bootstrap_draws_are_bounded(self): + n_tasks = 100 + excessive_bootstraps = MAX_BOOTSTRAP_DRAWS // n_tasks + 1 + with self.assertRaisesRegex(EvalkitError, "paired-draw limit"): + bootstrap_delta_ci( + [0] * n_tasks, + [1] * n_tasks, + n_boot=excessive_bootstraps, + ) + def test_malformed_direct_api_scores_are_contract_errors(self): - for value in ("not-a-number", None, object(), float("nan"), float("inf")): + for value in ( + "not-a-number", "1", True, None, object(), 10**1000, + float("nan"), float("inf"), + ): with self.subTest(value=value), self.assertRaises(EvalkitError): bootstrap_delta_ci([0], [value], n_boot=10) + def test_direct_api_scores_must_be_in_the_unit_interval(self): + for value in (-1, -0.00001, 1.00001, 2): + with self.subTest(value=value), self.assertRaisesRegex( + EvalkitError, "between 0 and 1" + ): + bootstrap_delta_ci([0], [value], n_boot=10) + class TestAACalibration(unittest.TestCase): def test_aa_does_not_reject(self): @@ -151,9 +190,27 @@ def test_exact_test_controls_type_one_error_under_a_seeded_null(self): b.append(right) rejected += int(mcnemar_paired(a, b, alpha=0.05).significant) rate = rejected / trials - self.assertGreater(rejected, 0) + self.assertGreaterEqual(rate, 0.025) self.assertLessEqual(rate, 0.075) + def test_paired_bootstrap_has_nominal_coverage_under_a_seeded_null(self): + rng = random.Random(20260825) + trials = 160 + covered = 0 + for trial in range(trials): + a = [] + b = [] + for _task in range(80): + left = int(rng.random() < 0.5) + right = 1 - left if rng.random() < 0.30 else left + a.append(left) + b.append(right) + ci = bootstrap_delta_ci(a, b, n_boot=300, seed=10_000 + trial) + covered += int(ci.low <= 0.0 <= ci.high) + coverage = covered / trials + self.assertGreaterEqual(coverage, 0.90) + self.assertLessEqual(coverage, 0.99) + class TestCompareContracts(unittest.TestCase): def test_mismatched_ids_are_refused(self): @@ -165,10 +222,38 @@ def test_duplicate_manifest_ids_refused(self): with self.assertRaises(EvalkitError): compare(["t1", "t1"], {"t1": 1}, {"t1": 0}) + def test_empty_count_table_refused(self): + with self.assertRaisesRegex(EvalkitError, "at least one paired observation"): + mcnemar_from_counts(0, 0, 0, 0) + def test_empty_manifest_refused(self): with self.assertRaises(EvalkitError): compare([], {}, {}) + def test_task_ids_are_nonempty_strings_without_coercion(self): + for task_id in (1, {}, [], None, True, "", " "): + with self.subTest(task_id=task_id), self.assertRaisesRegex( + EvalkitError, "non-empty JSON strings" + ): + compare([task_id], {}, {}) + with self.assertRaisesRegex(EvalkitError, "non-empty JSON strings"): + compare(["t1"], {"": 0}, {"t1": 1}) + + def test_scores_are_json_numbers_without_coercion(self): + for score in ("0", "1.0", True, False, None): + with self.subTest(score=score), self.assertRaisesRegex( + EvalkitError, "JSON numbers" + ): + compare(["t1"], {"t1": score}, {"t1": 1}) + with self.assertRaisesRegex(EvalkitError, "finite and between 0 and 1"): + compare(["t1"], {"t1": 10**1000}, {"t1": 1}) + with self.assertRaisesRegex(EvalkitError, "contain only a seeds array"): + compare( + ["t1"], + {"t1": {"seeds": [0, 1], "ignored": 1}}, + {"t1": [0, 1]}, + ) + def test_graded_refused_without_flag(self): with self.assertRaises(EvalkitError) as ctx: compare(["t1", "t2"], {"t1": 0.4, "t2": 0.9}, {"t1": 0.5, "t2": 0.8}) @@ -196,6 +281,11 @@ def test_multi_seed_variance_band(self): seed=2, ) self.assertEqual(len(report.per_seed), 3) + self.assertEqual( + [row["seed_index"] for row in report.per_seed], + [0, 1, 2], + ) + self.assertTrue(any("positional repeats" in note for note in report.notes)) self.assertIsNotNone(report.seed_mean_delta) self.assertGreaterEqual(report.seed_sd_delta, 0.0) self.assertAlmostEqual(report.rate_a, (2 / 3 + 1 / 3) / 2) @@ -222,8 +312,53 @@ def test_duplicating_seeds_within_tasks_does_not_inflate_inference(self): self.assertEqual(two.delta, many.delta) self.assertEqual(two.bootstrap.to_dict(), many.bootstrap.to_dict()) + def test_heterogeneous_seed_duplication_does_not_change_cluster_inference(self): + ids = ["t1", "t2", "t3", "t4"] + a = { + "t1": [0, 1], + "t2": [1, 0], + "t3": [0, 0], + "t4": [1, 1], + } + b = { + "t1": [1, 1], + "t2": [0, 0], + "t3": [0, 1], + "t4": [1, 0], + } + duplicated_a = {task_id: values * 50 for task_id, values in a.items()} + duplicated_b = {task_id: values * 50 for task_id, values in b.items()} + original = compare(ids, a, b, n_boot=500, seed=8) + duplicated = compare(ids, duplicated_a, duplicated_b, n_boot=500, seed=8) + self.assertEqual(original.delta, duplicated.delta) + self.assertEqual(original.bootstrap.to_dict(), duplicated.bootstrap.to_dict()) + self.assertIsNone(original.mcnemar) + self.assertIsNone(duplicated.mcnemar) + + def test_repeats_cannot_be_inflated_for_only_one_task(self): + with self.assertRaisesRegex( + EvalkitError, + "every task must have the same number of seed repeats", + ): + compare( + ["t1", "t2"], + {"t1": [0, 1] * 50, "t2": [0, 1]}, + {"t1": [1, 1] * 50, "t2": [1, 0]}, + n_boot=20, + ) + + def test_positional_repeat_sd_is_explicitly_noninferential(self): + report = compare( + ["t1", "t2"], + {"t1": [0, 1], "t2": [1, 1]}, + {"t1": [1, 1], "t2": [0, 1]}, + n_boot=20, + ) + self.assertTrue(any("not an uncertainty estimate" in note for note in report.notes)) + self.assertIn("descriptive sample sd", format_markdown(report)) + def test_invalid_compare_parameters_are_refused(self): - for alpha in (0, 1, float("nan")): + for alpha in (0, 1, float("nan"), "0.05", 10**1000): with self.subTest(alpha=alpha), self.assertRaises(EvalkitError): compare(["t1"], {"t1": 0}, {"t1": 1}, alpha=alpha) for n_boot in (0, -4, True): @@ -233,33 +368,25 @@ def test_invalid_compare_parameters_are_refused(self): class TestResultsCellReplay(unittest.TestCase): def test_malformed_reconstruction_inputs_are_contract_errors(self): - for rates in (("bad", 0.5), (None, 0.5), (float("nan"), 0.5), (0.5, 1.1)): + for rates in ( + ("bad", 0.5), ("0.5", 0.5), (None, 0.5), + (10**1000, 0.5), (float("nan"), 0.5), (0.5, 1.1), + ): with self.subTest(rates=rates), self.assertRaises(EvalkitError): reconstruct_paired_from_rates(10, *rates) for n in (True, 0, 1.5): with self.subTest(n=n), self.assertRaises(EvalkitError): reconstruct_paired_from_rates(n, 0.5, 0.5) - def test_published_searchqa_nano_gated_delta(self): + def test_published_searchqa_nano_gated_point_delta(self): cell = _load("results_searchqa_nano_gated.json") a, b = reconstruct_paired_from_rates(cell["n"], cell["baseline"], cell["after"]) self.assertEqual(len(a), cell["n"]) - self.assertAlmostEqual(sum(a) / cell["n"], cell["baseline"], places=3) - self.assertAlmostEqual(sum(b) / cell["n"], cell["after"], places=3) - ids = [f"q{i:04d}" for i in range(cell["n"])] - report = compare( - ids, - dict(zip(ids, a)), - dict(zip(ids, b)), - n_boot=800, - seed=42, - ) - self.assertAlmostEqual(report.delta, cell["published_delta"], places=3) - self.assertGreater(report.bootstrap.low, 0.0) - self.assertTrue(report.mcnemar.significant) - md = format_markdown(report) - self.assertIn("delta (B-A)", md) - self.assertIn("McNemar", md) + rate_a = math.fsum(a) / cell["n"] + rate_b = math.fsum(b) / cell["n"] + self.assertAlmostEqual(rate_a, cell["baseline"], places=3) + self.assertAlmostEqual(rate_b, cell["after"], places=3) + self.assertAlmostEqual(rate_b - rate_a, cell["published_delta"], places=3) class TestCLI(unittest.TestCase): @@ -287,6 +414,144 @@ def test_mismatch_cli_exit_two(self): rc = evalkit_main(["--manifest", man, "--a", a, "--b", b]) self.assertEqual(rc, 2) + def test_exactly_one_of_b_or_aa_is_required(self): + manifest = os.path.join(FIXTURE_DIR, "aa_manifest.json") + outcomes = os.path.join(FIXTURE_DIR, "aa_outcomes.json") + for extra in ([], ["--b", outcomes, "--aa"], ["--b", "", "--aa"]): + with self.subTest(extra=extra): + stdout = io.StringIO() + stderr = io.StringIO() + with redirect_stdout(stdout), redirect_stderr(stderr): + rc = evalkit_main([ + "--manifest", manifest, + "--a", outcomes, + "--json", + *extra, + ]) + self.assertEqual(rc, 2) + self.assertEqual(stdout.getvalue(), "") + self.assertIn("exactly one of --b or --aa", stderr.getvalue()) + + def test_umbrella_cli_enforces_b_or_aa_exclusivity(self): + manifest = os.path.join(FIXTURE_DIR, "aa_manifest.json") + outcomes = os.path.join(FIXTURE_DIR, "aa_outcomes.json") + base = [ + sys.executable, + "-m", + "skillopt_sleep", + "evalkit", + "--manifest", + manifest, + "--a", + outcomes, + "--json", + ] + for extra in ([], ["--b", outcomes, "--aa"], ["--b", "", "--aa"]): + with self.subTest(extra=extra): + proc = subprocess.run( + [*base, *extra], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(proc.returncode, 2) + self.assertEqual(proc.stdout, "") + self.assertIn("exactly one of --b or --aa", proc.stderr) + + def test_umbrella_cli_accepts_explicit_aa(self): + proc = subprocess.run( + [ + sys.executable, + "-m", + "skillopt_sleep", + "evalkit", + "--manifest", + os.path.join(FIXTURE_DIR, "aa_manifest.json"), + "--a", + os.path.join(FIXTURE_DIR, "aa_outcomes.json"), + "--aa", + "--boot", + "20", + "--json", + ], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertEqual(json.loads(proc.stdout)["delta"], 0.0) + + def test_duplicate_json_object_keys_are_refused(self): + with tempfile.TemporaryDirectory() as td: + man = os.path.join(td, "m.json") + a = os.path.join(td, "a.json") + b = os.path.join(td, "b.json") + with open(man, "w", encoding="utf-8") as handle: + handle.write('["t1"]') + with open(a, "w", encoding="utf-8") as handle: + handle.write('{"t1": 0, "t1": 1}') + with open(b, "w", encoding="utf-8") as handle: + handle.write('{"t1": 1}') + stdout = io.StringIO() + stderr = io.StringIO() + with redirect_stdout(stdout), redirect_stderr(stderr): + rc = evalkit_main([ + "--manifest", man, + "--a", a, + "--b", b, + "--json", + ]) + self.assertEqual(rc, 2) + self.assertEqual(stdout.getvalue(), "") + self.assertIn("duplicate JSON object key: 't1'", stderr.getvalue()) + + def test_task_named_outcomes_is_not_mistaken_for_wrapper(self): + with tempfile.TemporaryDirectory() as td: + paths = [] + for name, content in ( + ("m.json", '["outcomes"]'), + ("a.json", '{"outcomes": 0}'), + ("b.json", '{"outcomes": 1}'), + ): + path = os.path.join(td, name) + with open(path, "w", encoding="utf-8") as handle: + handle.write(content) + paths.append(path) + stdout = io.StringIO() + stderr = io.StringIO() + with redirect_stdout(stdout), redirect_stderr(stderr): + rc = evalkit_main([ + "--manifest", paths[0], + "--a", paths[1], + "--b", paths[2], + "--boot", "20", + "--json", + ]) + self.assertEqual(rc, 0, stderr.getvalue()) + self.assertEqual(json.loads(stdout.getvalue())["delta"], 1.0) + + def test_task_named_outcomes_accepts_seeded_object_values(self): + with tempfile.TemporaryDirectory() as td: + paths = [] + for name, content in ( + ("m.json", '["outcomes"]'), + ("a.json", '{"outcomes": {"seeds": [0, 1]}}'), + ("b.json", '{"outcomes": {"seeds": [1, 1]}}'), + ): + path = os.path.join(td, name) + with open(path, "w", encoding="utf-8") as handle: + handle.write(content) + paths.append(path) + stdout = io.StringIO() + stderr = io.StringIO() + with redirect_stdout(stdout), redirect_stderr(stderr): + rc = evalkit_main([ + "--manifest", paths[0], "--a", paths[1], "--b", paths[2], + "--boot", "20", "--json", + ]) + self.assertEqual(rc, 0, stderr.getvalue()) + self.assertEqual(json.loads(stdout.getvalue())["delta"], 0.5) + def test_malformed_json_and_shapes_are_clean_contract_errors(self): cases = ( ("{", '{"t1": 1}', '{"t1": 1}'), @@ -343,9 +608,79 @@ def test_nonfinite_input_is_refused_without_nonstandard_json_output(self): ]) self.assertEqual(rc, 2) self.assertEqual(stdout.getvalue(), "") - self.assertIn("finite", stderr.getvalue()) + self.assertIn("non-standard JSON constant", stderr.getvalue()) self.assertNotIn("NaN", stdout.getvalue()) + def test_nonstandard_json_constants_are_rejected_even_in_ignored_metadata(self): + for constant in ("NaN", "Infinity", "-Infinity"): + with self.subTest(constant=constant), tempfile.TemporaryDirectory() as td: + man = os.path.join(td, "m.json") + a = os.path.join(td, "a.json") + b = os.path.join(td, "b.json") + with open(man, "w", encoding="utf-8") as handle: + handle.write('{"ids": ["t1"], "ignored": ' + constant + "}") + with open(a, "w", encoding="utf-8") as handle: + handle.write('{"t1": 0}') + with open(b, "w", encoding="utf-8") as handle: + handle.write('{"t1": 1}') + stdout = io.StringIO() + stderr = io.StringIO() + with redirect_stdout(stdout), redirect_stderr(stderr): + rc = evalkit_main([ + "--manifest", man, "--a", a, "--b", b, "--json", + ]) + self.assertEqual(rc, 2) + self.assertEqual(stdout.getvalue(), "") + self.assertIn("non-standard JSON constant", stderr.getvalue()) + + def test_malformed_manifest_ids_are_rejected_by_the_cli(self): + invalid_ids = (1, {}, [], None, True, "", " ") + for task_id in invalid_ids: + with self.subTest(task_id=task_id), tempfile.TemporaryDirectory() as td: + man = os.path.join(td, "m.json") + a = os.path.join(td, "a.json") + with open(man, "w", encoding="utf-8") as handle: + json.dump({"tasks": [{"id": task_id}]}, handle) + with open(a, "w", encoding="utf-8") as handle: + json.dump({"t1": 0}, handle) + stderr = io.StringIO() + with redirect_stderr(stderr): + rc = evalkit_main(["--manifest", man, "--a", a, "--aa"]) + self.assertEqual(rc, 2) + self.assertIn("non-empty JSON strings", stderr.getvalue()) + + def test_conflicting_manifest_forms_are_rejected(self): + with tempfile.TemporaryDirectory() as td: + man = os.path.join(td, "m.json") + a = os.path.join(td, "a.json") + with open(man, "w", encoding="utf-8") as handle: + json.dump({"ids": ["t1"], "tasks": [{"id": "t1"}]}, handle) + with open(a, "w", encoding="utf-8") as handle: + json.dump({"t1": 0}, handle) + stderr = io.StringIO() + with redirect_stderr(stderr): + rc = evalkit_main(["--manifest", man, "--a", a, "--aa"]) + self.assertEqual(rc, 2) + self.assertIn("exactly one of ids, tasks, or outcomes", stderr.getvalue()) + + def test_input_errors_do_not_disclose_paths(self): + with tempfile.TemporaryDirectory() as td: + secret_name = "secret-customer-path.json" + path = os.path.join(td, secret_name) + stderr = io.StringIO() + with redirect_stderr(stderr): + rc = evalkit_main(["--manifest", path, "--a", path, "--aa"]) + self.assertEqual(rc, 2) + self.assertNotIn(td, stderr.getvalue()) + self.assertNotIn(secret_name, stderr.getvalue()) + self.assertIn("--manifest", stderr.getvalue()) + + def test_confidence_label_is_not_truncated_by_float_roundoff(self): + report = compare_aa(["t1"], {"t1": 1}, alpha=0.34, n_boot=20) + rendered = format_markdown(report) + self.assertIn("bootstrap 66% CI", rendered) + self.assertNotIn("bootstrap 65% CI", rendered) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_pi_integration.py b/tests/test_pi_integration.py index 30895ba7..3fb39966 100644 --- a/tests/test_pi_integration.py +++ b/tests/test_pi_integration.py @@ -56,7 +56,7 @@ def test_explicit_pi_source_routes_only_to_pi_harvester(): assert actual == expected pi.assert_called_once_with( - "/tmp/pi-home/agent/sessions", + cfg.pi_sessions_dir, scope="invoked", invoked_project="/repo/project", since_iso="2026-01-01T00:00:00Z", diff --git a/tests/test_sleep_adopt_skill_subset.py b/tests/test_sleep_adopt_skill_subset.py index 9c26c588..f6c32af5 100644 --- a/tests/test_sleep_adopt_skill_subset.py +++ b/tests/test_sleep_adopt_skill_subset.py @@ -33,14 +33,22 @@ def _sha(text): return hashlib.sha256(text.encode("utf-8")).hexdigest() +def _canonical(path): + return os.path.realpath(os.path.abspath(path)) + + +def _same_path(left, right): + return os.path.normcase(_canonical(left)) == os.path.normcase(_canonical(right)) + + def _read(path): - with open(path, encoding="utf-8") as f: + with open(path, encoding="utf-8", newline="") as f: return f.read() def _write(path, text): os.makedirs(os.path.dirname(path), exist_ok=True) - with open(path, "w", encoding="utf-8") as f: + with open(path, "w", encoding="utf-8", newline="") as f: f.write(text) @@ -55,7 +63,9 @@ def __init__( beta_body="# beta v1\n", ): self.tmp = tmp - self.live_root = os.path.join(tmp, "live") + # Keep the project's lexical spelling so status/latest receipts remain + # user-facing, but pin live targets to one canonical filesystem identity. + self.live_root = os.path.join(_canonical(tmp), "live") self.alpha_live = os.path.join(self.live_root, "alpha", "SKILL.md") self.beta_live = os.path.join(self.live_root, "beta", "SKILL.md") for path, body in ( @@ -90,6 +100,7 @@ class TestAdoptionIsConfinedToTheStagedRoots(unittest.TestCase): """ def _retarget(self, staging, skill_name, new_live): + new_live = _canonical(new_live) manifest_path = os.path.join(staging, "manifest.json") with open(manifest_path, encoding="utf-8") as handle: manifest = json.load(handle) @@ -324,6 +335,8 @@ def test_manifest_proposal_filename_cannot_escape_staging(self): self.assertEqual(_read(night.alpha_live), "# alpha v1\n") def test_adoption_preserves_existing_live_file_mode(self): + if os.name == "nt": + self.skipTest("Windows does not provide POSIX file-mode semantics") with tempfile.TemporaryDirectory() as tmp: night = TwoSkillNight(tmp) os.chmod(night.alpha_live, 0o640) @@ -338,7 +351,7 @@ def test_a_failed_write_rolls_the_whole_selection_back(self): real_write = staging_mod._write_atomic def boom(path, text, *, create_parents=True): - if path == night.beta_live: + if _same_path(path, night.beta_live): raise OSError("disk full") return real_write(path, text, create_parents=create_parents) @@ -359,7 +372,7 @@ def test_post_commit_live_write_error_rolls_the_whole_selection_back(self): def commit_then_fail(path, text, *, create_parents=True): result = real_write(path, text, create_parents=create_parents) - if path == night.beta_live: + if _same_path(path, night.beta_live): raise OSError("late close failure") return result @@ -385,7 +398,7 @@ def test_rollback_removes_files_that_did_not_exist_before(self): real_write = staging_mod._write_atomic def boom(path, text, *, create_parents=True): - if path == night.beta_live: + if _same_path(path, night.beta_live): raise OSError("disk full") return real_write(path, text, create_parents=create_parents) @@ -543,6 +556,8 @@ def test_repeated_noop_adoption_cannot_rewrite_receipt_or_backup(self): self.assertEqual(_read(backup_path), backup_before) def test_rollback_restores_original_mode_as_well_as_bytes(self): + if os.name == "nt": + self.skipTest("Windows does not provide POSIX file-mode semantics") from skillopt_sleep import staging as staging_mod with tempfile.TemporaryDirectory() as tmp: @@ -551,7 +566,7 @@ def test_rollback_restores_original_mode_as_well_as_bytes(self): real_write = staging_mod._write_atomic def boom(path, text, *, create_parents=True): - if path == night.beta_live: + if _same_path(path, night.beta_live): raise OSError("disk full") return real_write(path, text, create_parents=create_parents) @@ -574,7 +589,7 @@ def test_backup_failure_rolls_back_prior_live_writes(self): ) def boom(path, data, *, mode=None): - if path == beta_backup: + if _same_path(path, beta_backup): raise OSError("backup device full") return real_write_new(path, data, mode=mode) @@ -1561,7 +1576,7 @@ def test_concurrent_adoption_cleanly_refuses_one_writer(self): real_write = staging_mod._write_atomic def pause_first_live_write(path, text, *, create_parents=True): - if path == night.alpha_live and not entered.is_set(): + if _same_path(path, night.alpha_live) and not entered.is_set(): entered.set() if not release.wait(5): raise RuntimeError("test timed out waiting for release") @@ -1593,7 +1608,7 @@ def test_separate_nights_share_the_same_live_target_lock(self): from skillopt_sleep import staging as staging_mod with tempfile.TemporaryDirectory() as tmp: - live = os.path.join(tmp, "live", "alpha", "SKILL.md") + live = os.path.join(_canonical(tmp), "live", "alpha", "SKILL.md") _write(live, "# alpha v1\n") def stage(proposal): @@ -1616,7 +1631,7 @@ def stage(proposal): real_write = staging_mod._write_atomic def pause_first_live_write(path, text, *, create_parents=True): - if path == live and not entered.is_set(): + if _same_path(path, live) and not entered.is_set(): entered.set() if not release.wait(5): raise RuntimeError("test timed out waiting for release") @@ -1712,8 +1727,9 @@ def test_cycle_skips_empty_proposed_skill_with_a_note(self): class TestDurableAdoptionTransaction(unittest.TestCase): def _legacy_night(self, tmp): - skill = os.path.join(tmp, "live", "skill", "SKILL.md") - memory = os.path.join(tmp, "live", "CLAUDE.md") + live_root = os.path.join(_canonical(tmp), "live") + skill = os.path.join(live_root, "skill", "SKILL.md") + memory = os.path.join(live_root, "CLAUDE.md") _write(skill, "# skill v1\n") _write(memory, "# memory v1\n") staging = write_staging( @@ -1737,7 +1753,7 @@ def test_wal_is_durable_before_first_backup_and_removed_at_commit(self): real_write_new = staging_mod._write_new_bytes def observe_backup(path, data, *, mode=None): - if path == wal_path: + if _same_path(path, wal_path): return real_write_new(path, data, mode=mode) with open(wal_path, encoding="utf-8") as handle: wal = json.load(handle) @@ -1762,7 +1778,7 @@ def test_interrupted_transaction_is_recovered_before_retry(self): def commit_then_fail(path, text, *, create_parents=True): result = real_write(path, text, create_parents=create_parents) - if path == night.alpha_live: + if _same_path(path, night.alpha_live): raise OSError("simulated process interruption") return result @@ -1798,7 +1814,7 @@ def test_interrupted_transaction_recovers_before_corrupt_manifest_read(self): def commit_then_fail(path, text, *, create_parents=True): result = real_write(path, text, create_parents=create_parents) - if path == night.alpha_live: + if _same_path(path, night.alpha_live): raise OSError("simulated interruption") return result @@ -1827,7 +1843,7 @@ def test_interrupted_relative_staging_recovers_via_absolute_path(self): from skillopt_sleep import staging as staging_mod with tempfile.TemporaryDirectory() as tmp: - live = os.path.join(tmp, "live", "alpha", "SKILL.md") + live = os.path.join(_canonical(tmp), "live", "alpha", "SKILL.md") _write(live, "# alpha v1\n") previous_cwd = os.getcwd() try: @@ -1849,7 +1865,7 @@ def test_interrupted_relative_staging_recovers_via_absolute_path(self): def commit_then_fail(path, text, *, create_parents=True): result = real_write(path, text, create_parents=create_parents) - if path == live: + if _same_path(path, live): raise OSError("simulated interruption") return result @@ -1881,7 +1897,7 @@ def test_restart_cleans_own_hardlink_publication_temp(self): def commit_then_fail(path, text, *, create_parents=True): result = real_write(path, text, create_parents=create_parents) - if path == night.alpha_live: + if _same_path(path, night.alpha_live): raise OSError("simulated interruption") return result @@ -1917,7 +1933,7 @@ def test_rollback_preserves_concurrent_human_edit_and_retains_wal(self): real_write = staging_mod._write_atomic def fail_beta_after_human_edit(path, text, *, create_parents=True): - if path == night.beta_live: + if _same_path(path, night.beta_live): _write(night.alpha_live, "# concurrent human edit\n") raise OSError("beta disk failure") return real_write(path, text, create_parents=create_parents) @@ -1947,7 +1963,7 @@ def test_edit_during_receipt_publication_never_commits_a_false_receipt(self): real_write = staging_mod._write_atomic def edit_live_before_receipt(path, text, *, create_parents=True): - if path == receipt_path: + if _same_path(path, receipt_path): _write(night.alpha_live, "# concurrent human edit\n") return real_write(path, text, create_parents=create_parents) @@ -2060,7 +2076,7 @@ def test_legacy_manifest_is_pinned_and_adoption_has_a_receipt(self): def test_legacy_missing_targets_can_share_one_new_parent(self): with tempfile.TemporaryDirectory() as tmp: - live_root = os.path.join(tmp, "new-live") + live_root = os.path.join(_canonical(tmp), "new-live") skill = os.path.join(live_root, "SKILL.md") memory = os.path.join(live_root, "CLAUDE.md") staging = write_staging( @@ -2080,7 +2096,7 @@ def test_failed_legacy_adoption_removes_its_exact_new_directory_tree(self): from skillopt_sleep import staging as staging_mod with tempfile.TemporaryDirectory() as tmp: - live_root = os.path.join(tmp, "new", "nested", "live") + live_root = os.path.join(_canonical(tmp), "new", "nested", "live") skill = os.path.join(live_root, "SKILL.md") memory = os.path.join(live_root, "CLAUDE.md") staging = write_staging( @@ -2096,7 +2112,7 @@ def test_failed_legacy_adoption_removes_its_exact_new_directory_tree(self): real_write = staging_mod._write_atomic def fail_receipt(path, text, *, create_parents=True): - if path == receipt: + if _same_path(path, receipt): raise OSError("receipt device full") return real_write(path, text, create_parents=create_parents) @@ -2113,7 +2129,7 @@ def test_recovery_never_removes_a_replaced_created_directory(self): from skillopt_sleep import staging as staging_mod with tempfile.TemporaryDirectory() as tmp: - live_root = os.path.join(tmp, "new-live") + live_root = os.path.join(_canonical(tmp), "new-live") skill = os.path.join(live_root, "SKILL.md") memory = os.path.join(live_root, "CLAUDE.md") staging = write_staging( @@ -2130,7 +2146,7 @@ def test_recovery_never_removes_a_replaced_created_directory(self): real_write = staging_mod._write_atomic def fail_receipt(path, text, *, create_parents=True): - if path == receipt: + if _same_path(path, receipt): raise OSError("receipt device full") return real_write(path, text, create_parents=create_parents) @@ -2158,7 +2174,7 @@ def test_restart_recovery_removes_journaled_created_directories(self): from skillopt_sleep import staging as staging_mod with tempfile.TemporaryDirectory() as tmp: - live_root = os.path.join(tmp, "restart", "live") + live_root = os.path.join(_canonical(tmp), "restart", "live") skill = os.path.join(live_root, "SKILL.md") memory = os.path.join(live_root, "CLAUDE.md") staging = write_staging( @@ -2174,7 +2190,7 @@ def test_restart_recovery_removes_journaled_created_directories(self): real_write = staging_mod._write_atomic def fail_receipt(path, text, *, create_parents=True): - if path == receipt: + if _same_path(path, receipt): raise OSError("simulated interruption") return real_write(path, text, create_parents=create_parents) @@ -2231,7 +2247,7 @@ def test_legacy_second_target_failure_rolls_back_first(self): real_write = staging_mod._write_atomic def fail_memory(path, text, *, create_parents=True): - if path == memory: + if _same_path(path, memory): raise OSError("memory disk failure") return real_write(path, text, create_parents=create_parents) diff --git a/tests/test_sleep_engine.py b/tests/test_sleep_engine.py index ed23612f..3f975361 100644 --- a/tests/test_sleep_engine.py +++ b/tests/test_sleep_engine.py @@ -1375,7 +1375,9 @@ def test_cycle_stage_then_adopt_with_backup(self): def test_cycle_can_target_repo_scoped_skill_path(self): with tempfile.TemporaryDirectory() as proj, tempfile.TemporaryDirectory() as home: - target = os.path.abspath(os.path.join(proj, ".agents/skills/taste-skill/SKILL.md")) + target = os.path.realpath( + os.path.abspath(os.path.join(proj, ".agents/skills/taste-skill/SKILL.md")) + ) cfg = load_config( invoked_project=proj, projects="invoked", @@ -1744,7 +1746,7 @@ def test_cursor_path_overrides_expand_user_home(self): ) self.assertEqual( cfg.cursor_projects_dir, - os.path.join(os.path.expanduser("~/.cursor-custom"), "projects"), + os.path.join(os.path.abspath(os.path.expanduser("~/.cursor-custom")), "projects"), ) direct_cfg = load_config( @@ -1753,7 +1755,7 @@ def test_cursor_path_overrides_expand_user_home(self): ) self.assertEqual( direct_cfg.cursor_projects_dir, - os.path.join(os.path.expanduser("~/.cursor-config"), "projects"), + os.path.join(os.path.abspath(os.path.expanduser("~/.cursor-config")), "projects"), ) self.assertEqual( resolve_cursor_path(direct_cfg.get("cursor_path")), diff --git a/tests/test_sleep_scheduler_safety.py b/tests/test_sleep_scheduler_safety.py index f830ef90..992b19cd 100644 --- a/tests/test_sleep_scheduler_safety.py +++ b/tests/test_sleep_scheduler_safety.py @@ -11,6 +11,7 @@ class TestSleepSchedulerSafety(unittest.TestCase): + @unittest.skipIf(os.name == "nt", "POSIX runner quoting is not used on Windows") def test_posix_runner_quotes_every_path_and_argument(self): with tempfile.TemporaryDirectory(prefix="sleep $' quote ") as project: command = scheduler._runner_cmd( diff --git a/tests/test_sleep_skill_resolver.py b/tests/test_sleep_skill_resolver.py index e52beabe..423c50d8 100644 --- a/tests/test_sleep_skill_resolver.py +++ b/tests/test_sleep_skill_resolver.py @@ -29,6 +29,10 @@ def _write_skill(root, name, body="# skill\n"): return path +def _canonical(path): + return os.path.realpath(os.path.abspath(path)) + + def _symlink(test, source, link_name): """Create a symlink, or skip the test where the platform refuses one. @@ -178,7 +182,9 @@ def test_user_skills_root_comes_first_then_plugin_cache(self): ) os.makedirs(plugin_skills) cfg = load_config(claude_home=claude_home) - self.assertEqual(skill_search_roots(cfg), [skills, plugin_skills]) + self.assertEqual( + skill_search_roots(cfg), [_canonical(skills), _canonical(plugin_skills)] + ) def test_absent_roots_are_skipped(self): with tempfile.TemporaryDirectory() as tmp: @@ -249,7 +255,7 @@ def test_unreadable_plugin_cache_does_not_break_discovery(self): self.skipTest("directory is still readable after chmod 000") try: cfg = load_config(claude_home=claude_home) - self.assertEqual(skill_search_roots(cfg), [skills]) + self.assertEqual(skill_search_roots(cfg), [_canonical(skills)]) finally: try: os.chmod(cache, 0o700) @@ -279,7 +285,9 @@ def test_versioned_marketplace_layout_is_discovered(self): ) cfg = load_config(claude_home=claude_home) self.assertIn( - os.path.join(cache, "claude-plugins-official", "superpowers", "5.0.7", "skills"), + _canonical(os.path.join( + cache, "claude-plugins-official", "superpowers", "5.0.7", "skills" + )), skill_search_roots(cfg), ) res = resolve_skill("brainstorming", skill_search_roots(cfg)) @@ -292,11 +300,13 @@ def test_multiple_installed_versions_resolve_to_the_newest_not_ambiguous(self): # peer root would make an ordinary upgrade resolve AMBIGUOUS. with tempfile.TemporaryDirectory() as tmp: claude_home = os.path.join(tmp, ".claude") - plugin = os.path.join(claude_home, "plugins", "cache", - "claude-plugins-official", "chrome-devtools-mcp") + plugin = _canonical(os.path.join( + claude_home, "plugins", "cache", + "claude-plugins-official", "chrome-devtools-mcp" + )) for version in ["1.1.1", "1.5.0", "1.6.0"]: _write_skill(os.path.join(plugin, version, "skills"), "chrome-devtools") - newest = os.path.join(plugin, "1.6.0", "skills") + newest = _canonical(os.path.join(plugin, "1.6.0", "skills")) cfg = load_config(claude_home=claude_home) roots = skill_search_roots(cfg) @@ -310,13 +320,15 @@ def test_multiple_installed_versions_resolve_to_the_newest_not_ambiguous(self): def test_version_ordering_is_numeric_not_lexicographic(self): with tempfile.TemporaryDirectory() as tmp: claude_home = os.path.join(tmp, ".claude") - plugin = os.path.join(claude_home, "plugins", "cache", "market", "plugin") + plugin = _canonical(os.path.join( + claude_home, "plugins", "cache", "market", "plugin" + )) for version in ["1.9.0", "1.10.0"]: _write_skill(os.path.join(plugin, version, "skills"), "example-skill") cfg = load_config(claude_home=claude_home) roots = [r for r in skill_search_roots(cfg) if r.startswith(plugin)] # "1.10.0" < "1.9.0" as strings; it must still win as a version. - self.assertEqual(roots, [os.path.join(plugin, "1.10.0", "skills")]) + self.assertEqual(roots, [_canonical(os.path.join(plugin, "1.10.0", "skills"))]) def test_stable_release_beats_an_installed_prerelease(self): # Segment lists alone would rank 2.0.0-beta above 2.0.0, because a @@ -324,12 +336,14 @@ def test_stable_release_beats_an_installed_prerelease(self): # must never be preferred over the stable release it precedes. with tempfile.TemporaryDirectory() as tmp: claude_home = os.path.join(tmp, ".claude") - plugin = os.path.join(claude_home, "plugins", "cache", "market", "plugin") + plugin = _canonical(os.path.join( + claude_home, "plugins", "cache", "market", "plugin" + )) for version in ["2.0.0", "2.0.0-beta"]: _write_skill(os.path.join(plugin, version, "skills"), "example-skill") cfg = load_config(claude_home=claude_home) roots = [r for r in skill_search_roots(cfg) if r.startswith(plugin)] - self.assertEqual(roots, [os.path.join(plugin, "2.0.0", "skills")]) + self.assertEqual(roots, [_canonical(os.path.join(plugin, "2.0.0", "skills"))]) def test_version_key_orders_release_forms_sensibly(self): from skillopt_sleep.skill_resolver import _version_sort_key as key @@ -346,7 +360,7 @@ def test_legacy_unversioned_layout_still_works(self): plugin = os.path.join(claude_home, "plugins", "cache", "market", "plugin") expected = _write_skill(os.path.join(plugin, "skills"), "example-skill") cfg = load_config(claude_home=claude_home) - self.assertIn(os.path.join(plugin, "skills"), skill_search_roots(cfg)) + self.assertIn(_canonical(os.path.join(plugin, "skills")), skill_search_roots(cfg)) res = resolve_skill("example-skill", skill_search_roots(cfg)) self.assertEqual(res.status, FOUND) self.assertEqual(res.path, os.path.realpath(expected)) @@ -361,8 +375,8 @@ def test_two_marketplaces_each_contribute_a_root(self): _write_skill(cognee, "cognee-remember") cfg = load_config(claude_home=claude_home) roots = skill_search_roots(cfg) - self.assertIn(official, roots) - self.assertIn(cognee, roots) + self.assertIn(_canonical(official), roots) + self.assertIn(_canonical(cognee), roots) def test_legacy_target_skill_path_behavior_is_untouched(self): with tempfile.TemporaryDirectory() as tmp: diff --git a/tests/test_sleep_staging_fanout.py b/tests/test_sleep_staging_fanout.py index f7ad4b76..32121ac4 100644 --- a/tests/test_sleep_staging_fanout.py +++ b/tests/test_sleep_staging_fanout.py @@ -32,6 +32,10 @@ def _proposal(name="example-skill", body="# example\n", live=None, root="/tmp/li return SkillProposal(name, body, live) +def _canonical(path): + return os.path.realpath(os.path.abspath(os.path.normpath(path))) + + def _report(): return SleepReport(night=1, project="/repo/example", accepted=True, gate_action="accept_new_best") @@ -44,7 +48,7 @@ def test_one_row_per_skill_in_order(self): self.assertEqual([r["proposed_file"] for r in rows], ["proposed_SKILL.alpha.md", "proposed_SKILL.beta.md"]) self.assertEqual(rows[0]["live_skill_path"], - os.path.normpath("/tmp/live/alpha/SKILL.md")) + _canonical("/tmp/live/alpha/SKILL.md")) self.assertEqual( rows[0]["sha256"], hashlib.sha256(b"# example\n").hexdigest(), @@ -126,14 +130,14 @@ def test_absolute_paths_needing_normalisation_are_accepted(self): # duplicate separators everywhere, and every forward-slash absolute # path on Windows. Normalising first keeps the traversal guard. rows = skill_proposal_rows([_proposal("alpha", live="/tmp/live//alpha/SKILL.md")]) - self.assertEqual(rows[0]["live_skill_path"], os.path.normpath("/tmp/live/alpha/SKILL.md")) + self.assertEqual(rows[0]["live_skill_path"], _canonical("/tmp/live/alpha/SKILL.md")) def test_current_directory_segments_are_normalised_not_refused(self): rows = skill_proposal_rows([ _proposal("alpha", live="/tmp/live/./alpha/SKILL.md") ]) self.assertEqual(rows[0]["live_skill_path"], - os.path.normpath("/tmp/live/alpha/SKILL.md")) + _canonical("/tmp/live/alpha/SKILL.md")) def test_two_skills_targeting_one_file_are_refused(self): shared = "/tmp/live/shared/SKILL.md" @@ -297,7 +301,7 @@ def test_fan_out_adds_files_and_manifest_rows(self): rows = self._manifest(out)["skills"] self.assertEqual([r["skill_name"] for r in rows], ["alpha", "beta"]) self.assertEqual(rows[1]["live_skill_path"], - os.path.join(live_root, "beta", "SKILL.md")) + _canonical(os.path.join(live_root, "beta", "SKILL.md"))) self.assertEqual( rows[0]["sha256"], hashlib.sha256(b"# alpha\n").hexdigest(), @@ -368,6 +372,122 @@ def publish(index): ) as handle: self.assertEqual(handle.read().strip(), os.path.basename(latest)) + def test_latest_pointer_retries_a_transient_windows_sharing_violation(self): + from skillopt_sleep import staging as staging_mod + + with tempfile.TemporaryDirectory() as tmp: + root = os.path.join(tmp, ".skillopt-sleep", "staging") + night = os.path.join(root, "20260815-010203") + os.makedirs(night) + with open(os.path.join(night, "manifest.json"), "w", encoding="utf-8") as f: + f.write("{}") + real_replace = os.replace + calls = [] + + def transient_replace(source, destination): + calls.append((source, destination)) + if len(calls) == 1: + raise PermissionError("simulated Windows sharing violation") + return real_replace(source, destination) + + with mock.patch.object(staging_mod.os, "name", "nt"), mock.patch.object( + staging_mod.os, "replace", side_effect=transient_replace + ), mock.patch.object(staging_mod.time, "sleep") as sleep: + staging_mod._publish_latest(root, night) + + self.assertEqual(len(calls), 2) + sleep.assert_called_once_with(0.005) + with open(os.path.join(root, ".latest"), encoding="utf-8") as handle: + self.assertEqual(handle.read(), "20260815-010203\n") + + def test_generic_atomic_write_never_retries_permission_error(self): + from skillopt_sleep import staging as staging_mod + + with tempfile.TemporaryDirectory() as tmp: + destination = os.path.join(tmp, "live.md") + with open(destination, "wb") as handle: + handle.write(b"original") + with mock.patch.object(staging_mod.os, "name", "nt"), mock.patch.object( + staging_mod.os, + "replace", + side_effect=PermissionError("live file is busy"), + ) as replace, self.assertRaises(PermissionError): + staging_mod._write_atomic_bytes(destination, b"proposal") + + replace.assert_called_once() + with open(destination, "rb") as handle: + self.assertEqual(handle.read(), b"original") + self.assertFalse(any(name.startswith(".tmp-") for name in os.listdir(tmp))) + + def test_latest_pointer_persistent_error_preserves_destination_and_cleans_temp(self): + from skillopt_sleep import staging as staging_mod + + with tempfile.TemporaryDirectory() as tmp: + root = os.path.join(tmp, ".skillopt-sleep", "staging") + night = os.path.join(root, "20260815-010203") + os.makedirs(night) + with open(os.path.join(night, "manifest.json"), "w", encoding="utf-8") as f: + f.write("{}") + pointer = os.path.join(root, ".latest") + with open(pointer, "w", encoding="utf-8") as handle: + handle.write("20260814-010203\n") + + with mock.patch.object(staging_mod.os, "name", "nt"), mock.patch.object( + staging_mod.os, + "replace", + side_effect=PermissionError("pointer remains busy"), + ) as replace, mock.patch.object(staging_mod.time, "sleep"), self.assertRaises( + PermissionError + ): + staging_mod._publish_latest(root, night) + + self.assertEqual(replace.call_count, 21) + with open(pointer, encoding="utf-8") as handle: + self.assertEqual(handle.read(), "20260814-010203\n") + self.assertFalse(any(name.startswith(".tmp-") for name in os.listdir(root))) + + def test_staging_descendant_accepts_root_alias_but_rejects_child_symlink(self): + from skillopt_sleep import staging as staging_mod + + with tempfile.TemporaryDirectory() as tmp: + real_root = os.path.join(tmp, "real-staging") + os.makedirs(real_root) + alias_root = os.path.join(tmp, "staging-alias") + outside = os.path.join(tmp, "outside") + os.makedirs(outside) + try: + os.symlink(real_root, alias_root, target_is_directory=True) + os.symlink( + outside, + os.path.join(real_root, "child-alias"), + target_is_directory=True, + ) + except OSError: + self.skipTest("directory symlinks unavailable") + + ordinary_lexical = os.path.join(real_root, "backup.md") + with open(ordinary_lexical, "w", encoding="utf-8") as handle: + handle.write("backup") + # WAL/manifest paths are canonical, while callers can still supply + # the same staging root through a lexical alias. + ordinary = os.path.realpath(ordinary_lexical) + escaped = os.path.join(alias_root, "child-alias", "outside.md") + with open(os.path.join(outside, "outside.md"), "w", encoding="utf-8") as handle: + handle.write("outside") + + self.assertTrue( + staging_mod._existing_path_is_canonical_staging_descendant( + ordinary, + alias_root, + ) + ) + self.assertFalse( + staging_mod._existing_path_is_canonical_staging_descendant( + escaped, + alias_root, + ) + ) + def test_latest_ignores_a_symlinked_night(self): with tempfile.TemporaryDirectory() as tmp: real_night = write_staging( diff --git a/tests/test_superpowers_scenarios.py b/tests/test_superpowers_scenarios.py index be6e9ec0..a724a1c4 100644 --- a/tests/test_superpowers_scenarios.py +++ b/tests/test_superpowers_scenarios.py @@ -1,5 +1,6 @@ """Tests for Superpowers skill evaluation (offline, no API).""" import os +import re as _re import subprocess import tempfile from pathlib import Path @@ -7,8 +8,6 @@ import pytest -import re as _re - from skillopt_sleep.adapters.superpowers import ( VERIFICATION_SCENARIOS, _get_scenarios, @@ -269,6 +268,7 @@ def test_flaky_scenario_requires_observed_failure_before_success(self): results = [_score_check(c, "1 passed", None, evidence) for c in flaky["judge"]["checks"]] assert all(results) is False + @pytest.mark.skipif(os.name != "posix", reason="test executes POSIX pytest shims") def test_shim_counts_real_invocations(self): """The shim logs every pytest run, including `python -m pytest`.""" with tempfile.TemporaryDirectory() as tmpdir: @@ -288,6 +288,7 @@ def test_shim_counts_real_invocations(self): "failures": 0, } + @pytest.mark.skipif(os.name != "posix", reason="test executes POSIX pytest shims") def test_shim_handles_shell_metacharacters_in_paths(self): with tempfile.TemporaryDirectory() as tmpdir: ws = Path(tmpdir) / "space $HOME" @@ -302,6 +303,7 @@ def test_shim_handles_shell_metacharacters_in_paths(self): assert _pytest_run_count(log, "abc123") == 1 assert _pytest_outcome_counts(log, "abc123")["successes"] == 1 + @pytest.mark.skipif(os.name != "posix", reason="test executes POSIX pytest shims") def test_python_shim_matches_module_arguments_not_command_text(self): with tempfile.TemporaryDirectory() as tmpdir: ws = Path(tmpdir) @@ -323,6 +325,7 @@ def test_python_shim_matches_module_arguments_not_command_text(self): assert _pytest_run_count(log, "abc123") == 1 assert _pytest_outcome_counts(log, "abc123")["successes"] == 1 + @pytest.mark.skipif(os.name != "posix", reason="test executes POSIX pytest shims") def test_zero_work_and_skipped_runs_are_not_successes(self): """Exit code zero alone is not evidence that a test actually passed.""" with tempfile.TemporaryDirectory() as tmpdir: @@ -362,6 +365,7 @@ def test_pytest_after_edit_fails_closed_on_broken_source_symlink(self): (ws / "broken.py").symlink_to(ws / "missing.py") assert _pytest_after_edit(log, ws) is False + @pytest.mark.skipif(os.name != "posix", reason="test executes POSIX pytest shims") def test_shim_stamps_attempt_number(self): """SKILLOPT_ATTEMPT is set by the shim, so the flaky test can't be faked.""" with tempfile.TemporaryDirectory() as tmpdir: @@ -427,6 +431,7 @@ def test_harness_verify_ignores_project_pytest_hooks_and_config(self): ) assert _harness_verify(ws, dict(os.environ), test_paths=["test_guard.py"]) is True + @pytest.mark.skipif(os.name != "posix", reason="test executes POSIX agent shims") def test_agent_shim_does_not_reuse_stale_bytecode(self): with tempfile.TemporaryDirectory() as tmpdir: ws = Path(tmpdir) @@ -455,6 +460,7 @@ def test_agent_shim_does_not_reuse_stale_bytecode(self): } +@pytest.mark.skipif(os.name != "posix", reason="Superpowers adapter requires POSIX bash") class TestOverlayIntegration: """Mocked tests proving skill overlay and bootstrap are set up correctly.""" @@ -712,6 +718,7 @@ def _run(self, workspace): ) return result, mock_run + @pytest.mark.skipif(os.name != "posix", reason="scenario runner requires POSIX bash") def test_no_host_credentials_by_default(self): """Regression: host ~/.claude auth/config is never linked into scenario HOME.""" with tempfile.TemporaryDirectory() as tmpdir: @@ -720,6 +727,7 @@ def test_no_host_credentials_by_default(self): claude_dir = workspace / "home-test" / ".claude" assert list(claude_dir.iterdir()) == [] + @pytest.mark.skipif(os.name != "posix", reason="scenario runner requires POSIX bash") def test_env_is_scrubbed(self, monkeypatch): monkeypatch.setenv("SECRET_TOKEN", "leak-me") with tempfile.TemporaryDirectory() as tmpdir: @@ -729,6 +737,7 @@ def test_env_is_scrubbed(self, monkeypatch): assert "SECRET_TOKEN" not in env assert env["HOME"] == str(workspace / "home-test") + @pytest.mark.skipif(os.name != "posix", reason="scenario runner requires POSIX bash") def test_path_is_minimal_by_default(self, monkeypatch): """Host PATH is not inherited unless SKILLOPT_INHERIT_PATH=1.""" monkeypatch.setenv("PATH", f"/opt/hostonly/bin{os.pathsep}/usr/bin") @@ -741,6 +750,7 @@ def test_path_is_minimal_by_default(self, monkeypatch): assert ".skillopt" in path # shim dir still present assert "/usr/bin" in path + @pytest.mark.skipif(os.name != "posix", reason="scenario runner requires POSIX bash") def test_path_inherit_opt_in(self, monkeypatch): monkeypatch.setenv("PATH", f"/opt/hostonly/bin{os.pathsep}/usr/bin") monkeypatch.setenv("SKILLOPT_INHERIT_PATH", "1") @@ -749,6 +759,7 @@ def test_path_inherit_opt_in(self, monkeypatch): _, mock_run = self._run(workspace) assert "/opt/hostonly/bin" in mock_run.call_args.kwargs["env"]["PATH"] + @pytest.mark.skipif(os.name != "posix", reason="scenario runner requires POSIX bash") def test_skill_name_traversal_rejected(self): """A skill_name with path separators must not redirect the overlay write.""" with tempfile.TemporaryDirectory() as tmpdir: @@ -763,6 +774,7 @@ def test_skill_name_traversal_rejected(self): skill_overlay=None, workspace=workspace, ) + @pytest.mark.skipif(os.name != "posix", reason="scenario runner requires POSIX bash") def test_fails_closed_without_auth(self, monkeypatch): monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) with tempfile.TemporaryDirectory() as tmpdir: @@ -792,6 +804,7 @@ def test_harness_verify_drops_credential(self): assert mock_run.call_args.kwargs["env"]["PATH"] == "/scrubbed/bin" assert "-m" in mock_run.call_args[0][0] + @pytest.mark.skipif(os.name != "posix", reason="scenario runner requires POSIX bash") def test_missing_bootstrap_flags_error(self): """Absent using-superpowers SKILL.md must surface a distinct error.""" with tempfile.TemporaryDirectory() as tmpdir: @@ -820,6 +833,7 @@ def test_harness_verify_respects_timeout(self, monkeypatch): _harness_verify(ws / "p", {}, timeout=600) assert mock_run.call_args.kwargs["timeout"] == 600 + @pytest.mark.skipif(os.name != "posix", reason="scenario runner requires POSIX bash") def test_claude_bin_override(self, monkeypatch): monkeypatch.setenv("SKILLOPT_CLAUDE_BIN", "/custom/claude") with tempfile.TemporaryDirectory() as tmpdir: @@ -859,6 +873,7 @@ def test_symlinked_candidate_refused(self): with pytest.raises(ValueError, match="must not be a symlink"): SuperpowersEvaluator().evaluate(candidate_skill_path=str(link)) + @pytest.mark.skipif(os.name != "posix", reason="scenario runner requires POSIX bash") def test_private_runner_also_refuses_symlinked_candidate(self): with tempfile.TemporaryDirectory() as tmpdir: workspace = Path(tmpdir) @@ -884,6 +899,7 @@ def test_private_runner_also_refuses_symlinked_candidate(self): workspace=workspace, ) + @pytest.mark.skipif(os.name != "posix", reason="scenario runner requires POSIX bash") def test_symlinked_overlay_path_refused(self): """A symlinked skills/ component in the checkout must be refused, no write.""" with tempfile.TemporaryDirectory() as tmpdir: @@ -935,6 +951,7 @@ def test_git_timeout_has_clear_error(self): _run_git_step(["fetch", "origin"], Path(tmpdir), timeout=12) +@pytest.mark.skipif(os.name != "posix", reason="Superpowers adapter requires POSIX bash") class TestPermissionModes: """Tests for permission handling in cmd construction."""