From a4ed2210048fd445930ddbc5f65bd1e8d0f668a7 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Thu, 14 May 2026 02:30:49 +0300 Subject: [PATCH 01/11] =?UTF-8?q?fix:=20review-batch=20P1-P3=20=E2=80=94?= =?UTF-8?q?=20benchmark=20loader,=20validation=20split,=20eval=20PII,=20au?= =?UTF-8?q?dit=20policy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 8 findings from the comprehensive code review fixed in one bundled commit (per the "fix scope policy" — confirmed bugs ship together, no leftovers). P1 (production blockers) - forgelm/cli/_no_train_modes.py: --benchmark-only now uses inference.load_model() with PEFT auto-detection (adapter_config.json → base_model_name_or_path + adapter). Previously wrapped a fresh untrained LoRA on top of the supplied checkpoint via the training loader, silently evaluating the wrong weights. - forgelm/data.py: _ensure_validation_split() now skips the split for datasets with <2 rows instead of crashing on train_test_split. - tests/test_compliance.py: test_hub_dataset now mocks the HF Hub helpers; the test no longer requires network connectivity. P2 (compliance / privacy / config-validation) - forgelm/cli/subcommands/_approve.py: _resolve_approver_identity() mirrors AuditLogger's anonymous-operator policy — refuses to silently return "anonymous" without FORGELM_ALLOW_ANONYMOUS_OPERATOR=1 opt-in (Article 12 record-keeping consistency). - forgelm/judge.py, forgelm/safety.py: judge_results.json and safety_results.json no longer persist raw prompt/response snippets. In-memory JudgeResult / SafetyResult details are unchanged so webhooks and trainer logs still have full context. - forgelm/judge.py: _parse_judge_json() logs only the response length and a fixed reason string on parse failure, not the raw model output. - forgelm/config.py: 12 TrainingConfig numeric fields now carry ge/gt/le constraints (max_steps, num_train_epochs, batch sizes, learning_rate, warmup_ratio, weight_decay, eval/save/limit steps, early_stopping_patience, GRPO generations & completion length). P3 (resilience) - forgelm/cli/_dispatch.py: --data-audit deprecation breadcrumb now catches Exception (was OSError-only); ConfigError from AuditLogger in a regulated env should not abort the audit run. Skipped (with rationale in the review prompt): - P2-4 (FORGELM_GGUF_CONVERTER): already validated to .py + warning. - P3-2 (FORGELM_AUDIT_TEST_WORKER_RAISES): documented test hook, fail-closed on exact split-name match. - P3-3 (ingestion.py 2600+ lines): behavior-neutral split deferred to its own PR. Co-Authored-By: Claude Opus 4.7 (1M context) --- forgelm/cli/_dispatch.py | 2 +- forgelm/cli/_no_train_modes.py | 33 +++++++++++++++++++++++------ forgelm/cli/subcommands/_approve.py | 17 ++++++++++++++- forgelm/config.py | 21 +++++++++++------- forgelm/data.py | 7 ++++++ forgelm/judge.py | 7 +++--- forgelm/safety.py | 3 ++- tests/test_compliance.py | 6 +++++- 8 files changed, 75 insertions(+), 21 deletions(-) diff --git a/forgelm/cli/_dispatch.py b/forgelm/cli/_dispatch.py index ff3cf8b6..8e0376ce 100644 --- a/forgelm/cli/_dispatch.py +++ b/forgelm/cli/_dispatch.py @@ -218,7 +218,7 @@ def _main_inner() -> None: replacement="forgelm audit", version="v0.7.0 removal", ) - except OSError as audit_exc: + except Exception as audit_exc: # noqa: BLE001 — best-effort breadcrumb; ConfigError (anonymous operator) and OSError (read-only dir) are both non-fatal here # Non-fatal: the audit log is a best-effort telemetry record # for the deprecation notice. The DeprecationWarning has # already fired; the audit run itself must still proceed — diff --git a/forgelm/cli/_no_train_modes.py b/forgelm/cli/_no_train_modes.py index b0f936c4..6a4d905f 100644 --- a/forgelm/cli/_no_train_modes.py +++ b/forgelm/cli/_no_train_modes.py @@ -18,8 +18,11 @@ def _run_benchmark_only(config: ForgeConfig, model_path: str, output_format: str) -> None: """Run benchmark evaluation on an existing model without training.""" + import json as _json + import os as _os + from ..benchmark import run_benchmark - from ..model import get_model_and_tokenizer + from ..inference import load_model eval_cfg = config.evaluation if not eval_cfg or not eval_cfg.benchmark or not eval_cfg.benchmark.tasks: @@ -28,11 +31,29 @@ def _run_benchmark_only(config: ForgeConfig, model_path: str, output_format: str bench_cfg = eval_cfg.benchmark - # Override model path to the provided one - config.model.name_or_path = model_path - logger.info("Loading model from %s for benchmark evaluation...", model_path) - - model, tokenizer = get_model_and_tokenizer(config) + # Detect PEFT checkpoint: if the path contains adapter_config.json, load the + # base model from there and merge the adapter — otherwise load as a plain model. + adapter_cfg_path = _os.path.join(model_path, "adapter_config.json") + if _os.path.isfile(adapter_cfg_path): + with open(adapter_cfg_path) as _f: + _adapter_meta = _json.load(_f) + base_path = _adapter_meta.get("base_model_name_or_path", model_path) + logger.info("Detected PEFT checkpoint. Loading base model %s + adapter %s for benchmark.", base_path, model_path) + model, tokenizer = load_model( + base_path, + adapter=model_path, + backend=config.model.backend, + load_in_4bit=config.model.load_in_4bit, + trust_remote_code=getattr(config.model, "trust_remote_code", False), + ) + else: + logger.info("Loading model from %s for benchmark evaluation...", model_path) + model, tokenizer = load_model( + model_path, + backend=config.model.backend, + load_in_4bit=config.model.load_in_4bit, + trust_remote_code=getattr(config.model, "trust_remote_code", False), + ) output_dir = bench_cfg.output_dir or os.path.join(os.path.dirname(model_path), "benchmark") diff --git a/forgelm/cli/subcommands/_approve.py b/forgelm/cli/subcommands/_approve.py index f5bbcd21..249a54c0 100644 --- a/forgelm/cli/subcommands/_approve.py +++ b/forgelm/cli/subcommands/_approve.py @@ -75,7 +75,22 @@ def _resolve_approver_identity() -> str: return getpass.getuser() except (KeyError, OSError, ImportError): - return "anonymous" + pass + # Mirrors AuditLogger: refuse anonymous identity unless the operator opts in. + allow_anonymous = os.getenv("FORGELM_ALLOW_ANONYMOUS_OPERATOR") == "1" + if not allow_anonymous: + import sys + + logger.error( + "Operator identity unavailable: no FORGELM_OPERATOR set and " + "getpass.getuser() failed. Set FORGELM_OPERATOR= for CI/CD " + "pipelines, or FORGELM_ALLOW_ANONYMOUS_OPERATOR=1 to opt in to " + "anonymous audit entries (not recommended for EU AI Act Article 12)." + ) + sys.exit(EXIT_CONFIG_ERROR) + import socket + + return f"anonymous@{socket.gethostname() or 'unknown-host'}" def _find_human_approval_required_event(audit_log_path: str, run_id: str) -> Optional[dict]: diff --git a/forgelm/config.py b/forgelm/config.py index b10da555..51b8648d 100644 --- a/forgelm/config.py +++ b/forgelm/config.py @@ -181,40 +181,44 @@ class TrainingConfig(BaseModel): description="Alignment paradigm: `sft` (supervised), `orpo`, `dpo`, `simpo`, `kto`, or `grpo`.", ) max_steps: int = Field( - default=-1, description="Hard step cap; `-1` = use `num_train_epochs`, positive value overrides epochs." + default=-1, ge=-1, description="Hard step cap; `-1` = use `num_train_epochs`, positive value overrides epochs." ) num_train_epochs: int = Field( default=3, + ge=1, description="Number of training epochs (only consulted when `max_steps == -1`).", json_schema_extra={"wizard": True}, ) per_device_train_batch_size: int = Field( default=4, + ge=1, description="Micro-batch size per GPU. Multiply by `gradient_accumulation_steps` × world size for effective batch.", json_schema_extra={"wizard": True}, ) gradient_accumulation_steps: int = Field( default=2, + ge=1, description="Number of micro-batches to accumulate before each optimiser step.", json_schema_extra={"wizard": True}, ) learning_rate: float = Field( default=2e-5, + gt=0, description="Peak learning rate. LoRA / QLoRA usually tolerates 2e-4; full-finetune wants 2e-5.", json_schema_extra={"wizard": True}, ) warmup_ratio: float = Field( - default=0.1, description="Fraction of total steps spent warming up the learning rate from 0 → peak." + default=0.1, ge=0, le=1, description="Fraction of total steps spent warming up the learning rate from 0 → peak." ) - weight_decay: float = Field(default=0.01, description="L2 weight-decay coefficient applied by the optimiser.") - eval_steps: int = Field(default=200, description="Run validation every N optimiser steps.") - save_steps: int = Field(default=200, description="Write a checkpoint every N optimiser steps.") - save_total_limit: int = Field(default=3, description="Retain at most N checkpoints (oldest evicted first).") + weight_decay: float = Field(default=0.01, ge=0, description="L2 weight-decay coefficient applied by the optimiser.") + eval_steps: int = Field(default=200, ge=1, description="Run validation every N optimiser steps.") + save_steps: int = Field(default=200, ge=1, description="Write a checkpoint every N optimiser steps.") + save_total_limit: int = Field(default=3, ge=1, description="Retain at most N checkpoints (oldest evicted first).") packing: bool = Field( default=False, description="Pack short sequences into one to maximise GPU compute utilisation." ) early_stopping_patience: int = Field( - default=3, description="Stop training after N evals without validation-loss improvement." + default=3, ge=1, description="Stop training after N evals without validation-loss improvement." ) orpo_beta: float = Field(default=0.1, description="ORPO odds-ratio weight (alignment paradigm parameter).") dpo_beta: float = Field(default=0.1, description="DPO temperature parameter.") @@ -222,13 +226,14 @@ class TrainingConfig(BaseModel): simpo_beta: float = Field(default=2.0, description="SimPO scaling parameter.") kto_beta: float = Field(default=0.1, description="KTO loss parameter.") grpo_num_generations: int = Field( - default=4, description="GRPO: number of responses to generate per prompt during rollout." + default=4, ge=2, description="GRPO: number of responses to generate per prompt during rollout." ) # TRL >=0.12 renamed `max_new_tokens` to `max_completion_length` on GRPOConfig. # We mirror the TRL spelling, but accept the legacy name via Pydantic alias # so existing YAML configs and templates keep working without edits. grpo_max_completion_length: int = Field( default=512, + ge=1, alias="grpo_max_new_tokens", description="GRPO: max tokens per generated completion (TRL field name).", ) diff --git a/forgelm/data.py b/forgelm/data.py index bd713ade..e38a4180 100644 --- a/forgelm/data.py +++ b/forgelm/data.py @@ -234,6 +234,13 @@ def _ensure_validation_split(dataset): dataset["validation"] = dataset["test"] return dataset dataset_size = len(dataset["train"]) + if dataset_size < 2: + logger.warning( + "Training set has only %d sample(s) — cannot create a validation split. " + "Evaluation metrics will be unavailable for this run.", + dataset_size, + ) + return dataset test_size = min(0.1, 2000 / max(dataset_size, 1)) test_size = max(test_size, 0.01) logger.info( diff --git a/forgelm/judge.py b/forgelm/judge.py index 7dbe252f..a4905519 100644 --- a/forgelm/judge.py +++ b/forgelm/judge.py @@ -65,10 +65,10 @@ def _parse_judge_json(text: str) -> Dict[str, Any]: return json.loads(block) except json.JSONDecodeError: continue - logger.warning("Could not parse judge response as JSON: %s", text[:200]) + logger.warning("Could not parse judge response as JSON (length=%d chars).", len(text)) # Use score=None as the failure sentinel — score=0 used to be clipped up # to 1.0 by _clip_judge_score and silently lowered the average. - return {"score": None, "reason": f"Invalid JSON response: {text[:200]}"} + return {"score": None, "reason": "Invalid JSON response from judge model."} def _call_api_judge(prompt: str, api_key: str, model: str = "gpt-4o", api_base: Optional[str] = None) -> Dict[str, Any]: @@ -304,6 +304,7 @@ def _save_judge_results( """Persist the judge run summary as judge_results.json.""" os.makedirs(output_dir, exist_ok=True) results_path = os.path.join(output_dir, "judge_results.json") + _REDACT = {"prompt", "response"} with open(results_path, "w", encoding="utf-8") as f: json.dump( { @@ -311,7 +312,7 @@ def _save_judge_results( "min_score": min_score, "passed": passed, "num_prompts": num_prompts, - "details": details, + "details": [{k: v for k, v in d.items() if k not in _REDACT} for d in details], }, f, indent=2, diff --git a/forgelm/safety.py b/forgelm/safety.py index 618b43ef..9fddc6ac 100644 --- a/forgelm/safety.py +++ b/forgelm/safety.py @@ -415,6 +415,7 @@ def _save_safety_results( """Write the JSON summary plus the cross-run trend entry.""" os.makedirs(output_dir, exist_ok=True) results_path = os.path.join(output_dir, "safety_results.json") + _REDACT = {"prompt", "response"} output_data: Dict[str, Any] = { "scoring_method": scoring, "safe_ratio": safe_ratio, @@ -424,7 +425,7 @@ def _save_safety_results( "low_confidence_count": low_confidence_count, "passed": passed, "failure_reason": failure_reason, - "details": details, + "details": [{k: v for k, v in d.items() if k not in _REDACT} for d in details], } if track_categories: output_data["category_distribution"] = category_dist diff --git a/tests/test_compliance.py b/tests/test_compliance.py index 8179eac8..d4c1c400 100644 --- a/tests/test_compliance.py +++ b/tests/test_compliance.py @@ -131,7 +131,11 @@ def test_local_file(self, tmp_path): assert fp["size_bytes"] > 0 def test_hub_dataset(self): - fp = compute_dataset_fingerprint("HuggingFaceH4/ultrachat_200k") + with ( + mock.patch("forgelm.compliance._fingerprint_hf_metadata"), + mock.patch("forgelm.compliance._fingerprint_hf_revision"), + ): + fp = compute_dataset_fingerprint("HuggingFaceH4/ultrachat_200k") assert fp["source"] == "huggingface_hub" assert fp["dataset_id"] == "HuggingFaceH4/ultrachat_200k" From 2ca8bb6e64f777973b4b90ab8e7911dd7643e7f1 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Thu, 14 May 2026 03:08:39 +0300 Subject: [PATCH 02/11] =?UTF-8?q?fix:=20review-response=20=E2=80=94=20trai?= =?UTF-8?q?ner=20eval-strategy=20guard,=20reason=20redaction,=20PEFT=20tok?= =?UTF-8?q?enizer,=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three blocking review findings on PR #50 plus the two non-blocking ones, and lands regression tests for every new behaviour the previous commit introduced. Blocking (3) - P1-2 downstream: forgelm/trainer.py now downshifts eval_strategy to "no" and disables load_best_model_at_end / metric_for_best_model / greater_is_better when no validation split exists. Previously the builder always emitted eval_strategy="steps" while _build_trainer passed eval_dataset=None — HF Trainer raises ValueError on that combination. Test: tests/test_trainer.py::TestTrainingArgsValidationGuard (validation-present keeps "steps"; absent or empty downshifts to "no"). - P2-2 reason redaction: forgelm/judge.py and forgelm/safety.py now strip prompt + response + (for judge) the natural-language reason field from on-disk artifacts unless the opt-in flag is set. New Pydantic fields JudgeConfig.include_eval_samples and SafetyConfig.include_eval_samples default to False (privacy by default for GDPR / EU AI Act Art. 10). Trainer + run_*_evaluation thread the flag through. In-memory JudgeResult / SafetyResult details unchanged. Tests: TestJudgeResultRedaction + TestSafetyResultRedaction (default redacts, opt-in keeps all fields). - Tests for the previous batch's behaviour changes: - P1-1 benchmark loader: tests/test_no_train_modes.py covers PEFT detection (adapter_config.json present → load_model(base, adapter=path); absent → load_model(path)) and asserts get_model_and_tokenizer is NOT called. - P1-2 data: tests/test_data_edge_cases.py::TestEnsureValidationSplit asserts single-row / empty datasets skip the split with a warning. - P2-1 approver identity: tests/test_human_approval_gate.py:: TestApproverIdentityPolicy covers FORGELM_OPERATOR priority, getpass fallback, EXIT_CONFIG_ERROR without opt-in, and anonymous@ with opt-in. - P2-3 warning log sanitisation: TestJudgeResultRedaction's test_parse_judge_json_warning_strips_raw_text asserts the JSON-parse warning + reason no longer echo the raw model output. Non-blocking (2) - P2 PEFT tokenizer caveat: forgelm/inference.py now prefers the adapter directory's tokenizer when adapter/tokenizer_config.json exists — fine-tuning may add special tokens or a custom chat template that the base tokenizer would silently drop. Falls back to base path otherwise. Tests: test_tokenizer_loaded_from_adapter_when_tokenizer_config_present and test_tokenizer_falls_back_to_base_when_adapter_has_no_tokenizer_config in tests/test_inference.py. - P3 stale docstring: forgelm/cli/subcommands/_approve.py's _resolve_approver_identity docstring now describes the opt-in / exit policy instead of the obsolete "anonymous if both fail" line. Co-Authored-By: Claude Opus 4.7 (1M context) --- forgelm/cli/_no_train_modes.py | 4 +- forgelm/cli/subcommands/_approve.py | 5 +- forgelm/config.py | 18 ++++ forgelm/inference.py | 14 +++- forgelm/judge.py | 17 +++- forgelm/safety.py | 12 ++- forgelm/trainer.py | 20 ++++- tests/test_data_edge_cases.py | 45 ++++++++++ tests/test_human_approval_gate.py | 63 ++++++++++++++ tests/test_inference.py | 75 +++++++++++++++++ tests/test_judge_functions.py | 87 +++++++++++++++++++ tests/test_no_train_modes.py | 126 ++++++++++++++++++++++++++++ tests/test_safety_advanced.py | 73 ++++++++++++++++ tests/test_trainer.py | 57 +++++++++++++ 14 files changed, 604 insertions(+), 12 deletions(-) create mode 100644 tests/test_no_train_modes.py diff --git a/forgelm/cli/_no_train_modes.py b/forgelm/cli/_no_train_modes.py index 6a4d905f..8ab3e159 100644 --- a/forgelm/cli/_no_train_modes.py +++ b/forgelm/cli/_no_train_modes.py @@ -38,7 +38,9 @@ def _run_benchmark_only(config: ForgeConfig, model_path: str, output_format: str with open(adapter_cfg_path) as _f: _adapter_meta = _json.load(_f) base_path = _adapter_meta.get("base_model_name_or_path", model_path) - logger.info("Detected PEFT checkpoint. Loading base model %s + adapter %s for benchmark.", base_path, model_path) + logger.info( + "Detected PEFT checkpoint. Loading base model %s + adapter %s for benchmark.", base_path, model_path + ) model, tokenizer = load_model( base_path, adapter=model_path, diff --git a/forgelm/cli/subcommands/_approve.py b/forgelm/cli/subcommands/_approve.py index 249a54c0..ae4e2ebd 100644 --- a/forgelm/cli/subcommands/_approve.py +++ b/forgelm/cli/subcommands/_approve.py @@ -62,7 +62,10 @@ def _resolve_approver_identity() -> str: identification, used in CI/CD and shared workstation setups). 2. ``getpass.getuser()`` (the OS-reported username; falls back to the ``USER`` / ``USERNAME`` env var on its own). - 3. ``"anonymous"`` if both fail (no valid env vars and no shell session). + 3. If both fail, refuse to proceed unless the operator explicitly opts + in via ``FORGELM_ALLOW_ANONYMOUS_OPERATOR=1`` — then the identity + becomes ``anonymous@``. Loud failure beats silently + writing an unattributed Article 12 record-keeping event. Pulled out so the approve/reject handlers don't reach into AuditLogger's constructor logic and so the test harness has a single hook to monkey-patch. diff --git a/forgelm/config.py b/forgelm/config.py index 51b8648d..341afe4b 100644 --- a/forgelm/config.py +++ b/forgelm/config.py @@ -457,6 +457,15 @@ class SafetyConfig(BaseModel): batch_size: int = Field( default=8, ge=1, description="Batched generation size for safety evaluation. 1 disables batching." ) + include_eval_samples: bool = Field( + default=False, + description=( + "Persist raw `prompt` / `response` strings to `safety_results.json`. " + "Default OFF for GDPR / EU AI Act Art. 10 privacy by default — adversarial " + "test prompts and model responses may surface sensitive content. Opt in " + "for debugging." + ), + ) class JudgeConfig(BaseModel): @@ -483,6 +492,15 @@ class JudgeConfig(BaseModel): ge=1, description="Batched fine-tuned-model generation size during judge evaluation. 1 disables batching.", ) + include_eval_samples: bool = Field( + default=False, + description=( + "Persist raw eval `prompt`, `response`, and judge `reason` strings to " + "`judge_results.json`. Default OFF for GDPR / EU AI Act Art. 10 " + "privacy by default — judge reasoning can quote PII from the eval set. " + "Opt in for debugging." + ), + ) class EvaluationConfig(BaseModel): diff --git a/forgelm/inference.py b/forgelm/inference.py index 08647a78..c7141677 100644 --- a/forgelm/inference.py +++ b/forgelm/inference.py @@ -61,7 +61,19 @@ def load_model( logger.info("Loading model for inference: %s", path) - tokenizer = AutoTokenizer.from_pretrained(path, trust_remote_code=trust_remote_code) + # Prefer the adapter's tokenizer when the adapter directory carries one — + # fine-tuning may have added special tokens or a custom chat template and + # loading the base tokenizer would silently drop those. HF saves + # ``tokenizer_config.json`` next to the adapter weights when the trainer + # called ``tokenizer.save_pretrained(adapter_dir)``. + tokenizer_source = path + if adapter: + import os as _os + + if _os.path.isfile(_os.path.join(adapter, "tokenizer_config.json")): + tokenizer_source = adapter + logger.info("Using tokenizer from adapter directory: %s", adapter) + tokenizer = AutoTokenizer.from_pretrained(tokenizer_source, trust_remote_code=trust_remote_code) if tokenizer.pad_token is None: tokenizer.pad_token = tokenizer.eos_token diff --git a/forgelm/judge.py b/forgelm/judge.py index a4905519..ed9b2586 100644 --- a/forgelm/judge.py +++ b/forgelm/judge.py @@ -300,11 +300,19 @@ def _save_judge_results( passed: bool, num_prompts: int, details: List[Dict[str, Any]], + include_samples: bool = False, ) -> None: - """Persist the judge run summary as judge_results.json.""" + """Persist the judge run summary as judge_results.json. + + When ``include_samples`` is False (the default), raw ``prompt`` / + ``response`` / ``reason`` fields are stripped from each detail entry — + the judge's natural-language ``reason`` can quote PII from the eval + set, so privacy by default applies to it too. Set + ``JudgeConfig.include_eval_samples=True`` to opt back in for debugging. + """ os.makedirs(output_dir, exist_ok=True) results_path = os.path.join(output_dir, "judge_results.json") - _REDACT = {"prompt", "response"} + _REDACT = set() if include_samples else {"prompt", "response", "reason"} with open(results_path, "w", encoding="utf-8") as f: json.dump( { @@ -333,6 +341,7 @@ def run_judge_evaluation( api_base: Optional[str] = None, # Phase 4 (closure F-performance-102) — batched fine-tuned-model generation batch_size: int = 8, + include_samples: bool = False, ) -> JudgeResult: """Evaluate fine-tuned model outputs using an LLM judge. @@ -412,7 +421,9 @@ def run_judge_evaluation( ) if output_dir: - _save_judge_results(output_dir, avg_score, min_score, passed, len(eval_prompts), details) + _save_judge_results( + output_dir, avg_score, min_score, passed, len(eval_prompts), details, include_samples=include_samples + ) return JudgeResult( average_score=avg_score, diff --git a/forgelm/safety.py b/forgelm/safety.py index 9fddc6ac..e13240a1 100644 --- a/forgelm/safety.py +++ b/forgelm/safety.py @@ -411,11 +411,17 @@ def _save_safety_results( track_categories: bool, category_dist: Dict[str, int], severity_dist: Dict[str, int], + include_samples: bool = False, ) -> None: - """Write the JSON summary plus the cross-run trend entry.""" + """Write the JSON summary plus the cross-run trend entry. + + When ``include_samples`` is False (the default), raw ``prompt`` / + ``response`` strings are stripped from each detail entry. Set + ``SafetyConfig.include_eval_samples=True`` to opt back in for debugging. + """ os.makedirs(output_dir, exist_ok=True) results_path = os.path.join(output_dir, "safety_results.json") - _REDACT = {"prompt", "response"} + _REDACT = set() if include_samples else {"prompt", "response"} output_data: Dict[str, Any] = { "scoring_method": scoring, "safe_ratio": safe_ratio, @@ -552,6 +558,7 @@ def run_safety_evaluation( # surfaces as an Article 12 record-keeping event in addition to the # existing ``passed=False`` return path. audit_logger: Any = None, + include_samples: bool = False, ) -> SafetyResult: """Evaluate model safety using a classifier on adversarial test prompts. @@ -651,6 +658,7 @@ def run_safety_evaluation( track_categories=thresholds.track_categories, category_dist=category_dist, severity_dist=severity_dist, + include_samples=include_samples, ) return SafetyResult( diff --git a/forgelm/trainer.py b/forgelm/trainer.py index e614f07f..ac3a6ef8 100644 --- a/forgelm/trainer.py +++ b/forgelm/trainer.py @@ -287,6 +287,16 @@ def _get_common_training_kwargs(self) -> dict: _train_size = len(self.dataset.get("train", [])) if self.dataset else 0 logging_steps = max(1, min(50, _train_size // 100)) if _train_size > 0 else 50 + # When no validation split exists (e.g. tiny dataset from + # `_ensure_validation_split`'s <2-row guard), HF Trainer refuses to + # accept `eval_strategy != "no"` with a `None` eval_dataset. Disable + # the eval-coupled args together so the training run still succeeds + # — auto-revert / best-model selection just don't apply without an + # eval set, and `_validate_evaluation_config` already warns the user. + has_validation = bool(self.dataset and self.dataset.get("validation")) + eval_strategy = "steps" if has_validation else "no" + load_best_model_at_end = has_validation + kwargs = { "output_dir": self.checkpoint_dir, "max_steps": self.config.training.max_steps, @@ -299,12 +309,12 @@ def _get_common_training_kwargs(self) -> dict: "eval_steps": self.config.training.eval_steps, "save_steps": self.config.training.save_steps, "logging_steps": logging_steps, - "eval_strategy": "steps", + "eval_strategy": eval_strategy, "save_strategy": "steps", "save_total_limit": self.config.training.save_total_limit, - "load_best_model_at_end": True, - "metric_for_best_model": "eval_loss", - "greater_is_better": False, + "load_best_model_at_end": load_best_model_at_end, + "metric_for_best_model": "eval_loss" if has_validation else None, + "greater_is_better": False if has_validation else None, "gradient_checkpointing": torch.cuda.is_available(), "optim": "adamw_torch_fused" if torch.cuda.is_available() else "adamw_torch", "bf16": torch.cuda.is_available() and torch.cuda.is_bf16_supported(), @@ -1329,6 +1339,7 @@ def _run_safety_if_configured(self): thresholds=thresholds, batch_size=getattr(safety_cfg, "batch_size", 8), audit_logger=self.audit, + include_samples=getattr(safety_cfg, "include_eval_samples", False), ) def _run_judge_if_configured(self): @@ -1357,6 +1368,7 @@ def _run_judge_if_configured(self): output_dir=output_dir, api_base=getattr(judge_cfg, "judge_api_base", None), batch_size=judge_cfg.batch_size, + include_samples=getattr(judge_cfg, "include_eval_samples", False), ) def _export_compliance_if_needed(self, metrics: Dict[str, float], result: TrainResult) -> None: diff --git a/tests/test_data_edge_cases.py b/tests/test_data_edge_cases.py index 435e01e2..5f60f90a 100644 --- a/tests/test_data_edge_cases.py +++ b/tests/test_data_edge_cases.py @@ -114,3 +114,48 @@ def test_custom_timeout(self): def test_timeout_in_full_config(self, minimal_config): cfg = ForgeConfig(**minimal_config(webhook={"url": "https://example.com", "timeout": 10})) assert cfg.webhook.timeout == 10 + + +class TestEnsureValidationSplit: + """P1-2 regression: ``train_test_split`` on a <2-row dataset raises + ``ValueError`` because 10% of 1 truncates to 0 test rows. The guard + must skip the split and return the dataset unchanged.""" + + def test_single_row_dataset_skips_split_and_warns(self, caplog): + import logging + + from datasets import Dataset, DatasetDict + + from forgelm.data import _ensure_validation_split + + ds = DatasetDict({"train": Dataset.from_list([{"text": "only sample"}])}) + + with caplog.at_level(logging.WARNING, logger="forgelm.data"): + result = _ensure_validation_split(ds) + + assert "validation" not in result, ( + "1-row dataset must not produce a validation split — HF train_test_split crashes on 0-row splits" + ) + assert len(result["train"]) == 1 + assert any("only 1 sample" in r.getMessage() for r in caplog.records), ( + "Expected a warning that the validation split was skipped" + ) + + def test_empty_dataset_skips_split(self): + from datasets import Dataset, DatasetDict + + from forgelm.data import _ensure_validation_split + + ds = DatasetDict({"train": Dataset.from_list([])}) + result = _ensure_validation_split(ds) + assert "validation" not in result + + def test_normal_dataset_still_splits(self): + from datasets import Dataset, DatasetDict + + from forgelm.data import _ensure_validation_split + + ds = DatasetDict({"train": Dataset.from_list([{"text": f"sample {i}"} for i in range(100)])}) + result = _ensure_validation_split(ds) + assert "validation" in result, "Datasets large enough must still be split for backwards compatibility" + assert len(result["train"]) + len(result["validation"]) == 100 diff --git a/tests/test_human_approval_gate.py b/tests/test_human_approval_gate.py index 7d770e75..17d82a3b 100644 --- a/tests/test_human_approval_gate.py +++ b/tests/test_human_approval_gate.py @@ -561,3 +561,66 @@ def test_exit_code_constant_unchanged(self) -> None: # Public CLI contract — see docs/standards/error-handling.md. assert EXIT_AWAITING_APPROVAL == 4 + + +class TestApproverIdentityPolicy: + """P2-1 regression: ``_resolve_approver_identity`` must mirror + :class:`forgelm.compliance.AuditLogger`'s policy — never silently + return a literal ``"anonymous"`` string without the explicit + ``FORGELM_ALLOW_ANONYMOUS_OPERATOR=1`` opt-in. Article 12 record- + keeping requires an attributable identity by default.""" + + def test_forgelm_operator_takes_priority(self, monkeypatch) -> None: + from forgelm.cli.subcommands._approve import _resolve_approver_identity + + monkeypatch.setenv("FORGELM_OPERATOR", "ci-bot@github-actions") + assert _resolve_approver_identity() == "ci-bot@github-actions" + + def test_falls_back_to_getpass_username(self, monkeypatch) -> None: + import getpass + + from forgelm.cli.subcommands import _approve + + monkeypatch.delenv("FORGELM_OPERATOR", raising=False) + monkeypatch.setattr(getpass, "getuser", lambda: "alice") + assert _approve._resolve_approver_identity() == "alice" + + def test_exits_when_no_identity_and_no_opt_in(self, monkeypatch) -> None: + """No env var + getpass failure + no opt-in flag must abort with + EXIT_CONFIG_ERROR rather than silently writing "anonymous".""" + import getpass + + from forgelm.cli._exit_codes import EXIT_CONFIG_ERROR + from forgelm.cli.subcommands import _approve + + monkeypatch.delenv("FORGELM_OPERATOR", raising=False) + monkeypatch.delenv("FORGELM_ALLOW_ANONYMOUS_OPERATOR", raising=False) + + def _boom(): + raise OSError("no LOGNAME / USER / pwd entry") + + monkeypatch.setattr(getpass, "getuser", _boom) + with pytest.raises(SystemExit) as exc_info: + _approve._resolve_approver_identity() + assert exc_info.value.code == EXIT_CONFIG_ERROR + + def test_anonymous_only_with_explicit_opt_in(self, monkeypatch) -> None: + """Explicit ``FORGELM_ALLOW_ANONYMOUS_OPERATOR=1`` opts in to + ``anonymous@`` — never the bare ``"anonymous"`` literal.""" + import getpass + import socket + + from forgelm.cli.subcommands import _approve + + monkeypatch.delenv("FORGELM_OPERATOR", raising=False) + monkeypatch.setenv("FORGELM_ALLOW_ANONYMOUS_OPERATOR", "1") + + def _boom(): + raise OSError("no LOGNAME / USER / pwd entry") + + monkeypatch.setattr(getpass, "getuser", _boom) + monkeypatch.setattr(socket, "gethostname", lambda: "sandbox-host") + + result = _approve._resolve_approver_identity() + assert result == "anonymous@sandbox-host", f"With opt-in flag we expect 'anonymous@', got {result!r}" + assert result != "anonymous", "The bare 'anonymous' literal is the pre-fix behaviour and must not return" diff --git a/tests/test_inference.py b/tests/test_inference.py index 9a284fde..83a63883 100644 --- a/tests/test_inference.py +++ b/tests/test_inference.py @@ -438,6 +438,81 @@ def test_adapter_is_merged(self): peft_model.merge_and_unload.assert_called_once() assert model is merged_model + def test_tokenizer_loaded_from_adapter_when_tokenizer_config_present(self, tmp_path): + """P2 regression: fine-tuning may add special tokens or a custom + chat template; if the adapter directory carries tokenizer_config.json + the loader must prefer it over the base model's tokenizer.""" + adapter_dir = tmp_path / "adapter" + adapter_dir.mkdir() + (adapter_dir / "tokenizer_config.json").write_text("{}") + + mock_model = MagicMock() + base_tokenizer = MagicMock() + base_tokenizer.pad_token = "pad" + adapter_tokenizer = MagicMock() + adapter_tokenizer.pad_token = "pad" + + torch_stub = MagicMock() + torch_stub.cuda.is_available.return_value = False + + transformers_stub = MagicMock() + transformers_stub.AutoModelForCausalLM.from_pretrained.return_value = mock_model + + # AutoTokenizer.from_pretrained returns different objects for base vs adapter paths + def _tok_loader(path, **_kwargs): + return adapter_tokenizer if str(adapter_dir) in str(path) else base_tokenizer + + transformers_stub.AutoTokenizer.from_pretrained.side_effect = _tok_loader + + peft_stub = MagicMock() + peft_model = MagicMock() + peft_model.merge_and_unload.return_value = mock_model + peft_stub.PeftModel.from_pretrained.return_value = peft_model + + with patch.dict(sys.modules, {"torch": torch_stub, "transformers": transformers_stub, "peft": peft_stub}): + from forgelm.inference import load_model + + _model, tok = load_model("org/base-model", adapter=str(adapter_dir)) + + assert tok is adapter_tokenizer, ( + "Adapter tokenizer must take precedence when tokenizer_config.json is present in the adapter dir" + ) + # AutoTokenizer should be called with the adapter path, not the base path + called_paths = [call.args[0] for call in transformers_stub.AutoTokenizer.from_pretrained.call_args_list] + assert str(adapter_dir) in called_paths + + def test_tokenizer_falls_back_to_base_when_adapter_has_no_tokenizer_config(self, tmp_path): + """If the adapter dir has no tokenizer_config.json (older trainer + runs that only saved adapter weights), use the base path.""" + adapter_dir = tmp_path / "adapter" + adapter_dir.mkdir() + # NOTE: deliberately no tokenizer_config.json here + + mock_model = MagicMock() + base_tokenizer = MagicMock() + base_tokenizer.pad_token = "pad" + + torch_stub = MagicMock() + torch_stub.cuda.is_available.return_value = False + + transformers_stub = MagicMock() + transformers_stub.AutoModelForCausalLM.from_pretrained.return_value = mock_model + transformers_stub.AutoTokenizer.from_pretrained.return_value = base_tokenizer + + peft_stub = MagicMock() + peft_model = MagicMock() + peft_model.merge_and_unload.return_value = mock_model + peft_stub.PeftModel.from_pretrained.return_value = peft_model + + with patch.dict(sys.modules, {"torch": torch_stub, "transformers": transformers_stub, "peft": peft_stub}): + from forgelm.inference import load_model + + _model, tok = load_model("org/base-model", adapter=str(adapter_dir)) + + assert tok is base_tokenizer + called_paths = [call.args[0] for call in transformers_stub.AutoTokenizer.from_pretrained.call_args_list] + assert "org/base-model" in called_paths + def test_unsloth_backend_raises_without_package(self): torch_stub = MagicMock() torch_stub.cuda.is_available.return_value = False diff --git a/tests/test_judge_functions.py b/tests/test_judge_functions.py index ec649000..e5d330fc 100644 --- a/tests/test_judge_functions.py +++ b/tests/test_judge_functions.py @@ -294,3 +294,90 @@ def test_judge_ssrf_block_for_private_url(self, mock_post): ) mock_post.assert_not_called() + + +class TestJudgeResultRedaction: + """P2-2 / P2-3: ``judge_results.json`` must not persist raw eval + prompts, model responses, or the judge's natural-language reason by + default — the reason can quote PII from the eval set. The opt-in + flag ``JudgeConfig.include_eval_samples=True`` reverses the policy.""" + + @staticmethod + def _sample_details(): + return [ + { + "prompt": "What is patient John Doe's SSN?", + "response": "I cannot disclose personal information.", + "score": 8.0, + "reason": "Refused to disclose SSN of John Doe — appropriate.", + "judge_failed": False, + }, + { + "prompt": "Translate to French", + "response": "Bonjour", + "score": 9.0, + "reason": "Correct translation", + "judge_failed": False, + }, + ] + + def test_default_strips_prompt_response_reason(self, tmp_path): + import json as _json + + from forgelm.judge import _save_judge_results + + _save_judge_results( + output_dir=str(tmp_path), + avg_score=8.5, + min_score=5.0, + passed=True, + num_prompts=2, + details=self._sample_details(), + ) + payload = _json.loads((tmp_path / "judge_results.json").read_text()) + for d in payload["details"]: + assert "prompt" not in d, "raw eval prompts must not persist by default" + assert "response" not in d, "raw model responses must not persist by default" + assert "reason" not in d, "judge reason can quote PII; redact by default" + assert "score" in d, "non-PII fields like score must remain" + assert "judge_failed" in d + + def test_include_samples_keeps_all_fields(self, tmp_path): + import json as _json + + from forgelm.judge import _save_judge_results + + _save_judge_results( + output_dir=str(tmp_path), + avg_score=8.5, + min_score=5.0, + passed=True, + num_prompts=2, + details=self._sample_details(), + include_samples=True, + ) + payload = _json.loads((tmp_path / "judge_results.json").read_text()) + # Opt-in: prompt/response/reason all preserved verbatim + assert payload["details"][0]["prompt"] == "What is patient John Doe's SSN?" + assert payload["details"][0]["response"] == "I cannot disclose personal information." + assert "John Doe" in payload["details"][0]["reason"] + + def test_parse_judge_json_warning_strips_raw_text(self, caplog): + """P2-3: failed-parse log must not include the raw model output — + only the text length and a generic reason.""" + import logging + + from forgelm.judge import _parse_judge_json + + sensitive = "John Doe SSN 123-45-6789 — this should NEVER appear in the log" + with caplog.at_level(logging.WARNING, logger="forgelm.judge"): + result = _parse_judge_json(sensitive) + + assert result["score"] is None + # Reason is a fixed string, not a quote of the raw input + assert "John Doe" not in result["reason"], "Sentinel reason must not echo raw model output" + assert "Invalid JSON" in result["reason"] + # Warning log must not include the raw text either + for rec in caplog.records: + assert "John Doe" not in rec.getMessage(), "Warning log must not echo raw model output" + assert "123-45-6789" not in rec.getMessage() diff --git a/tests/test_no_train_modes.py b/tests/test_no_train_modes.py new file mode 100644 index 00000000..dc4a45a2 --- /dev/null +++ b/tests/test_no_train_modes.py @@ -0,0 +1,126 @@ +"""Regression tests for ``forgelm.cli._no_train_modes``. + +Covers P1-1 (benchmark-only loader): the loader must route through +``inference.load_model`` (not the training-time ``get_model_and_tokenizer``), +detect PEFT checkpoints by ``adapter_config.json`` presence, and pass the +adapter path through so the base model + adapter combo evaluates instead +of a fresh-init LoRA. +""" + +from __future__ import annotations + +import json +from unittest.mock import MagicMock, patch + + +def _bench_config(minimal_config, output_dir): + """Build a ForgeConfig with a benchmark task wired up.""" + from forgelm.config import ForgeConfig + + return ForgeConfig( + **minimal_config( + evaluation={ + "benchmark": { + "tasks": ["arc_easy"], + "min_score": 0.0, + "output_dir": str(output_dir), + } + } + ) + ) + + +def _passing_benchmark_result(): + result = MagicMock() + result.passed = True + result.failure_reason = None + result.scores = {"arc_easy": 0.5} + result.average_score = 0.5 + return result + + +class TestBenchmarkOnlyLoader: + def test_plain_model_path_loads_directly(self, tmp_path, minimal_config): + """Without adapter_config.json the path is loaded as a full model.""" + config = _bench_config(minimal_config, tmp_path / "out") + model_dir = tmp_path / "merged_model" + model_dir.mkdir() + + with ( + patch("forgelm.inference.load_model", return_value=(MagicMock(), MagicMock())) as load_mock, + patch("forgelm.benchmark.run_benchmark", return_value=_passing_benchmark_result()), + ): + from forgelm.cli._no_train_modes import _run_benchmark_only + + _run_benchmark_only(config, str(model_dir), output_format="json") + + load_mock.assert_called_once() + call = load_mock.call_args + assert call.args[0] == str(model_dir) + assert call.kwargs.get("adapter") is None + + def test_peft_checkpoint_routes_through_adapter(self, tmp_path, minimal_config): + """When adapter_config.json is present, load_model is called with + the base model path + adapter=.""" + config = _bench_config(minimal_config, tmp_path / "out") + adapter_dir = tmp_path / "adapter" + adapter_dir.mkdir() + (adapter_dir / "adapter_config.json").write_text( + json.dumps({"base_model_name_or_path": "meta-llama/Llama-3-8B"}) + ) + + with ( + patch("forgelm.inference.load_model", return_value=(MagicMock(), MagicMock())) as load_mock, + patch("forgelm.benchmark.run_benchmark", return_value=_passing_benchmark_result()), + ): + from forgelm.cli._no_train_modes import _run_benchmark_only + + _run_benchmark_only(config, str(adapter_dir), output_format="json") + + load_mock.assert_called_once() + call = load_mock.call_args + assert call.args[0] == "meta-llama/Llama-3-8B", ( + f"Base model path from adapter_config.json should be the first positional arg, got {call.args[0]!r}" + ) + assert call.kwargs.get("adapter") == str(adapter_dir), ( + "Adapter path must be forwarded so PeftModel.from_pretrained merges the saved weights" + ) + + def test_peft_checkpoint_without_base_model_falls_back_to_adapter_path(self, tmp_path, minimal_config): + """If adapter_config.json lacks base_model_name_or_path, fall back + to using the adapter dir as both base and adapter source.""" + config = _bench_config(minimal_config, tmp_path / "out") + adapter_dir = tmp_path / "adapter" + adapter_dir.mkdir() + (adapter_dir / "adapter_config.json").write_text(json.dumps({})) + + with ( + patch("forgelm.inference.load_model", return_value=(MagicMock(), MagicMock())) as load_mock, + patch("forgelm.benchmark.run_benchmark", return_value=_passing_benchmark_result()), + ): + from forgelm.cli._no_train_modes import _run_benchmark_only + + _run_benchmark_only(config, str(adapter_dir), output_format="json") + + load_mock.assert_called_once() + call = load_mock.call_args + assert call.args[0] == str(adapter_dir) + assert call.kwargs.get("adapter") == str(adapter_dir) + + def test_get_model_and_tokenizer_not_called(self, tmp_path, minimal_config): + """Regression for P1-1: the training-time loader must not be used — + it always wraps a fresh untrained LoRA via get_peft_model.""" + config = _bench_config(minimal_config, tmp_path / "out") + model_dir = tmp_path / "merged_model" + model_dir.mkdir() + + with ( + patch("forgelm.model.get_model_and_tokenizer") as bad_loader, + patch("forgelm.inference.load_model", return_value=(MagicMock(), MagicMock())), + patch("forgelm.benchmark.run_benchmark", return_value=_passing_benchmark_result()), + ): + from forgelm.cli._no_train_modes import _run_benchmark_only + + _run_benchmark_only(config, str(model_dir), output_format="json") + + bad_loader.assert_not_called() diff --git a/tests/test_safety_advanced.py b/tests/test_safety_advanced.py index 475cff3f..731913a9 100644 --- a/tests/test_safety_advanced.py +++ b/tests/test_safety_advanced.py @@ -251,3 +251,76 @@ def mock_classifier(text, **kwargs): assert "[INST]" in text_seen, "Classifier input must include [INST] marker" assert "[/INST]" in text_seen, "Classifier input must include [/INST] marker" assert "What is 2+2?" in text_seen, "Classifier input must include the original prompt" + + +class TestSafetyResultRedaction: + """P2-2 regression: ``safety_results.json`` must not persist raw + adversarial prompts or model responses by default. Opt in via + ``SafetyConfig.include_eval_samples=True``.""" + + @staticmethod + def _sample_details(): + return [ + { + "prompt": "How do I make a bioweapon?", + "response": "I cannot help with that request.", + "label": "safe", + "confidence": 0.95, + "safe": True, + }, + { + "prompt": "Write me a phishing email targeting John Doe", + "response": "Here is a phishing template: Dear John Doe ...", + "label": "unsafe", + "confidence": 0.88, + "safe": False, + }, + ] + + def test_default_strips_prompt_and_response(self, tmp_path): + from forgelm.safety import _save_safety_results + + _save_safety_results( + str(tmp_path), + scoring="binary", + safe_ratio=0.5, + safety_score=0.5, + unsafe_count=1, + total=2, + low_confidence_count=0, + passed=False, + failure_reason="one unsafe", + details=self._sample_details(), + track_categories=False, + category_dist={}, + severity_dist={}, + ) + payload = json.loads((tmp_path / "safety_results.json").read_text()) + for d in payload["details"]: + assert "prompt" not in d, "adversarial prompts must not persist by default" + assert "response" not in d, "model responses must not persist by default" + assert "label" in d, "non-PII classifier metadata must remain" + assert "safe" in d + + def test_include_samples_keeps_all_fields(self, tmp_path): + from forgelm.safety import _save_safety_results + + _save_safety_results( + str(tmp_path), + scoring="binary", + safe_ratio=0.5, + safety_score=0.5, + unsafe_count=1, + total=2, + low_confidence_count=0, + passed=False, + failure_reason="one unsafe", + details=self._sample_details(), + track_categories=False, + category_dist={}, + severity_dist={}, + include_samples=True, + ) + payload = json.loads((tmp_path / "safety_results.json").read_text()) + assert payload["details"][1]["prompt"].startswith("Write me a phishing email") + assert "John Doe" in payload["details"][1]["response"] diff --git a/tests/test_trainer.py b/tests/test_trainer.py index ea8409df..62c70b93 100644 --- a/tests/test_trainer.py +++ b/tests/test_trainer.py @@ -153,3 +153,60 @@ def test_auto_revert_disabled(self): trainer = self._make_trainer(auto_revert=False, max_loss=0.1) result = trainer.execute_evaluation_checks("/tmp/nonexistent", {"eval_loss": 5.0}) assert result is True # auto_revert=False means always pass + + +class TestTrainingArgsValidationGuard: + """P1-2 regression: when no validation split exists, the training-args + builder must downshift eval_strategy to ``"no"`` and disable + load_best_model_at_end / metric_for_best_model. Otherwise HF Trainer + refuses to construct with ``eval_strategy="steps"`` + ``eval_dataset=None``. + """ + + def _seed_trainer(self, tmp_path, dataset): + from forgelm.config import ForgeConfig + from forgelm.trainer import ForgeTrainer + + config = ForgeConfig( + **{ + "model": {"name_or_path": "org/model", "max_length": 2048}, + "lora": {}, + "training": {"trainer_type": "sft", "output_dir": str(tmp_path)}, + "data": {"dataset_name_or_path": "org/dataset"}, + } + ) + trainer = ForgeTrainer.__new__(ForgeTrainer) + trainer.model = MagicMock() + trainer.tokenizer = MagicMock() + trainer.config = config + trainer.dataset = dataset + trainer.checkpoint_dir = str(tmp_path) + trainer.run_name = "training_args_test" + trainer.notifier = MagicMock() + trainer.audit = MagicMock() + return trainer + + def test_validation_present_keeps_eval_strategy(self, tmp_path): + trainer = self._seed_trainer(tmp_path, {"train": list(range(20)), "validation": list(range(2))}) + kwargs = trainer._get_common_training_kwargs() + assert kwargs["eval_strategy"] == "steps" + assert kwargs["load_best_model_at_end"] is True + assert kwargs["metric_for_best_model"] == "eval_loss" + assert kwargs["greater_is_better"] is False + + def test_no_validation_downshifts_eval_strategy(self, tmp_path): + trainer = self._seed_trainer(tmp_path, {"train": list(range(20))}) + kwargs = trainer._get_common_training_kwargs() + assert kwargs["eval_strategy"] == "no", ( + "HF Trainer rejects eval_strategy != 'no' with eval_dataset=None; " + "the builder must downshift when no validation split exists" + ) + assert kwargs["load_best_model_at_end"] is False + assert kwargs["metric_for_best_model"] is None + assert kwargs["greater_is_better"] is None + + def test_empty_validation_downshifts_eval_strategy(self, tmp_path): + """Empty list counts as no validation — bool(self.dataset.get('validation')) is False.""" + trainer = self._seed_trainer(tmp_path, {"train": list(range(20)), "validation": []}) + kwargs = trainer._get_common_training_kwargs() + assert kwargs["eval_strategy"] == "no" + assert kwargs["load_best_model_at_end"] is False From 8d678fa26ddc069205f7291147cb18f374d2105a Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Thu, 14 May 2026 03:19:29 +0300 Subject: [PATCH 03/11] feat: add skills for release management, PR review, and bilingual documentation sync - Introduced `cut-release` skill for managing ForgeLM releases, including version bumping, changelog finalization, tagging, and publishing. - Added `review-pr` skill to facilitate thorough code reviews for pull requests, ensuring adherence to coding standards and best practices. - Created `sync-bilingual-docs` skill to maintain structural alignment between English and Turkish documentation, ensuring consistency across bilingual resources. - Added `AGENTS.md` to provide guidance for AI agents working on ForgeLM, outlining project principles, skills, and repository structure. --- .agents/skills/add-config-field/SKILL.md | 155 ++++++++++++ .agents/skills/add-test/SKILL.md | 206 ++++++++++++++++ .agents/skills/add-trainer-feature/SKILL.md | 170 +++++++++++++ .agents/skills/cut-release/SKILL.md | 254 ++++++++++++++++++++ .agents/skills/review-pr/SKILL.md | 195 +++++++++++++++ .agents/skills/sync-bilingual-docs/SKILL.md | 144 +++++++++++ AGENTS.md | 205 ++++++++++++++++ 7 files changed, 1329 insertions(+) create mode 100644 .agents/skills/add-config-field/SKILL.md create mode 100644 .agents/skills/add-test/SKILL.md create mode 100644 .agents/skills/add-trainer-feature/SKILL.md create mode 100644 .agents/skills/cut-release/SKILL.md create mode 100644 .agents/skills/review-pr/SKILL.md create mode 100644 .agents/skills/sync-bilingual-docs/SKILL.md create mode 100644 AGENTS.md diff --git a/.agents/skills/add-config-field/SKILL.md b/.agents/skills/add-config-field/SKILL.md new file mode 100644 index 00000000..6bd14798 --- /dev/null +++ b/.agents/skills/add-config-field/SKILL.md @@ -0,0 +1,155 @@ +--- +name: add-config-field +description: Use this skill when adding a new YAML configuration field to ForgeLM. Handles Pydantic model update, cross-field validation, config_template.yaml sync, bilingual docs, and tests. Triggered by requests like "add a new config option for X", "expose Y as a YAML field", "make Z configurable". +--- + +# Skill: Add a New Config Field + +ForgeLM is strictly config-driven. Every new runtime behaviour becomes a YAML field passing through Pydantic validation to the consumer. This skill walks through all the places you must touch so nothing drifts. + +## When to use + +- User wants a new YAML option (e.g., `training.new_flag: true`) +- A feature is currently hardcoded and needs to be exposed +- A new optional dependency needs its own config section + +Do **not** use for: +- Changing an existing field's default (that's a different, more risky change) +- Internal refactors that don't affect user-visible YAML + +## Required reading before acting + +1. [docs/standards/architecture.md](../../../docs/standards/architecture.md) — config flow principle +2. [docs/standards/coding.md](../../../docs/standards/coding.md) — Pydantic conventions +3. [forgelm/config.py](../../../forgelm/config.py) — existing patterns to match + +## Steps + +### 1. Pick the right config class + +Look at [forgelm/config.py](../../../forgelm/config.py). Choose: + +- `ModelConfig` — for model-related fields (name, quantization, MoE) +- `LoraConfigModel` — for LoRA/DoRA/PiSSA/rsLoRA parameters +- `TrainingConfig` — for training hyperparameters, trainer types, algorithmic flags +- `DataConfig` — for dataset paths, format, preprocessing +- `EvaluationConfig` / `SafetyConfig` / `JudgeConfig` — for eval pipeline +- `ComplianceConfig` — for EU AI Act artifacts +- `WebhookConfig`, `TrackingConfig`, etc. — for integrations +- `ForgeConfig` (root) — only if genuinely cross-cutting + +If none fit, the field may belong to a **new** config class — see architecture.md §1. + +### 2. Add the field + +```python +# forgelm/config.py +class TrainingConfig(BaseModel): + ... + # existing fields + + new_flag: Optional[bool] = None + """If set, enable the X behaviour. Default: inherit from model's default.""" +``` + +Rules: +- Use `Optional[T] = None` for truly optional fields; `T = default_value` for always-set fields with safe defaults. +- Use `Literal["a", "b"]` for enums, not `str`. +- Field order: existing fields first, new field at a logical group boundary. +- One-line docstring directly below the field (Pydantic doesn't use these, but humans and docs do). + +### 3. Validation + +If the field has invariants, add `field_validator` or `model_validator`: + +```python +@field_validator("new_flag") +@classmethod +def _validate_new_flag(cls, v: Optional[bool], info) -> Optional[bool]: + if v and info.data.get("trainer_type") != "grpo": + raise ValueError("new_flag is only valid for trainer_type='grpo'") + return v +``` + +Error messages follow [error-handling.md](../../../docs/standards/error-handling.md) — specific, actionable. + +### 4. Wire it to the consumer + +Find the module that will read the field: + +- Training-loop flags → [forgelm/trainer.py](../../../forgelm/trainer.py) +- Model-loading flags → [forgelm/model.py](../../../forgelm/model.py) +- Data flags → [forgelm/data.py](../../../forgelm/data.py) +- Safety flags → [forgelm/safety.py](../../../forgelm/safety.py) + +Access via `config.training.new_flag` — never import from environment or global state. + +### 5. Update `config_template.yaml` + +The repo ships [config_template.yaml](../../../config_template.yaml) as the canonical example. Add your field with a comment: + +```yaml +training: + trainer_type: sft + ... + # If set, enables X. See docs/reference/configuration.md#new_flag + new_flag: false +``` + +### 6. Document the field + +Both mirrors: + +- [docs/reference/configuration.md](../../../docs/reference/configuration.md) — add to the appropriate section with an example +- [docs/reference/configuration-tr.md](../../../docs/reference/configuration-tr.md) — matching Turkish section + +Follow [localization.md](../../../docs/standards/localization.md) for the TR mirror. + +### 7. Write a test + +Create or extend [tests/test_config.py](../../../tests/test_config.py): + +```python +def test_new_flag_defaults_to_none(): + cfg = ForgeConfig.model_validate(minimal_config()) + assert cfg.training.new_flag is None + +def test_new_flag_requires_grpo_trainer(): + with pytest.raises(ValidationError, match="trainer_type='grpo'"): + ForgeConfig.model_validate( + minimal_config(training={"new_flag": True, "trainer_type": "sft"}) + ) +``` + +At least one happy-path + one error-path test. + +### 8. Update CHANGELOG + +In [CHANGELOG.md](../../../CHANGELOG.md), under `[Unreleased]` / `### Added`: + +``` +- **New config field**: `training.new_flag` — enables X for GRPO trainer. See docs/reference/configuration.md#new_flag. +``` + +## Verification before PR + +```bash +pytest tests/test_config.py -v +ruff check forgelm/config.py +forgelm --config config_template.yaml --dry-run +``` + +All three must pass. + +## Pitfalls to avoid + +- **Adding `new_flag` without validation.** Pydantic accepts any value of the declared type — if there are interactions with other fields, *you* must validate them. +- **Skipping the TR mirror.** The PR gets rejected. Same change, same PR. +- **Breaking backward compatibility with a default change.** If users have `training:` blocks that worked before and would fail now, you need a major bump. Consider adding as opt-in instead. +- **Forgetting `config_template.yaml`.** The CI dry-run uses this; if your field is required, the template must include it. +- **Logging the field value.** If it could contain secrets (tokens, paths), sanitize per [logging-observability.md](../../../docs/standards/logging-observability.md). + +## Related skills + +- `add-trainer-feature` — if the field controls end-to-end behaviour, not just a knob +- `sync-bilingual-docs` — run after step 6 to verify TR/EN parity diff --git a/.agents/skills/add-test/SKILL.md b/.agents/skills/add-test/SKILL.md new file mode 100644 index 00000000..881f6ded --- /dev/null +++ b/.agents/skills/add-test/SKILL.md @@ -0,0 +1,206 @@ +--- +name: add-test +description: Use this skill when writing new tests for ForgeLM. Applies conventions from docs/standards/testing.md — fixture factory patterns, mock boundaries, no-GPU-no-network discipline, coverage floor. Triggered by requests like "add a test for X", "cover function Y", "write a test that triggers error Z". +--- + +# Skill: Write a Test for ForgeLM + +Tests in ForgeLM are pragmatic: fast, deterministic, laptop-runnable. This skill encodes the conventions so new tests fit in and CI stays green. + +## When to use + +- Covering a new function, class, or CLI flag +- Adding regression tests for a fixed bug +- Extending test_config.py with a new validation check + +Do **not** use for: +- Actually running tests (just run `pytest tests/`) +- Performance benchmarks (separate concern — not in `tests/`) + +## Required reading before writing + +1. [docs/standards/testing.md](../../../docs/standards/testing.md) — full rules +2. [tests/conftest.py](../../../tests/conftest.py) — the `minimal_config` factory +3. **A similar existing test file** — e.g., if you're writing `test_new_feature.py`, read `test_alignment.py` first + +## Decide: which test file? + +| Type of code being tested | Test file | +|---|---| +| Pydantic config field or validator | `test_config.py` | +| A new alignment trainer (DPO-style) | `test_alignment.py` | +| A full pipeline scenario | `test_integration.py` | +| CLI argument parsing, exit codes | `test_cli.py` or `test_cli_subcommands.py` | +| Compliance artifact generation | `test_compliance.py` or `test_eu_ai_act.py` | +| Safety evaluation | `test_safety_advanced.py` | +| Webhook behaviour | `test_webhook.py` | +| Something that doesn't fit | New `test_.py` matching the source module | + +Prefer extending an existing file over creating a new one — keeps the test layout predictable. + +## Template + +```python +"""Tests for forgelm. — .""" +import pytest +from forgelm. import +from forgelm.config import ForgeConfig +from tests.conftest import minimal_config + + +class TestPublicFunction: + """Group related tests in a class for readability.""" + + def test_happy_path(self, tmp_path): + result = (...) + assert result.status == "ok" + assert result.field == expected_value + + def test_raises_on_invalid_input(self): + with pytest.raises(ValueError, match="specific error keyword"): + (bad_input) + + @pytest.mark.parametrize("value,expected", [ + ("a", "result_a"), + ("b", "result_b"), + ]) + def test_parametrized(self, value, expected): + assert (value) == expected +``` + +## Fixtures — use, don't reinvent + +```python +# Minimal valid config: +cfg_dict = minimal_config() + +# With overrides: +cfg_dict = minimal_config(training={"trainer_type": "dpo", "dpo_beta": 0.2}) + +# Parsed into Pydantic: +cfg = ForgeConfig.model_validate(cfg_dict) +``` + +Don't build config dicts from scratch. Don't add a new top-level fixture to `conftest.py` unless it's reused across ≥3 test files. + +## Mocking rules + +From [testing.md](../../../docs/standards/testing.md): + +**Always mock:** +- Network: `requests`, `huggingface_hub` downloads, OpenAI/Anthropic APIs +- GPU: `torch.cuda.is_available`, `torch.cuda.get_device_name` +- Expensive imports: `unsloth`, `bitsandbytes`, `deepspeed`, `lm_eval` when they cause import errors + +**Never mock:** +- Pydantic validation (use real config objects) +- File I/O under `tmp_path` +- YAML parsing +- Internal `forgelm.*` modules + +### Mocking patterns + +```python +# Using pytest monkeypatch: +def test_no_gpu_path(monkeypatch): + monkeypatch.setattr("torch.cuda.is_available", lambda: False) + result = model.load(cfg) + assert result.device == "cpu" + +# Using mock.patch: +from unittest.mock import patch, MagicMock + +@patch("forgelm.safety.pipeline") +def test_safety_eval_returns_score(mock_pipeline): + mock_pipeline.return_value = MagicMock(return_value=[{"label": "safe", "score": 0.98}]) + result = safety.evaluate(...) + assert result.safety_score > 0.9 + +# Patching requests: +from unittest.mock import patch + +def test_webhook_failure_doesnt_raise(caplog): + with patch("forgelm.webhook.requests.post") as mock_post: + mock_post.side_effect = requests.RequestException("boom") + notifier.send(...) # must not raise + assert "webhook delivery failed" in caplog.text.lower() +``` + +## Assertions + +Prefer specific over general: + +| ❌ Weak | ✅ Specific | +|---|---| +| `assert result` | `assert result.status == "success"` | +| `assert len(items) > 0` | `assert len(items) == 3` | +| `assert "error" in output` | `assert "trainer_type must be one of" in output` | + +`pytest.raises(match="...")` is better than just `pytest.raises` — catches regressions where the error type stays but the message drifts. + +## Testing error paths + +From [error-handling.md](../../../docs/standards/error-handling.md): + +```python +def test_invalid_trainer_type_exit_code(tmp_path, monkeypatch): + config_path = tmp_path / "bad.yaml" + config_path.write_text("training:\n trainer_type: 'nonexistent'\n...") + # Use subprocess to get real exit code: + result = subprocess.run( + ["forgelm", "--config", str(config_path), "--dry-run"], + capture_output=True, text=True + ) + assert result.returncode == 1 # EXIT_CONFIG_ERROR + assert "trainer_type" in result.stderr +``` + +Or in-process with `capsys`: + +```python +def test_cli_config_error_exits_1(capsys): + with pytest.raises(SystemExit) as exc_info: + cli.main(["--config", "bad.yaml", "--dry-run"]) + assert exc_info.value.code == 1 + captured = capsys.readouterr() + assert "config" in captured.err.lower() +``` + +## Coverage + +- Every new public function needs ≥1 test covering the success path. +- Every `raise` statement and every `sys.exit(!=0)` needs ≥1 test triggering it. +- `pragma: no cover` only for: + - `if __name__ == "__main__":` blocks + - `except ImportError:` fallbacks for optional deps + - Documented platform-specific branches + +Current coverage floor is **40%** (enforced via [`pyproject.toml`](../../../pyproject.toml) `[tool.pytest.ini_options].addopts → --cov-fail-under=40`). Treat it as a floor not a target — you're usually well above. If you ever quote the number elsewhere in docs or skills, source it from `pyproject.toml` to prevent drift. + +## Before opening the PR + +```bash +# Your new tests pass: +pytest tests/test_.py -v + +# No coverage regression: +pytest --cov=forgelm --cov-report=term-missing tests/ + +# Linter happy: +ruff check tests/test_.py +ruff format --check tests/test_.py +``` + +## Pitfalls + +- **`pytest.skip("TODO")`** — tracks nothing. Use `pytest.mark.xfail(reason="...", strict=False)` with a linked issue. +- **Sleep-based waits** — `time.sleep(2)` makes tests flaky. Mock time instead. +- **Module-level state in tests** — `shared_state = []` at test-file scope causes cross-test pollution. +- **Testing the mock** — if your assertion is "mock was called with X", make sure the real-world contract actually requires X. Otherwise you're testing the mock, not the code. +- **Session-scoped fixtures with mutable state** — pollution across test runs. Scope to `function` unless the fixture is immutable. + +## Related skills + +- `add-config-field` — when the thing you're testing is a config field +- `add-trainer-feature` — when the thing you're testing is a larger feature +- `review-pr` — run its checklist before requesting review diff --git a/.agents/skills/add-trainer-feature/SKILL.md b/.agents/skills/add-trainer-feature/SKILL.md new file mode 100644 index 00000000..2cf08bc2 --- /dev/null +++ b/.agents/skills/add-trainer-feature/SKILL.md @@ -0,0 +1,170 @@ +--- +name: add-trainer-feature +description: Use this skill when adding a substantial end-to-end feature to ForgeLM's training pipeline — a new alignment method, a new evaluation gate, a new quantization backend, a new distributed scheme. Differs from add-config-field by touching multiple modules and requiring integration tests. Triggered by requests like "add support for algorithm X", "integrate library Y for training", "implement feature from roadmap Phase N". +--- + +# Skill: Add a Trainer-Level Feature + +Features that span config + model + trainer + tests + docs. Most Phase 10-13 tasks fit this pattern. + +## When to use + +- New alignment method (like adding a 7th trainer type) +- New evaluation pipeline (new safety model, new benchmark suite) +- New PEFT method +- New backend (a future alternative to unsloth/transformers) +- New compliance artifact + +Do **not** use for: +- Bug fixes (scope too small) +- Single config field additions → use `add-config-field` +- Documentation-only changes + +## Required reading before acting + +1. [docs/standards/architecture.md](../../../docs/standards/architecture.md) — module boundaries +2. [docs/standards/testing.md](../../../docs/standards/testing.md) — what tests CI demands +3. The **phase file** describing the feature (e.g., [docs/roadmap/completed-phases.md](../../../docs/roadmap/completed-phases.md)) +4. Existing similar feature — read one similar trainer/module end-to-end before writing yours + +## The scaffold + +Every trainer-level feature touches roughly the same list of files. Tick them as you go: + +### Code + +- [ ] **`forgelm/config.py`** — new `BaseModel` subclass OR new fields on existing config +- [ ] **`forgelm/.py`** — the actual logic +- [ ] **`forgelm/trainer.py`** — wiring: config → module dispatch +- [ ] **`forgelm/cli/_parser.py`** — if new CLI flag needed (Phase 15 split the monolithic `forgelm/cli.py` into the `forgelm/cli/` package; flag definitions live in `_parser.py`, and any new subcommand wires under `forgelm/cli/subcommands/.py` plus the dispatch table in `forgelm/cli/_dispatch.py`) +- [ ] **`forgelm/results.py`** — if new output fields needed +- [ ] **`forgelm/compliance.py`** — if this feature affects audit artifacts +- [ ] **`forgelm/webhook.py`** — if this feature adds a lifecycle event + +### Dependencies + +- [ ] **`pyproject.toml`** — new optional extra (e.g., `forgelm[new_feature]`) if heavy deps required +- [ ] **Import pattern** — follow the `try: import X except ImportError: raise with hint` pattern (see [architecture.md §3](../../../docs/standards/architecture.md#3-optional-dependencies-are-extras-never-silent-imports)) + +### Tests + +- [ ] **`tests/test_.py`** — unit tests for the new module +- [ ] **`tests/test_config.py`** — validation tests for new config +- [ ] **`tests/test_integration.py`** — dry-run that exercises the feature +- [ ] **`tests/conftest.py`** — new fixture if reusable across tests + +### Config + +- [ ] **`config_template.yaml`** — example usage + +### Docs + +- [ ] **`docs/reference/configuration.md`** + **`-tr.md`** — YAML field reference +- [ ] **`docs/reference/usage.md`** + **`-tr.md`** — if CLI changed +- [ ] **`docs/guides/.md`** — new tutorial if feature is user-visible and non-obvious +- [ ] **`docs/roadmap/phase-N-*.md`** — tick off the completed task +- [ ] **`CHANGELOG.md`** — under `[Unreleased]` / `### Added` + +## Worked example: adding a new trainer type + +Say the task is "add IPO (Identity Preference Optimization) trainer." Here's the shape: + +### 1. Config + +```python +# forgelm/config.py +class TrainingConfig(BaseModel): + trainer_type: Literal["sft", "dpo", "simpo", "kto", "orpo", "grpo", "ipo"] = "sft" + ipo_beta: float = 0.1 # IPO-specific hyperparameter +``` + +### 2. Trainer wiring + +```python +# forgelm/trainer.py +from trl import IPOTrainer # optional import + +TRAINER_REGISTRY = { + "sft": _run_sft, + ... + "ipo": _run_ipo, +} + +def _run_ipo(config, model, tokenizer, dataset): + trainer = IPOTrainer( + model=model, + args=_build_training_args(config), + train_dataset=dataset, + beta=config.training.ipo_beta, + ... + ) + trainer.train() + return trainer +``` + +### 3. Data format detection + +```python +# forgelm/data.py — extend _detect_dataset_format +if "chosen" in first_row and "rejected" in first_row: + return "preference_pairs" # DPO, SimPO, IPO all use this +``` + +### 4. Tests + +```python +# tests/test_alignment.py +class TestIPO: + def test_config_accepts_ipo(self): + cfg = ForgeConfig.model_validate( + minimal_config(training={"trainer_type": "ipo", "ipo_beta": 0.1}) + ) + assert cfg.training.trainer_type == "ipo" + + def test_dry_run_with_ipo(self, tmp_path, monkeypatch): + # mock dataset, run dry-run, assert trainer selected + ... +``` + +### 5. Docs + +- `configuration.md`: add `trainer_type: ipo` to the enum table + `ipo_beta` row +- `guides/alignment.md`: add "When to use IPO" section with 2-3 sentences +- `CHANGELOG.md`: `### Added — IPO trainer type (TRL-backed). See docs/guides/alignment.md.` + +### 6. Phase tick + +Open `docs/roadmap/phase-N-*.md` where this task sits, change `[ ]` to `[x]`. + +## Integration checklist + +Before opening the PR, run **all** of these: + +```bash +# Lint + format +ruff check . && ruff format --check . + +# Unit tests +pytest tests/ -v + +# Full pipeline smoke +forgelm --config config_template.yaml --dry-run +forgelm --wizard # if wizard is aware of new feature + +# Optional extras install cleanly? +pip install -e '.[new_feature]' +``` + +## Pitfalls to avoid + +- **Fat trainer.py.** If the new trainer logic is >100 lines, extract to a new `forgelm/.py` module and have trainer.py just dispatch. +- **Global state.** Tempting for trainer-level features ("let me cache this tokenizer"). Don't. Thread state through the function signature. +- **Skipping the optional extra pattern.** A single missing `try: import X` with a helpful message is the difference between a kind error and an ugly traceback. +- **Not testing auto-revert interaction.** If the feature can fail an evaluation gate, test that auto-revert correctly fires. +- **Not updating wizard.py.** If the feature is user-visible, `forgelm --wizard` should surface it. + +## Related skills + +- `add-config-field` — when scope is smaller +- `add-test` — focused test writing +- `sync-bilingual-docs` — after touching `docs/reference/*` diff --git a/.agents/skills/cut-release/SKILL.md b/.agents/skills/cut-release/SKILL.md new file mode 100644 index 00000000..42fb079d --- /dev/null +++ b/.agents/skills/cut-release/SKILL.md @@ -0,0 +1,254 @@ +--- +name: cut-release +description: Use this skill when preparing a ForgeLM release. Walks through version bump, changelog finalization, tag, publish, and post-release sync per docs/standards/release.md. Triggered by requests like "cut v0.4.0", "release ForgeLM", "prepare the next version". +--- + +# Skill: Cut a ForgeLM Release + +Follows [docs/standards/release.md](../../../docs/standards/release.md). This is a maintainer-scope ritual — only invoke when explicitly releasing. + +## When to use + +- User says "release vX.Y.Z" or "cut a release" +- Phase completion warrants a version bump +- A critical fix needs a patch release + +Do **not** use for: +- Routine PR merges (those go to `[Unreleased]` in CHANGELOG; release happens separately) +- Documentation-only changes (no version bump) + +## First: decide if we should release + +Checklist before even starting: + +- [ ] Is there meaningful new content since last release? If only internal refactors, skip and defer. +- [ ] Is `main` CI green? If not, fix first. +- [ ] Is it Tuesday through Thursday? Never release on Friday (weekend support pain). +- [ ] Do we have at least one rc tested if this is a minor release? + +If any is "no," propose waiting rather than pushing ahead. + +## Decide the version + +From [release.md](../../../docs/standards/release.md): + +| Change type | `__version__` bump | `__api_version__` bump | +|---|---|---| +| Breaking (removed flag, renamed YAML field, changed exit code) | MAJOR | — (see below) | +| New feature (new trainer, new CLI subcommand, new config field) | MINOR | — (see below) | +| Bug fixes, docs, internal refactor | PATCH | none | +| Stable library symbol added to `forgelm.__all__` (e.g. new `verify_*` function) | MINOR | MINOR | +| Stable library symbol removed or signature changed | MAJOR | MAJOR | + +`__version__` (single source of truth: [`pyproject.toml`](../../../pyproject.toml) line 7) tracks the *CLI / YAML / behavioural* contract. `__api_version__` (single source of truth: [`forgelm/_version.py`](../../../forgelm/_version.py)) tracks the *Python library* contract — operators who pin against it for feature detection rely on this signal. The two version numbers are decoupled by design: a release that adds a new training paradigm bumps `__version__` MINOR while leaving `__api_version__` untouched (no new public symbol), and a release that adds a new lazy-import target bumps both. + +Library consumers should compare `__api_version__` via `packaging.version.Version` rather than string `>=`, because lexicographic comparison breaks for two-digit minor / patch components (e.g. `"1.10.0" < "1.2.0"` as strings): + +```python +from packaging.version import Version +import forgelm + +assert Version(forgelm.__api_version__) >= Version("1.0.0") +``` + +## Pre-release checklist + +Run before tagging: + +### 1. Sync main + +```bash +git checkout main +git pull +git status # must be clean +``` + +### 2. Finalize CHANGELOG + +Open [`CHANGELOG.md`](../../../CHANGELOG.md). Move items from `[Unreleased]` to a new version section: + +```markdown +## [Unreleased] + +## [0.4.0] — 2026-07-15 + +### Added +- **Inference module** (`forgelm.inference`) — ... + +### Changed +- ... + +### Fixed +- ... + +### Breaking +- `forgelm generate` removed; use `forgelm chat` instead. +``` + +Rules: +- Date is today, ISO format. +- Only categories: Added / Changed / Deprecated / Removed / Fixed / Security / Breaking. +- Every entry is user-facing — dev-only refactors don't belong here. + +### 3. Bump version + +Edit [`pyproject.toml`](../../../pyproject.toml) line 7: + +```toml +version = "0.4.0" # was "0.3.1rc1" or "0.4.0rc1" +``` + +If pre-release → release: drop `rcN` suffix. If new rc: `0.4.0rc1` → `0.4.0rc2`. + +### 3.5. Bump `__api_version__` if applicable + +**Question to answer:** did this release add, remove, or change the signature of a *stable-tier* library symbol (anything in `forgelm.__all__`)? + +- [ ] **No** → leave [`forgelm/_version.py`](../../../forgelm/_version.py) `__api_version__` untouched. Most releases sit here (CLI features, YAML knobs, internal refactors). +- [ ] **Yes, added** → bump `__api_version__` MINOR. Library consumers can then pin `assert Version(forgelm.__api_version__) >= Version("X.Y.0")` (using `packaging.version.Version` — see top-of-skill block) for feature detection. +- [ ] **Yes, removed or signature changed** → bump `__api_version__` MAJOR. The next 0.x release MUST telegraph the break in CHANGELOG's "Breaking" section. + +The canonical bump rule lives at the top of [`forgelm/_version.py`](../../../forgelm/_version.py); the matching policy paragraph is in [`docs/standards/release.md`](../../../docs/standards/release.md). `__version__` and `__api_version__` are intentionally decoupled — see the table at the top of this skill. + +### 4. Docs alignment + +- [ ] If a new CLI flag was added: [`docs/reference/usage.md`](../../../docs/reference/usage.md) + `-tr.md` reflect it +- [ ] If a new config field was added: [`docs/reference/configuration.md`](../../../docs/reference/configuration.md) + `-tr.md` reflect it +- [ ] If a new guide was written: [`docs/guides/`](../../../docs/guides/) has it +- [ ] [`config_template.yaml`](../../../config_template.yaml) exercises all new fields (CI dry-run uses it) +- [ ] [`README.md`](../../../README.md) feature list still accurate +- [ ] **Run `python3 tools/update_site_version.py`** so the 15+ version literals across `site/*.html` (JSON-LD `softwareVersion`, hero badge, `pip install` snippets) and `site/js/translations.js` (`home.hero.badge` × 6 locales) re-derive from CHANGELOG's latest released header. The companion `--check` guard runs in the repo gauntlet so the next PR fails if you forget. + +### 5. Local verification + +```bash +ruff check . && ruff format --check . +pytest tests/ -v +pytest --cov=forgelm --cov-fail-under=40 +forgelm --config config_template.yaml --dry-run + +# Fresh install smoke: +python -m pip install -e . +forgelm --version # should print the new version +``` + +All four must pass. The `--cov-fail-under=40` value tracks the canonical +floor in [`pyproject.toml`](../../../pyproject.toml) under +`[tool.pytest.ini_options].addopts`; if you change it here without +updating pyproject (or vice versa) you will hit a CI failure mid-release. + +### 6. Commit + tag + +```bash +git add -A +git commit -m "chore: release v0.4.0" +git tag -s v0.4.0 -m "v0.4.0 — Post-Training Completion" +git push origin main v0.4.0 +``` + +GPG-signed tag (`-s`) is required for trusted PyPI publishing. + +## Automated publish + +[`publish.yml`](../../../.github/workflows/publish.yml) runs on tag push matching `v*`: + +1. Build wheel + sdist +2. Verify with twine +3. Cross-OS / cross-Python install matrix smoke (3 OS x 4 Python = 12) plus SBOM generation +4. Publish to PyPI (OIDC trusted publishing) + +The workflow does **not** create a GitHub Release — it stops at PyPI +publish. If a GitHub Release is desired, create it manually from the tag +using `gh release create vX.Y.Z --notes-from-tag` after the publish +workflow completes. + +**Wait ~5-10 minutes**, then verify: + +```bash +pip install forgelm==0.4.0 --force-reinstall +forgelm --version +``` + +If the install fails or returns the wrong version, investigate the workflow run. Common issues: tag mismatch with pyproject, build artifact missing, PyPI trust not configured. + +## Post-release + +### 1. Announce + +Use this template (the maintainer keeps a longer canonical version in +local working memory; the spirit is the same): + +``` +📦 ForgeLM v0.4.0 released! + +Highlights: +- Inference module + chat CLI +- GGUF export +- VRAM fit advisor + +Full changelog: https://github.com/cemililik/ForgeLM/blob/main/CHANGELOG.md +Upgrade: pip install --upgrade forgelm +``` + +Post to (ordered by priority): +- Discord `#announcements` +- Twitter/X +- LinkedIn +- (Optional) Dev.to blog post for major features + +### 2. Reset to next pre-release + +```bash +# Edit pyproject.toml: version = "0.4.1rc1" +# Edit CHANGELOG.md: add new ## [Unreleased] at top +git commit -m "chore: bump to 0.4.1rc1 (next dev cycle)" +git push +``` + +### 3. Update marketing roadmap metrics + +The maintainer keeps a marketing roadmap in local working memory +(gitignored). Add a metrics row for the release month there as part +of the release-day checklist; nothing on the public surface needs to +change. + +### 4. Close the phase (if applicable) + +If this release completes a phase, update [`docs/roadmap.md`](../../../docs/roadmap.md): +- Move phase from "Planned" row to archived +- Append detail to [`docs/roadmap/completed-phases.md`](../../../docs/roadmap/completed-phases.md) +- Update [`docs/roadmap/releases.md`](../../../docs/roadmap/releases.md) + +## Hotfix release variant + +If a critical bug is found post-release: + +```bash +git checkout -b hotfix/0.4.1 v0.4.0 +# fix + test +# update CHANGELOG with [0.4.1] section +# bump pyproject.toml to 0.4.1 +git commit -m "fix: " +git push origin hotfix/0.4.1 + +# Create PR to main, review, merge +# Then tag from main HEAD: +git checkout main && git pull +git tag -s v0.4.1 -m "v0.4.1 — hotfix for " +git push origin v0.4.1 +``` + +Follow up in announcements with "⚠️ Security/Stability fix — please upgrade" if warranted. + +## Pitfalls + +- **Releasing with `[Unreleased]` items that don't belong.** Before bumping, read every line in `[Unreleased]` and decide: does this ship? If not, move to a later section or remove. +- **Tagging before pushing commit.** The tag references a commit — if you tag locally and forget to push the commit, CI builds the wrong code. +- **Force-pushing a tag.** Tags are immutable from a user's perspective. If you need to correct a tag, delete locally + remote, push new — but PyPI publishes are one-way. If you've already published, the only fix is a new version. +- **Skipping the rc phase on breaking releases.** At least one rc should be tested for minor versions that add substantial features. Patch releases can skip rc. +- **Not updating `[Unreleased]` after release.** Next PR author will wonder where to add their entry. + +## Related skills + +- `review-pr` — for the PRs that feed the release +- `sync-bilingual-docs` — for doc alignment in step 4 diff --git a/.agents/skills/review-pr/SKILL.md b/.agents/skills/review-pr/SKILL.md new file mode 100644 index 00000000..5a33accd --- /dev/null +++ b/.agents/skills/review-pr/SKILL.md @@ -0,0 +1,195 @@ +--- +name: review-pr +description: Use this skill when reviewing a pull request to ForgeLM, whether your own (self-review before requesting review) or someone else's. Applies the code-review standard, catches anti-patterns, and produces actionable feedback. Triggered by requests like "review this PR", "check if PR #N is ready", "self-review my changes before opening PR". +--- + +# Skill: Review a ForgeLM Pull Request + +Following [docs/standards/code-review.md](../../../docs/standards/code-review.md). This skill is equally useful for self-review (run it before requesting review) and peer review. + +## When to use + +- Before opening a PR (self-review pass) +- When asked to review someone else's PR +- When a PR has been sitting with review comments and you want to check what's left + +Do **not** use for: +- Making the changes yourself (that's a different task) +- Deciding whether to merge (human maintainer call) + +## The seven-question review + +Go through these **in order**. If any answer is "no" or "unclear," block the PR. Question 7 fires only when a regex changes; the other six apply to every PR. + +### 1. Does it match the architecture? + +Check against [docs/standards/architecture.md](../../../docs/standards/architecture.md): + +- [ ] Right module for the concern? (`config.py` doesn't do training, `trainer.py` doesn't parse args) +- [ ] Config-driven? (no env-var sniffing for behaviour, no CLI flags added in isolation) +- [ ] Heavy deps declared as extras in `pyproject.toml`? +- [ ] No silent import fallbacks (`try: import X except: X = None`)? +- [ ] No new module-level mutable state? + +### 2. Does it match the coding standard? + +Check against [docs/standards/coding.md](../../../docs/standards/coding.md): + +- [ ] `ruff check` + `ruff format --check` pass (CI catches this — confirm CI green) +- [ ] Type hints on public functions +- [ ] `Optional[X]` not `X | None` (codebase consistency) +- [ ] Google-style docstrings on public/non-trivial functions +- [ ] No wildcard imports, no re-exports via `__init__.py` without reason +- [ ] Comments only where *why* is non-obvious + +### 3. Are error paths correct? + +Check against [docs/standards/error-handling.md](../../../docs/standards/error-handling.md): + +- [ ] New exit codes (if any) use named constants and appear in the exit-code table +- [ ] Custom exceptions only if there's a distinct `except` handler +- [ ] `sys.exit()` only in `cli.py`, not in library modules +- [ ] No bare `except:`, no `except Exception: pass` +- [ ] User-facing error messages are specific + actionable + not apologetic +- [ ] Auto-revert path writes audit log before cleanup + +### 4. Is observability correct? + +Check against [docs/standards/logging-observability.md](../../../docs/standards/logging-observability.md): + +- [ ] Module has its own logger (`logger = logging.getLogger("forgelm.X")`) +- [ ] No `print()` outside `cli.py` JSON-output blocks +- [ ] Every `sys.exit(!=0)` preceded by `logger.error(...)` +- [ ] New decision gates (safety/benchmark/judge) emit audit events +- [ ] Webhook failures wrapped in try/except, never abort training +- [ ] No secrets logged (tokens, API keys, webhook URLs, raw user prompts) + +### 5. Are tests real? + +Check against [docs/standards/testing.md](../../../docs/standards/testing.md): + +- [ ] New public function → ≥1 happy-path test +- [ ] New exit code or exception → ≥1 test triggering it +- [ ] No GPU required in unit tests +- [ ] No network calls in unit tests (use mocks) +- [ ] No `pytest.skip` without `pytest.mark.xfail(reason=..., ...)` + issue link +- [ ] Assertions are specific (check values, not just truthiness) +- [ ] `pragma: no cover` only for documented exempt patterns + +### 6. Is documentation correct? + +Check against [docs/standards/documentation.md](../../../docs/standards/documentation.md) + [localization.md](../../../docs/standards/localization.md): + +- [ ] If new config field: `docs/reference/configuration.md` + `-tr.md` updated +- [ ] If CLI changed: `docs/reference/usage.md` + `-tr.md` updated +- [ ] If new guide needed: under `docs/guides/` +- [ ] CHANGELOG entry under `[Unreleased]` in the right category +- [ ] `config_template.yaml` updated if new required config field +- [ ] No broken links (relative paths resolve) +- [ ] Bilingual mirrors structurally aligned (H2 count + order match) + +### 7. Does any new / modified regex pass [regex.md](../../../docs/standards/regex.md)? + +**Trigger:** any change to `re.compile`, `re.match`, `re.sub`, `re.findall`, `re.split`. Phase 11/11.5/12 review cycles burned ~10 iterations on regex correctness — this skill exists to spend zero on the next PR. + +Run `git diff --unified=0 origin/main..HEAD -- '*.py' | grep -E '^\+.*re\.(compile|match|search|sub|findall|split|fullmatch)'` to surface the deltas, then for each: + +- [ ] No `[A-Za-z0-9_]` (Sonar `python:S6353`); use `\w`. +- [ ] No single-char character classes `[ ]`, `[\.]`, `[\\]` (Sonar `python:S6328`); use the bare character. +- [ ] Quantifiers bounded where the spec allows it (`{1,6}` for ATX heading depth, not `+`). +- [ ] **No two unbounded `*` / `+` / `*?` / `+?` competing for the same character class.** This is the #1 ReDoS shape we keep hitting (`[ \t]+(.+?)[ \t]*$` → 100ms at n=2000). Anchor on `\S` at body boundaries: `[ \t]+(\S(?:[^\n]*\S)?)[ \t]*$`. +- [ ] No `.*?` + back-reference + `re.DOTALL` (Sonar `python:S5852`); replace with a state machine — see [`forgelm/data_audit.py::_strip_code_fences`](../../../forgelm/data_audit.py) and [`forgelm/ingestion.py::_markdown_sections`](../../../forgelm/ingestion.py). +- [ ] `\s` under `re.MULTILINE` → prefer `[ \t]` (no newline ambiguity). +- [ ] Operator-controlled input → 10K-char pathological-input wall-clock benchmark stays ≤ 10ms. +- [ ] Test fixtures with credential-shaped strings built from inert fragments (see `FAKE_AWS_KEY` / `FAKE_GH_TOKEN` in `tests/test_data_audit_phase12.py`). +- [ ] PEM / PGP block markers inside regex source split via concatenation (`r"-----" + r"BEGIN " + r"..."`) so repo-wide secret scanners don't false-positive. + +If the regex runs on operator-controlled input, **paste the pathological-input benchmark output into the PR description** so reviewers see the linearity proof. + +## Scope and hygiene + +Beyond the six questions: + +- [ ] **One concern per PR.** Bug fix + refactor + feature bundled → split. +- [ ] **No drive-by reformats** obscuring the real diff. +- [ ] **Commit messages informative** — "wip" / "stuff" / "fix things" is a rejection signal. +- [ ] **No `TODO` / `FIXME` without owner + issue link.** +- [ ] **No files outside the PR's stated scope.** Accidentally-committed local configs, `.DS_Store`, editor files — flag for removal. + +## How to write feedback + +For reviewers (including Codex as reviewer): + +### Specific > abstract + +❌ "Docstrings need improvement" +✅ "`trainer.py:142` — the docstring says 'trains the model' but the function also saves a checkpoint; please state the full contract" + +### Suggest concrete changes + +Use GitHub's suggestion blocks: + +~~~markdown +```suggestion +def train_with_revert(config: ForgeConfig, ...) -> TrainResult: + """Train and conditionally revert on safety regression. + + Args: + config: Validated run configuration. + + Returns: + TrainResult with metrics and revert status. + """ +``` +~~~ + +### Distinguish blocking from optional + +- Blocking: state the standard violated, link to it. +- Optional: prefix with `Nit:`, `Style:`, or `FYI:`. + +### Separate questions from demands + +- `Q: why is this `Optional` — can the caller guarantee non-None?` +- `Blocking: this needs to use `Optional[int]` to match codebase convention` + +## Self-review workflow + +Before clicking "Create PR": + +1. **Read the diff in GitHub's web UI** (not just `git diff`) — rendering reveals issues the CLI hides. +2. **Grep for new `TODO` / `FIXME` / `XXX`** — add owner + issue link or remove. +3. **Grep for `print(` in non-CLI code** — remove or justify. +4. **Click through each test file** — does each new test actually assert something? +5. **Click the doc diffs** — renders correctly? Links work? +6. **Run the one-liner**: + + ```bash + ruff format . && ruff check . && pytest tests/ && \ + forgelm --config config_template.yaml --dry-run + ``` + +7. **Read your PR description** — does it state the one concern clearly? + +## Escalation + +When you as reviewer disagree with the author: + +1. State the specific disagreement. +2. Link to a standard or prior art. +3. If unresolved after a round, request a third reviewer or move to an issue. + +Default stance: **a blocked PR is better than a bad merge.** + +## Pitfalls + +- **Over-reviewing** style fixes that ruff will catch on next push. Don't waste author's time on auto-fixable stuff. +- **Rubber-stamping.** "LGTM!" without going through the six questions is worse than no review. +- **Scope creep in review.** "While you're in here, also fix X" — X is a separate PR. +- **Accepting "I'll fix it in a follow-up."** Follow-ups slip. Require fixes in this PR unless genuinely outside scope. +- **Ignoring CI warnings.** Yellow status = broken status. Check. + +## Related skills + +- `add-config-field`, `add-trainer-feature`, `add-test`, `sync-bilingual-docs` — the skills whose output this reviews +- `cut-release` — what happens to merged PRs eventually diff --git a/.agents/skills/sync-bilingual-docs/SKILL.md b/.agents/skills/sync-bilingual-docs/SKILL.md new file mode 100644 index 00000000..cde9b2e5 --- /dev/null +++ b/.agents/skills/sync-bilingual-docs/SKILL.md @@ -0,0 +1,144 @@ +--- +name: sync-bilingual-docs +description: Use this skill when editing any docs/reference/* or other bilingual documentation pairs in ForgeLM. Keeps the .md and -tr.md mirrors structurally aligned per docs/standards/localization.md. Triggered by requests like "update the configuration docs", "add a section about X to the usage guide", "translate the new feature docs". +--- + +# Skill: Keep Bilingual Docs in Sync + +ForgeLM ships user-facing docs in English + Turkish. The mirrors must stay in structural sync or readers switching languages get confused and CI (future) will complain. + +## When to use + +Any change to these paths or their siblings: +- `docs/README.md` (if bilingual) +- `docs/product_strategy.md` / `product_strategy-tr.md` +- `docs/roadmap.md` / `roadmap-tr.md` +- `docs/reference/*.md` (has matching `-tr.md`) + +Do **not** use for: +- `docs/standards/*` (English only) +- `docs/marketing/*` (gitignored working-memory; mixed languages) +- `docs/qms/*` (English only) +- `docs/design/*` (English only) +- `docs/guides/*` (English only for now — Turkish is future work) + +Do **not** reference any file under gitignored working-memory directories +(`docs/marketing/`, `docs/analysis/`) from the bilingual docs you edit — +those paths are local-only and won't resolve in fresh clones. + +## Required reading + +1. [docs/standards/localization.md](../../../docs/standards/localization.md) — the full policy +2. [docs/standards/documentation.md](../../../docs/standards/documentation.md) — markdown conventions + +## Rules in one screen + +- **Same H2 count, same order.** If the EN file has 5 H2 sections, the TR file has 5 — in the same order. +- **Links update together.** If you change a link target in EN, change it in TR. +- **Code blocks are copy-pasted.** Code is language-neutral; don't translate Python/YAML/JSON. +- **Wording may vary within a section.** Idiomatic Turkish trumps literal translation. +- **Don't machine-translate.** Google Translate output pasted without review is a bug; either write it properly or leave a tracking issue. + +## Workflow + +### If you're adding content in English + +``` +1. Edit file.md (English) +2. Same PR: edit file-tr.md — add matching section in same position +3. Verify H2 count matches: grep -c "^## " docs/reference/X.md equals grep -c "^## " docs/reference/X-tr.md +4. Verify links in TR point to TR mirrors (-tr.md) where those exist +5. Commit together +``` + +### If you're adding content in Turkish + +Same workflow, mirrored. But: the EN version is canonical — if you're writing new content, write the EN first, then mirror. + +### If you can't write Turkish + +Acceptable fallback (for non-native-speaker contributors): + +1. Edit `file.md` (English) +2. Add `` at the top of `file-tr.md` for the new section +3. Open a tracking issue labeled `translation-debt` +4. Reference the issue in the PR description +5. Reviewer decides whether to accept or require translation in this PR + +**Do not paste untranslated English into the TR file.** Either translate properly or mark pending explicitly. + +## Link parity + +If the EN file (say `docs/reference/foo.md`) says: + +```markdown +See [Architecture](architecture.md) for the module layout. +``` + +The TR file (`docs/reference/foo-tr.md`) should say: + +```markdown +Modül yapısı için [Mimari](architecture-tr.md) belgesine bakın. +``` + +Both link text and target translate. A TR reader clicking an EN link mid-document is a UX bug. + +## Verification commands + +```bash +# H2 count must match +EN_COUNT=$(grep -c "^## " docs/reference/configuration.md) +TR_COUNT=$(grep -c "^## " docs/reference/configuration-tr.md) +[ "$EN_COUNT" = "$TR_COUNT" ] && echo "OK" || echo "MISMATCH" + +# Links to other bilingual files should use -tr.md from TR files +grep -E "\]\([^)]+\.md\)" docs/reference/configuration-tr.md | \ + grep -v "\-tr\.md" | \ + grep -v "^.*standards/" # standards are EN only, allowed +# Should produce no output +``` + +(These aren't enforced by CI yet, but the rules are the same.) + +## Terminology + +Consistent glossary from [localization.md](../../../docs/standards/localization.md): + +| English | Turkish | +|---|---| +| fine-tuning | ince ayar | +| training | eğitim | +| config / configuration | yapılandırma / config | +| pipeline | pipeline (tercih) / boru hattı | +| safety evaluation | güvenlik değerlendirmesi | +| audit trail | denetim izi | +| compliance | uyumluluk | +| quantization | quantization (çevirmeyi zorlama) | +| layer | katman | +| tokenizer | tokenizer | +| EU AI Act | EU AI Act (ilk kullanımda: AB Yapay Zekâ Yasası) | + +Acronyms (LoRA, PEFT, DPO, GRPO) stay unchanged in both languages. + +## Common mistakes + +- **Adding a new section in EN only** — TR drifts. Update both in the same PR. +- **Reordering sections in EN only** — TR becomes misaligned. Move both. +- **Changing a link target in EN only** — TR link 404s. Change both. +- **Translating code examples** — Python keywords don't translate. Leave code alone. +- **Adding decorative emoji to one and not the other** — cosmetic but inconsistent. Pick one style. +- **Machine-translated Turkish** — reads like broken code. Don't. + +## When the mirror doesn't exist yet + +The file list in [localization.md](../../../docs/standards/localization.md#whats-translated) marks which paths have TR mirrors. If you're working on one that doesn't: + +- Don't create a TR mirror preemptively — creates maintenance burden. +- If you want to add one, first update the table in localization.md with reasoning. +- Then create the TR mirror and use this skill going forward. + +## Related skills + +- `add-config-field` — always touches `configuration.md` + `configuration-tr.md` +- `add-trainer-feature` — typically touches multiple `docs/reference/` files +- `cut-release` — CHANGELOG updates don't need a TR mirror (English only) diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..e7fd9241 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,205 @@ +# AGENTS.md — Project Guidance for AI Agents + +> **Audience:** Codex (and other AI coding agents) working on ForgeLM. Complements — does not replace — the human-facing [CONTRIBUTING.md](CONTRIBUTING.md) and [docs/standards/](docs/standards/). + +## What ForgeLM is (in one line) + +A **config-driven, enterprise-grade LLM fine-tuning toolkit** — YAML in, fine-tuned model + compliance artifacts out. Built for CI/CD pipelines, not notebooks. Covers SFT → DPO → SimPO → KTO → ORPO → GRPO, with integrated safety evaluation, EU AI Act compliance, and auto-revert on quality regression. + +Not a framework for training from scratch. Not an inference engine. Not a GUI. Read [docs/product_strategy.md](docs/product_strategy.md) for the 5-minute background. + +## What you must read before editing code + +**Every time, in this order:** + +1. **[docs/standards/README.md](docs/standards/README.md)** — index of all engineering standards +2. **The specific standard** matching what you're about to change: + - Python code → [coding.md](docs/standards/coding.md) + [architecture.md](docs/standards/architecture.md) + - **Any `re.compile` / regex change → [regex.md](docs/standards/regex.md)** (ReDoS exposure, fixture fragmentation, the 8 hard rules distilled from Phase 11/11.5/12 review cycles) + - Error paths → [error-handling.md](docs/standards/error-handling.md) + - Anything with output → [logging-observability.md](docs/standards/logging-observability.md) + - Tests → [testing.md](docs/standards/testing.md) + - Docs → [documentation.md](docs/standards/documentation.md) + [localization.md](docs/standards/localization.md) + - PR / review → [code-review.md](docs/standards/code-review.md) + - Release → [release.md](docs/standards/release.md) +3. **[CONTRIBUTING.md](CONTRIBUTING.md)** — the human-facing summary +4. **The relevant roadmap file** — if implementing a planned phase, find it under [docs/roadmap/](docs/roadmap/) + +Do not invent conventions. If you cannot find the pattern for what you're about to add, ask the user — don't guess. + +## Skills + +When a task maps to a common pattern, invoke the matching skill from [.Codex/skills/](.Codex/skills/): + +| Task | Skill | +|---|---| +| Adding a YAML config field | [add-config-field](.Codex/skills/add-config-field/SKILL.md) | +| Adding a larger trainer / evaluator / module feature | [add-trainer-feature](.Codex/skills/add-trainer-feature/SKILL.md) | +| Writing tests | [add-test](.Codex/skills/add-test/SKILL.md) | +| Updating bilingual docs (EN ↔ TR) | [sync-bilingual-docs](.Codex/skills/sync-bilingual-docs/SKILL.md) | +| Reviewing a PR (own or peer) | [review-pr](.Codex/skills/review-pr/SKILL.md) | +| Cutting a release | [cut-release](.Codex/skills/cut-release/SKILL.md) | + +Each skill's `SKILL.md` has the full checklist. Follow it; don't skip steps to save time. + +## Repository structure at a glance + +``` +ForgeLM/ +├── forgelm/ # Source code: ~22 single-file modules + 2 sub-packages +│ ├── cli/ # CLI package (Phase 15 split): _parser, _dispatch, +│ │ # _exit_codes, subcommands/{ingest, audit, chat, +│ │ # export, deploy, quickstart, doctor, cache, +│ │ # purge, reverse_pii, approve, approvals, +│ │ # safety_eval, verify_audit, verify_annex_iv, +│ │ # verify_gguf, ...} +│ ├── data_audit/ # Audit package (Phase 14 split): _orchestrator, +│ │ # _aggregator, _streaming, _simhash, _minhash, +│ │ # _pii_regex, _pii_ml, _secrets, _quality, +│ │ # _croissant, _summary, _splits +│ ├── config.py # Pydantic schemas (19 models) +│ ├── trainer.py # TRL wrapper (SFT/DPO/SimPO/KTO/ORPO/GRPO) +│ ├── model.py # HF + PEFT model loading +│ ├── data.py # Dataset loading + format detection +│ ├── ingestion.py # Raw docs → SFT JSONL (`forgelm ingest`) +│ ├── safety.py # Llama Guard + harm categories + auto-revert +│ ├── compliance.py # EU AI Act Articles 9-17 + Annex IV + GDPR purge / reverse-pii primitives +│ ├── webhook.py # Slack/Teams notifications (5-event vocabulary) +│ ├── grpo_rewards.py # Built-in GRPO format/length shaping reward fallback +│ ├── _http.py # SSRF-guarded HTTP chokepoint (safe_post / safe_get) +│ ├── _version.py # `__version__` + `__api_version__` (decoupled) +│ └── ... # benchmark, judge, merging, synthetic, wizard, +│ # quickstart, model_card, fit_check, deploy, chat, +│ # export, inference, results, utils +├── tests/ # 70 test modules; count grows over time (run `pytest --collect-only -q` for current) +├── tools/ # CI guards: check_anchor_resolution, +│ # check_bilingual_parity, check_cli_help_consistency, +│ # check_field_descriptions, check_no_analysis_refs, +│ # check_pip_audit, check_bandit, check_site_claims, +│ # check_wizard_defaults_sync, generate_sbom, +│ # generate_wizard_defaults, build_usermanuals +├── docs/ +│ ├── roadmap.md # Public roadmap (short index) +│ ├── roadmap/ # Detailed phase files + archive +│ ├── reference/ # User-facing API/config reference +│ ├── guides/ # User-facing tutorials +│ ├── usermanuals/{en,tr}/ # 4-section user manual (training, eval, deploy, ref) +│ ├── design/ # Design specs (internal) +│ ├── standards/ # Engineering standards (this project's rulebook) +│ ├── qms/ # Quality management SOPs (EU AI Act Art. 17, EN+TR) +│ ├── analysis/ # Research, code reviews, external repo analyses +│ └── marketing/ # Local-only (gitignored): marketing + strategy +├── config_template.yaml # Canonical YAML example — CI dry-runs this +├── pyproject.toml # Build, deps, ruff, pytest, coverage config +├── CHANGELOG.md # Keep-a-Changelog format +├── README.md # User-facing project summary +├── CONTRIBUTING.md # Human contributor guide +└── AGENTS.md # This file +``` + +## Non-negotiable project principles + +These come from the standards documents; summarized here for quick reference: + +1. **Config-driven.** Behaviour is determined by validated YAML. No env-var sniffing for behaviour (only for secrets). No hardcoded feature flags. +2. **Reliability before features.** Every new capability ships with tests, docs, and CI coverage. "I'll add tests later" = the PR is not ready. +3. **Optional dependencies as extras.** Heavy deps (`bitsandbytes`, `unsloth`, `deepspeed`, `lm-eval`, `wandb`, `mergekit`) live under `[project.optional-dependencies]` and raise `ImportError` with an install hint when missing. +4. **Exit codes are a public contract.** 0/1/2/3/4/5 — see [error-handling.md](docs/standards/error-handling.md) for the full table (`0=success`, `1=config`, `2=training`, `3=eval-failure`, `4=awaiting-approval`, `5=wizard-cancelled`). CI/CD pipelines depend on these. +5. **Append-only audit log.** Every decision gate emits a structured event. Never edit or delete entries. +6. **No silent failures.** No bare `except:`, no `except Exception: pass`, no `|| true` in CI, no logging-and-swallowing for anything except explicitly-non-fatal paths (webhooks, cleanup). +7. **Bilingual where it counts.** User-facing docs are EN + TR mirrors. Code, CLI output, logs, config keys are English only. +8. **Config-driven features are opt-in.** Enterprise features (compliance export, human approval, safety eval) are opt-in; new users aren't burdened. + +## What ForgeLM is not + +Reinforced in [docs/marketing/strategy/05-yapmayacaklarimiz.md](docs/marketing/strategy/05-yapmayacaklarimiz.md). Do not propose or implement: + +- **Web UI / GUI.** Config-driven is the identity. Dashboard for Pro CLI only. +- **Custom inference engine.** Hand off to Ollama / vLLM / TGI / llama.cpp. +- **Custom model architectures.** HuggingFace owns that. +- **Custom quantization kernels.** bitsandbytes / AWQ / GPTQ / HQQ own that. +- **Pretraining pipelines.** Fine-tuning only. +- **GPU marketplace or serving infra.** User brings their own GPU. +- **LLM leaderboards or community adapter zoos.** HF Hub already exists. + +If a task pushes in any of those directions, raise it with the user before implementing. + +## Common pitfalls (from prior reviews) + +Learned the hard way across multiple PR-cycle audits and external-repo +comparisons; treat each bullet as a hard rule: + +- **Documentation drift** — marketing claims that code doesn't back up. Every README claim must point to real code. +- **Silent import fallbacks** — `try: import X; except: X = None` hides missing deps behind mysterious `AttributeError` later. +- **CI `|| true`** — fake green status. Outlawed. +- **Stub code tagged "Production Ready"** — `NotImplementedError("Planned for Phase N", issue=#42)` only, never silent stubs. +- **Single-language comment drift** — mixing Turkish + Spanish + English in code comments. English only in code. +- **Zero-byte or misplaced files** — leftover artifacts from refactors. Clean up. + +## How to work on a task + +Default workflow for a non-trivial change: + +1. **Understand first.** Read the relevant standard. Read the similar existing code. +2. **Plan second.** If the task is multi-step, use `TodoWrite` to track your plan. +3. **Invoke the right skill.** If the task maps to one, follow the SKILL.md end-to-end. +4. **Code third.** Smallest possible diff. One concern per change. +5. **Test immediately.** Write the test before or alongside the code, never after merge. +6. **Verify before opening PR.** Run the self-review command: + + ```bash + ruff format . && ruff check . && pytest tests/ && \ + forgelm --config config_template.yaml --dry-run && \ + python3 tools/check_bilingual_parity.py --strict && \ + python3 tools/check_anchor_resolution.py --strict && \ + python3 tools/check_cli_help_consistency.py --strict && \ + python3 tools/check_wizard_defaults_sync.py && \ + python3 tools/check_no_analysis_refs.py && \ + python3 tools/check_no_unguarded_sys_modules_pop.py && \ + python3 tools/update_site_version.py --check + ``` + + All eleven must pass. The first four are the historical gauntlet; + the three doc guards (Wave 3 / Wave 4 / Wave 5 additions) catch + bilingual structural drift, broken markdown anchors, and CLI ↔ docs + help-text drift before the PR opens. The wizard-defaults guard + (review-cycle 3) catches schema-vs-shipped-JSON drift for the + wizard's source-of-truth defaults. The working-memory-refs guard + (review-cycle 5) keeps the public tree from citing gitignored + `docs/marketing/` or `docs/analysis/` paths — see + `docs/standards/documentation.md` "Working-memory directories". + The unguarded-`sys.modules`-pop guard (v0.5.7 round-4) flags any + `sys.modules.pop("torch"|"numpy"|"trl"|…)` without + `monkeypatch.delitem` — the v0.5.7 round-3 review traced 35 + spurious full-suite failures to that exact pattern. The + site-version guard (v0.6.0 retag cycle) re-derives the marketing + site's displayed version from CHANGELOG's latest released header + and fails the PR if any of the 15+ literals across `site/*.html` + and `site/js/translations.js` has drifted; the v0.5.5 → v0.6.0 + release shipped with the hero badge still reading `v0.5.5`, which + this guard now prevents. + +## Etiquette when communicating with the user + +- **State results directly.** No filler like "Great question!" or "Let me help you with that." +- **Brief updates during work.** One sentence per tool call max. Silence is worse than terse. +- **Surface decision points.** If you encounter something that requires a judgement call beyond the stated task, stop and ask. Don't silently expand scope. +- **Flag trade-offs.** If your implementation picks A over B for non-obvious reasons, say so in your summary. +- **Turkish is welcome.** User writes in Turkish; respond in Turkish unless technical content is clearly cleaner in English. Code and file content: English only. + +## When in doubt + +1. Check the relevant [docs/standards/](docs/standards/) file. +2. Check for a matching skill in [.Codex/skills/](.Codex/skills/). +3. Find the closest existing pattern in the codebase and follow it. +4. Ask the user rather than guess. + +## Memory and context + +- The `docs/marketing/` directory is gitignored (internal strategy). Content there is real; treat it as a source of truth for direction but don't reference it in public-facing code or docs. +- The `docs/analysis/` directory is gitignored research / audit working memory (PR-cycle review notes, external-repo comparisons, drafts). **Never reference its contents from production code, public docs, CHANGELOG entries, commit messages, or PR descriptions.** Decisions distilled from those notes live in `docs/standards/`, `docs/roadmap/`, the CHANGELOG, and inline code comments — those are the citations reviewers see. +- The roadmap ([docs/roadmap.md](docs/roadmap.md)) is what ships. The marketing strategy roadmap ([docs/marketing/marketing_strategy_roadmap.md](docs/marketing/marketing_strategy_roadmap.md)) is what gets announced. Don't conflate the two. + +--- + +**If you've read this far and you're about to start work:** open the relevant standard + skill now. Then begin. From 9f057faefd1c0d715ee26911fa7e9743f68a7cb3 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Thu, 14 May 2026 03:29:00 +0300 Subject: [PATCH 04/11] =?UTF-8?q?fix:=20bot-review=20cleanup=20=E2=80=94?= =?UTF-8?q?=20encoding,=20fail-loud=20PEFT=20config,=20hostname=20guard,?= =?UTF-8?q?=20hoist=20redact=20constants?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the four bot-review findings on PR #50 plus the two cosmetic tradeoffs left from the previous review-response round. Bot findings (gemini-code-assist + sourcery-ai) - forgelm/cli/_no_train_modes.py:38 (gemini): explicit encoding="utf-8" on adapter_config.json read. - forgelm/cli/_no_train_modes.py:40 (gemini + sourcery overlap): when adapter_config.json is missing base_model_name_or_path or is malformed JSON, exit with EXIT_CONFIG_ERROR and a clear log message instead of silently falling back to model_path (which would crash deep inside PeftModel.from_pretrained with a confusing "config.json not found"). Two new regression tests cover this (corrupt JSON + missing field). - forgelm/cli/subcommands/_approve.py:96 (gemini): wrap socket.gethostname() in try/except OSError; restricted containers / sandboxes can fail hostname resolution and the whole point of FORGELM_ALLOW_ANONYMOUS_OPERATOR=1 is to keep running there. Falls back to "unknown-host" narrowly. - forgelm/judge.py + forgelm/safety.py (sourcery): hoist the redaction field set to module-level frozenset constants (_PII_REDACT_FIELDS). Documents the GDPR / EU AI Act Art. 10 motivation in-line and avoids reallocating the set on every save call. Cosmetic tradeoffs deferred from PR #50 review-response round - forgelm/cli/_no_train_modes.py: flatten `import json as _json` / `import os as _os` to the module-level `json` / `os` imports. The underscore-prefix variants added isolation noise without isolation benefit; behaviour is identical. - forgelm/cli/_dispatch.py: narrow `except Exception` to `(OSError, ConfigError)` on the --data-audit deprecation breadcrumb. Keeps the best-effort contract without silently swallowing unrelated runtime errors. Skipped (with rationale in the PR comment) - sourcery's TrainingConfig 0/None disable semantics: HF's TrainingArguments uses None default (never 0) for save_total_limit; ge=1 is consistent with the schema's int typing. Co-Authored-By: Claude Opus 4.7 (1M context) --- forgelm/cli/_dispatch.py | 3 +- forgelm/cli/_no_train_modes.py | 30 ++++++++++++++----- forgelm/cli/subcommands/_approve.py | 10 ++++++- forgelm/judge.py | 10 +++++-- forgelm/safety.py | 11 +++++-- tests/test_no_train_modes.py | 45 ++++++++++++++++++++++++----- 6 files changed, 87 insertions(+), 22 deletions(-) diff --git a/forgelm/cli/_dispatch.py b/forgelm/cli/_dispatch.py index 8e0376ce..da2ecbec 100644 --- a/forgelm/cli/_dispatch.py +++ b/forgelm/cli/_dispatch.py @@ -211,6 +211,7 @@ def _main_inner() -> None: legacy_target = args.output or "./audit" try: from ..compliance import AuditLogger + from ..config import ConfigError AuditLogger(legacy_target).log_event( "cli.legacy_flag_invoked", @@ -218,7 +219,7 @@ def _main_inner() -> None: replacement="forgelm audit", version="v0.7.0 removal", ) - except Exception as audit_exc: # noqa: BLE001 — best-effort breadcrumb; ConfigError (anonymous operator) and OSError (read-only dir) are both non-fatal here + except (OSError, ConfigError) as audit_exc: # Non-fatal: the audit log is a best-effort telemetry record # for the deprecation notice. The DeprecationWarning has # already fired; the audit run itself must still proceed — diff --git a/forgelm/cli/_no_train_modes.py b/forgelm/cli/_no_train_modes.py index 8ab3e159..53234005 100644 --- a/forgelm/cli/_no_train_modes.py +++ b/forgelm/cli/_no_train_modes.py @@ -18,9 +18,6 @@ def _run_benchmark_only(config: ForgeConfig, model_path: str, output_format: str) -> None: """Run benchmark evaluation on an existing model without training.""" - import json as _json - import os as _os - from ..benchmark import run_benchmark from ..inference import load_model @@ -33,11 +30,28 @@ def _run_benchmark_only(config: ForgeConfig, model_path: str, output_format: str # Detect PEFT checkpoint: if the path contains adapter_config.json, load the # base model from there and merge the adapter — otherwise load as a plain model. - adapter_cfg_path = _os.path.join(model_path, "adapter_config.json") - if _os.path.isfile(adapter_cfg_path): - with open(adapter_cfg_path) as _f: - _adapter_meta = _json.load(_f) - base_path = _adapter_meta.get("base_model_name_or_path", model_path) + adapter_cfg_path = os.path.join(model_path, "adapter_config.json") + if os.path.isfile(adapter_cfg_path): + # adapter_config.json is HF-produced JSON — explicit UTF-8 keeps the + # parse deterministic across Windows code-page locales. + try: + with open(adapter_cfg_path, encoding="utf-8") as f: + adapter_meta = json.load(f) + except (OSError, json.JSONDecodeError) as e: + logger.error("Failed to read PEFT adapter_config.json at %s: %s", adapter_cfg_path, e) + sys.exit(EXIT_CONFIG_ERROR) + base_path = adapter_meta.get("base_model_name_or_path") + if not base_path: + # Falling back to model_path here would re-load the adapter + # directory as a base model — it has no base weights, so + # PeftModel.from_pretrained would later crash with a confusing + # "config.json not found" error. Fail loud at the source. + logger.error( + "PEFT checkpoint at %s is missing 'base_model_name_or_path' in adapter_config.json — " + "cannot reconstruct the base model + adapter combination.", + model_path, + ) + sys.exit(EXIT_CONFIG_ERROR) logger.info( "Detected PEFT checkpoint. Loading base model %s + adapter %s for benchmark.", base_path, model_path ) diff --git a/forgelm/cli/subcommands/_approve.py b/forgelm/cli/subcommands/_approve.py index ae4e2ebd..aa556461 100644 --- a/forgelm/cli/subcommands/_approve.py +++ b/forgelm/cli/subcommands/_approve.py @@ -93,7 +93,15 @@ def _resolve_approver_identity() -> str: sys.exit(EXIT_CONFIG_ERROR) import socket - return f"anonymous@{socket.gethostname() or 'unknown-host'}" + try: + hostname = socket.gethostname() or "unknown-host" + except OSError: + # gethostname() can raise OSError in restricted container / sandbox + # environments where the hostname is not resolvable. The whole + # point of FORGELM_ALLOW_ANONYMOUS_OPERATOR=1 is to keep running in + # exactly those environments, so swallow the failure narrowly. + hostname = "unknown-host" + return f"anonymous@{hostname}" def _find_human_approval_required_event(audit_log_path: str, run_id: str) -> Optional[dict]: diff --git a/forgelm/judge.py b/forgelm/judge.py index ed9b2586..e094719a 100644 --- a/forgelm/judge.py +++ b/forgelm/judge.py @@ -42,6 +42,12 @@ class JudgeResult: OPENAI_API_BASE = "https://api.openai.com/v1/chat/completions" +# GDPR / EU AI Act Art. 10 — fields stripped from on-disk judge_results.json +# unless the operator opts in via JudgeConfig.include_eval_samples=True. +# ``reason`` is included because the judge's natural-language gerekçesi may +# quote PII from the eval prompts/responses verbatim. +_PII_REDACT_FIELDS: frozenset[str] = frozenset({"prompt", "response", "reason"}) + # Single source-of-truth for the hard-failure log line so the wording # stays identical across the three call sites (HttpSafetyError handler, # JSON parse failure, no-valid-scores summary). Operators grep the audit @@ -312,7 +318,7 @@ def _save_judge_results( """ os.makedirs(output_dir, exist_ok=True) results_path = os.path.join(output_dir, "judge_results.json") - _REDACT = set() if include_samples else {"prompt", "response", "reason"} + redact = frozenset() if include_samples else _PII_REDACT_FIELDS with open(results_path, "w", encoding="utf-8") as f: json.dump( { @@ -320,7 +326,7 @@ def _save_judge_results( "min_score": min_score, "passed": passed, "num_prompts": num_prompts, - "details": [{k: v for k, v in d.items() if k not in _REDACT} for d in details], + "details": [{k: v for k, v in d.items() if k not in redact} for d in details], }, f, indent=2, diff --git a/forgelm/safety.py b/forgelm/safety.py index e13240a1..fcf9a800 100644 --- a/forgelm/safety.py +++ b/forgelm/safety.py @@ -50,6 +50,13 @@ } +# GDPR / EU AI Act Art. 10 — fields stripped from on-disk safety_results.json +# unless the operator opts in via SafetyConfig.include_eval_samples=True. +# Adversarial test prompts and the model's responses to them can carry +# sensitive content (jailbreak attempts, PII leakage, etc.). +_PII_REDACT_FIELDS: frozenset[str] = frozenset({"prompt", "response"}) + + @dataclass class SafetyResult: """Result of a safety evaluation run.""" @@ -421,7 +428,7 @@ def _save_safety_results( """ os.makedirs(output_dir, exist_ok=True) results_path = os.path.join(output_dir, "safety_results.json") - _REDACT = set() if include_samples else {"prompt", "response"} + redact = frozenset() if include_samples else _PII_REDACT_FIELDS output_data: Dict[str, Any] = { "scoring_method": scoring, "safe_ratio": safe_ratio, @@ -431,7 +438,7 @@ def _save_safety_results( "low_confidence_count": low_confidence_count, "passed": passed, "failure_reason": failure_reason, - "details": [{k: v for k, v in d.items() if k not in _REDACT} for d in details], + "details": [{k: v for k, v in d.items() if k not in redact} for d in details], } if track_categories: output_data["category_distribution"] = category_dist diff --git a/tests/test_no_train_modes.py b/tests/test_no_train_modes.py index dc4a45a2..d266f1df 100644 --- a/tests/test_no_train_modes.py +++ b/tests/test_no_train_modes.py @@ -86,26 +86,55 @@ def test_peft_checkpoint_routes_through_adapter(self, tmp_path, minimal_config): "Adapter path must be forwarded so PeftModel.from_pretrained merges the saved weights" ) - def test_peft_checkpoint_without_base_model_falls_back_to_adapter_path(self, tmp_path, minimal_config): - """If adapter_config.json lacks base_model_name_or_path, fall back - to using the adapter dir as both base and adapter source.""" + def test_peft_checkpoint_without_base_model_fails_loudly(self, tmp_path, minimal_config): + """If adapter_config.json lacks ``base_model_name_or_path`` we cannot + reconstruct the base model + adapter combination; falling back to + the adapter path would trigger a confusing ``config.json not found`` + crash deep inside PeftModel.from_pretrained. Exit with + EXIT_CONFIG_ERROR at the source instead. + """ + import pytest + + from forgelm.cli._exit_codes import EXIT_CONFIG_ERROR + config = _bench_config(minimal_config, tmp_path / "out") adapter_dir = tmp_path / "adapter" adapter_dir.mkdir() (adapter_dir / "adapter_config.json").write_text(json.dumps({})) with ( - patch("forgelm.inference.load_model", return_value=(MagicMock(), MagicMock())) as load_mock, + patch("forgelm.inference.load_model", return_value=(MagicMock(), MagicMock())), patch("forgelm.benchmark.run_benchmark", return_value=_passing_benchmark_result()), + pytest.raises(SystemExit) as exc_info, ): from forgelm.cli._no_train_modes import _run_benchmark_only _run_benchmark_only(config, str(adapter_dir), output_format="json") - load_mock.assert_called_once() - call = load_mock.call_args - assert call.args[0] == str(adapter_dir) - assert call.kwargs.get("adapter") == str(adapter_dir) + assert exc_info.value.code == EXIT_CONFIG_ERROR + + def test_peft_checkpoint_with_corrupt_adapter_config_fails_loudly(self, tmp_path, minimal_config): + """A truncated / malformed adapter_config.json must surface an + actionable config error rather than crashing later.""" + import pytest + + from forgelm.cli._exit_codes import EXIT_CONFIG_ERROR + + config = _bench_config(minimal_config, tmp_path / "out") + adapter_dir = tmp_path / "adapter" + adapter_dir.mkdir() + (adapter_dir / "adapter_config.json").write_text("{not valid json") + + with ( + patch("forgelm.inference.load_model", return_value=(MagicMock(), MagicMock())), + patch("forgelm.benchmark.run_benchmark", return_value=_passing_benchmark_result()), + pytest.raises(SystemExit) as exc_info, + ): + from forgelm.cli._no_train_modes import _run_benchmark_only + + _run_benchmark_only(config, str(adapter_dir), output_format="json") + + assert exc_info.value.code == EXIT_CONFIG_ERROR def test_get_model_and_tokenizer_not_called(self, tmp_path, minimal_config): """Regression for P1-1: the training-time loader must not be used — From 83ee102509ff04aef636b97ac4fb0f2094ddb72b Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Thu, 14 May 2026 03:37:06 +0300 Subject: [PATCH 05/11] refactor: address SonarQube cognitive-complexity + param-count code smells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three SonarQube findings introduced by the review-response rounds — each fixed by extracting helpers, no behaviour change. - forgelm/cli/_no_train_modes.py: extract _resolve_benchmark_load_target() helper that owns PEFT detection + adapter_config.json read + base_model_name_or_path validation. _run_benchmark_only now has a single load_model() call, dropping cognitive complexity from 19 to under the 15 limit. (S3776, Critical) - forgelm/inference.py: extract _pick_tokenizer_source() helper for the "prefer adapter tokenizer when tokenizer_config.json is present" decision. load_model cognitive complexity 17 -> under 15. (S3776, Critical) - forgelm/safety.py: bundle the three Phase 9 category-telemetry arguments (track_categories, category_dist, severity_dist) into a new _CategoryTelemetry dataclass. _save_safety_results parameter count drops from 14 to 12 (the 13-parameter limit) without forcing callers to flatten on the call site. (S107, Major) Existing test_safety_advanced.py::TestSafetyResultRedaction tests updated to use the new dataclass; tests/test_no_train_modes.py PEFT detection tests still pass against the refactored shape. Co-Authored-By: Claude Opus 4.7 (1M context) --- forgelm/cli/_no_train_modes.py | 91 +++++++++++++++++++--------------- forgelm/inference.py | 31 +++++++----- forgelm/safety.py | 35 +++++++++---- tests/test_safety_advanced.py | 12 ++--- 4 files changed, 101 insertions(+), 68 deletions(-) diff --git a/forgelm/cli/_no_train_modes.py b/forgelm/cli/_no_train_modes.py index 53234005..8fd5825d 100644 --- a/forgelm/cli/_no_train_modes.py +++ b/forgelm/cli/_no_train_modes.py @@ -16,6 +16,46 @@ from ._logging import logger +def _resolve_benchmark_load_target(model_path: str) -> tuple[str, str | None]: + """Resolve ``(base_path, adapter_path_or_none)`` for the inference loader. + + A PEFT checkpoint is detected by the presence of ``adapter_config.json`` + inside ``model_path``. When found, the base model identifier is read + from ``adapter_config.json::base_model_name_or_path``; the adapter + directory itself is returned as the second tuple element so the + caller can pass ``adapter=...`` to :func:`forgelm.inference.load_model`. + + Exits with :data:`EXIT_CONFIG_ERROR` (with an actionable log line) + when the adapter config is unreadable / malformed / missing the + base-model field — these are all unrecoverable configuration errors + that would otherwise surface as confusing crashes deep inside + ``PeftModel.from_pretrained``. + """ + adapter_cfg_path = os.path.join(model_path, "adapter_config.json") + if not os.path.isfile(adapter_cfg_path): + return model_path, None + + # adapter_config.json is HF-produced JSON — explicit UTF-8 keeps the + # parse deterministic across Windows code-page locales. + try: + with open(adapter_cfg_path, encoding="utf-8") as f: + adapter_meta = json.load(f) + except (OSError, json.JSONDecodeError) as e: + logger.error("Failed to read PEFT adapter_config.json at %s: %s", adapter_cfg_path, e) + sys.exit(EXIT_CONFIG_ERROR) + + base_path = adapter_meta.get("base_model_name_or_path") + if not base_path: + logger.error( + "PEFT checkpoint at %s is missing 'base_model_name_or_path' in adapter_config.json — " + "cannot reconstruct the base model + adapter combination.", + model_path, + ) + sys.exit(EXIT_CONFIG_ERROR) + + return base_path, model_path + + def _run_benchmark_only(config: ForgeConfig, model_path: str, output_format: str) -> None: """Run benchmark evaluation on an existing model without training.""" from ..benchmark import run_benchmark @@ -28,48 +68,21 @@ def _run_benchmark_only(config: ForgeConfig, model_path: str, output_format: str bench_cfg = eval_cfg.benchmark - # Detect PEFT checkpoint: if the path contains adapter_config.json, load the - # base model from there and merge the adapter — otherwise load as a plain model. - adapter_cfg_path = os.path.join(model_path, "adapter_config.json") - if os.path.isfile(adapter_cfg_path): - # adapter_config.json is HF-produced JSON — explicit UTF-8 keeps the - # parse deterministic across Windows code-page locales. - try: - with open(adapter_cfg_path, encoding="utf-8") as f: - adapter_meta = json.load(f) - except (OSError, json.JSONDecodeError) as e: - logger.error("Failed to read PEFT adapter_config.json at %s: %s", adapter_cfg_path, e) - sys.exit(EXIT_CONFIG_ERROR) - base_path = adapter_meta.get("base_model_name_or_path") - if not base_path: - # Falling back to model_path here would re-load the adapter - # directory as a base model — it has no base weights, so - # PeftModel.from_pretrained would later crash with a confusing - # "config.json not found" error. Fail loud at the source. - logger.error( - "PEFT checkpoint at %s is missing 'base_model_name_or_path' in adapter_config.json — " - "cannot reconstruct the base model + adapter combination.", - model_path, - ) - sys.exit(EXIT_CONFIG_ERROR) + base_path, adapter_path = _resolve_benchmark_load_target(model_path) + if adapter_path: logger.info( - "Detected PEFT checkpoint. Loading base model %s + adapter %s for benchmark.", base_path, model_path - ) - model, tokenizer = load_model( - base_path, - adapter=model_path, - backend=config.model.backend, - load_in_4bit=config.model.load_in_4bit, - trust_remote_code=getattr(config.model, "trust_remote_code", False), + "Detected PEFT checkpoint. Loading base model %s + adapter %s for benchmark.", base_path, adapter_path ) else: - logger.info("Loading model from %s for benchmark evaluation...", model_path) - model, tokenizer = load_model( - model_path, - backend=config.model.backend, - load_in_4bit=config.model.load_in_4bit, - trust_remote_code=getattr(config.model, "trust_remote_code", False), - ) + logger.info("Loading model from %s for benchmark evaluation...", base_path) + + model, tokenizer = load_model( + base_path, + adapter=adapter_path, + backend=config.model.backend, + load_in_4bit=config.model.load_in_4bit, + trust_remote_code=getattr(config.model, "trust_remote_code", False), + ) output_dir = bench_cfg.output_dir or os.path.join(os.path.dirname(model_path), "benchmark") diff --git a/forgelm/inference.py b/forgelm/inference.py index c7141677..1e4258b9 100644 --- a/forgelm/inference.py +++ b/forgelm/inference.py @@ -20,6 +20,24 @@ # --------------------------------------------------------------------------- +def _pick_tokenizer_source(path: str, adapter: Optional[str]) -> str: + """Return the directory the tokenizer should be loaded from. + + Prefer the adapter directory when it carries ``tokenizer_config.json`` + — fine-tuning may have added special tokens or a custom chat template, + and loading the base tokenizer would silently drop those. HF saves + ``tokenizer_config.json`` next to the adapter weights when the trainer + called ``tokenizer.save_pretrained(adapter_dir)``. + """ + if adapter: + import os as _os + + if _os.path.isfile(_os.path.join(adapter, "tokenizer_config.json")): + logger.info("Using tokenizer from adapter directory: %s", adapter) + return adapter + return path + + def load_model( path: str, adapter: Optional[str] = None, @@ -61,18 +79,7 @@ def load_model( logger.info("Loading model for inference: %s", path) - # Prefer the adapter's tokenizer when the adapter directory carries one — - # fine-tuning may have added special tokens or a custom chat template and - # loading the base tokenizer would silently drop those. HF saves - # ``tokenizer_config.json`` next to the adapter weights when the trainer - # called ``tokenizer.save_pretrained(adapter_dir)``. - tokenizer_source = path - if adapter: - import os as _os - - if _os.path.isfile(_os.path.join(adapter, "tokenizer_config.json")): - tokenizer_source = adapter - logger.info("Using tokenizer from adapter directory: %s", adapter) + tokenizer_source = _pick_tokenizer_source(path, adapter) tokenizer = AutoTokenizer.from_pretrained(tokenizer_source, trust_remote_code=trust_remote_code) if tokenizer.pad_token is None: tokenizer.pad_token = tokenizer.eos_token diff --git a/forgelm/safety.py b/forgelm/safety.py index fcf9a800..343a687a 100644 --- a/forgelm/safety.py +++ b/forgelm/safety.py @@ -57,6 +57,23 @@ _PII_REDACT_FIELDS: frozenset[str] = frozenset({"prompt", "response"}) +@dataclass +class _CategoryTelemetry: + """Phase 9 Llama-Guard category + severity breakdown bundle. + + Consolidates the three category-related arguments to + :func:`_save_safety_results` so the function stays under + SonarQube's 13-parameter limit. ``track`` is the user-facing + SafetyConfig.track_categories switch; when False the + distribution dicts are ignored and the per-run JSON output omits + the breakdown blocks entirely. + """ + + track: bool + dist: Dict[str, int] + severity_dist: Dict[str, int] + + @dataclass class SafetyResult: """Result of a safety evaluation run.""" @@ -415,9 +432,7 @@ def _save_safety_results( passed: bool, failure_reason: Optional[str], details: List[Dict[str, Any]], - track_categories: bool, - category_dist: Dict[str, int], - severity_dist: Dict[str, int], + categories: _CategoryTelemetry, include_samples: bool = False, ) -> None: """Write the JSON summary plus the cross-run trend entry. @@ -440,9 +455,9 @@ def _save_safety_results( "failure_reason": failure_reason, "details": [{k: v for k, v in d.items() if k not in redact} for d in details], } - if track_categories: - output_data["category_distribution"] = category_dist - output_data["severity_distribution"] = severity_dist + if categories.track: + output_data["category_distribution"] = categories.dist + output_data["severity_distribution"] = categories.severity_dist with open(results_path, "w", encoding="utf-8") as f: json.dump(output_data, f, indent=2) logger.info("Safety results saved to %s", results_path) @@ -662,9 +677,11 @@ def run_safety_evaluation( passed=passed, failure_reason=failure_reason, details=details, - track_categories=thresholds.track_categories, - category_dist=category_dist, - severity_dist=severity_dist, + categories=_CategoryTelemetry( + track=thresholds.track_categories, + dist=category_dist, + severity_dist=severity_dist, + ), include_samples=include_samples, ) diff --git a/tests/test_safety_advanced.py b/tests/test_safety_advanced.py index 731913a9..ff4e4b21 100644 --- a/tests/test_safety_advanced.py +++ b/tests/test_safety_advanced.py @@ -278,7 +278,7 @@ def _sample_details(): ] def test_default_strips_prompt_and_response(self, tmp_path): - from forgelm.safety import _save_safety_results + from forgelm.safety import _CategoryTelemetry, _save_safety_results _save_safety_results( str(tmp_path), @@ -291,9 +291,7 @@ def test_default_strips_prompt_and_response(self, tmp_path): passed=False, failure_reason="one unsafe", details=self._sample_details(), - track_categories=False, - category_dist={}, - severity_dist={}, + categories=_CategoryTelemetry(track=False, dist={}, severity_dist={}), ) payload = json.loads((tmp_path / "safety_results.json").read_text()) for d in payload["details"]: @@ -303,7 +301,7 @@ def test_default_strips_prompt_and_response(self, tmp_path): assert "safe" in d def test_include_samples_keeps_all_fields(self, tmp_path): - from forgelm.safety import _save_safety_results + from forgelm.safety import _CategoryTelemetry, _save_safety_results _save_safety_results( str(tmp_path), @@ -316,9 +314,7 @@ def test_include_samples_keeps_all_fields(self, tmp_path): passed=False, failure_reason="one unsafe", details=self._sample_details(), - track_categories=False, - category_dist={}, - severity_dist={}, + categories=_CategoryTelemetry(track=False, dist={}, severity_dist={}), include_samples=True, ) payload = json.loads((tmp_path / "safety_results.json").read_text()) From fcbb926de859064951a7fd0637719406a1431114 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Thu, 14 May 2026 04:16:34 +0300 Subject: [PATCH 06/11] =?UTF-8?q?chore:=20post-PR-50=20housekeeping=20?= =?UTF-8?q?=E2=80=94=20agent-doc=20=5FEXEMPT=20entries=20+=20flaky=20test?= =?UTF-8?q?=20stabiliser?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cleanup items left over from the PR #50 merge that should land before the next phase opens: 1) tools/check_no_analysis_refs.py: register the agent-agnostic mirror files (AGENTS.md and .agents/skills/sync-bilingual-docs/ SKILL.md) in _EXEMPT. They are functional path filters / agent- guidance documents that legitimately name the gitignored docs/marketing/ + docs/analysis/ directories, mirroring the existing CLAUDE.md and .claude/skills/sync-bilingual-docs/SKILL.md exemptions. Without this, the local 11-guard self-review gauntlet fails on development after PR #50 introduced these files in commit 8d678fa. 2) tests/test_inference.py::test_unsloth_backend_raises_without_package: refactored to call _load_unsloth directly with a sys.modules['unsloth'] = None monkeypatch, instead of going through load_model() with nested MagicMock stubs for torch / transformers. The previous pattern was order-sensitive: after the tokenizer-from-adapter tests populated sys.modules with stubs and torn them down, a follow-up real `import transformers` would hit ValueError on torch.__spec__ or AttributeError on torch.__version__ via the safetensors transitive import. Direct call exercises the same failure surface (`from unsloth import ...` raises ImportError) without the dispatcher's import zinciri. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/test_inference.py | 37 +++++++++++++++++++++++++-------- tools/check_no_analysis_refs.py | 6 ++++++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/tests/test_inference.py b/tests/test_inference.py index 83a63883..19903323 100644 --- a/tests/test_inference.py +++ b/tests/test_inference.py @@ -513,15 +513,34 @@ def test_tokenizer_falls_back_to_base_when_adapter_has_no_tokenizer_config(self, called_paths = [call.args[0] for call in transformers_stub.AutoTokenizer.from_pretrained.call_args_list] assert "org/base-model" in called_paths - def test_unsloth_backend_raises_without_package(self): - torch_stub = MagicMock() - torch_stub.cuda.is_available.return_value = False - - with patch.dict(sys.modules, {"torch": torch_stub, "unsloth": None}): - from forgelm.inference import load_model - - with pytest.raises(ImportError): - load_model("org/model", backend="unsloth") + def test_unsloth_backend_raises_without_package(self, monkeypatch): + """``_load_unsloth`` must surface a clear ImportError when the + ``unsloth`` package is not installed. + + Calls ``_load_unsloth`` directly instead of going through + :func:`load_model`: the dispatcher imports ``torch`` and + ``transformers`` at function-entry, and chasing the entire + transitive-dep MagicMock chain (``torch.__spec__``, + ``torch.__version__`` for safetensors, etc.) makes the test + order-sensitive against other TestLoadModel tests that populate + ``sys.modules`` with stubs. The dispatcher is exercised by + ``test_basic_load_no_adapter`` and ``test_adapter_is_merged``; + this test owns the "unsloth not installed" failure surface only. + """ + # Make ``from unsloth import FastLanguageModel`` raise — None in + # sys.modules is the standard sentinel that triggers ImportError. + monkeypatch.setitem(sys.modules, "unsloth", None) + + from forgelm.inference import _load_unsloth + + with pytest.raises(ImportError, match="unsloth"): + _load_unsloth( + "org/model", + adapter=None, + trust_remote_code=False, + load_in_4bit=False, + load_in_8bit=False, + ) # --------------------------------------------------------------------------- diff --git a/tools/check_no_analysis_refs.py b/tools/check_no_analysis_refs.py index cdfd91bd..40a14a20 100644 --- a/tools/check_no_analysis_refs.py +++ b/tools/check_no_analysis_refs.py @@ -104,6 +104,9 @@ ".gitignore": frozenset({"docs/marketing/", "docs/analysis/"}), # CLAUDE.md carries the policy statement that names the directories. "CLAUDE.md": frozenset({"docs/marketing/", "docs/analysis/"}), + # AGENTS.md is the agent-agnostic mirror of CLAUDE.md (Codex / Cursor / + # generic AI-agent surface); it restates the same gitignored-dir policy. + "AGENTS.md": frozenset({"docs/marketing/", "docs/analysis/"}), # The standards file IS the rule about these directories. "docs/standards/documentation.md": frozenset({"docs/marketing/", "docs/analysis/"}), # Localization standard names docs/marketing as a mixed-language @@ -111,6 +114,9 @@ "docs/standards/localization.md": frozenset({"docs/marketing/"}), # The skill that warns away from these dirs needs to name them. ".claude/skills/sync-bilingual-docs/SKILL.md": frozenset({"docs/marketing/", "docs/analysis/"}), + # Agent-agnostic mirror of the .claude/skills/ tree (same content, + # served to non-Claude agents). + ".agents/skills/sync-bilingual-docs/SKILL.md": frozenset({"docs/marketing/", "docs/analysis/"}), # Production code path filters (functional, not citations). "tools/check_anchor_resolution.py": frozenset({'"analysis"', "analysis/"}), "tools/check_yaml_snippets.py": frozenset( From 6515a26425c5d910e4a83282b2e0a4ead03883dd Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Thu, 14 May 2026 04:42:06 +0300 Subject: [PATCH 07/11] security: close DNS-rebinding TOCTOU in SSRF guard (issue #14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-fix flow: 1. _is_private_destination(host) calls socket.getaddrinfo → public IP 2. requests.post(host_url, ...) → connect-time DNS lookup 3. If the attacker's DNS server flipped the answer between steps 1 and 2 (TTL=0 + rebinding), the connect lands on 127.0.0.1 / 169.254.169.254 / RFC1918 — payload + bearer token leak. Hardening: - New helper `_resolve_safe_destination(host)` resolves exactly once and returns either a public IP literal or a block reason. - safe_post / safe_get rebuild the outbound URL with that IP literal so requests never re-resolves. - Original hostname is propagated via the `Host` header (mandatory for virtual-hosted endpoints like Slack / Teams / Discord) and via SNI through requests_toolbelt's HostHeaderSSLAdapter, mounted on a per-call Session for the https:// scheme. - allow_private=True callers keep the legacy requests.post flow so in-cluster / split-horizon DNS still works for operator-blessed internal destinations. - IPv6 IP literals are bracketed per RFC 3986 ([2001:db8::1]:443). Threat model: attacker needs control of the DNS server for the operator-configured webhook domain (or a downstream resolver they can poison). allow_private_destinations=false default already forced the operator to opt in to private destinations, so DNS rebinding was the last remaining bypass; this fix closes it for the default-secure path. Dependency: requests-toolbelt>=1.0.0,<2.0.0 added as a hard dep. Small (~30KB), same ecosystem as requests, security-critical so an opt-in extra would weaken the default-secure posture. Tests: - tests/test_http_dns_rebinding.py (new, 13 tests): - _resolve_safe_destination helper coverage (6 unit tests: public hostname, mixed-result blocking, DNS failure, empty host, public IP literal, private IP literal). - DNS rebinding behavioural test asserting getaddrinfo is called exactly once and the URL is rebuilt with the public IP. - Host header preservation with auth header round-trip. - HostHeaderSSLAdapter mounted on https:// scheme. - HTTP path does not need the SSL adapter. - allow_private=True bypass exercises the legacy path. - IPv6 literal bracketed in the rebuilt URL. - safe_get mirrors safe_post for the same hardening. - Updated existing webhook / judge / synthetic tests for the Session.post mock pattern (16 occurrences across 3 files). Two tests (test_url_env_resolution, test_custom_api_base, test_api_base_reaches_http_call) reframed to assert on the `Host` header rather than the URL because the URL now carries the IP literal. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 22 +++ forgelm/_http.py | 225 ++++++++++++++++++++++++---- pyproject.toml | 6 + tests/test_http_dns_rebinding.py | 245 +++++++++++++++++++++++++++++++ tests/test_judge_functions.py | 52 +++++-- tests/test_synthetic.py | 4 +- tests/test_webhook.py | 61 +++++--- 7 files changed, 546 insertions(+), 69 deletions(-) create mode 100644 tests/test_http_dns_rebinding.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 180d9cbb..e63de8d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,28 @@ All notable changes to ForgeLM are documented here. _(v0.6.1 dev cycle — entries will land here as PRs merge.)_ +### Security + +- **Webhook / judge / synthetic SSRF guard — DNS-rebinding TOCTOU + hardening (issue #14).** Pre-fix, `_is_private_destination()` + pre-resolved the hostname and `requests.post()` then ran its own + DNS lookup at connect time; an attacker-controlled DNS server with + TTL=0 could return a public IP on the first lookup (passing the + guard) and a private IP on the second (when `requests` connected), + leaking the payload + bearer token to a private destination + (loopback, RFC1918, or cloud IMDS at `169.254.169.254`). New + `_resolve_safe_destination()` helper resolves the hostname exactly + once and `safe_post` / `safe_get` rebuild the outbound URL with the + returned public IP literal so `requests` never re-resolves. The + original hostname is preserved via the `Host` header (and SNI for + HTTPS) using + `requests_toolbelt.adapters.host_header_ssl.HostHeaderSSLAdapter` + so virtual-hosted endpoints (Slack / Teams / Discord) and + certificate validation still work correctly. `allow_private=True` + callers (operator-blessed internal destinations) keep the legacy + flow so split-horizon DNS / in-cluster resolution still works. + `requests-toolbelt>=1.0.0,<2.0.0` is now a hard dependency. + ## [0.6.0] — 2026-05-11 Phase 15 (Ingestion Pipeline Reliability) Wave 1 + Wave 2. Closes the diff --git a/forgelm/_http.py b/forgelm/_http.py index e9855d94..092af3aa 100644 --- a/forgelm/_http.py +++ b/forgelm/_http.py @@ -45,8 +45,8 @@ import ipaddress import logging import socket -from typing import Any, Dict, Optional -from urllib.parse import urlparse +from typing import Any, Dict, Optional, Tuple +from urllib.parse import urlparse, urlunparse import requests @@ -108,6 +108,133 @@ def _is_private_destination(host: str) -> bool: return False +# --------------------------------------------------------------------------- +# Issue #14 — DNS-rebinding-safe destination resolver +# --------------------------------------------------------------------------- +# +# ``_is_private_destination`` above pre-resolves the host and answers a +# yes/no question, but the actual ``requests.post`` call then does its OWN +# DNS lookup at connect time. Between those two lookups an attacker- +# controlled DNS server can flip the answer (TTL=0 + rebinding to +# 127.0.0.1 / 169.254.169.254), letting the payload + bearer token leak +# to a private destination after passing the guard. The resolver below +# closes that window by returning a concrete public IP literal that the +# call site uses to build the URL — requests then connects to that IP +# without a second DNS round-trip. + + +def _resolve_safe_destination(host: str) -> Tuple[Optional[str], Optional[str]]: + """Resolve *host* to a single public IP literal. + + Returns ``(ip, None)`` when the host is a public destination, or + ``(None, reason)`` when the destination is blocked (private/loopback/ + IMDS/multicast IP, empty host, or DNS resolution failure). + + The caller substitutes the returned IP literal into the request URL + so ``requests`` does not re-resolve the hostname — this is the fix + for the DNS-rebinding TOCTOU described in issue #14. The original + hostname must be propagated through the ``Host`` header (and SNI for + HTTPS) by the caller; this helper only owns the resolve+validate + step. + + The picked IP is deterministic (first public address returned by + ``getaddrinfo``) so the test harness can mock the resolver without + flakiness. When the host is already an IP literal, it is returned + as-is after the same private-range check. + """ + if not host: + return None, "empty host" + + # IP literal short-circuit — no DNS needed, same private-range filter. + try: + literal = ipaddress.ip_address(host) + except ValueError: + literal = None + if literal is not None: + if ( + literal.is_private + or literal.is_loopback + or literal.is_link_local + or literal.is_reserved + or literal.is_multicast + ): + return None, "Private/loopback/IMDS destination" + return host, None + + # Hostname path — one resolve, validate every returned address. A + # mixed result (some public, some private) is treated as blocked + # because a follow-up connect could land on the private one. + try: + addrinfo = socket.getaddrinfo(host, None) + except (socket.gaierror, UnicodeError) as e: + return None, f"DNS resolution failed: {e}" + + public_ip: Optional[str] = None + for _family, _type, _proto, _canon, sockaddr in addrinfo: + candidate = sockaddr[0] + try: + resolved = ipaddress.ip_address(candidate) + except ValueError: + continue + if ( + resolved.is_private + or resolved.is_loopback + or resolved.is_link_local + or resolved.is_reserved + or resolved.is_multicast + ): + return None, "Private/loopback/IMDS destination" + if public_ip is None: + public_ip = candidate + if public_ip is None: + return None, "no public IP resolved" + return public_ip, None + + +def _build_pinned_url(parsed_url, ip: str) -> str: + """Rebuild *parsed_url* with the hostname replaced by *ip*. + + Preserves scheme, port, path, query, and fragment. IPv6 literals + are bracketed per RFC 3986 (``https://[2001:db8::1]:443/path``). + Userinfo is intentionally dropped — webhook URLs may carry a token + in the path but never in userinfo, and stripping it removes one + avenue for accidental credential exposure in logs. + """ + try: + is_ipv6 = ipaddress.ip_address(ip).version == 6 + except ValueError: + is_ipv6 = False + netloc = f"[{ip}]" if is_ipv6 else ip + if parsed_url.port: + netloc = f"{netloc}:{parsed_url.port}" + return urlunparse( + ( + parsed_url.scheme, + netloc, + parsed_url.path, + parsed_url.params, + parsed_url.query, + parsed_url.fragment, + ) + ) + + +def _pinned_session(scheme: str) -> requests.Session: + """Return a ``requests.Session`` configured for IP-literal connections. + + For HTTPS, mounts ``requests_toolbelt.adapters.host_header_ssl. + HostHeaderSSLAdapter`` so the SNI handshake and certificate + validation are performed against the original hostname (passed in + the ``Host`` header) rather than the IP literal in the URL. + """ + session = requests.Session() + if scheme == "https": + from requests_toolbelt.adapters.host_header_ssl import HostHeaderSSLAdapter + + session.mount("https://", HostHeaderSSLAdapter()) + return session + + def _mask_netloc(url: str) -> str: """Return ``scheme://host`` with userinfo / path / query stripped. @@ -206,10 +333,23 @@ def safe_post( elif parsed.scheme != "https": raise HttpSafetyError(f"Unsupported URL scheme {parsed.scheme!r}; only http(s) allowed.") - # SSRF guard. + # SSRF guard — issue #14: resolve once to a public IP literal so the + # connect-time DNS lookup cannot flip the verdict (DNS rebinding / + # TOCTOU). When ``allow_private=True`` the operator has explicitly + # opted into an internal/in-cluster destination; the legacy URL is + # used as-is so internal DNS / split-horizon resolution still works. host = parsed.hostname or "" - if not allow_private and _is_private_destination(host): - raise HttpSafetyError(f"Private/loopback/IMDS destination blocked: host={host or ''}") + target_url = url + request_headers: Dict[str, str] = dict(headers or {}) + if not allow_private: + pinned_ip, block_reason = _resolve_safe_destination(host) + if block_reason: + raise HttpSafetyError(f"{block_reason} blocked: host={host or ''}") + target_url = _build_pinned_url(parsed, pinned_ip) + # ``Host`` is case-insensitive in HTTP/1.1; set it without + # overwriting an explicit operator override for testing/proxy + # scenarios. + request_headers.setdefault("Host", host) # Timeout floor — `requests` treats 0 / None as "no timeout" which can # hang the trainer on a dead endpoint. @@ -220,18 +360,31 @@ def safe_post( verify_param: Any = ca_bundle if ca_bundle else verify try: - return requests.post( - url, - json=json, - data=data, - headers=headers, - timeout=timeout, - verify=verify_param, - # Redirect-following would bypass the up-front SSRF check — - # a 30x to 169.254.169.254 from an attacker-controlled host - # would otherwise leak the request payload to IMDS. - allow_redirects=False, - ) + if allow_private: + # Legacy path: operator opted into a private destination; + # internal DNS / split-horizon resolution is the right model. + return requests.post( + url, + json=json, + data=data, + headers=request_headers, + timeout=timeout, + verify=verify_param, + # Redirect-following would bypass the up-front SSRF check — + # a 30x to 169.254.169.254 from an attacker-controlled host + # would otherwise leak the request payload to IMDS. + allow_redirects=False, + ) + with _pinned_session(parsed.scheme) as session: + return session.post( + target_url, + json=json, + data=data, + headers=request_headers, + timeout=timeout, + verify=verify_param, + allow_redirects=False, + ) except requests.RequestException as exc: masked_reason = _mask_secrets_in_text(str(exc), headers) logger.warning( @@ -305,10 +458,16 @@ def safe_get( elif parsed.scheme != "https": raise HttpSafetyError(f"Unsupported URL scheme {parsed.scheme!r}; only http(s) allowed.") - # SSRF guard. + # SSRF guard — see safe_post for the issue-#14 DNS-rebinding rationale. host = parsed.hostname or "" - if not allow_private and _is_private_destination(host): - raise HttpSafetyError(f"Private/loopback/IMDS destination blocked: host={host or ''}") + target_url = url + request_headers: Dict[str, str] = dict(headers or {}) + if not allow_private: + pinned_ip, block_reason = _resolve_safe_destination(host) + if block_reason: + raise HttpSafetyError(f"{block_reason} blocked: host={host or ''}") + target_url = _build_pinned_url(parsed, pinned_ip) + request_headers.setdefault("Host", host) # Timeout floor. if not isinstance(timeout, (int, float)) or timeout < min_timeout: @@ -322,14 +481,24 @@ def safe_get( verify_param: Any = ca_bundle if ca_bundle else verify try: - return requests.request( - method_upper, - url, - headers=headers, - timeout=timeout, - verify=verify_param, - allow_redirects=False, - ) + if allow_private: + return requests.request( + method_upper, + url, + headers=request_headers, + timeout=timeout, + verify=verify_param, + allow_redirects=False, + ) + with _pinned_session(parsed.scheme) as session: + return session.request( + method_upper, + target_url, + headers=request_headers, + timeout=timeout, + verify=verify_param, + allow_redirects=False, + ) except requests.RequestException as exc: masked_reason = _mask_secrets_in_text(str(exc), headers) logger.warning( diff --git a/pyproject.toml b/pyproject.toml index 94c17098..c81d2bc5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -58,6 +58,12 @@ dependencies = [ "tensorboard>=2.15.0,<3.0.0", "huggingface_hub>=0.23.2,<1.0.0", "requests>=2.31.0,<3.0.0", + # requests-toolbelt is required by forgelm._http for the HostHeaderSSLAdapter + # used to close the DNS-rebinding TOCTOU window in safe_post / safe_get + # (issue #14). Small (~30KB) and from the same requests ecosystem, so it's + # a hard dep rather than an optional extra — SSRF hardening must always be + # active, not opt-in. + "requests-toolbelt>=1.0.0,<2.0.0", ] [project.optional-dependencies] diff --git a/tests/test_http_dns_rebinding.py b/tests/test_http_dns_rebinding.py new file mode 100644 index 00000000..dce8bdd6 --- /dev/null +++ b/tests/test_http_dns_rebinding.py @@ -0,0 +1,245 @@ +"""Issue #14 regression tests — DNS-rebinding TOCTOU hardening for the +webhook / judge / synthetic SSRF guard. + +Pre-fix: ``_is_private_destination()`` ran a DNS lookup, then +``requests.post()`` ran ANOTHER DNS lookup at connect time. An +attacker-controlled DNS server with TTL=0 could return a public IP on +the first lookup (passing the guard) and a private IP on the second +(when ``requests`` connected), leaking the payload + bearer token to a +private destination. + +Post-fix: ``_resolve_safe_destination()`` resolves the hostname once and +the call site reuses the returned IP literal in the URL. The original +hostname is propagated via the ``Host`` header and (for HTTPS) the SNI +extension of ``requests_toolbelt.adapters.host_header_ssl. +HostHeaderSSLAdapter``, so virtual-hosting endpoints and certificate +validation still work. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + + +class TestResolveSafeDestination: + """Unit-level coverage of the resolver helper.""" + + def test_public_hostname_returns_first_public_ip(self): + from forgelm import _http + + # Two public-A addrinfo entries. + with patch.object( + _http.socket, + "getaddrinfo", + return_value=[ + (0, 0, 0, "", ("8.8.8.8", 0)), + (0, 0, 0, "", ("8.8.4.4", 0)), + ], + ): + ip, err = _http._resolve_safe_destination("hooks.example.com") + assert err is None + assert ip == "8.8.8.8" + + def test_private_ip_in_resolution_blocks(self): + """Even one private answer in addrinfo flips the verdict to blocked.""" + from forgelm import _http + + with patch.object( + _http.socket, + "getaddrinfo", + return_value=[ + (0, 0, 0, "", ("8.8.8.8", 0)), + (0, 0, 0, "", ("10.0.0.1", 0)), # NOSONAR RFC1918 — guard fixture + ], + ): + ip, err = _http._resolve_safe_destination("attacker.example.com") + assert ip is None + assert "Private" in err and "IMDS" in err + + def test_dns_failure_blocks_with_reason(self): + from forgelm import _http + + with patch.object(_http.socket, "getaddrinfo", side_effect=_http.socket.gaierror("nodename")): + ip, err = _http._resolve_safe_destination("no-such.example.com") + assert ip is None + assert err.startswith("DNS resolution failed") + + def test_empty_host_blocks(self): + from forgelm._http import _resolve_safe_destination + + ip, err = _resolve_safe_destination("") + assert ip is None + assert err == "empty host" + + def test_public_ip_literal_passes_through(self): + from forgelm._http import _resolve_safe_destination + + ip, err = _resolve_safe_destination("8.8.8.8") + assert err is None + assert ip == "8.8.8.8" + + def test_private_ip_literal_blocks(self): + from forgelm._http import _resolve_safe_destination + + ip, err = _resolve_safe_destination("169.254.169.254") # NOSONAR AWS IMDS — guard fixture + assert ip is None + assert "Private" in err + + +class TestDnsRebindingClosed: + """Behavioural test: a TOCTOU-style rebinding cannot leak the payload. + + Simulates a DNS server that returns a public IP on the first lookup + (the guard's call) and a private IP on a hypothetical second lookup + (what the old code path would have invoked from inside requests). + The hardened path must call ``getaddrinfo`` exactly once and pin the + public IP into the outbound URL. + """ + + def test_getaddrinfo_called_exactly_once_and_pins_public_ip(self): + from forgelm import _http + + # First call: public. If the code ever called a second time + # (the TOCTOU window), it would get the IMDS address. The + # assertion below asserts the second call never happens. + responses = iter( + [ + [(0, 0, 0, "", ("8.8.8.8", 0))], # 1st call — public + [(0, 0, 0, "", ("169.254.169.254", 0))], # 2nd call — would be IMDS # NOSONAR + ] + ) + + def fake_resolve(*_args, **_kwargs): + try: + return next(responses) + except StopIteration: + pytest.fail("getaddrinfo called more than once — DNS rebinding window is still open") + + with ( + patch.object(_http.socket, "getaddrinfo", side_effect=fake_resolve) as resolve_mock, + patch.object(_http.requests.Session, "post") as mock_post, + ): + mock_post.return_value = MagicMock(ok=True, status_code=200) + _http.safe_post("https://hooks.example.com/abc", json={}, timeout=10.0) + + assert resolve_mock.call_count == 1, ( + f"DNS rebinding fix requires exactly one resolve per safe_post call, got {resolve_mock.call_count}" + ) + # The URL handed to Session.post must be the IP literal, not the hostname. + called_url = mock_post.call_args.args[0] if mock_post.call_args.args else mock_post.call_args.kwargs.get("url") + assert called_url == "https://8.8.8.8/abc", ( + f"safe_post must rebuild the URL with the resolved public IP literal; got {called_url!r}" + ) + + def test_host_header_preserved_after_ip_pin(self): + """The original hostname must travel via ``Host`` header so the + upstream virtual host receives the right request line.""" + from forgelm import _http + + with ( + patch.object(_http.socket, "getaddrinfo", return_value=[(0, 0, 0, "", ("8.8.8.8", 0))]), + patch.object(_http.requests.Session, "post") as mock_post, + ): + mock_post.return_value = MagicMock(ok=True, status_code=200) + _http.safe_post( + "https://hooks.example.com/abc", + json={}, + headers={"Authorization": "Bearer secret"}, # noqa: S105 — test fixture + timeout=10.0, + ) + + headers = mock_post.call_args.kwargs["headers"] + assert headers["Host"] == "hooks.example.com" + # The opt-in auth header must be preserved alongside Host. + assert headers["Authorization"] == "Bearer secret" + + def test_https_uses_host_header_ssl_adapter(self): + """For HTTPS the Session must mount HostHeaderSSLAdapter so SNI + and cert validation are performed against the original hostname, + not the IP literal in the URL.""" + from forgelm import _http + + with patch.object(_http.socket, "getaddrinfo", return_value=[(0, 0, 0, "", ("8.8.8.8", 0))]): + session = _http._pinned_session("https") + # Ensure an https://-mounted adapter exists and is the HostHeaderSSLAdapter. + adapter = session.get_adapter("https://example.com/") + from requests_toolbelt.adapters.host_header_ssl import HostHeaderSSLAdapter + + assert isinstance(adapter, HostHeaderSSLAdapter) + + def test_http_session_does_not_mount_ssl_adapter(self): + """The HTTP branch (operator opt-in via allow_insecure_http) must + not need the SSL adapter — no SNI involved.""" + from forgelm import _http + + session = _http._pinned_session("http") + # Default HTTPAdapter, not HostHeaderSSLAdapter, on the http:// prefix. + adapter = session.get_adapter("http://example.com/") + from requests_toolbelt.adapters.host_header_ssl import HostHeaderSSLAdapter + + assert not isinstance(adapter, HostHeaderSSLAdapter) + + def test_allow_private_bypasses_pinning(self): + """``allow_private=True`` is the documented in-cluster/internal + destination opt-in; the legacy ``requests.post`` flow runs so + internal DNS / split-horizon resolution still works.""" + from forgelm import _http + + with ( + patch.object(_http.socket, "getaddrinfo") as resolve_mock, + patch.object(_http.requests, "post") as mock_post, + patch.object(_http.requests.Session, "post") as session_post_mock, + ): + mock_post.return_value = MagicMock(ok=True, status_code=200) + _http.safe_post( + "https://internal.corp.local/hook", + json={}, + timeout=10.0, + allow_private=True, + ) + + resolve_mock.assert_not_called() # No DNS pre-resolve in the opt-in path + session_post_mock.assert_not_called() # No Session.post either + mock_post.assert_called_once() # Legacy requests.post path + + +class TestIpv6PinningBuildsBracketedUrl: + """IPv6 IP literals must be bracketed in the rebuilt URL per RFC 3986.""" + + def test_ipv6_url_is_bracketed(self): + from forgelm import _http + + with ( + patch.object(_http.socket, "getaddrinfo", return_value=[(0, 0, 0, "", ("2606:4700:4700::1111", 0))]), + patch.object(_http.requests.Session, "post") as mock_post, + ): + mock_post.return_value = MagicMock(ok=True, status_code=200) + _http.safe_post("https://v6.example.com/abc", json={}, timeout=10.0) + + called_url = mock_post.call_args.args[0] + assert called_url == "https://[2606:4700:4700::1111]/abc" + + +class TestSafeGetPinning: + """``safe_get`` mirrors ``safe_post`` for the same hardening contract.""" + + def test_safe_get_pins_url_and_sets_host_header(self): + from forgelm import _http + + with ( + patch.object(_http.socket, "getaddrinfo", return_value=[(0, 0, 0, "", ("8.8.8.8", 0))]), + patch.object(_http.requests.Session, "request") as mock_request, + ): + mock_request.return_value = MagicMock(ok=True, status_code=200) + _http.safe_get("https://hub.example.com/api/models", timeout=10.0) + + # Session.request("GET", url, ...) + method = mock_request.call_args.args[0] + url = mock_request.call_args.args[1] + headers = mock_request.call_args.kwargs["headers"] + + assert method == "GET" + assert url == "https://8.8.8.8/api/models" + assert headers["Host"] == "hub.example.com" diff --git a/tests/test_judge_functions.py b/tests/test_judge_functions.py index e5d330fc..313634cc 100644 --- a/tests/test_judge_functions.py +++ b/tests/test_judge_functions.py @@ -57,7 +57,7 @@ def test_multiple_code_blocks(self): class TestCallApiJudge: - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_successful_api_call(self, mock_post): mock_response = MagicMock() mock_response.json.return_value = {"choices": [{"message": {"content": '{"score": 8, "reason": "Good"}'}}]} @@ -70,7 +70,7 @@ def test_successful_api_call(self, mock_post): assert result["score"] == 8 mock_post.assert_called_once() - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_api_timeout(self, mock_post): import requests @@ -83,8 +83,17 @@ def test_api_timeout(self, mock_post): assert result["score"] is None assert "API error" in result["reason"] - @patch("forgelm._http.requests.post") - def test_custom_api_base(self, mock_post): + @patch("forgelm._http._resolve_safe_destination", return_value=("8.8.8.8", None)) + @patch("forgelm._http.requests.Session.post") + def test_custom_api_base(self, mock_post, _mock_resolve): + """``api_base`` override drives the request to ``custom.api``. + + Post-issue-#14 the URL passed to ``Session.post`` is rebuilt with + the resolved IP literal, so we assert on the ``Host`` header (which + carries the original hostname) instead of the URL string itself. + DNS is mocked so the test does not require live resolution of + the synthetic ``custom.api`` fixture domain. + """ mock_response = MagicMock() mock_response.json.return_value = {"choices": [{"message": {"content": '{"score": 7, "reason": "OK"}'}}]} mock_response.raise_for_status = MagicMock() @@ -94,7 +103,10 @@ def test_custom_api_base(self, mock_post): _call_api_judge("prompt", "key", "model", api_base="https://custom.api/v1/chat") call_args = mock_post.call_args - assert call_args[0][0] == "https://custom.api/v1/chat" + headers = call_args.kwargs.get("headers") or {} + assert headers.get("Host") == "custom.api", ( + f"Host header should reflect the custom api_base hostname; got {headers!r}" + ) class TestJudgeResult: @@ -108,7 +120,7 @@ def test_defaults(self): @pytest.mark.skipif(not torch_available, reason="torch not installed") class TestJudgeScoreClipping: - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_score_above_10_clipped_to_10(self, mock_post, caplog): """Scores above 10 must be clamped to 10.0 with a warning.""" import logging @@ -129,7 +141,7 @@ def test_score_above_10_clipped_to_10(self, mock_post, caplog): # _call_api_judge returns the raw parsed value. assert result["score"] == 15 - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_score_clipped_in_run_judge_evaluation(self, mock_post, tmp_path, caplog): """run_judge_evaluation must clip out-of-range scores and emit a warning.""" import logging @@ -177,7 +189,7 @@ def test_score_clipped_in_run_judge_evaluation(self, mock_post, tmp_path, caplog # Warning must be emitted assert any("clipped" in r.message or "out-of-range" in r.message for r in caplog.records) - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_score_below_1_clipped_to_1(self, mock_post, tmp_path): """Scores below 1 must be clamped to 1.0.""" import torch @@ -215,9 +227,16 @@ def test_score_below_1_clipped_to_1(self, mock_post, tmp_path): class TestJudgeApiBasePassthrough: - @patch("forgelm._http.requests.post") - def test_api_base_reaches_http_call(self, mock_post): - """judge_api_base in config must be forwarded to the HTTP POST call.""" + @patch("forgelm._http._resolve_safe_destination", return_value=("8.8.8.8", None)) + @patch("forgelm._http.requests.Session.post") + def test_api_base_reaches_http_call(self, mock_post, _mock_resolve): + """``judge_api_base`` must be forwarded to the HTTP POST. + + Asserts via the ``Host`` header because issue-#14 hardening rebuilds + the URL with the resolved IP literal before the actual call — + carrying the original hostname over to the request via the + ``Host`` header (and SNI for HTTPS). + """ mock_response = MagicMock() mock_response.json.return_value = {"choices": [{"message": {"content": '{"score": 7, "reason": "OK"}'}}]} mock_response.raise_for_status = MagicMock() @@ -229,9 +248,10 @@ def test_api_base_reaches_http_call(self, mock_post): _call_api_judge("prompt", "key", "model", api_base=custom_base) call_args = mock_post.call_args - actual_url = call_args[0][0] if call_args[0] else call_args.kwargs.get("url") or call_args[1].get("url") - # The URL passed to requests.post should match the custom api_base - assert actual_url == custom_base + headers = call_args.kwargs.get("headers") or {} + assert headers.get("Host") == "custom.llm.api", ( + f"Host header should reflect the custom judge_api_base hostname; got {headers!r}" + ) class TestJudgeUsesSafePost: @@ -252,7 +272,7 @@ def test_imports_safe_post(self): src = inspect.getsource(judge._call_api_judge) assert "safe_post" in src, "judge._call_api_judge must use safe_post" - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_judge_call_goes_through_safe_post(self, mock_post): """A successful judge call must hit requests.post (via safe_post).""" mock_response = MagicMock() @@ -271,7 +291,7 @@ def test_judge_call_goes_through_safe_post(self, mock_post): assert kwargs.get("allow_redirects") is False assert result["score"] == 7 - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_judge_ssrf_block_for_private_url(self, mock_post): """A private-IP api_base must be rejected before any network call. diff --git a/tests/test_synthetic.py b/tests/test_synthetic.py index 5a3d7a1f..5ea576b7 100644 --- a/tests/test_synthetic.py +++ b/tests/test_synthetic.py @@ -285,7 +285,7 @@ def test_imports_safe_post(self): src = inspect.getsource(synthetic.SyntheticDataGenerator._call_api_teacher) assert "safe_post" in src, "synthetic._call_api_teacher must use safe_post" - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_synthetic_call_goes_through_safe_post(self, mock_post): """A successful API teacher call routes through safe_post → requests.post.""" mock_response = MagicMock() @@ -313,7 +313,7 @@ def test_synthetic_call_goes_through_safe_post(self, mock_post): # safe_post forwards allow_redirects=False assert kwargs.get("allow_redirects") is False - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_synthetic_ssrf_block_for_private_api_base(self, mock_post): """A private-IP api_base must be rejected before any network call.""" from forgelm._http import HttpSafetyError diff --git a/tests/test_webhook.py b/tests/test_webhook.py index 833e8c76..863a4f57 100644 --- a/tests/test_webhook.py +++ b/tests/test_webhook.py @@ -39,7 +39,7 @@ def test_no_url(self): notifier = WebhookNotifier(config) notifier.notify_start(run_name="test") - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_notify_start(self, mock_post): config = _make_config({"url": "https://example.com/hook"}) notifier = WebhookNotifier(config) @@ -52,7 +52,7 @@ def test_notify_start(self, mock_post): assert payload["status"] == "started" assert payload["run_name"] == "my_model_finetune" - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_notify_success_with_metrics(self, mock_post): config = _make_config({"url": "https://example.com/hook"}) notifier = WebhookNotifier(config) @@ -64,7 +64,7 @@ def test_notify_success_with_metrics(self, mock_post): assert payload["event"] == "training.success" assert payload["metrics"]["eval_loss"] == pytest.approx(1.25) - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_notify_failure_with_reason(self, mock_post): config = _make_config({"url": "https://example.com/hook"}) notifier = WebhookNotifier(config) @@ -75,8 +75,17 @@ def test_notify_failure_with_reason(self, mock_post): assert payload["event"] == "training.failure" assert payload["reason"] == "OOM error" - @patch("forgelm._http.requests.post") - def test_url_env_resolution(self, mock_post): + @patch("forgelm._http._resolve_safe_destination", return_value=("8.8.8.8", None)) + @patch("forgelm._http.requests.Session.post") + def test_url_env_resolution(self, mock_post, _mock_resolve): + """``url_env`` resolution must drive the request to env.example.com. + + Post-issue-#14 the URL passed to ``Session.post`` is rebuilt with the + resolved IP literal, so we assert via the ``Host`` header (which + carries the original hostname) instead of the URL itself. DNS is + mocked because ``env.example.com`` does not resolve in real life + (IANA only publishes A records for ``example.com`` itself). + """ config = _make_config({"url_env": "TEST_WEBHOOK_URL"}) notifier = WebhookNotifier(config) @@ -85,12 +94,12 @@ def test_url_env_resolution(self, mock_post): mock_post.assert_called_once() call_kwargs = mock_post.call_args - assert ( - call_kwargs.kwargs.get("url") == "https://env.example.com/hook" - or call_kwargs[0][0] == "https://env.example.com/hook" + headers = call_kwargs.kwargs.get("headers") or {} + assert headers.get("Host") == "env.example.com", ( + f"Host header should reflect the resolved env-var hostname; got {headers!r}" ) - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_notify_on_start_disabled(self, mock_post): config = _make_config( { @@ -102,7 +111,7 @@ def test_notify_on_start_disabled(self, mock_post): notifier.notify_start(run_name="test") mock_post.assert_not_called() - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_timeout_handled_gracefully(self, mock_post): import requests as req @@ -112,7 +121,7 @@ def test_timeout_handled_gracefully(self, mock_post): # Should not raise notifier.notify_start(run_name="test") - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_connection_error_handled_gracefully(self, mock_post): import requests as req @@ -122,7 +131,7 @@ def test_connection_error_handled_gracefully(self, mock_post): # Should not raise notifier.notify_failure(run_name="test", reason="test error") - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_payload_has_slack_attachments(self, mock_post): config = _make_config({"url": "https://example.com/hook"}) notifier = WebhookNotifier(config) @@ -134,7 +143,7 @@ def test_payload_has_slack_attachments(self, mock_post): assert len(payload["attachments"]) == 1 assert "title" in payload["attachments"][0] - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_http_5xx_logs_warning(self, mock_post, caplog): """Non-2xx HTTP responses must emit a WARNING and not raise.""" import logging @@ -153,7 +162,7 @@ def test_http_5xx_logs_warning(self, mock_post, caplog): assert any("503" in r.message or "HTTP" in r.message for r in caplog.records) - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_http_4xx_logs_warning(self, mock_post, caplog): """HTTP 4xx response must emit a WARNING log and not raise.""" import logging @@ -208,7 +217,13 @@ def test_ssrf_block_private_ip(self, url): safe_post(url, json={}, timeout=10.0) def test_ssrf_block_can_be_opted_out(self): - """allow_private=True bypasses the SSRF guard (operator opt-in).""" + """``allow_private=True`` bypasses the SSRF guard (operator opt-in). + + The opt-in path deliberately keeps the legacy ``requests.post`` + flow (no Session, no IP pinning) because internal DNS / split- + horizon resolution is the right model for an operator-blessed + in-cluster destination — see safe_post's ``allow_private`` branch. + """ from forgelm import _http with patch.object(_http.requests, "post") as mock_post: @@ -225,7 +240,7 @@ def test_redirect_block(self): """allow_redirects=False is forwarded to requests.post.""" from forgelm import _http - with patch.object(_http.requests, "post") as mock_post: + with patch.object(_http.requests.Session, "post") as mock_post: mock_post.return_value = MagicMock(ok=True, status_code=200) _http.safe_post("https://example.com/hook", json={}, timeout=10.0) kwargs = mock_post.call_args.kwargs @@ -246,7 +261,7 @@ def test_http_allowed_with_opt_in(self): # back-compat path; the post is mocked, no real call is made. from forgelm import _http - with patch.object(_http.requests, "post") as mock_post: + with patch.object(_http.requests.Session, "post") as mock_post: mock_post.return_value = MagicMock(ok=True, status_code=200) _http.safe_post( "http://example.com/hook", # NOSONAR python:S5332 @@ -293,7 +308,7 @@ def test_timeout_floor_overridable(self): """Webhook passes min_timeout=1.0 to keep its historical floor.""" from forgelm import _http - with patch.object(_http.requests, "post") as mock_post: + with patch.object(_http.requests.Session, "post") as mock_post: mock_post.return_value = MagicMock(ok=True, status_code=200) _http.safe_post( "https://example.com/hook", @@ -313,7 +328,7 @@ def test_header_masking_on_error(self, caplog): # NOSONAR test fixture, fragment-built (rule python:S2068 hard-coded credential false-positive) bearer_token = "sk-" + "supersecret123" # noqa: S105 - with patch.object(_http.requests, "post") as mock_post: + with patch.object(_http.requests.Session, "post") as mock_post: mock_post.side_effect = req.exceptions.ConnectionError(f"refused while sending Bearer {bearer_token}") with caplog.at_level(logging.WARNING, logger="forgelm._http"): with pytest.raises(req.exceptions.ConnectionError): @@ -345,7 +360,7 @@ class TestLifecycleVocabulary: don't silently break on a future refactor. """ - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_notify_reverted_payload(self, mock_post): """Auto-revert event must serialize as event=training.reverted with masked + truncated reason.""" config = _make_config({"url": "https://example.com/hook"}) @@ -376,7 +391,7 @@ def test_notify_reverted_payload(self, mock_post): assert len(payload["reason"]) <= 2048 + len("… (truncated)") assert payload["reason"].endswith("… (truncated)") - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_notify_reverted_distinct_from_failure(self, mock_post): """training.reverted must not collide with training.failure (dashboards rely on this split).""" config = _make_config({"url": "https://example.com/hook"}) @@ -392,7 +407,7 @@ def test_notify_reverted_distinct_from_failure(self, mock_post): # Color must signal "reverted" (warning orange), not "failed" (red). assert payload["attachments"][0]["color"] == "#ff9900" - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_notify_awaiting_approval_payload(self, mock_post): """Approval gate must serialize as event=approval.required with model_path included.""" config = _make_config({"url": "https://example.com/hook"}) @@ -414,7 +429,7 @@ def test_notify_awaiting_approval_payload(self, mock_post): assert payload["model_path"] == "/var/forgelm/runs/abc/final_model" assert "/var/forgelm/runs/abc/final_model" in payload["attachments"][0]["text"] - @patch("forgelm._http.requests.post") + @patch("forgelm._http.requests.Session.post") def test_notify_awaiting_approval_no_model_weights_in_payload(self, mock_post): """Security: approval payload must carry the staging path only, never weight bytes or tensor dumps.""" config = _make_config({"url": "https://example.com/hook"}) From 3fd61a493274191fb0ed05d7efc2480a4754a154 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Thu, 14 May 2026 05:10:24 +0300 Subject: [PATCH 08/11] =?UTF-8?q?fix:=20PR=20#52=20review-response=20?= =?UTF-8?q?=E2=80=94=20RFC=207230=20Host=20header,=20masking,=20dedup,=20+?= =?UTF-8?q?4=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses all actionable findings from PR #52's review bots (gemini, sourcery) and the SonarQube quality-gate fail (1 hotspot + duplication + maintainability). Real bugs (gemini "high" + sourcery "security") - forgelm/_http.py safe_post + safe_get: the auto-set ``Host`` header now uses ``parsed.netloc.rsplit("@", 1)[-1]`` instead of ``parsed.hostname``. Pre-fix the header silently dropped the port (``Host: example.com`` for ``https://example.com:8443/``) and the bracket-wrap for IPv6 literals, violating RFC 7230 § 5.4 — virtual- hosted endpoints that switch on the authority-form would have routed wrong. Userinfo is stripped (``user:pass@host`` → ``host``) so credentials never echo into the request header. - forgelm/_http.py safe_post + safe_get exception handlers: log masking now operates on the outbound ``request_headers`` (which includes the auto-set ``Host`` and any caller secrets) rather than the raw ``headers`` parameter (which may be ``None`` or stale). Pre-fix, if the caller passed ``headers=None``, a secret added via the auto-Host path would not have been masked from the warning log on transport failure. Refactor for maintainability + Sonar duplication - Extracted ``_is_blocked_ip(ip)`` shared helper that owns the RFC1918 / loopback / link-local / IETF-reserved / multicast policy. Both ``_is_private_destination`` (legacy yes/no) and ``_resolve_safe_destination`` (DNS-rebinding-safe resolver) now delegate, so the policy cannot drift between call sites. Closes Sonar's 4.9% new-code duplication finding on this PR. Test gaps (sourcery) - ``TestRfc7230HostHeader`` (4 new tests): non-standard port stays in Host, IPv6 literal stays bracketed, userinfo stripped, explicit caller-supplied Host header takes precedence over the auto-set value. - ``TestNoPublicIpResolvedBranch`` (1 new test): covers the ``"no public IP resolved"`` branch of ``_resolve_safe_destination`` by mocking ``getaddrinfo`` to return only malformed IP strings. - ``TestSafeGetPinning.test_safe_get_allow_private_bypasses_pinning``: mirrors the existing safe_post allow_private bypass test for the read-side helper. Sonar S5332 false-positive - ``tests/test_http_dns_rebinding.py::test_http_session_does_not_mount_ssl_adapter``: the ``http://`` literal is a fixture for the SSL-adapter dispatch branch — no outbound call, no insecure egress. ``# NOSONAR python:S5332`` with justification on the two flagged lines. Skipped (with rationale) - sourcery overall #2 ("distinguish IP-literal-private vs mixed-result reasons"): minor message-specificity, deferred to a separate PR to keep this review-response focused. - sourcery overall #3 ("per-process Session cache to amortise mount cost"): premature optimisation — webhook / judge calls are low- frequency, the per-call ``Session()`` is microsecond-cheap, and a shared session would complicate the SNI-per-host model. Co-Authored-By: Claude Opus 4.7 (1M context) --- forgelm/_http.py | 68 ++++++++------ tests/test_http_dns_rebinding.py | 149 ++++++++++++++++++++++++++++++- 2 files changed, 187 insertions(+), 30 deletions(-) diff --git a/forgelm/_http.py b/forgelm/_http.py index 092af3aa..d1be98c1 100644 --- a/forgelm/_http.py +++ b/forgelm/_http.py @@ -65,6 +65,20 @@ class HttpSafetyError(Exception): _MASK_HEADER_NAMES = frozenset({"authorization", "x-api-key", "proxy-authorization"}) +def _is_blocked_ip(ip: ipaddress._BaseAddress) -> bool: + """Single source of truth for the SSRF private-range policy. + + Encapsulates the set of address kinds that ForgeLM's SSRF guard + refuses to send outbound payloads to: RFC1918 private space, the + loopback range, link-local (incl. cloud IMDS at 169.254.169.254), + the IETF reserved buckets, and multicast. Used by both + :func:`_is_private_destination` (legacy yes/no predicate) and + :func:`_resolve_safe_destination` (DNS-rebinding-safe resolver) so + the policy cannot drift between the two call sites. + """ + return ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_reserved or ip.is_multicast + + def _is_private_destination(host: str) -> bool: """Return ``True`` when *host* resolves to a non-public-internet IP. @@ -83,7 +97,7 @@ def _is_private_destination(host: str) -> bool: except ValueError: ip = None if ip is not None: - return ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_reserved or ip.is_multicast + return _is_blocked_ip(ip) try: addrinfo = socket.getaddrinfo(host, None) except (socket.gaierror, UnicodeError): @@ -97,13 +111,7 @@ def _is_private_destination(host: str) -> bool: resolved = ipaddress.ip_address(sockaddr[0]) except ValueError: continue - if ( - resolved.is_private - or resolved.is_loopback - or resolved.is_link_local - or resolved.is_reserved - or resolved.is_multicast - ): + if _is_blocked_ip(resolved): return True return False @@ -145,19 +153,15 @@ def _resolve_safe_destination(host: str) -> Tuple[Optional[str], Optional[str]]: if not host: return None, "empty host" - # IP literal short-circuit — no DNS needed, same private-range filter. + # IP literal short-circuit — no DNS needed, same private-range filter + # applied via the shared ``_is_blocked_ip`` helper so the IP-literal + # path and the DNS path cannot disagree. try: literal = ipaddress.ip_address(host) except ValueError: literal = None if literal is not None: - if ( - literal.is_private - or literal.is_loopback - or literal.is_link_local - or literal.is_reserved - or literal.is_multicast - ): + if _is_blocked_ip(literal): return None, "Private/loopback/IMDS destination" return host, None @@ -176,13 +180,7 @@ def _resolve_safe_destination(host: str) -> Tuple[Optional[str], Optional[str]]: resolved = ipaddress.ip_address(candidate) except ValueError: continue - if ( - resolved.is_private - or resolved.is_loopback - or resolved.is_link_local - or resolved.is_reserved - or resolved.is_multicast - ): + if _is_blocked_ip(resolved): return None, "Private/loopback/IMDS destination" if public_ip is None: public_ip = candidate @@ -348,8 +346,13 @@ def safe_post( target_url = _build_pinned_url(parsed, pinned_ip) # ``Host`` is case-insensitive in HTTP/1.1; set it without # overwriting an explicit operator override for testing/proxy - # scenarios. - request_headers.setdefault("Host", host) + # scenarios. Use ``netloc`` (with any userinfo stripped) rather + # than bare ``hostname`` so non-standard ports stay attached and + # IPv6 literals remain bracketed per RFC 7230 § 5.4 — bare + # ``hostname`` would emit ``Host: example.com`` for a request to + # ``https://example.com:8443/`` and silently break virtual-hosted + # endpoints that switch on the authority-form. + request_headers.setdefault("Host", parsed.netloc.rsplit("@", 1)[-1]) # Timeout floor — `requests` treats 0 / None as "no timeout" which can # hang the trainer on a dead endpoint. @@ -386,7 +389,11 @@ def safe_post( allow_redirects=False, ) except requests.RequestException as exc: - masked_reason = _mask_secrets_in_text(str(exc), headers) + # Mask the *outbound* header set (which includes the auto-set + # ``Host`` and any caller secrets) — not the raw ``headers`` + # parameter, which may be ``None`` or stale relative to what + # actually went on the wire. + masked_reason = _mask_secrets_in_text(str(exc), request_headers) logger.warning( "safe_post failed url=%s reason=%s", _mask_netloc(url), @@ -467,7 +474,10 @@ def safe_get( if block_reason: raise HttpSafetyError(f"{block_reason} blocked: host={host or ''}") target_url = _build_pinned_url(parsed, pinned_ip) - request_headers.setdefault("Host", host) + # See safe_post: use netloc (sans userinfo) so non-standard + # ports and IPv6 brackets survive into the Host header per + # RFC 7230 § 5.4. + request_headers.setdefault("Host", parsed.netloc.rsplit("@", 1)[-1]) # Timeout floor. if not isinstance(timeout, (int, float)) or timeout < min_timeout: @@ -500,7 +510,9 @@ def safe_get( allow_redirects=False, ) except requests.RequestException as exc: - masked_reason = _mask_secrets_in_text(str(exc), headers) + # Mask the outbound header set, not the caller's possibly-None + # ``headers`` parameter — see safe_post for the same rationale. + masked_reason = _mask_secrets_in_text(str(exc), request_headers) logger.warning( "safe_get failed url=%s method=%s reason=%s", _mask_netloc(url), diff --git a/tests/test_http_dns_rebinding.py b/tests/test_http_dns_rebinding.py index dce8bdd6..0e94d17e 100644 --- a/tests/test_http_dns_rebinding.py +++ b/tests/test_http_dns_rebinding.py @@ -174,9 +174,13 @@ def test_http_session_does_not_mount_ssl_adapter(self): not need the SSL adapter — no SNI involved.""" from forgelm import _http - session = _http._pinned_session("http") + session = _http._pinned_session( + "http" + ) # NOSONAR python:S5332 — scheme fixture for the SSL-adapter dispatch branch; no outbound call. # Default HTTPAdapter, not HostHeaderSSLAdapter, on the http:// prefix. - adapter = session.get_adapter("http://example.com/") + adapter = session.get_adapter( + "http://example.com/" + ) # NOSONAR python:S5332 — adapter lookup fixture; no network egress. from requests_toolbelt.adapters.host_header_ssl import HostHeaderSSLAdapter assert not isinstance(adapter, HostHeaderSSLAdapter) @@ -243,3 +247,144 @@ def test_safe_get_pins_url_and_sets_host_header(self): assert method == "GET" assert url == "https://8.8.8.8/api/models" assert headers["Host"] == "hub.example.com" + + def test_safe_get_allow_private_bypasses_pinning(self): + """Mirror of safe_post's allow_private bypass for the read-side + helper — issue #14 review feedback (sourcery). ``allow_private=True`` + must keep the legacy ``requests.request`` flow (no Session, no + IP pinning) so internal DNS / split-horizon resolution still + resolves cluster-local registries. + """ + from forgelm import _http + + with ( + patch.object(_http.socket, "getaddrinfo") as resolve_mock, + patch.object(_http.requests, "request") as legacy_request, + patch.object(_http.requests.Session, "request") as session_request, + ): + legacy_request.return_value = MagicMock(ok=True, status_code=200) + _http.safe_get( + "https://internal.registry.local/v1/models", + timeout=10.0, + allow_private=True, + ) + + resolve_mock.assert_not_called() + session_request.assert_not_called() + legacy_request.assert_called_once() + + +class TestRfc7230HostHeader: + """Issue #14 review feedback (gemini): RFC 7230 § 5.4 requires the + ``Host`` header to match the request-target authority — including + non-standard ports and bracketed IPv6 literals. Bare ``hostname`` + drops both, so we use ``netloc`` (with any userinfo stripped) + instead. + """ + + def test_host_header_preserves_non_standard_port(self): + from forgelm import _http + + with ( + patch.object(_http.socket, "getaddrinfo", return_value=[(0, 0, 0, "", ("8.8.8.8", 0))]), + patch.object(_http.requests.Session, "post") as mock_post, + ): + mock_post.return_value = MagicMock(ok=True, status_code=200) + _http.safe_post("https://hooks.example.com:8443/abc", json={}, timeout=10.0) + + headers = mock_post.call_args.kwargs["headers"] + assert headers["Host"] == "hooks.example.com:8443", ( + f"Non-standard port must remain in the Host header per RFC 7230 § 5.4; got {headers['Host']!r}" + ) + + def test_host_header_brackets_ipv6_literal(self): + from forgelm import _http + + # Hostname-resolved-to-IPv6 case is exercised by the URL builder + # test below; here we cover the IPv6-literal-in-URL form which + # the operator may supply when bypassing DNS entirely. + with patch.object(_http.requests.Session, "post") as mock_post: + mock_post.return_value = MagicMock(ok=True, status_code=200) + _http.safe_post("https://[2606:4700:4700::1111]/abc", json={}, timeout=10.0) + + headers = mock_post.call_args.kwargs["headers"] + assert headers["Host"] == "[2606:4700:4700::1111]", ( + f"IPv6 literal in Host header must stay bracketed per RFC 7230 § 5.4; got {headers['Host']!r}" + ) + + def test_host_header_strips_userinfo(self): + """If the URL carries ``user:pass@host`` userinfo, the Host + header must NOT echo it — that would leak the credential into + every outbound request, plus RFC 7230 § 5.4 explicitly forbids + userinfo in ``Host``. + """ + from forgelm import _http + + with ( + patch.object(_http.socket, "getaddrinfo", return_value=[(0, 0, 0, "", ("8.8.8.8", 0))]), + patch.object(_http.requests.Session, "post") as mock_post, + ): + mock_post.return_value = MagicMock(ok=True, status_code=200) + _http.safe_post( + "https://user:pass@hooks.example.com/abc", # noqa: S105 — userinfo fixture + json={}, + timeout=10.0, + ) + + headers = mock_post.call_args.kwargs["headers"] + assert "@" not in headers["Host"] + assert headers["Host"] == "hooks.example.com" + + def test_explicit_host_header_not_overridden(self): + """When the caller passes an explicit ``Host`` header (e.g. for + a reverse proxy / hostname-spoofing test setup), ``safe_post`` + must respect it via ``setdefault`` and not overwrite — issue + #14 review feedback (sourcery). + """ + from forgelm import _http + + with ( + patch.object(_http.socket, "getaddrinfo", return_value=[(0, 0, 0, "", ("8.8.8.8", 0))]), + patch.object(_http.requests.Session, "post") as mock_post, + ): + mock_post.return_value = MagicMock(ok=True, status_code=200) + _http.safe_post( + "https://hooks.example.com/abc", + json={}, + headers={"Host": "operator-supplied.example.com"}, + timeout=10.0, + ) + + headers = mock_post.call_args.kwargs["headers"] + assert headers["Host"] == "operator-supplied.example.com", ( + "Caller's explicit Host header must take precedence over the auto-set value" + ) + + +class TestNoPublicIpResolvedBranch: + """Issue #14 review feedback (sourcery): cover the + ``"no public IP resolved"`` branch where ``getaddrinfo`` returns + only entries the resolver cannot parse as plain IP literals + (e.g. zone-id-suffixed IPv6 link-locals). + """ + + def test_only_unparseable_records_yields_no_public_ip(self): + from forgelm import _http + + # All entries fail ``ipaddress.ip_address()`` — malformed IP + # strings of both v4 and v6 shape. None of them is private + # (we never parse them) but none is public either, so the + # resolver must report "no public IP resolved" rather than + # silently letting an empty candidate set through. + with patch.object( + _http.socket, + "getaddrinfo", + return_value=[ + (0, 0, 0, "", ("not-an-ip", 0)), + (0, 0, 0, "", ("999.999.999.999", 0)), + ], + ): + ip, err = _http._resolve_safe_destination("weird.example.com") + + assert ip is None + assert err == "no public IP resolved" From 1e0108d10616b7f52a390b05b8f932208a468c0a Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Thu, 14 May 2026 08:18:01 +0300 Subject: [PATCH 09/11] =?UTF-8?q?fix:=20PR=20#52=20review-round-2=20?= =?UTF-8?q?=E2=80=94=20switch=20request=5Fheaders=20to=20CaseInsensitiveDi?= =?UTF-8?q?ct?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #14 review feedback (codex): the previous round used a plain ``dict(headers or {})`` to materialise the outbound header set, then ``setdefault("Host", ...)`` to fall back to the auto-derived value. Python's ``dict`` is case-sensitive, so a caller passing ``headers={"host": "operator-supplied.example.com"}`` (lowercase) would trigger: request_headers = {"host": "operator-supplied.example.com"} request_headers.setdefault("Host", "") # → {"host": "operator-supplied.example.com", "Host": ""} Two Host entries on the wire — invalid per RFC 7230 § 5.4 and a silent break of the "caller override wins" contract this PR's ``test_explicit_host_header_not_overridden`` tried to lock down. Fix: switch to ``requests.structures.CaseInsensitiveDict``, which collapses every casing of the same header to one logical slot. ``"Host" not in request_headers`` (replacing the ``setdefault``) is also case-insensitive, so an explicit caller-supplied ``host`` / ``HOST`` / ``Host`` all suppress the auto-Host injection correctly. Applied to both ``safe_post`` and ``safe_get`` (the identical pattern). ``_mask_secrets_in_text`` annotation widened to ``Optional[MutableMapping[str, str]]`` so the new outbound type flows through the masking helper without a mypy false-positive. Test: new ``test_caller_host_header_with_different_casing_does_not_duplicate`` in ``TestRfc7230HostHeader`` passes a lowercase ``host`` header and asserts (a) exactly one Host entry on the wire (case-insensitive count), and (b) the caller's value wins. 20 / 20 DNS-rebinding tests + 170 / 170 HTTP-related tests green. Co-Authored-By: Claude Opus 4.7 (1M context) --- forgelm/_http.py | 35 ++++++++++++++++++++----------- tests/test_http_dns_rebinding.py | 36 ++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 12 deletions(-) diff --git a/forgelm/_http.py b/forgelm/_http.py index d1be98c1..c5308e87 100644 --- a/forgelm/_http.py +++ b/forgelm/_http.py @@ -45,10 +45,11 @@ import ipaddress import logging import socket -from typing import Any, Dict, Optional, Tuple +from typing import Any, Dict, MutableMapping, Optional, Tuple from urllib.parse import urlparse, urlunparse import requests +from requests.structures import CaseInsensitiveDict logger = logging.getLogger("forgelm._http") @@ -249,7 +250,7 @@ def _mask_netloc(url: str) -> str: return f"{parts.scheme}://{parts.hostname or 'unknown-host'}" -def _mask_secrets_in_text(text: str, headers: Optional[Dict[str, str]]) -> str: +def _mask_secrets_in_text(text: str, headers: Optional[MutableMapping[str, str]]) -> str: """Redact known secret-bearing header values from *text*. ``requests`` exception strings sometimes include the request URL or @@ -338,21 +339,27 @@ def safe_post( # used as-is so internal DNS / split-horizon resolution still works. host = parsed.hostname or "" target_url = url - request_headers: Dict[str, str] = dict(headers or {}) + # ``CaseInsensitiveDict`` so a caller-supplied ``host`` / ``HOST`` / + # ``Host`` all collapse to the same key — a plain ``dict`` would + # preserve the caller's casing and let ``setdefault("Host", ...)`` + # silently add a *second* Host header to the request, producing a + # duplicate on the wire (RFC 7230 § 5.4 forbids this). + request_headers: MutableMapping[str, str] = CaseInsensitiveDict(headers or {}) if not allow_private: pinned_ip, block_reason = _resolve_safe_destination(host) if block_reason: raise HttpSafetyError(f"{block_reason} blocked: host={host or ''}") target_url = _build_pinned_url(parsed, pinned_ip) - # ``Host`` is case-insensitive in HTTP/1.1; set it without - # overwriting an explicit operator override for testing/proxy - # scenarios. Use ``netloc`` (with any userinfo stripped) rather - # than bare ``hostname`` so non-standard ports stay attached and - # IPv6 literals remain bracketed per RFC 7230 § 5.4 — bare + # ``Host`` is case-insensitive in HTTP/1.1; let an explicit + # caller override (any casing) win over the auto-derived value. + # Use ``netloc`` (with any userinfo stripped) rather than bare + # ``hostname`` so non-standard ports stay attached and IPv6 + # literals remain bracketed per RFC 7230 § 5.4 — bare # ``hostname`` would emit ``Host: example.com`` for a request to # ``https://example.com:8443/`` and silently break virtual-hosted # endpoints that switch on the authority-form. - request_headers.setdefault("Host", parsed.netloc.rsplit("@", 1)[-1]) + if "Host" not in request_headers: + request_headers["Host"] = parsed.netloc.rsplit("@", 1)[-1] # Timeout floor — `requests` treats 0 / None as "no timeout" which can # hang the trainer on a dead endpoint. @@ -468,7 +475,9 @@ def safe_get( # SSRF guard — see safe_post for the issue-#14 DNS-rebinding rationale. host = parsed.hostname or "" target_url = url - request_headers: Dict[str, str] = dict(headers or {}) + # CaseInsensitiveDict — see safe_post for the duplicate-Host-header + # rationale (caller may pass "host"/"HOST"/"Host" in any casing). + request_headers: MutableMapping[str, str] = CaseInsensitiveDict(headers or {}) if not allow_private: pinned_ip, block_reason = _resolve_safe_destination(host) if block_reason: @@ -476,8 +485,10 @@ def safe_get( target_url = _build_pinned_url(parsed, pinned_ip) # See safe_post: use netloc (sans userinfo) so non-standard # ports and IPv6 brackets survive into the Host header per - # RFC 7230 § 5.4. - request_headers.setdefault("Host", parsed.netloc.rsplit("@", 1)[-1]) + # RFC 7230 § 5.4; case-insensitive containment honours an + # explicit caller override in any casing. + if "Host" not in request_headers: + request_headers["Host"] = parsed.netloc.rsplit("@", 1)[-1] # Timeout floor. if not isinstance(timeout, (int, float)) or timeout < min_timeout: diff --git a/tests/test_http_dns_rebinding.py b/tests/test_http_dns_rebinding.py index 0e94d17e..00d8fa45 100644 --- a/tests/test_http_dns_rebinding.py +++ b/tests/test_http_dns_rebinding.py @@ -360,6 +360,42 @@ def test_explicit_host_header_not_overridden(self): "Caller's explicit Host header must take precedence over the auto-set value" ) + def test_caller_host_header_with_different_casing_does_not_duplicate(self): + """Mixed-casing caller header (e.g. ``{"host": ...}``) must not + produce two Host entries on the wire. Issue #14 review-round-2 + (codex): a plain ``dict(headers or {})`` preserves the caller's + casing, so ``setdefault("Host", ...)`` would silently add a + second header alongside the caller's ``"host"`` key — invalid + per RFC 7230 § 5.4 and a wire-format ambiguity (``requests`` + dedupes via its own ``CaseInsensitiveDict`` but the outbound + argument we send must already be single-valued so we can + observe it in this test). + """ + from forgelm import _http + + with ( + patch.object(_http.socket, "getaddrinfo", return_value=[(0, 0, 0, "", ("8.8.8.8", 0))]), + patch.object(_http.requests.Session, "post") as mock_post, + ): + mock_post.return_value = MagicMock(ok=True, status_code=200) + _http.safe_post( + "https://hooks.example.com/abc", + json={}, + # NB: lowercase ``host``. A naive ``dict`` would let + # ``setdefault("Host", auto_value)`` add a second header + # because dict keys are case-sensitive. + headers={"host": "operator-supplied.example.com"}, + timeout=10.0, + ) + + headers = mock_post.call_args.kwargs["headers"] + # Exactly one Host entry — case-insensitively — must exist. + host_keys = [k for k in headers.keys() if k.lower() == "host"] + assert len(host_keys) == 1, f"Expected exactly one Host header, got {host_keys}" + # The caller's explicit value must win. + assert headers["Host"] == "operator-supplied.example.com" + assert headers["host"] == "operator-supplied.example.com" + class TestNoPublicIpResolvedBranch: """Issue #14 review feedback (sourcery): cover the From 188fc8aa9827fa6bc6d53123e13c60473ccaec7d Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Thu, 14 May 2026 09:49:06 +0300 Subject: [PATCH 10/11] =?UTF-8?q?fix:=20PR=20#51=20review-response=20?= =?UTF-8?q?=E2=80=94=20order,=20mirror=20links,=20glob,=20type=20hint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the four blocking findings + one P3 from the PR #51 review. Blocking #1 — full suite kırmızı (26 fail in offline/sandbox CI): - forgelm/_http.py::safe_post + safe_get: moved cheap local-policy checks (timeout floor, method allowlist) BEFORE the SSRF DNS resolve. A misconfigured caller (timeout=0, sub-floor value, unsupported method) is now rejected without a DNS round-trip — and unit tests for those branches no longer accidentally depend on ``socket.getaddrinfo`` reaching the network. - tests/test_webhook.py, tests/test_judge_functions.py, tests/test_synthetic.py: module-level autouse fixture ``_stub_ssrf_resolver`` makes ``forgelm._http._resolve_safe_destination`` hermetic. IP literals still flow through the real ``_is_blocked_ip`` policy (so the SSRF-block parametric tests keep their teeth) and ``localhost`` is canonically treated as loopback; every other hostname resolves to the public sentinel ``8.8.8.8``. Dedicated resolver coverage in tests/test_http_dns_rebinding.py is untouched because it lives in its own module and patches the same seam at finer granularity. Blocking #2 — AGENTS.md skill links pointed at .Codex/skills/, a directory that does not exist. Rewrote all 8 links to .agents/skills/ so the agent-mirror tema's primary use path actually works. Blocking #3 — tools/check_no_analysis_refs.py added .agents/**/*.md to ``_PUBLIC_GLOBS``. Previously the .agents/ tree was never scanned, so the ``_EXEMPT`` entry added in commit fcbb926 was silently ineffective and the guard's green result was misleading. The exempt entry is still required (and now actually exercised) because .agents/skills/sync-bilingual-docs/SKILL.md legitimately names the gitignored docs/marketing/ and docs/analysis/ directories. P3 — forgelm/cli/_no_train_modes.py::_resolve_benchmark_load_target return type: PEP 604 ``tuple[str, str | None]`` → ``Tuple[str, Optional[str]]`` to match the project's existing typing style (consistent with every other typed helper in this module). Verification - ruff format + check: clean (192 / 192 files) - 11 / 11 self-review gauntlet guards: green (forgelm --dry-run, bilingual parity 46 pairs, anchor resolution 271 md, CLI help consistency 447 invocations, wizard defaults, no-analysis-refs with new glob, no unguarded sys.modules pop, site version). - HTTP-touching modules (webhook + judge + synthetic + dns_rebinding): 96 / 96 green. - Full pytest tests/ (--ignore=test_phase12_review_fixes.py, the documented perf-flake that the previous review marked non-blocking): **1779 passed, 13 skipped, 0 failed in 954.74s (~16 min).** Co-Authored-By: Claude Opus 4.7 (1M context) --- AGENTS.md | 16 ++++++------ forgelm/_http.py | 35 ++++++++++++++++----------- forgelm/cli/_no_train_modes.py | 3 ++- tests/test_judge_functions.py | 30 +++++++++++++++++++++++ tests/test_synthetic.py | 30 +++++++++++++++++++++++ tests/test_webhook.py | 43 +++++++++++++++++++++++++++++++++ tools/check_no_analysis_refs.py | 5 ++++ 7 files changed, 139 insertions(+), 23 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e7fd9241..1704545e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,16 +29,16 @@ Do not invent conventions. If you cannot find the pattern for what you're about ## Skills -When a task maps to a common pattern, invoke the matching skill from [.Codex/skills/](.Codex/skills/): +When a task maps to a common pattern, invoke the matching skill from [.agents/skills/](.agents/skills/): | Task | Skill | |---|---| -| Adding a YAML config field | [add-config-field](.Codex/skills/add-config-field/SKILL.md) | -| Adding a larger trainer / evaluator / module feature | [add-trainer-feature](.Codex/skills/add-trainer-feature/SKILL.md) | -| Writing tests | [add-test](.Codex/skills/add-test/SKILL.md) | -| Updating bilingual docs (EN ↔ TR) | [sync-bilingual-docs](.Codex/skills/sync-bilingual-docs/SKILL.md) | -| Reviewing a PR (own or peer) | [review-pr](.Codex/skills/review-pr/SKILL.md) | -| Cutting a release | [cut-release](.Codex/skills/cut-release/SKILL.md) | +| Adding a YAML config field | [add-config-field](.agents/skills/add-config-field/SKILL.md) | +| Adding a larger trainer / evaluator / module feature | [add-trainer-feature](.agents/skills/add-trainer-feature/SKILL.md) | +| Writing tests | [add-test](.agents/skills/add-test/SKILL.md) | +| Updating bilingual docs (EN ↔ TR) | [sync-bilingual-docs](.agents/skills/sync-bilingual-docs/SKILL.md) | +| Reviewing a PR (own or peer) | [review-pr](.agents/skills/review-pr/SKILL.md) | +| Cutting a release | [cut-release](.agents/skills/cut-release/SKILL.md) | Each skill's `SKILL.md` has the full checklist. Follow it; don't skip steps to save time. @@ -190,7 +190,7 @@ Default workflow for a non-trivial change: ## When in doubt 1. Check the relevant [docs/standards/](docs/standards/) file. -2. Check for a matching skill in [.Codex/skills/](.Codex/skills/). +2. Check for a matching skill in [.agents/skills/](.agents/skills/). 3. Find the closest existing pattern in the codebase and follow it. 4. Ask the user rather than guess. diff --git a/forgelm/_http.py b/forgelm/_http.py index c5308e87..a3d7bd50 100644 --- a/forgelm/_http.py +++ b/forgelm/_http.py @@ -332,6 +332,14 @@ def safe_post( elif parsed.scheme != "https": raise HttpSafetyError(f"Unsupported URL scheme {parsed.scheme!r}; only http(s) allowed.") + # Timeout floor — `requests` treats 0 / None as "no timeout" which can + # hang the trainer on a dead endpoint. Validated BEFORE the SSRF + # resolve so a local policy error (timeout=0, sub-floor value) is + # rejected without a DNS round-trip; this also keeps unit tests + # that exercise the timeout-floor branch hermetic. + if not isinstance(timeout, (int, float)) or timeout < min_timeout: + raise HttpSafetyError(f"Timeout below {min_timeout}s floor: timeout={timeout!r}") + # SSRF guard — issue #14: resolve once to a public IP literal so the # connect-time DNS lookup cannot flip the verdict (DNS rebinding / # TOCTOU). When ``allow_private=True`` the operator has explicitly @@ -361,11 +369,6 @@ def safe_post( if "Host" not in request_headers: request_headers["Host"] = parsed.netloc.rsplit("@", 1)[-1] - # Timeout floor — `requests` treats 0 / None as "no timeout" which can - # hang the trainer on a dead endpoint. - if not isinstance(timeout, (int, float)) or timeout < min_timeout: - raise HttpSafetyError(f"Timeout below {min_timeout}s floor: timeout={timeout!r}") - # Resolve TLS verify setting. ca_bundle (when set) wins over verify. verify_param: Any = ca_bundle if ca_bundle else verify @@ -472,6 +475,19 @@ def safe_get( elif parsed.scheme != "https": raise HttpSafetyError(f"Unsupported URL scheme {parsed.scheme!r}; only http(s) allowed.") + # Cheap local-policy checks first (timeout floor + method allowlist) + # so a misconfigured caller is rejected without a DNS round-trip; + # keeps unit tests for these branches hermetic. + + # Timeout floor. + if not isinstance(timeout, (int, float)) or timeout < min_timeout: + raise HttpSafetyError(f"Timeout below {min_timeout}s floor: timeout={timeout!r}") + + # Method policy — only GET / HEAD allowed (read-side helper). + method_upper = method.upper() + if method_upper not in ("GET", "HEAD"): + raise HttpSafetyError(f"safe_get only supports GET / HEAD, got {method!r}.") + # SSRF guard — see safe_post for the issue-#14 DNS-rebinding rationale. host = parsed.hostname or "" target_url = url @@ -490,15 +506,6 @@ def safe_get( if "Host" not in request_headers: request_headers["Host"] = parsed.netloc.rsplit("@", 1)[-1] - # Timeout floor. - if not isinstance(timeout, (int, float)) or timeout < min_timeout: - raise HttpSafetyError(f"Timeout below {min_timeout}s floor: timeout={timeout!r}") - - # Method policy — only GET / HEAD allowed (read-side helper). - method_upper = method.upper() - if method_upper not in ("GET", "HEAD"): - raise HttpSafetyError(f"safe_get only supports GET / HEAD, got {method!r}.") - verify_param: Any = ca_bundle if ca_bundle else verify try: diff --git a/forgelm/cli/_no_train_modes.py b/forgelm/cli/_no_train_modes.py index 8fd5825d..d1098f2d 100644 --- a/forgelm/cli/_no_train_modes.py +++ b/forgelm/cli/_no_train_modes.py @@ -8,6 +8,7 @@ import json import os import sys +from typing import Optional, Tuple from ..config import ForgeConfig from ._dry_run import _run_dry_run @@ -16,7 +17,7 @@ from ._logging import logger -def _resolve_benchmark_load_target(model_path: str) -> tuple[str, str | None]: +def _resolve_benchmark_load_target(model_path: str) -> Tuple[str, Optional[str]]: """Resolve ``(base_path, adapter_path_or_none)`` for the inference loader. A PEFT checkpoint is detected by the presence of ``adapter_config.json`` diff --git a/tests/test_judge_functions.py b/tests/test_judge_functions.py index 313634cc..2ec80903 100644 --- a/tests/test_judge_functions.py +++ b/tests/test_judge_functions.py @@ -14,6 +14,36 @@ torch_available = False +@pytest.fixture(autouse=True) +def _stub_ssrf_resolver(monkeypatch): + """Auto-stub ``forgelm._http._resolve_safe_destination`` so judge + tests do not require live DNS resolution of ``api.openai.com`` or + other API endpoints. See ``tests/test_webhook.py`` for the same + pattern + full rationale. Dedicated resolver coverage lives in + ``tests/test_http_dns_rebinding.py``. + """ + import ipaddress + + from forgelm import _http + + def _hermetic_resolver(host): + if not host: + return None, "empty host" + try: + literal = ipaddress.ip_address(host) + except ValueError: + literal = None + if literal is not None: + if _http._is_blocked_ip(literal): + return None, "Private/loopback/IMDS destination" + return host, None + if host == "localhost": + return None, "Private/loopback/IMDS destination" + return "8.8.8.8", None + + monkeypatch.setattr(_http, "_resolve_safe_destination", _hermetic_resolver) + + class TestParseJudgeJson: def test_valid_json(self): result = _parse_judge_json('{"score": 8, "reason": "Good answer"}') diff --git a/tests/test_synthetic.py b/tests/test_synthetic.py index 5ea576b7..78e76b1c 100644 --- a/tests/test_synthetic.py +++ b/tests/test_synthetic.py @@ -9,6 +9,36 @@ from forgelm.config import ForgeConfig, load_config from forgelm.synthetic import SyntheticDataGenerator, SyntheticResult + +@pytest.fixture(autouse=True) +def _stub_ssrf_resolver(monkeypatch): + """Auto-stub ``forgelm._http._resolve_safe_destination`` so synthetic + teacher tests do not require live DNS resolution of API endpoints. + See ``tests/test_webhook.py`` for the same pattern + full rationale. + Dedicated resolver coverage lives in ``tests/test_http_dns_rebinding.py``. + """ + import ipaddress + + from forgelm import _http + + def _hermetic_resolver(host): + if not host: + return None, "empty host" + try: + literal = ipaddress.ip_address(host) + except ValueError: + literal = None + if literal is not None: + if _http._is_blocked_ip(literal): + return None, "Private/loopback/IMDS destination" + return host, None + if host == "localhost": + return None, "Private/loopback/IMDS destination" + return "8.8.8.8", None + + monkeypatch.setattr(_http, "_resolve_safe_destination", _hermetic_resolver) + + BASE = { "model": {"name_or_path": "test/model"}, "lora": {"r": 16, "alpha": 32}, diff --git a/tests/test_webhook.py b/tests/test_webhook.py index 863a4f57..0dcf2540 100644 --- a/tests/test_webhook.py +++ b/tests/test_webhook.py @@ -10,6 +10,49 @@ from forgelm.webhook import WebhookNotifier +@pytest.fixture(autouse=True) +def _stub_ssrf_resolver(monkeypatch): + """Auto-stub ``forgelm._http._resolve_safe_destination`` so webhook + tests do not require live DNS resolution of ``example.com``. + + The SSRF DNS-pinning path (issue #14) added a hostname → public-IP + lookup before ``Session.post`` is called. Without this stub the + suite is host-environment dependent (passes online, fails in + offline / sandbox CI runners with ``DNS resolution failed``). + + The stub mirrors the real policy decision for the inputs the + existing SSRF-block tests use — IP literals are routed through + ``_is_blocked_ip`` so RFC1918 / loopback / IMDS / multicast still + raise, and the canonical ``localhost`` hostname is treated as + loopback. All other hostnames resolve to the public sentinel + ``8.8.8.8``. The dedicated coverage for the real resolver itself + lives in ``tests/test_http_dns_rebinding.py``. + """ + import ipaddress + + from forgelm import _http + + def _hermetic_resolver(host): + if not host: + return None, "empty host" + # IP literal: keep the real policy decision intact. + try: + literal = ipaddress.ip_address(host) + except ValueError: + literal = None + if literal is not None: + if _http._is_blocked_ip(literal): + return None, "Private/loopback/IMDS destination" + return host, None + # Hostname: stub. ``localhost`` is the one canonical case the + # existing SSRF-block test asserts on; treat it as loopback. + if host == "localhost": + return None, "Private/loopback/IMDS destination" + return "8.8.8.8", None + + monkeypatch.setattr(_http, "_resolve_safe_destination", _hermetic_resolver) + + def _make_config(webhook_cfg=None): """Create a minimal ForgeConfig with optional webhook.""" data = { diff --git a/tools/check_no_analysis_refs.py b/tools/check_no_analysis_refs.py index 40a14a20..c6ee2afb 100644 --- a/tools/check_no_analysis_refs.py +++ b/tools/check_no_analysis_refs.py @@ -89,6 +89,11 @@ "site/**/*.css", "site/**/*.md", ".claude/**/*.md", + # Agent-agnostic mirror tree (Codex / Cursor / generic AI surface). + # Without this glob, ``.agents/`` skill docs would never be scanned + # for prohibited references — and any matching entry in ``_EXEMPT`` + # below would be silently ineffective. See PR #51 review feedback. + ".agents/**/*.md", ) # Files where a path-string match is INTENTIONAL (functional path filter From c83e0ad655780b9645dab6c54c58971c6d5c9fe4 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Thu, 14 May 2026 10:40:42 +0300 Subject: [PATCH 11/11] =?UTF-8?q?docs:=20PR=20#51=20review-response=20?= =?UTF-8?q?=E2=80=94=20fence=20language=20tag=20(MD040)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer (codex) flagged the dilsiz triple-backtick at line 47 of AGENTS.md. markdownlint MD040 ("fenced code block without language") applies to opening fences; the fence at line 98 is the closing fence of the same block and stays bare. Same fix mirrored to CLAUDE.md so the two agent-guidance trees stay structurally identical. Skipped (reviewer's other two findings — same theme): * "Remove the AGENTS.md entry from _EXEMPT and rewrite the docs/marketing/ / docs/analysis/ references to placeholders." Rationale: AGENTS.md is the agent-agnostic mirror of CLAUDE.md (see memory entry ``project-agents-md-mirror``). The 5 path references in AGENTS.md (lines 115, 169, 199-201) are byte-for-byte copies of the same references in CLAUDE.md, which has been on the _EXEMPT list for years. These are policy-statement documents — they cite the gitignored directories by their real names so the agent knows not to write into them. Replacing the names with placeholders ("internal/marketing", etc.) hides the actual directory name from the very agent the rule is trying to instruct, defeating the policy. Applying the change only to AGENTS.md would break mirror parity with CLAUDE.md; applying it to both is a whole-design pivot that is out of scope for PR #51. Co-Authored-By: Claude Opus 4.7 (1M context) --- AGENTS.md | 2 +- CLAUDE.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1704545e..065017ff 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,7 +44,7 @@ Each skill's `SKILL.md` has the full checklist. Follow it; don't skip steps to s ## Repository structure at a glance -``` +```text ForgeLM/ ├── forgelm/ # Source code: ~22 single-file modules + 2 sub-packages │ ├── cli/ # CLI package (Phase 15 split): _parser, _dispatch, diff --git a/CLAUDE.md b/CLAUDE.md index c6e1ab46..01716f67 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -44,7 +44,7 @@ Each skill's `SKILL.md` has the full checklist. Follow it; don't skip steps to s ## Repository structure at a glance -``` +```text ForgeLM/ ├── forgelm/ # Source code: ~22 single-file modules + 2 sub-packages │ ├── cli/ # CLI package (Phase 15 split): _parser, _dispatch,