diff --git a/src/soup_cli/commands/_eval_v0550.py b/src/soup_cli/commands/_eval_v0550.py index f69730f..aa114d9 100644 --- a/src/soup_cli/commands/_eval_v0550.py +++ b/src/soup_cli/commands/_eval_v0550.py @@ -254,6 +254,15 @@ def register(app: typer.Typer, console: Console) -> None: False, "--json-only", help="Suppress Rich output; emit a single JSON verdict line.", ), + suite: str = typer.Option( + None, "--suite", + help=( + "Locked eval suite (from `soup eval lock`) to validate as a gate " + "precondition — a missing / unparseable suite BLOCKS the check " + "(exit 1). cwd-contained. The generated pre-push hook passes " + "$GATE_SUITE here so the locked suite is actually enforced." + ), + ), ) -> None: """Run-vs-run regression check (paired-bootstrap CI). @@ -265,6 +274,26 @@ def register(app: typer.Typer, console: Console) -> None: """ import json as _json + # The locked suite is a hard precondition when supplied: validate it + # exists, is under cwd, and parses. Previously the hook wrote $GATE_SUITE + # but never used it, so a deleted / tampered locked suite silently + # passed the gate. + if suite: + from soup_cli.utils.eval_lock_coverage import load_locked_suite + from soup_cli.utils.paths import is_under_cwd + + if not is_under_cwd(suite): + console.print(f"[red]--suite is outside cwd:[/] {escape(str(suite))}") + raise typer.Exit(1) + try: + load_locked_suite(suite) + except (FileNotFoundError, ValueError, OSError) as exc: + console.print( + f"[red]Locked eval suite invalid — gate blocked:[/] " + f"{escape(str(exc))}" + ) + raise typer.Exit(1) from exc + from soup_cli.experiment.tracker import ExperimentTracker from soup_cli.utils.eval_gate_hook import ( GateThresholds, diff --git a/src/soup_cli/commands/bench.py b/src/soup_cli/commands/bench.py index 7e1ab4b..96478e7 100644 --- a/src/soup_cli/commands/bench.py +++ b/src/soup_cli/commands/bench.py @@ -194,6 +194,11 @@ def bench( start_load = time.time() try: model_obj, tokenizer = _load_model(str(model_path), base, device) + except typer.Exit: + # typer.Exit subclasses RuntimeError, so the broad except below would + # swallow an already-reported CLI exit and mis-print "Failed to load + # model: 1". Let it propagate unchanged. + raise except (OSError, ImportError, RuntimeError, ValueError) as exc: console.print(f"[red]Failed to load model:[/] {exc}") raise typer.Exit(1) from exc diff --git a/src/soup_cli/commands/data.py b/src/soup_cli/commands/data.py index cedac4d..20313c7 100644 --- a/src/soup_cli/commands/data.py +++ b/src/soup_cli/commands/data.py @@ -812,6 +812,13 @@ def split_data( console.print("[red]Specify at least one of --val or --test.[/]") raise typer.Exit(1) + # Reject negatives: a negative val/test slipped past the `>= total` check + # and produced a negative slice (e.g. --val -10 sent 90 rows to val, 10 to + # train — a silently inverted split). + if (val is not None and val < 0) or (test is not None and test < 0): + console.print("[red]--val and --test must be non-negative.[/]") + raise typer.Exit(1) + data = load_raw_data(file_path) if not data: console.print("[red]Dataset is empty.[/]") diff --git a/src/soup_cli/commands/eval.py b/src/soup_cli/commands/eval.py index 04fb725..f2410b3 100644 --- a/src/soup_cli/commands/eval.py +++ b/src/soup_cli/commands/eval.py @@ -516,7 +516,11 @@ def auto( run_id=None, device=None, ) - except SystemExit: + except (typer.Exit, SystemExit): + # benchmark() signals failure with typer.Exit (a RuntimeError, NOT + # SystemExit), so the old `except SystemExit` never caught it and a + # benchmark failure aborted the whole auto-eval instead of falling + # through to the custom eval below. console.print("[yellow]Benchmark eval skipped (see above).[/]") # Run custom eval if specified diff --git a/src/soup_cli/commands/fetch.py b/src/soup_cli/commands/fetch.py index 2f4f20b..bf78ed4 100644 --- a/src/soup_cli/commands/fetch.py +++ b/src/soup_cli/commands/fetch.py @@ -94,8 +94,11 @@ def fetch( # Symlink-at-target rejection (TOCTOU defence) — matches v0.33.0 #22 / # v0.40.2 #51 / v0.43.0 Part C policy. Apply BEFORE the existence check # so a symlink-with-no-real-file can never be silently overwritten. + # lstat the ORIGINAL path — os.path.realpath() already resolved any symlink, + # so lstat(real_target) inspected the link's TARGET and S_ISLNK was never + # true, letting the write follow the link and clobber its target. try: - link_stat = os.lstat(real_target) + link_stat = os.lstat(target_path) except FileNotFoundError: link_stat = None except OSError as exc: diff --git a/src/soup_cli/commands/train.py b/src/soup_cli/commands/train.py index 15ae282..e8d3a4b 100644 --- a/src/soup_cli/commands/train.py +++ b/src/soup_cli/commands/train.py @@ -717,6 +717,34 @@ def train( script_args.extend( ["--reward-hack-mitigation", reward_hack_mitigation] ) + # Pass through the remaining run-shaping flags — these were + # silently dropped on re-exec, so a multi-GPU run ignored the + # eval gate, HF push, trust-remote-code, tracker, diagnose gate, + # and the governance/energy artifacts the user asked for. + if gate: + script_args.extend(["--gate", gate]) + if push_as: + script_args.extend(["--push-as", push_as]) + if hf_resume: + script_args.append("--hf-resume") + if trust_remote_code: + script_args.append("--trust-remote-code") + if tracker: + script_args.extend(["--tracker", tracker]) + if diagnose_gate: + script_args.extend(["--diagnose-gate", diagnose_gate]) + if annex_xi: + script_args.extend(["--annex-xi", annex_xi]) + if repro_receipt: + script_args.extend(["--repro-receipt", repro_receipt]) + if profile_run: + script_args.append("--profile") + if track_energy: + script_args.append("--track-energy") + if energy_country: + script_args.extend(["--energy-country", energy_country]) + if energy_out: + script_args.extend(["--energy-out", energy_out]) if yes: script_args.append("--yes") argv = build_accelerate_argv( diff --git a/src/soup_cli/data/formats.py b/src/soup_cli/data/formats.py index 318b756..0196b21 100644 --- a/src/soup_cli/data/formats.py +++ b/src/soup_cli/data/formats.py @@ -184,6 +184,13 @@ def _convert_kto(row: dict) -> dict: raise ValueError( f"KTO label must be true/false, got string: {raw_label!r}" ) + elif isinstance(raw_label, bool): + label = raw_label + elif isinstance(raw_label, (int, float)): + # Both the ±1 convention (+1 desirable / -1 undesirable) and the 0/1 + # convention map "positive == desirable". `bool(-1)` is True, which + # would silently INVERT a -1 "bad" label — flip it to False here. + label = raw_label > 0 else: label = bool(raw_label) return { diff --git a/src/soup_cli/data/traces/parsers.py b/src/soup_cli/data/traces/parsers.py index 6805619..7106177 100644 --- a/src/soup_cli/data/traces/parsers.py +++ b/src/soup_cli/data/traces/parsers.py @@ -178,7 +178,10 @@ def parse_soup_serve(path: str) -> Iterator[Trace]: if not dir_path.is_dir(): return for file_path in sorted(dir_path.glob("*.jsonl")): - for line in file_path.read_text(encoding="utf-8").splitlines(): + # utf-8-sig strips a leading BOM (common on Windows-written JSONL); + # plain utf-8 left the BOM on line 1, so json.loads failed and the + # first record of every BOM'd file was silently dropped. + for line in file_path.read_text(encoding="utf-8-sig").splitlines(): if not line.strip(): continue try: diff --git a/src/soup_cli/registry/store.py b/src/soup_cli/registry/store.py index 5b439d4..ed336c8 100644 --- a/src/soup_cli/registry/store.py +++ b/src/soup_cli/registry/store.py @@ -534,14 +534,15 @@ class RegistryStore: if self.get(parent_id) is None: raise ValueError(f"parent entry not found: {parent_id}") # Cycle check: if parent already has child as an ancestor, adding this - # edge would close a loop. Walk from parent_id upward and reject if - # child_id is reachable. - for anc in self.get_ancestors(parent_id): - if anc["id"] == child_id: - raise ValueError( - "lineage would introduce a cycle " - f"({child_id} -> {parent_id} -> ... -> {child_id})" - ) + # edge would close a loop. Use an UNBOUNDED reachability walk — the old + # code walked get_ancestors(parent_id) whose max_depth=10 cap silently + # accepted a cycle-closing edge >10 hops away (the `soup loop watch` + # daemon builds long chains), corrupting the DAG. + if self._reaches_ancestor(parent_id, child_id): + raise ValueError( + "lineage would introduce a cycle " + f"({child_id} -> {parent_id} -> ... -> {child_id})" + ) now = datetime.now().isoformat() conn = self._get_conn() conn.execute( @@ -580,6 +581,36 @@ class RegistryStore: frontier = new_frontier return ancestors + def _reaches_ancestor(self, start_id: str, target_id: str) -> bool: + """True iff ``target_id`` is an ancestor of ``start_id`` (any depth). + + Unbounded BFS terminated by a ``seen`` set — used for cycle detection in + :meth:`add_lineage`, where a depth cap would let a far-away cycle-closing + edge slip through. Queries only ``parent_id`` (no hydrate) so it stays + cheap even on long lineage chains. + """ + conn = self._get_conn() + seen: set[str] = set() + frontier = [start_id] + while frontier: + placeholders = ",".join("?" * len(frontier)) + rows = conn.execute( + f"""SELECT parent_id FROM registry_lineage + WHERE child_id IN ({placeholders})""", + tuple(frontier), + ).fetchall() + new_frontier: list[str] = [] + for row in rows: + pid = row["parent_id"] + if pid == target_id: + return True + if pid in seen: + continue + seen.add(pid) + new_frontier.append(pid) + frontier = new_frontier + return False + def get_descendants(self, entry_id: str, *, max_depth: int = 10) -> list[dict]: """BFS walk downwards from entry_id across lineage children.""" conn = self._get_conn() diff --git a/src/soup_cli/trainer/distill.py b/src/soup_cli/trainer/distill.py index a04e335..7fb5c1f 100644 --- a/src/soup_cli/trainer/distill.py +++ b/src/soup_cli/trainer/distill.py @@ -47,12 +47,18 @@ def _compute_distill_term( teacher_logits: "_torch_typ.Tensor", divergence: str, temperature: float, + labels: "_torch_typ.Tensor | None" = None, + attention_mask: "_torch_typ.Tensor | None" = None, ) -> "_torch_typ.Tensor": """Pure tensor kernel: divergence between student and teacher logits. Both logits are ``(batch, seq, vocab)``. Temperature softens the distributions before the divergence is computed (Hinton). The result is - a scalar mean over the token-level divergences. + a scalar mean over the token-level divergences, restricted to the trained + tokens: ``labels != -100`` when ``labels`` is given (excludes padding AND + prompt), else ``attention_mask`` (excludes padding), else all positions. + Averaging over padding/prompt tokens (the pre-fix behaviour) diluted the + signal with the divergence on positions the student is not trained on. Raises: TypeError: ``temperature`` not numeric or is bool. @@ -74,20 +80,32 @@ def _compute_distill_term( temp = float(temperature) s = student_logits / temp t = teacher_logits / temp + + def _masked_mean(per_token: "_torch_typ.Tensor") -> "_torch_typ.Tensor": + """Mean of a ``(batch, seq)`` per-token divergence over trained tokens.""" + if labels is not None: + mask = labels != -100 + elif attention_mask is not None: + mask = attention_mask.bool() + else: + return per_token.mean() + mask = mask.to(per_token.dtype) + denom = mask.sum().clamp(min=1.0) + return (per_token * mask).sum() / denom + + kl_div = torch.nn.functional.kl_div if divergence == "forward_kl": - # KL(teacher || student). Use kl_div which expects log-probs of the - # student and probs of the teacher. + # KL(teacher || student): student log-probs, teacher probs. reduction= + # "none" keeps per-token so we can mask before averaging. log_s = torch.log_softmax(s, dim=-1) p_t = torch.softmax(t, dim=-1) - return torch.nn.functional.kl_div( - log_s, p_t, reduction="batchmean" - ) * (temp * temp) + per_token = kl_div(log_s, p_t, reduction="none").sum(dim=-1) + return _masked_mean(per_token) * (temp * temp) if divergence == "reverse_kl": log_t = torch.log_softmax(t, dim=-1) p_s = torch.softmax(s, dim=-1) - return torch.nn.functional.kl_div( - log_t, p_s, reduction="batchmean" - ) * (temp * temp) + per_token = kl_div(log_t, p_s, reduction="none").sum(dim=-1) + return _masked_mean(per_token) * (temp * temp) if divergence == "js": # Jensen-Shannon: 0.5 (KL(p||m) + KL(q||m)), m = 0.5 (p + q). log_s = torch.log_softmax(s, dim=-1) @@ -96,9 +114,9 @@ def _compute_distill_term( p_t = log_t.exp() m = 0.5 * (p_s + p_t) log_m = m.clamp(min=1e-12).log() - kl_pm = torch.nn.functional.kl_div(log_m, p_s, reduction="batchmean") - kl_qm = torch.nn.functional.kl_div(log_m, p_t, reduction="batchmean") - return 0.5 * (kl_pm + kl_qm) * (temp * temp) + kl_pm = kl_div(log_m, p_s, reduction="none").sum(dim=-1) + kl_qm = kl_div(log_m, p_t, reduction="none").sum(dim=-1) + return 0.5 * (_masked_mean(kl_pm) + _masked_mean(kl_qm)) * (temp * temp) raise ValueError(f"Unknown divergence {divergence!r}") @@ -605,8 +623,12 @@ class DistillTrainerWrapper: ) anchor = _minillm_cb.anchor_term(model) else: + # Mask padding + prompt tokens so the divergence is measured + # only over the completion tokens (parity with the ULD path). distill_loss = _compute_distill_term( - student_logits, teacher_logits, divergence, temperature + student_logits, teacher_logits, divergence, temperature, + labels=labels, + attention_mask=inputs.get("attention_mask"), ) total = _CE_WEIGHT * ce_loss + _DISTILL_WEIGHT * distill_loss if anchor is not None: diff --git a/src/soup_cli/trainer/ipo.py b/src/soup_cli/trainer/ipo.py index 4791a58..66e6127 100644 --- a/src/soup_cli/trainer/ipo.py +++ b/src/soup_cli/trainer/ipo.py @@ -292,7 +292,10 @@ class IPOTrainerWrapper: tcfg = self.config.training variant_cbs = build_dpo_variant_callbacks( - beta_start=tcfg.dpo_beta, + # IPO's regularization strength is `ipo_tau` (passed as trl's beta, + # see setup()); the β-schedule must anneal from that, not the DPO + # default `dpo_beta`, which would overwrite the user's τ at step 0. + beta_start=tcfg.ipo_tau, beta_end=tcfg.dpo_beta_end, schedule=tcfg.dpo_beta_schedule, total_steps=0, diff --git a/src/soup_cli/trainer/ppo.py b/src/soup_cli/trainer/ppo.py index be99eaf..7bbe887 100644 --- a/src/soup_cli/trainer/ppo.py +++ b/src/soup_cli/trainer/ppo.py @@ -262,7 +262,9 @@ class PPOTrainerWrapper: trainer_kwargs["ref_model"] = None if "reward_model" in ppo_trainer_params: trainer_kwargs["reward_model"] = self._get_or_create_reward_model( - cfg, tcfg + cfg, tcfg, + reward_funcs_supplied=bool(reward_funcs) + and "reward_funcs" in ppo_trainer_params, ) if "value_model" in ppo_trainer_params: trainer_kwargs["value_model"] = self._create_value_model(cfg, tcfg) @@ -313,13 +315,17 @@ class PPOTrainerWrapper: self._num_epochs = tcfg.epochs self._max_length = cfg.data.max_length - def _get_or_create_reward_model(self, cfg, tcfg): - """Get existing reward model or create one for trl experimental PPO API. + def _get_or_create_reward_model(self, cfg, tcfg, *, reward_funcs_supplied=False): + """Get the reward model for trl's PPO API (an nn.Module, not a callable). - The experimental PPOTrainer requires an nn.Module reward model (not a callable). - If we have a loaded reward model instance, use it. Otherwise, load one from - the configured reward_model path, or create a fresh AutoModelForSequenceClassification - from the base model. + Uses a loaded instance, else loads the configured ``reward_model`` path. + If neither exists, PPO has NO real reward signal: a fresh + ``AutoModelForSequenceClassification`` has a randomly-initialised + regression head, so optimising the policy against it trains toward + noise (silent, GPU-hours wasted). A ``reward_fn`` (callable) cannot fill + this nn.Module slot on trl's PPO API. Fail loudly instead — unless the + trainer is separately consuming ``reward_funcs`` (a newer trl API), in + which case the reward model is unused and a neutral head is fine. """ if self.reward_model_instance is not None: return self.reward_model_instance @@ -331,12 +337,23 @@ class PPOTrainerWrapper: tcfg=tcfg, ) - # Fallback: create a sequence classification model from the base model + if not reward_funcs_supplied: + raise RuntimeError( + "PPO requires a trained reward_model (set training.reward_model " + "to a path or HF id). trl's PPO reward-model slot needs an " + "nn.Module; a reward_fn cannot drive it, and creating one from " + "the base model would train the policy against a randomly-" + "initialised reward head. Use `task: grpo` for reward-function-" + "based RL instead." + ) + + # reward_funcs handles the reward signal on this trl API; the reward + # model is a required-but-unused formality (neutral head is fine). from transformers import AutoModelForSequenceClassification console.print( - "[yellow]No reward_model path specified. Creating reward model " - f"from base model: {cfg.base}[/]" + "[dim]reward_funcs active; creating a neutral reward-model " + f"placeholder from base model: {cfg.base}[/]" ) reward_model = AutoModelForSequenceClassification.from_pretrained( cfg.base, diff --git a/src/soup_cli/trainer/sft.py b/src/soup_cli/trainer/sft.py index eec6892..e9f38a4 100644 --- a/src/soup_cli/trainer/sft.py +++ b/src/soup_cli/trainer/sft.py @@ -1203,9 +1203,31 @@ class SFTTrainerWrapper: f"{self._output_dir!r}" ) offload_save_dir = candidate - with offload_context( - tcfg.activation_offloading, save_dir=offload_save_dir - ): + import contextlib + + with contextlib.ExitStack() as _train_ctx: + _train_ctx.enter_context( + offload_context(tcfg.activation_offloading, save_dir=offload_save_dir) + ) + # LongLoRA S² shifted-sparse attention (v0.49.0 schema). The override + # monkeypatches attention.forward for the duration of training and + # was previously never installed (use_longlora validated but shipped + # plain attention). Enter defensively: an install failure on the + # current transformers degrades to plain attention with a warning + # instead of crashing the run. Arch compat is already schema-gated. + if getattr(tcfg, "use_longlora", False) and self.config.backend == "transformers": + from soup_cli.utils.longlora import apply_longlora_forward_override + + try: + _train_ctx.enter_context( + apply_longlora_forward_override(self.model) + ) + console.print("[green]LongLoRA S² attention override active[/]") + except Exception as exc: # noqa: BLE001 — fall back to plain attn + console.print( + "[yellow]LongLoRA override could not be installed " + f"({exc}); training with plain attention.[/]" + ) self.trainer.train(resume_from_checkpoint=resume_from_checkpoint) duration = time.time() - start diff --git a/src/soup_cli/ui/app.py b/src/soup_cli/ui/app.py index f10f169..beb2887 100644 --- a/src/soup_cli/ui/app.py +++ b/src/soup_cli/ui/app.py @@ -334,15 +334,17 @@ def create_app(host: str = "127.0.0.1", port: int = 7860): @app.post("/api/data/inspect", dependencies=[Depends(_verify_token)]) def inspect_data(req: DataInspectRequest): from soup_cli.data.loader import load_raw_data + from soup_cli.utils.paths import is_under_cwd - # Path traversal protection: resolve and check against cwd - allowed_root = Path.cwd().resolve() + # Path traversal protection. Use realpath + commonpath containment + # (is_under_cwd) — the old str.startswith check let a sibling like + # ".../project-secrets" pass as under ".../project". try: resolved = Path(req.path).resolve() except (ValueError, OSError): raise HTTPException(status_code=400, detail="Invalid path") - if not str(resolved).startswith(str(allowed_root)): + if not is_under_cwd(req.path): raise HTTPException( status_code=403, detail="Access denied: path outside working directory" ) diff --git a/src/soup_cli/utils/block_expansion.py b/src/soup_cli/utils/block_expansion.py index 15a8756..41d8271 100644 --- a/src/soup_cli/utils/block_expansion.py +++ b/src/soup_cli/utils/block_expansion.py @@ -197,6 +197,11 @@ def apply_llama_pro_freeze(model: Any, num_new_blocks: int) -> int: if layers is None: return 0 total = len(layers) + # Defence-in-depth: never treat more than `total` layers as "new" — an + # unclamped over-request would push `new_start` to 0 and unfreeze the whole + # model. Callers should pass the actual appended count (see the shared + # helper), but clamp here too so direct callers stay safe. + n = min(n, total) new_start = max(0, total - n) # Freeze everything first. for param in model.parameters(): @@ -278,22 +283,28 @@ def apply_block_expansion_if_configured( n = getattr(tcfg, "expand_layers", None) if not n: return _count_layers(model) + orig_total = _count_layers(model) new_total = expand_model_blocks(model, n) + # expand_model_blocks CLAMPS the requested count to the available base + # blocks, so the number actually appended can be < n. The freeze below must + # target that real count — passing the raw request would leave original + # layers trainable when over-requested, defeating LLaMA Pro entirely. + added = int(new_total) - int(orig_total) if console is not None: console.print( f"[green]LLaMA Pro:[/] expanded to {int(new_total)} layers " - f"(+{int(n)} zero-init blocks)" + f"(+{added} zero-init blocks)" ) freeze = getattr(tcfg, "freeze_trainable_layers", None) # Project policy ``is None`` over falsy — but a value of 0 means "no # positive freeze direction", so the canonical "train only new blocks" # path runs iff the user opted in with a positive ``freeze_trainable_layers``. - if freeze is not None and freeze > 0: - trainable = apply_llama_pro_freeze(model, n) + if freeze is not None and freeze > 0 and added > 0: + trainable = apply_llama_pro_freeze(model, added) if console is not None: console.print( f"[green]LLaMA Pro freeze:[/] {trainable:,} parameters " - f"trainable (only the {int(n)} new blocks)" + f"trainable (only the {added} new blocks)" ) return new_total diff --git a/src/soup_cli/utils/deploy_measure.py b/src/soup_cli/utils/deploy_measure.py index ed22674..57666b2 100644 --- a/src/soup_cli/utils/deploy_measure.py +++ b/src/soup_cli/utils/deploy_measure.py @@ -64,7 +64,13 @@ class MeasureResult: verdict: str # "OK" | "MINOR" | "MAJOR" -def compute_cache_key(*, base_sha: str, profile_name: str, tasks_sha: str) -> str: +def compute_cache_key( + *, + base_sha: str, + profile_name: str, + tasks_sha: str, + candidates: "Optional[Sequence[str]]" = None, +) -> str: """Build a deterministic cache key from the input tuple. Callers should pass FULL SHA-256 hex strings (64 chars) for the two @@ -74,6 +80,13 @@ def compute_cache_key(*, base_sha: str, profile_name: str, tasks_sha: str) -> st probability ≈ 1 in 2³² across ~4 billion cache entries) but is the operator-facing policy, not a property enforced here. Future callers that truncate further should expect collisions in proportion. + + ``candidates`` MUST be included in the key: the cached rows are one + :class:`MeasureResult` per candidate, so a run measuring ``["4bit"]`` + must not be served for a later ``["4bit", "8bit"]`` request (which would + silently drop the 8bit measurement). Omitting ``candidates`` reproduces + the pre-fix key for back-compat, but the live orchestrator always passes + the list. """ for name, value in ( ("base_sha", base_sha), @@ -96,6 +109,17 @@ def compute_cache_key(*, base_sha: str, profile_name: str, tasks_sha: str) -> st hasher.update(profile_name.encode("utf-8")) hasher.update(b"\x1f") hasher.update(tasks_sha.encode("utf-8")) + if candidates is not None: + if isinstance(candidates, (str, bytes)) or not isinstance(candidates, Sequence): + raise TypeError("candidates must be a sequence of strings") + hasher.update(b"\x1f") + for cand in candidates: + if not isinstance(cand, str): + raise TypeError( + f"each candidate must be str, got {type(cand).__name__}" + ) + hasher.update(cand.encode("utf-8")) + hasher.update(b"\x1e") return hasher.hexdigest()[:32] @@ -560,6 +584,7 @@ def run_measure( tasks_sha = sha_of_file(tasks_file) key = compute_cache_key( base_sha=base_sha, profile_name=profile_name, tasks_sha=tasks_sha, + candidates=list(candidates), ) cache = load_cache(cache_path) diff --git a/src/soup_cli/utils/edit_kernels.py b/src/soup_cli/utils/edit_kernels.py index 1056734..807c6d3 100644 --- a/src/soup_cli/utils/edit_kernels.py +++ b/src/soup_cli/utils/edit_kernels.py @@ -520,7 +520,10 @@ def apply_alphaedit_edit( ) key_t = key.to(down.weight.dtype) denom = float(torch.dot(key_t, key_t).item()) - if denom <= 0.0: + # NaN <= 0.0 is False, so a non-finite denom would slip past a bare + # `denom <= 0.0` and corrupt the weights in place (matches the sibling + # `_rank1_update` guard). Reject non-finite AND non-positive. + if not math.isfinite(denom) or denom <= 0.0: raise ValueError("key vector has zero norm; cannot apply AlphaEdit update") # Logical [out, in] ROME update (delta is OUTPUT-space, key is INPUT-space). rome_update = torch.outer(delta.to(down.weight.dtype), key_t) / denom diff --git a/src/soup_cli/utils/eval_gate_hook.py b/src/soup_cli/utils/eval_gate_hook.py index 7930bcd..5e060b4 100644 --- a/src/soup_cli/utils/eval_gate_hook.py +++ b/src/soup_cli/utils/eval_gate_hook.py @@ -296,7 +296,8 @@ if [ -z "$CANDIDATE_RUN_ID" ]; then exit 0 fi -soup eval against "$BASELINE_RUN_ID" --candidate "$CANDIDATE_RUN_ID" --json-only \\ +soup eval against "$BASELINE_RUN_ID" --candidate "$CANDIDATE_RUN_ID" \\ + --suite "$GATE_SUITE" --json-only \\ || {{ echo "[soup] pre-push gate blocked: regression vs $BASELINE_RUN_ID" >&2 exit 1 diff --git a/src/soup_cli/utils/gguf_quant.py b/src/soup_cli/utils/gguf_quant.py index 7aa9314..0605cf4 100644 --- a/src/soup_cli/utils/gguf_quant.py +++ b/src/soup_cli/utils/gguf_quant.py @@ -393,7 +393,14 @@ def _prepare_calibration_text(calibration_data: str, staged_dir: Path) -> Path: "calibration_data became a symlink during the export " "(TOCTOU defence): refusing to open." ) from exc - os.close(fd) + # Read from THIS fd. The previous code closed it and re-opened `src` + # with a plain open(), re-introducing the symlink-swap window between + # the O_NOFOLLOW check and the actual read. + src_fh = os.fdopen(fd, encoding="utf-8") + else: + # No O_NOFOLLOW (Windows): enforce_under_cwd_and_no_symlink's + # dispatch-time check is the portable backstop. + src_fh = open(src, encoding="utf-8") out = staged_dir / "calib.txt" line_count = 0 total_bytes = 0 @@ -405,9 +412,7 @@ def _prepare_calibration_text(calibration_data: str, staged_dir: Path) -> Path: sanitised = text.replace("\x00", "").replace("\n", " ") return sanitised[:max_per_line] - with open(src, encoding="utf-8") as fh_in, open( - out, "w", encoding="utf-8" - ) as fh_out: + with src_fh as fh_in, open(out, "w", encoding="utf-8") as fh_out: for raw_line in fh_in: line = raw_line.strip() if not line: diff --git a/src/soup_cli/utils/namespace_pin.py b/src/soup_cli/utils/namespace_pin.py index 912930a..2919816 100644 --- a/src/soup_cli/utils/namespace_pin.py +++ b/src/soup_cli/utils/namespace_pin.py @@ -297,6 +297,22 @@ def _is_backward(current: str, recorded: str) -> bool: return current < recorded +def _created_at_differs(current: str, recorded: str) -> bool: + """Return True when ``current`` is a DIFFERENT moment than ``recorded``. + + A pinned repo's ``created_at`` is immutable, so ANY drift — forward OR + backward — means the namespace was re-created (the AI-Jacking / repo- + recreation attack this pin targets). The previous gate only flagged a + *backward* jump, but a recreated repo gets a *later* timestamp and sailed + straight through. Offset-aware equality via ``fromisoformat``; falls back + to string inequality when either value fails to parse (legacy rows). + """ + try: + return datetime.fromisoformat(current) != datetime.fromisoformat(recorded) + except ValueError: + return current != recorded + + def record_repo_first_seen( store: NamespacePinStore, *, @@ -386,13 +402,13 @@ def verify_namespace( ) author_match = existing.author == current_author - # Compare timestamps numerically rather than lexicographically — the - # latter works for fixed-format UTC strings but is fragile if a non-Z - # offset arrives from HF Hub. Fall back to a lexicographic compare - # only when parsing fails (legacy data). - created_at_backward = _is_backward(current_created_at, existing.created_at) + # Flag ANY created_at drift (forward or backward): a pinned repo's creation + # time is immutable, so a changed timestamp means the namespace was + # re-created. The old `_is_backward`-only check missed the repo-recreation + # attack (which produces a LATER timestamp). + created_at_changed = _created_at_differs(current_created_at, existing.created_at) - if author_match and not created_at_backward: + if author_match and not created_at_changed: return NamespaceVerifyReport( repo_id=repo_id, ok=True, @@ -400,8 +416,8 @@ def verify_namespace( recorded=existing, ) - # Mismatch — either author changed or created_at jumped backward. - # Author comparison is case-insensitive: HF Hub authors are + # Mismatch — either author changed or created_at drifted (in either + # direction). Author comparison is case-insensitive: HF Hub authors are # case-insensitive, and a user typing the wrong case should not # fail the gate (security-review LOW fix). if override is not None and override.lower() == current_author.lower(): diff --git a/src/soup_cli/utils/preference_combine.py b/src/soup_cli/utils/preference_combine.py index c891f9d..b1f6d2a 100644 --- a/src/soup_cli/utils/preference_combine.py +++ b/src/soup_cli/utils/preference_combine.py @@ -128,19 +128,41 @@ def compute_simpo_term( return -_logsigmoid(logits).mean() -def compute_orpo_term(pol_chosen, pol_rejected, alpha: float): +def compute_orpo_term( + pol_chosen, + pol_rejected, + alpha: float, + chosen_lens=None, + rejected_lens=None, +): """Reference-free odds-ratio preference loss (ORPO). Uses the response-log-prob formulation ``-log σ(log(p_w) - log(p_l) + log(1-p_l) - log(1-p_w))`` scaled by ``alpha``. Approximates the full ORPO loss without the SFT term — caller is expected to mix in SFT via its own weight if desired. + + ``pol_chosen`` / ``pol_rejected`` are *summed* sequence log-probs. On a real + sequence the summed log-prob is ≈ −45, so ``exp()`` underflows to 0 and the + ``log(1 − p)`` odds-ratio correction collapses to 0 — degenerating the loss + to a plain log-prob difference. TRL avoids this with ``average_log_prob``; + pass ``chosen_lens`` / ``rejected_lens`` (response token counts) to + length-normalise the log-probs here so ``exp()`` yields a real per-token + probability and the odds-ratio term is meaningful. """ import torch - log_odds_chosen = pol_chosen - torch.log1p(-torch.exp(pol_chosen).clamp(max=1 - 1e-7)) - log_odds_rejected = pol_rejected - torch.log1p( - -torch.exp(pol_rejected).clamp(max=1 - 1e-7) + if chosen_lens is not None and rejected_lens is not None: + chosen_lens = torch.clamp(chosen_lens.float(), min=1.0) + rejected_lens = torch.clamp(rejected_lens.float(), min=1.0) + lp_chosen = pol_chosen / chosen_lens + lp_rejected = pol_rejected / rejected_lens + else: + lp_chosen = pol_chosen + lp_rejected = pol_rejected + log_odds_chosen = lp_chosen - torch.log1p(-torch.exp(lp_chosen).clamp(max=1 - 1e-7)) + log_odds_rejected = lp_rejected - torch.log1p( + -torch.exp(lp_rejected).clamp(max=1 - 1e-7) ) sigm_term = _logsigmoid(log_odds_chosen - log_odds_rejected) return (-alpha * sigm_term).mean() @@ -271,8 +293,13 @@ def attach_weighted_preference_combine(trainer: object, weights: Mapping[str, fl alpha = ( float(alpha_attr) if alpha_attr is not None else 1.0 ) + # Length-normalise when the batch carries response + # lengths/labels — otherwise summed log-probs underflow + # exp() and the odds-ratio correction degenerates. terms["orpo"] = compute_orpo_term( - pol_chosen, pol_rejected, alpha + pol_chosen, pol_rejected, alpha, + chosen_lens=_read_lens(inputs, "chosen"), + rejected_lens=_read_lens(inputs, "rejected"), ) elif name == "bco": # BCO is data-format-incompatible — already rejected @@ -311,6 +338,30 @@ def _read_logps(obj, name: str): return getattr(obj, name, None) +def _read_lens(obj, prefix: str): + """Best-effort per-side response-length tensor for length-normalised terms. + + Tries an explicit ``_lens`` key first, then derives the count from + ``_labels`` (non-``-100`` tokens = the loss-masked response). Returns + None when neither is present, in which case the caller keeps the (degenerate) + summed-log-prob behaviour rather than crashing. + """ + lens = _read_logps(obj, f"{prefix}_lens") + if lens is not None: + return lens + labels = _read_logps(obj, f"{prefix}_labels") + if labels is None: + return None + try: + import torch + + if not isinstance(labels, torch.Tensor): + return None + return (labels != -100).sum(dim=-1) + except Exception: # noqa: BLE001 — best-effort; degrade to no normalisation + return None + + def _pick_primary(weights: Mapping[str, float]) -> str: """Return the loss with the highest weight (deterministic on ties).""" return max(sorted(weights), key=lambda k: weights[k]) diff --git a/src/soup_cli/utils/rl_checkpoint.py b/src/soup_cli/utils/rl_checkpoint.py index 50f8b67..891dfb5 100644 --- a/src/soup_cli/utils/rl_checkpoint.py +++ b/src/soup_cli/utils/rl_checkpoint.py @@ -217,6 +217,33 @@ def _step_number(name: str) -> int: return -1 +def _is_main_process() -> bool: + """True on the rank-0 / non-distributed process. + + Only the main process may write RL checkpoints — concurrent writes from + every rank to the same directory race and corrupt the (non-atomic) saves. + Prefers ``torch.distributed`` when initialised; otherwise reads the + launcher env vars (accelerate / torchrun set ``RANK`` / ``LOCAL_RANK``). + """ + import os + + try: + import torch.distributed as dist + + if dist.is_available() and dist.is_initialized(): + return dist.get_rank() == 0 + except Exception: # noqa: BLE001 — torch missing / not initialised + pass + for var in ("RANK", "LOCAL_RANK"): + raw = os.environ.get(var) + if raw is not None: + try: + return int(raw) == 0 + except ValueError: + return True + return True + + class RLCheckpointCallback(_TrainerCallbackBase): # type: ignore[misc, valid-type] """Live HF TrainerCallback for mid-epoch RL checkpoints (v0.71.11 #238). @@ -281,6 +308,15 @@ class RLCheckpointCallback(_TrainerCallbackBase): # type: ignore[misc, valid-ty root = self._ckpt_root() ckpt_dir = os.path.join(root, f"step-{int(step)}") + + # Multi-process guard: only the main (rank-0) process writes. Every rank + # would otherwise race on the SAME paths — the non-atomic + # torch.save(optimizer.pt) + save_pretrained can interleave and corrupt + # the checkpoint, after which the rollback ladder silently restores + # nothing. Non-main ranks return the (rank-0-written) path unchanged. + if not _is_main_process(): + return ckpt_dir + os.makedirs(ckpt_dir, exist_ok=True) if model is not None and hasattr(model, "save_pretrained"): @@ -297,7 +333,11 @@ class RLCheckpointCallback(_TrainerCallbackBase): # type: ignore[misc, valid-ty try: import torch - torch.save(optimizer.state_dict(), opt_out) + # Atomic write: torch.save straight to opt_out could leave a + # truncated file if interrupted. Stage to .tmp then rename. + tmp_out = opt_out + ".tmp" + torch.save(optimizer.state_dict(), tmp_out) + os.replace(tmp_out, opt_out) has_optimizer = True except Exception: # noqa: BLE001 — best-effort, manifest reflects it has_optimizer = False diff --git a/src/soup_cli/utils/sglang.py b/src/soup_cli/utils/sglang.py index 505cb4f..075ce23 100644 --- a/src/soup_cli/utils/sglang.py +++ b/src/soup_cli/utils/sglang.py @@ -112,10 +112,13 @@ def create_sglang_app( app = FastAPI(title="Soup Inference Server (SGLang)", version="1.0.0") + # Loopback-only CORS (parity with the transformers / vLLM servers). The + # wildcard the loopback fix never reached let any web page read this local + # server's responses. app.add_middleware( CORSMiddleware, - allow_origins=["*"], - allow_methods=["*"], + allow_origin_regex=r"^https?://(localhost|127\.0\.0\.1)(:\d+)?$", + allow_methods=["GET", "POST", "OPTIONS"], allow_headers=["*"], ) diff --git a/src/soup_cli/utils/terraform_plan.py b/src/soup_cli/utils/terraform_plan.py index 1d19e1a..fa2f1a0 100644 --- a/src/soup_cli/utils/terraform_plan.py +++ b/src/soup_cli/utils/terraform_plan.py @@ -219,10 +219,25 @@ _DEFAULT_SPOT_PRICE = 0.30 # ~$/hr for a 24 GB consumer GPU _DEFAULT_PEAK_VRAM = 8.0 +def _to_float(value: Any, default: float) -> float: + """Coerce a config number to float, tolerating ``batch_size: "auto"``. + + ``soup train`` accepts ``batch_size: "auto"`` (int-or-"auto" per schema), so + the plan estimator must not crash on ``float("auto")`` — fall back to the + baseline for the heuristic instead. + """ + try: + return float(value) + except (TypeError, ValueError): + return default + + def _estimate_runtime_minutes(config: Mapping[str, Any]) -> float: training = config.get("training", {}) if isinstance(config, Mapping) else {} - epochs = float(training.get("epochs", 1) if isinstance(training, Mapping) else 1) - batch_size = float(training.get("batch_size", 4) if isinstance(training, Mapping) else 4) + if not isinstance(training, Mapping): + training = {} + epochs = _to_float(training.get("epochs", 1), 1.0) + batch_size = _to_float(training.get("batch_size", 4), 4.0) # Soft heuristic: 5 minutes per epoch at batch_size=4 baseline. base = 5.0 * epochs if batch_size > 0: diff --git a/tests/test_code_review_high.py b/tests/test_code_review_high.py new file mode 100644 index 0000000..46bd191 --- /dev/null +++ b/tests/test_code_review_high.py @@ -0,0 +1,413 @@ +"""Regression tests for the HIGH findings in CODE_REVIEW.md. + +Grouped by the review's own subsections. Each asserts the specific broken +behavior is now correct. +""" + +from __future__ import annotations + +import types +from pathlib import Path + +import pytest + +import soup_cli + +# ═══════════════════════ HIGH — training correctness ═══════════════════════ + + +def test_ppo_refuses_silent_random_reward_head(): + """PPO with only a reward_fn (no reward_model) must fail loudly, not build + a randomly-initialised reward head and train the policy against noise.""" + from soup_cli.trainer.ppo import PPOTrainerWrapper + + wrapper = object.__new__(PPOTrainerWrapper) + wrapper.reward_model_instance = None + wrapper.device = "cpu" + wrapper.trust_remote_code = False + wrapper._trust_remote_code = False + cfg = types.SimpleNamespace(base="hf-internal-testing/tiny-random-gpt2") + tcfg = types.SimpleNamespace(reward_model=None) + with pytest.raises(RuntimeError, match="reward_model"): + wrapper._get_or_create_reward_model(cfg, tcfg) + + +def test_alphaedit_rejects_nonfinite_denominator(monkeypatch): + """A NaN key-norm must be rejected (NaN <= 0.0 is False, so the old bare + `denom <= 0.0` guard let it corrupt weights in place).""" + torch = pytest.importorskip("torch") + import soup_cli.utils.edit_kernels as ek + + class _Down: + def __init__(self): + self.weight = torch.zeros(2, 2) + + monkeypatch.setattr(ek, "_locate_decoder_layers", lambda m: ["layer0"]) + monkeypatch.setattr(ek, "_down_proj", lambda layers, layer: _Down()) + monkeypatch.setattr( + ek, "_capture_key", lambda *a, **k: torch.tensor([float("nan"), float("nan")]) + ) + monkeypatch.setattr(ek, "_optimise_residual", lambda *a, **k: torch.tensor([1.0, 1.0])) + + with pytest.raises(ValueError, match="zero norm"): + ek.apply_alphaedit_edit( + object(), object(), subject="s", target="t", layer=0, device="cpu" + ) + + +def test_orpo_length_normalization_restores_odds_ratio(): + """With summed sequence log-probs exp() underflows and the odds-ratio + correction collapses; passing lengths length-normalises and restores it.""" + torch = pytest.importorskip("torch") + from soup_cli.utils.preference_combine import compute_orpo_term + + pol_chosen = torch.tensor([-40.0]) + pol_rejected = torch.tensor([-50.0]) + lens = torch.tensor([20.0]) + + degenerate = float(compute_orpo_term(pol_chosen, pol_rejected, 1.0)) + normalized = float( + compute_orpo_term( + pol_chosen, pol_rejected, 1.0, chosen_lens=lens, rejected_lens=lens + ) + ) + import math as _math + + assert _math.isfinite(normalized) + # The summed-logp version underflows to ~0 loss; the length-normalised + # version has a meaningful (larger) odds-ratio loss. + assert normalized - degenerate > 1e-2 + + +def test_ipo_beta_schedule_uses_ipo_tau_not_dpo_beta(): + src = (Path(soup_cli.__file__).parent / "trainer" / "ipo.py").read_text( + encoding="utf-8" + ) + assert "beta_start=tcfg.ipo_tau" in src + assert "beta_start=tcfg.dpo_beta" not in src + + +def test_beta_schedule_callback_sets_beta_from_start(): + """At step 0 the schedule must apply beta_start — so passing ipo_tau (not + the DPO default) is what keeps the user's τ intact.""" + from soup_cli.utils.dpo_variants import BetaScheduleCallback + + trainer = types.SimpleNamespace(beta=0.5) # e.g. the user's ipo_tau + cb = BetaScheduleCallback(beta_start=0.5, beta_end=0.1, total_steps=10, schedule="linear") + cb.attach(trainer) + state = types.SimpleNamespace(max_steps=10, global_step=0) + cb.on_train_begin(None, state, None) + cb.on_step_begin(None, state, None) + assert abs(trainer.beta - 0.5) < 1e-9 + + +def test_distill_term_masks_padding_and_prompt(): + """The default distillation divergence must be measured only over trained + tokens (labels != -100), not padding/prompt.""" + torch = pytest.importorskip("torch") + from soup_cli.trainer.distill import _compute_distill_term + + student = torch.zeros(1, 3, 4) + teacher = torch.zeros(1, 3, 4) + # Positions 0,1 diverge wildly; position 2 is identical (KL 0). + student[0, 0, 0] = 10.0 + teacher[0, 0, 3] = 10.0 + student[0, 1, 1] = 10.0 + teacher[0, 1, 2] = 10.0 + labels = torch.tensor([[-100, -100, 5]]) # only the last token is trained + + masked = float(_compute_distill_term(student, teacher, "forward_kl", 1.0, labels=labels)) + unmasked = float(_compute_distill_term(student, teacher, "forward_kl", 1.0)) + assert masked < 1e-5, masked # only the identical (trained) position counts + assert unmasked > 0.1, unmasked # divergent prompt positions dominate unmasked + + +def test_kto_negative_one_label_is_undesirable(): + """`bool(-1)` is True; a -1 label in the ±1 convention must map to False.""" + from soup_cli.data.formats import _convert_kto + + base = {"prompt": "p", "completion": "c"} + assert _convert_kto({**base, "label": -1})["label"] is False + assert _convert_kto({**base, "label": 1})["label"] is True + assert _convert_kto({**base, "label": 0})["label"] is False + assert _convert_kto({**base, "label": True})["label"] is True + assert _convert_kto({**base, "label": False})["label"] is False + assert _convert_kto({**base, "label": "false"})["label"] is False + + +def test_apply_llama_pro_freeze_freezes_all_but_new_blocks(): + pytest.importorskip("torch") + import torch.nn as nn + + from soup_cli.utils.block_expansion import apply_llama_pro_freeze + + class _M(nn.Module): + def __init__(self): + super().__init__() + self.model = nn.Module() + self.model.layers = nn.ModuleList([nn.Linear(2, 2) for _ in range(4)]) + + model = _M() + apply_llama_pro_freeze(model, 2) # keep only the last 2 blocks trainable + trainable = [ + all(p.requires_grad for p in layer.parameters()) for layer in model.model.layers + ] + assert trainable == [False, False, True, True] + + +def test_block_expansion_freeze_uses_actual_added_not_requested(monkeypatch): + """When expand_layers over-requests, the freeze must target the ACTUAL + appended count (clamped), else original layers stay trainable.""" + import soup_cli.utils.block_expansion as be + + class _Inner: + def __init__(self, n): + self.layers = list(range(n)) + + class _Model: + def __init__(self, n): + self.model = _Inner(n) + + model = _Model(4) # 4 base layers + + def fake_expand(m, n): + added = min(n, 4) # expand_model_blocks clamps to available base blocks + m.model.layers = list(range(4 + added)) + return 4 + added + + captured = {} + + def fake_freeze(m, count): + captured["count"] = count + return 123 + + monkeypatch.setattr(be, "expand_model_blocks", fake_expand) + monkeypatch.setattr(be, "apply_llama_pro_freeze", fake_freeze) + + tcfg = types.SimpleNamespace(expand_layers=10, freeze_trainable_layers=1) + be.apply_block_expansion_if_configured(model, tcfg) + assert captured["count"] == 4 # actual added, NOT the over-requested 10 + + +def _src(rel: str) -> str: + return (Path(soup_cli.__file__).parent / rel).read_text(encoding="utf-8") + + +# ═══════════════════════ HIGH — features that silently do nothing ═══════════ + + +def test_longlora_wired_into_sft_train(): + src = _src("trainer/sft.py") + assert "apply_longlora_forward_override" in src, "LongLoRA override never installed" + + +def test_gpus_reexec_passes_run_shaping_flags(): + src = _src("commands/train.py") + for flag in ('"--gate"', '"--push-as"', '"--trust-remote-code"', '"--tracker"', + '"--diagnose-gate"', '"--annex-xi"', '"--repro-receipt"'): + assert "script_args" in src and flag in src, f"re-exec drops {flag}" + + +def test_pre_push_hook_enforces_gate_suite(): + from soup_cli.utils.eval_gate_hook import render_pre_push_hook + + hook = render_pre_push_hook(baseline_run_id="run-abc123", suite_path="evals/locked.json") + assert '--suite "$GATE_SUITE"' in hook + + +def test_eval_against_blocks_on_unloadable_locked_suite(tmp_path, monkeypatch): + from typer.testing import CliRunner + + from soup_cli.commands.eval import app + + monkeypatch.chdir(tmp_path) + result = CliRunner().invoke( + app, + ["against", "base-run", "--candidate", "cand-run", "--suite", "missing.json"], + ) + assert result.exit_code == 1, (result.output, repr(result.exception)) + + +def test_deploy_measure_cache_key_includes_candidates(): + from soup_cli.utils.deploy_measure import compute_cache_key + + common = dict(base_sha="a" * 16, profile_name="p", tasks_sha="b" * 64) + k_one = compute_cache_key(**common, candidates=["4bit"]) + k_two = compute_cache_key(**common, candidates=["4bit", "8bit"]) + k_none = compute_cache_key(**common) # back-compat: same as pre-fix key + assert k_one != k_two, "different candidate sets must not share a cache key" + assert k_none not in (k_one, k_two) + + +# ═══════════════════════ HIGH — security ═══════════════════════ + + +def test_sglang_cors_is_loopback_only(): + src = _src("utils/sglang.py") + assert 'allow_origins=["*"]' not in src + assert "localhost|127" in src # loopback-only regex + + +def test_fetch_lstats_original_path_not_realpath(): + src = _src("commands/fetch.py") + assert "os.lstat(target_path)" in src + assert "os.lstat(real_target)" not in src + + +def test_ui_inspect_uses_commonpath_containment(): + src = _src("ui/app.py") + assert "is_under_cwd(req.path)" in src + + +def test_is_under_cwd_rejects_sibling_prefix(tmp_path, monkeypatch): + from soup_cli.utils.paths import is_under_cwd + + proj = tmp_path / "project" + proj.mkdir() + (tmp_path / "project-secrets").mkdir() + monkeypatch.chdir(proj) + assert is_under_cwd(str(proj / "d.jsonl")) is True + # The sibling shares the "project" prefix but is NOT under cwd. + assert is_under_cwd(str(tmp_path / "project-secrets" / "d.jsonl")) is False + + +def test_registry_lineage_cycle_detected_beyond_depth_10(tmp_path, monkeypatch): + monkeypatch.setenv("SOUP_REGISTRY_DB_PATH", str(tmp_path / "reg.db")) + from soup_cli.registry.store import RegistryStore + + with RegistryStore() as store: + ids = [ + store.push( + name=f"m{i}", tag="v1", base_model="b", task="sft", + run_id=None, config={}, + ) + for i in range(15) + ] + # Chain child->parent 14 deep: ids[0] -> ids[1] -> ... -> ids[14]. + for i in range(14): + store.add_lineage( + child_id=ids[i], parent_id=ids[i + 1], relation="forked_from" + ) + # ids[14] is an ancestor of ids[0] 14 hops away — past the old depth-10 + # cap. The unbounded walk must still catch the cycle. + with pytest.raises(ValueError, match="cycle"): + store.add_lineage( + child_id=ids[14], parent_id=ids[0], relation="forked_from" + ) + + +def test_gguf_calib_reads_from_nofollow_fd_no_reopen(): + src = _src("utils/gguf_quant.py") + assert "os.fdopen(fd" in src + assert "os.close(fd)" not in src # the close+reopen TOCTOU window is gone + + +def test_namespace_created_at_differs_both_directions(): + from soup_cli.utils.namespace_pin import _created_at_differs + + assert _created_at_differs("2026-01-02T00:00:00", "2026-01-01T00:00:00") is True + assert _created_at_differs("2026-01-01T00:00:00", "2026-01-02T00:00:00") is True + assert _created_at_differs("2026-01-01T00:00:00", "2026-01-01T00:00:00") is False + + +def test_namespace_pin_flags_forward_created_at_drift(tmp_path): + from soup_cli.utils.namespace_pin import NamespacePinStore, verify_namespace + + store = NamespacePinStore(str(tmp_path / "pins.db")) + first = verify_namespace( + store, repo_id="org/model", current_author="alice", + current_created_at="2026-01-01T00:00:00", + ) + assert first.ok is True # trust on first use + # Same author, LATER created_at (repo re-created / AI-Jacking) must be flagged. + recreated = verify_namespace( + store, repo_id="org/model", current_author="alice", + current_created_at="2026-06-01T00:00:00", + ) + assert recreated.ok is False + + +# ═══════════════════════ HIGH — robustness / cross-platform ═══════════════════ + + +def test_typer_exit_is_a_runtimeerror(): + import typer + + # This is WHY the bench.py `except RuntimeError` swallowed it and the + # eval.py `except SystemExit` missed it. + assert issubclass(typer.Exit, RuntimeError) + assert not issubclass(typer.Exit, SystemExit) + + +def test_bench_reraises_typer_exit(): + assert "except typer.Exit:" in _src("commands/bench.py") + + +def test_eval_auto_catches_typer_exit(): + assert "except (typer.Exit, SystemExit):" in _src("commands/eval.py") + + +def test_data_split_rejects_negative_val(tmp_path, monkeypatch): + from typer.testing import CliRunner + + from soup_cli.commands.data import app + + monkeypatch.chdir(tmp_path) + (tmp_path / "d.jsonl").write_text( + '{"messages": [{"role": "user", "content": "x"}]}\n' * 20, encoding="utf-8" + ) + result = CliRunner().invoke( + app, ["split", "d.jsonl", "--val", "-10", "--absolute"] + ) + assert result.exit_code == 1, (result.output, repr(result.exception)) + assert "non-negative" in result.output + + +def test_trace_parser_reads_bom_first_record(tmp_path): + from soup_cli.data.traces.parsers import parse_soup_serve + + trace_dir = tmp_path / "traces" + trace_dir.mkdir() + # utf-8-sig writes a BOM before line 1. + (trace_dir / "t.jsonl").write_text( + '{"prompt": "p1", "response": "r1"}\n{"prompt": "p2", "response": "r2"}\n', + encoding="utf-8-sig", + ) + traces = list(parse_soup_serve(str(trace_dir))) + assert len(traces) == 2, "BOM dropped the first record" + assert traces[0].prompt == "p1" + + +def test_plan_estimate_handles_batch_size_auto(): + from soup_cli.utils.terraform_plan import _estimate_runtime_minutes + + minutes = _estimate_runtime_minutes( + {"training": {"epochs": 2, "batch_size": "auto"}} + ) + assert minutes > 0 # float("auto") no longer crashes + + +def test_rl_checkpoint_only_main_process_writes(tmp_path, monkeypatch): + import os + + monkeypatch.chdir(tmp_path) + from soup_cli.utils.rl_checkpoint import ( + RLCheckpointConfig, + build_rl_checkpoint_callback, + ) + + cb = build_rl_checkpoint_callback( + RLCheckpointConfig(save_every_steps=1), output_dir="run", task="grpo" + ) + + class _M: + def save_pretrained(self, path): + os.makedirs(path, exist_ok=True) + + monkeypatch.setenv("RANK", "1") # non-main rank + path = cb.save_checkpoint(step=1, model=_M(), optimizer=None) + assert not os.path.exists(path), "non-main rank must not write the checkpoint" + + monkeypatch.setenv("RANK", "0") # main rank + path2 = cb.save_checkpoint(step=2, model=_M(), optimizer=None) + assert os.path.exists(os.path.join(path2, "manifest.json"))