From 0813261e892aaadd3cd0b7671c7ca47d82271f14 Mon Sep 17 00:00:00 2001 From: Chirag Jangir <184008125+chiragjangiir@users.noreply.github.com> Date: Thu, 22 Oct 2026 13:34:53 +0530 Subject: [PATCH 1/3] fix(purge): accept retention-only config file for --check-policy (#1054) --- forgelm/cli/subcommands/_purge.py | 55 ++++++++++++++++++++++++++++--- tests/test_gdpr_erasure.py | 28 ++++++++++++++++ 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/forgelm/cli/subcommands/_purge.py b/forgelm/cli/subcommands/_purge.py index 22ceb3f7..753900d8 100644 --- a/forgelm/cli/subcommands/_purge.py +++ b/forgelm/cli/subcommands/_purge.py @@ -1132,7 +1132,11 @@ def _run_purge_check_policy(args, output_format: str) -> None: from forgelm.config import ConfigError try: - config_loaded = _maybe_load_config(getattr(args, "config", None), strict=True) + config_loaded = _maybe_load_config( + getattr(args, "config", None), + strict=True, + allow_retention_only=True, + ) except (ConfigError, OSError, _yaml.YAMLError) as exc: # OSError covers FileNotFoundError + permission denied; ConfigError # wraps Pydantic ValidationError raised by ``load_config``; @@ -1413,8 +1417,13 @@ def _resolve_artefact_age( # --------------------------------------------------------------------------- -def _maybe_load_config(config_path: Optional[str], *, strict: bool = False): - """Load the ``ForgeConfig`` from YAML. +def _maybe_load_config( + config_path: Optional[str], + *, + strict: bool = False, + allow_retention_only: bool = False, +): + """Load the ``ForgeConfig`` (or retention-only config) from YAML. ``strict=False`` (default) is the best-effort mode used by the row-id / run-id erasure paths: those subcommands work without a @@ -1430,6 +1439,10 @@ def _maybe_load_config(config_path: Optional[str], *, strict: bool = False): must surface as the original ``ConfigError`` / ``OSError`` so the operator sees the validation failure instead of a misleading "no `retention:` block" notice. + + When ``allow_retention_only=True``, if the YAML lacks training/model + keys but contains a ``retention:`` block, that block is loaded and + validated as a ``RetentionConfig`` wrapped in a lightweight namespace. """ if not config_path: return None @@ -1441,7 +1454,41 @@ def _maybe_load_config(config_path: Optional[str], *, strict: bool = False): # ``docs/standards/error-handling.md``. import yaml as _yaml - from forgelm.config import ConfigError, load_config + from forgelm.config import ConfigError, RetentionConfig, load_config + + if allow_retention_only: + import os + from types import SimpleNamespace + from pydantic import ValidationError + + if not os.path.exists(config_path): + raise OSError(f"Configuration file not found: {config_path}") + + with open(config_path, "r", encoding="utf-8") as f: + try: + yaml_data = _yaml.safe_load(f) + except _yaml.YAMLError as e: + raise ConfigError(f"Invalid YAML syntax in {config_path}: {e}") from e + + if not isinstance(yaml_data, dict): + raise ConfigError( + f"Configuration file must contain a YAML mapping, got {type(yaml_data).__name__}" + ) + + if "retention" in yaml_data and "model" not in yaml_data: + # Retention-only YAML: validate specifically with RetentionConfig + retention_raw = yaml_data.get("retention") + if not isinstance(retention_raw, dict): + raise ConfigError( + f"Configuration validation failed: 'retention' must be a mapping, got {type(retention_raw).__name__}" + ) + try: + retention_obj = RetentionConfig(**retention_raw) + except ValidationError as e: + raise ConfigError(f"Configuration validation failed:\n{e}") from e + except (TypeError, ValueError) as e: + raise ConfigError(f"Configuration validation failed: {e}") from e + return SimpleNamespace(retention=retention_obj) try: return load_config(config_path) diff --git a/tests/test_gdpr_erasure.py b/tests/test_gdpr_erasure.py index 5e379a1a..1e7299de 100644 --- a/tests/test_gdpr_erasure.py +++ b/tests/test_gdpr_erasure.py @@ -1112,6 +1112,34 @@ def test_check_policy_no_config_succeeds_with_zero(self, tmp_path: Path) -> None _run_purge_cmd(args, output_format="json") assert ei.value.code == 0 + def test_check_policy_retention_only_config_succeeds(self, tmp_path: Path, capsys) -> None: + """#1054: a config file with only a retention block is accepted and validated.""" + import json + from forgelm.cli.subcommands._purge import _run_purge_cmd + + cfg = tmp_path / "retention-only.yaml" + cfg.write_text("retention:\n ephemeral_artefact_retention_days: 30\n") + args = _build_args(check_policy=True, config=str(cfg), output_dir=str(tmp_path)) + with pytest.raises(SystemExit) as ei: + _run_purge_cmd(args, output_format="json") + assert ei.value.code == 0 + captured = capsys.readouterr() + data = json.loads(captured.out) + assert data["success"] is True + assert "violations" in data + assert "count" in data + + def test_check_policy_retention_only_invalid_exits_config_error(self, tmp_path: Path) -> None: + """#1054: an invalid retention block in a retention-only file exits EXIT_CONFIG_ERROR (1).""" + from forgelm.cli.subcommands._purge import _run_purge_cmd + + cfg = tmp_path / "retention-invalid.yaml" + cfg.write_text("retention:\n invalid_horizon_field: 30\n") + args = _build_args(check_policy=True, config=str(cfg), output_dir=str(tmp_path)) + with pytest.raises(SystemExit) as ei: + _run_purge_cmd(args, output_format="json") + assert ei.value.code == 1 + class TestAtomicityFsyncFdPinning: """F-21-T-06: the existing `test_atomic_rewrite_fsyncs_before_rename` From 5e42ee5d787337a99d0662a79bf79ca93b3638a4 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Fri, 23 Oct 2026 17:30:07 +0300 Subject: [PATCH 2/3] fix(purge): validate a retention-only --check-policy config strictly (#1054) Finish the retention-only config support along the lines of the review: - Move the loader to forgelm/cli/_config_load.py as _load_retention_policy(). Only a file whose single top-level key is `retention` skips full validation; any other key (a misspelt `retension:`, a partial `training:` block) goes through load_config and exits 1 instead of being ignored. - Validate the block with hide_input_in_errors, as ForgeConfig does, so a validation error names the field without echoing its raw value. - `retention: null` gives the "nothing to enforce" note, as in a full config; a missing file keeps load_config's not-found message. - Remove the now-unused `strict` parameter of _maybe_load_config; _purge.py goes from 1259 LOC (over its 1224 budget) to 1213. - Tests: the configured horizon is the one applied (a 60-day-old report against a 30-day horizon), other top-level keys mean a full config, the null block, hidden raw input and a missing file. - Record the behaviour in the purge reference, the GDPR erasure guide and the GDPR user-manual page (EN + TR), and in CHANGELOG. Refs #1054 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 ++ docs/guides/gdpr_erasure-tr.md | 2 +- docs/guides/gdpr_erasure.md | 2 +- docs/reference/purge_subcommand-tr.md | 2 +- docs/reference/purge_subcommand.md | 2 +- docs/usermanuals/en/compliance/gdpr.md | 2 +- docs/usermanuals/tr/compliance/gdpr.md | 2 +- forgelm/cli/_config_load.py | 34 +++++++++- forgelm/cli/subcommands/_purge.py | 93 ++++++-------------------- tests/test_gdpr_erasure.py | 84 +++++++++++++++++++++-- 10 files changed, 138 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bab99b6..5bb3917c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -116,6 +116,10 @@ All notable changes to ForgeLM are documented here. `good first issue`s with their size and area) from the README and CONTRIBUTING, and record in `CONTRIBUTING.md`, `CLAUDE.md` and `AGENTS.md` that the front door is chosen with labels and views, never by closing issues. +- `forgelm purge --check-policy --config` accepts a file whose only top-level key is `retention:`, so a scheduled + retention report no longer needs a full training config next to it. A file with any other top-level key, a misspelt + `retension:` included, is still validated as a full config and exits 1. `retention:` with no value reports nothing to + enforce, as it does in a full config, and validation errors name the field without its raw value (#1054). ### Removed diff --git a/docs/guides/gdpr_erasure-tr.md b/docs/guides/gdpr_erasure-tr.md index 7027a49f..1e4aef7b 100644 --- a/docs/guides/gdpr_erasure-tr.md +++ b/docs/guides/gdpr_erasure-tr.md @@ -70,7 +70,7 @@ $ forgelm purge --run-id fg-abc123def456 --kind artefacts --output-dir ./outputs $ forgelm purge --check-policy --config configs/run.yaml --output-dir ./outputs ``` -Salt-okunur tarama. Output dizinini gezer, her artefact'ın yaşını (canonical: `final_model.staging./` dizini için koşumun ilk audit event'i, yoksa audit-log genesis `timestamp`'i; fallback: filesystem `mtime` + `age_source=mtime` flag'i ile) yüklü config'in `retention:` horizon'larına karşı kıyaslar ve structured violation list yayar. +Salt-okunur tarama. Output dizinini gezer, her artefact'ın yaşını (canonical: `final_model.staging./` dizini için koşumun ilk audit event'i, yoksa audit-log genesis `timestamp`'i; fallback: filesystem `mtime` + `age_source=mtime` flag'i ile) yüklü config'in `retention:` horizon'larına karşı kıyaslar ve structured violation list yayar. `--config`, eğitim config'i ya da yalnızca `retention:` bloğunu içeren bir dosya olabilir; başka bir üst düzey anahtar içeren dosya tam config olarak doğrulanır. **Başarılı policy raporları 0 ile çıkar** (rapor-değil-gate semantiği — design §10 Q5). Config-load başarısızlıkları `EXIT_CONFIG_ERROR` (sıfır olmayan) ile çıkar: eksik / okunamayan / Pydantic validation'ı geçemeyen explicit `--config`, "loader failed" durumunu "ihlal yok" sanmamak için `EXIT_CONFIG_ERROR` olarak yüzeye çıkar. CI gate isteyen operatörler `--output-format json` kullanır ve raporu kendileri sınar, örneğin `set -o pipefail` altında `jq -e '.success == true and has("count") and .count == 0'`: `count`, hiçbir `retention:` bloğu yüklenmediğinde yoktur; böylece gate hiç kontrol edilmemiş bir dizinde geçmek yerine başarısız olur, ve `--output-dir` gerçek eğitim çıktı dizini olmalıdır (olmayan bir dizin `count: 0` raporlar). Bu, public exit-code kontratını her ForgeLM subcommand'ında tutarlı tutar. diff --git a/docs/guides/gdpr_erasure.md b/docs/guides/gdpr_erasure.md index 3e6842e6..74df41f7 100644 --- a/docs/guides/gdpr_erasure.md +++ b/docs/guides/gdpr_erasure.md @@ -69,7 +69,7 @@ $ forgelm purge --run-id fg-abc123def456 --kind artefacts --output-dir ./outputs $ forgelm purge --check-policy --config configs/run.yaml --output-dir ./outputs ``` -Read-only scan. Walks the output directory, compares each artefact's age (canonical: the run's first audit event for a `final_model.staging./` directory, else the audit log's genesis `timestamp`; fallback: filesystem `mtime` with `age_source=mtime` flag) against the loaded config's `retention:` horizons, and emits a structured violation list. +Read-only scan. Walks the output directory, compares each artefact's age (canonical: the run's first audit event for a `final_model.staging./` directory, else the audit log's genesis `timestamp`; fallback: filesystem `mtime` with `age_source=mtime` flag) against the loaded config's `retention:` horizons, and emits a structured violation list. `--config` can be the training config or a file that holds only the `retention:` block; a file with any other top-level key is validated as a full config. **Successful policy reports exit 0** (report-not-gate semantic per design §10 Q5). Config-load failures exit with `EXIT_CONFIG_ERROR` (non-zero): an explicit `--config` that is missing, unreadable, or fails Pydantic validation surfaces as `EXIT_CONFIG_ERROR` so the operator does not mistake "loader failed" for "no violations". Operators wiring a CI gate use `--output-format json` and test the report themselves, for example `jq -e '.success == true and has("count") and .count == 0'` under `set -o pipefail`: `count` is absent when no `retention:` block was loaded, so the gate fails instead of passing on a directory that was never checked, and `--output-dir` must be the real training output directory (a missing one reports `count: 0`). This keeps the public exit-code contract consistent across every ForgeLM subcommand. diff --git a/docs/reference/purge_subcommand-tr.md b/docs/reference/purge_subcommand-tr.md index 6da12fa5..e003c9fa 100644 --- a/docs/reference/purge_subcommand-tr.md +++ b/docs/reference/purge_subcommand-tr.md @@ -56,7 +56,7 @@ JSONL eğitim corpus'undan `id` (veya `row_id`) field'ı ile tanımlanan tek bir ### Retention-policy raporu (`--check-policy`) -Salt-okunur tarama. `` üzerinde dolaşır, her artefact'ın yaşını (kanonik: `final_model.staging./` dizini için koşumun ilk audit event'i, yoksa audit-log genesis `timestamp`'i; fallback: dosya sistemi `mtime`'ı, `age_source=mtime` flag'iyle) yüklü config'in `retention:` horizon'larıyla karşılaştırır ve yapısal bir ihlal listesi yayar. +Salt-okunur tarama. `` üzerinde dolaşır, her artefact'ın yaşını (kanonik: `final_model.staging./` dizini için koşumun ilk audit event'i, yoksa audit-log genesis `timestamp`'i; fallback: dosya sistemi `mtime`'ı, `age_source=mtime` flag'iyle) yüklü config'in `retention:` horizon'larıyla karşılaştırır ve yapısal bir ihlal listesi yayar. Config, tam bir eğitim config'i ya da tek üst düzey anahtarı `retention:` olan bir YAML dosyası olabilir; başka bir üst düzey anahtar içeren dosya tam config olarak doğrulanır. **Paylaşılan çıktı dizinleri.** `compliance/`, `data_audit_report.json` ve ham-belge dizinlerinin yaşı, dosyanın en son yazıldığı andan değil, audit log'un *ilk* event'inden sayılır. Birçok koşum için yeniden kullanılan bir dizinde bugün yazılmış bir paket de ufku aşmış olarak raporlanabilir: raporun listelediği bir şeyi silmeden önce dosyanın kendi tarihine bakın. diff --git a/docs/reference/purge_subcommand.md b/docs/reference/purge_subcommand.md index 0dcbc01a..35a4f363 100644 --- a/docs/reference/purge_subcommand.md +++ b/docs/reference/purge_subcommand.md @@ -56,7 +56,7 @@ Erases a single row identified by its `id` (or `row_id`) field from a JSONL trai ### Retention-policy report (`--check-policy`) -Read-only scan. Walks ``, compares each artefact's age (canonical: the run's first audit event for a `final_model.staging./` directory, else the audit log's genesis `timestamp`; fallback: filesystem `mtime` with `age_source=mtime`) against the loaded config's `retention:` horizons, and emits a structured violation list. +Read-only scan. Walks ``, compares each artefact's age (canonical: the run's first audit event for a `final_model.staging./` directory, else the audit log's genesis `timestamp`; fallback: filesystem `mtime` with `age_source=mtime`) against the loaded config's `retention:` horizons, and emits a structured violation list. The config can be a full training config or a YAML file whose only top-level key is `retention:`; a file with any other top-level key is validated as a full config. **Shared output directories.** `compliance/`, `data_audit_report.json` and the raw-document directories are aged from the audit log's *first* event, not from when the file was last written. In a directory reused for many runs, a bundle written today can be reported as past its horizon: check the file's own date before deleting anything the report lists. diff --git a/docs/usermanuals/en/compliance/gdpr.md b/docs/usermanuals/en/compliance/gdpr.md index 2ff22d6e..b1838af4 100644 --- a/docs/usermanuals/en/compliance/gdpr.md +++ b/docs/usermanuals/en/compliance/gdpr.md @@ -53,7 +53,7 @@ retention: ephemeral_artefact_retention_days: 365 ``` -`forgelm purge --check-policy --config config.yaml --output-dir ` lists the artefacts in that directory that are past their horizon and exits 0 however many it finds (without `--output-dir` it scans the current directory, not `training.output_dir`). Nothing is deleted automatically and nothing is written to the audit log for it; deleting what is overdue is your storage layer's job. (`retention.enforce` is reserved: no command acts on it yet.) ForgeLM does not create `raw_documents/` or `ingestion_output/`, so `raw_documents_retention_days` has an effect only if you keep source documents under `/raw_documents/` or `ingestion_output/`. +`forgelm purge --check-policy --config config.yaml --output-dir ` lists the artefacts in that directory that are past their horizon and exits 0 however many it finds (without `--output-dir` it scans the current directory, not `training.output_dir`). `config.yaml` can be your training config or a file that holds only the `retention:` block above. Nothing is deleted automatically and nothing is written to the audit log for it; deleting what is overdue is your storage layer's job. (`retention.enforce` is reserved: no command acts on it yet.) ForgeLM does not create `raw_documents/` or `ingestion_output/`, so `raw_documents_retention_days` has an effect only if you keep source documents under `/raw_documents/` or `ingestion_output/`. ## Data subject requests diff --git a/docs/usermanuals/tr/compliance/gdpr.md b/docs/usermanuals/tr/compliance/gdpr.md index b28bff59..e3c862a8 100644 --- a/docs/usermanuals/tr/compliance/gdpr.md +++ b/docs/usermanuals/tr/compliance/gdpr.md @@ -53,7 +53,7 @@ retention: ephemeral_artefact_retention_days: 365 ``` -`forgelm purge --check-policy --config config.yaml --output-dir ` o dizinde ufku aşmış artefaktları listeler ve kaç tane bulursa bulsun 0 ile çıkar (`--output-dir` verilmezse `training.output_dir`'i değil, bulunulan dizini tarar). Hiçbir şey otomatik silinmez ve bunun için audit log'a bir şey yazılmaz; süresi geçeni silmek depolama katmanınızın işidir. (`retention.enforce` ayrılmıştır: henüz hiçbir komut onu kullanmaz.) ForgeLM `raw_documents/` veya `ingestion_output/` dizinlerini oluşturmaz; bu yüzden `raw_documents_retention_days`, yalnızca kaynak dokümanları `/raw_documents/` ya da `ingestion_output/` altında tutuyorsanız etkili olur. +`forgelm purge --check-policy --config config.yaml --output-dir ` o dizinde ufku aşmış artefaktları listeler ve kaç tane bulursa bulsun 0 ile çıkar (`--output-dir` verilmezse `training.output_dir`'i değil, bulunulan dizini tarar). `config.yaml`, eğitim config'iniz ya da yalnızca yukarıdaki `retention:` bloğunu içeren bir dosya olabilir. Hiçbir şey otomatik silinmez ve bunun için audit log'a bir şey yazılmaz; süresi geçeni silmek depolama katmanınızın işidir. (`retention.enforce` ayrılmıştır: henüz hiçbir komut onu kullanmaz.) ForgeLM `raw_documents/` veya `ingestion_output/` dizinlerini oluşturmaz; bu yüzden `raw_documents_retention_days`, yalnızca kaynak dokümanları `/raw_documents/` ya da `ingestion_output/` altında tutuyorsanız etkili olur. ## Veri sahibi talepleri diff --git a/forgelm/cli/_config_load.py b/forgelm/cli/_config_load.py index 0338fb8e..3d89034b 100644 --- a/forgelm/cli/_config_load.py +++ b/forgelm/cli/_config_load.py @@ -1,15 +1,16 @@ -"""Config loading + offline-flag application for training mode.""" +"""Config loading + offline-flag application for training mode, and the ``purge --check-policy`` retention loader.""" from __future__ import annotations import os import sys +from typing import Optional import yaml -from pydantic import ValidationError +from pydantic import ConfigDict, TypeAdapter, ValidationError from .._strict_json import dumps_strict -from ..config import ConfigError, load_config +from ..config import ConfigError, RetentionConfig, load_config from ._exit_codes import EXIT_CONFIG_ERROR from ._logging import logger @@ -73,6 +74,33 @@ def _load_config_or_exit(config_path: str, json_output: bool): sys.exit(EXIT_CONFIG_ERROR) +def _load_retention_policy(config_path: Optional[str]) -> Optional[RetentionConfig]: + """Return the ``retention:`` block that ``forgelm purge --check-policy`` reports against. + + The file is either a full ForgeLM config or one whose only top-level key is ``retention``: a scheduled + retention report needs no training config (#1054). Any other top-level key makes it a full config, validated + as one, so a misspelt ``retension:`` fails instead of being ignored. ``retention: null`` returns ``None``, as + it does in a full config. Raises what ``load_config`` raises (``FileNotFoundError``, ``OSError``, + ``ConfigError``), and like ``ForgeConfig`` leaves raw input values out of validation messages. + """ + if not config_path: + return None + if not os.path.exists(config_path): + raise FileNotFoundError(f"Configuration file not found: {config_path}") + with open(config_path, encoding="utf-8") as fh: + try: + data = yaml.safe_load(fh) + except yaml.YAMLError as exc: + raise ConfigError(f"Invalid YAML syntax in {config_path}: {exc}") from exc + if not (isinstance(data, dict) and set(data) == {"retention"}): + return load_config(config_path).retention + adapter = TypeAdapter(Optional[RetentionConfig], config=ConfigDict(hide_input_in_errors=True, title="retention")) + try: + return adapter.validate_python(data["retention"]) + except ValidationError as exc: + raise ConfigError(f"Configuration validation failed:\n{exc}") from exc + + def _disable_hub_telemetry() -> None: """Keep the ``forgelm`` CLI from sending Hugging Face's anonymous usage ping. diff --git a/forgelm/cli/subcommands/_purge.py b/forgelm/cli/subcommands/_purge.py index 753900d8..a283249d 100644 --- a/forgelm/cli/subcommands/_purge.py +++ b/forgelm/cli/subcommands/_purge.py @@ -1125,22 +1125,21 @@ def _run_purge_check_policy(args, output_format: str) -> None: malformed YAML or a Pydantic validation error must surface as ``EXIT_CONFIG_ERROR`` rather than silently degrading to a "no retention block" notice (which the operator would mistake - for "no violations"). + for "no violations"). The file may hold only ``retention:`` + (#1054); see ``_load_retention_policy``. """ import yaml as _yaml from forgelm.config import ConfigError + from .._config_load import _load_retention_policy + try: - config_loaded = _maybe_load_config( - getattr(args, "config", None), - strict=True, - allow_retention_only=True, - ) + retention = _load_retention_policy(getattr(args, "config", None)) except (ConfigError, OSError, _yaml.YAMLError) as exc: # OSError covers FileNotFoundError + permission denied; ConfigError - # wraps Pydantic ValidationError raised by ``load_config``; - # ``yaml.YAMLError`` mirrors the inner catch tuple in + # wraps Pydantic ValidationError and YAML parse errors; + # ``yaml.YAMLError`` mirrors the catch tuple in # ``_maybe_load_config`` so a malformed YAML that bypasses the # ConfigError wrap still surfaces a formatted operator-facing # message instead of a stack trace. Any other class would be a @@ -1150,7 +1149,7 @@ def _run_purge_check_policy(args, output_format: str) -> None: f"--check-policy could not load --config: {exc}", EXIT_CONFIG_ERROR, ) - if config_loaded is None or getattr(config_loaded, "retention", None) is None: + if retention is None: msg = ( "No `retention:` block in the loaded config; nothing to enforce. " "See `docs/guides/gdpr_erasure.md` for the schema." @@ -1162,7 +1161,7 @@ def _run_purge_check_policy(args, output_format: str) -> None: sys.exit(EXIT_SUCCESS) output_dir = args.output_dir or "." - violations = _scan_retention_violations(config_loaded.retention, output_dir) + violations = _scan_retention_violations(retention, output_dir) if output_format == "json": print(dumps_strict({"success": True, "violations": violations, "count": len(violations)}, indent=2)) else: @@ -1417,32 +1416,16 @@ def _resolve_artefact_age( # --------------------------------------------------------------------------- -def _maybe_load_config( - config_path: Optional[str], - *, - strict: bool = False, - allow_retention_only: bool = False, -): - """Load the ``ForgeConfig`` (or retention-only config) from YAML. - - ``strict=False`` (default) is the best-effort mode used by the - row-id / run-id erasure paths: those subcommands work without a - config (the corpus-row erasure is intentionally config-agnostic), - so a missing / unreadable / invalid YAML falls back to ``None``. - When *config_path* was explicitly supplied the fallback is logged - at ``WARNING`` (not silent) so an operator relying on it for - external-copies detection sees that the load failed. - - ``strict=True`` is used by ``--check-policy`` where the operator - is *explicitly* asking for a retention-policy report against the - loaded config. In that mode a malformed YAML or a schema error - must surface as the original ``ConfigError`` / ``OSError`` so the - operator sees the validation failure instead of a misleading - "no `retention:` block" notice. - - When ``allow_retention_only=True``, if the YAML lacks training/model - keys but contains a ``retention:`` block, that block is loaded and - validated as a ``RetentionConfig`` wrapped in a lightweight namespace. +def _maybe_load_config(config_path: Optional[str]): + """Load the ``ForgeConfig`` from YAML, best effort. + + Used by the row-id erasure path, which works without a config (the + corpus-row erasure is intentionally config-agnostic), so a missing / + unreadable / invalid YAML falls back to ``None``. When *config_path* + was explicitly supplied the fallback is logged at ``WARNING`` (not + silent) so an operator relying on it for external-copies detection + sees that the load failed. ``--check-policy`` loads its config + strictly, through ``_load_retention_policy``. """ if not config_path: return None @@ -1454,47 +1437,11 @@ def _maybe_load_config( # ``docs/standards/error-handling.md``. import yaml as _yaml - from forgelm.config import ConfigError, RetentionConfig, load_config - - if allow_retention_only: - import os - from types import SimpleNamespace - from pydantic import ValidationError - - if not os.path.exists(config_path): - raise OSError(f"Configuration file not found: {config_path}") - - with open(config_path, "r", encoding="utf-8") as f: - try: - yaml_data = _yaml.safe_load(f) - except _yaml.YAMLError as e: - raise ConfigError(f"Invalid YAML syntax in {config_path}: {e}") from e - - if not isinstance(yaml_data, dict): - raise ConfigError( - f"Configuration file must contain a YAML mapping, got {type(yaml_data).__name__}" - ) - - if "retention" in yaml_data and "model" not in yaml_data: - # Retention-only YAML: validate specifically with RetentionConfig - retention_raw = yaml_data.get("retention") - if not isinstance(retention_raw, dict): - raise ConfigError( - f"Configuration validation failed: 'retention' must be a mapping, got {type(retention_raw).__name__}" - ) - try: - retention_obj = RetentionConfig(**retention_raw) - except ValidationError as e: - raise ConfigError(f"Configuration validation failed:\n{e}") from e - except (TypeError, ValueError) as e: - raise ConfigError(f"Configuration validation failed: {e}") from e - return SimpleNamespace(retention=retention_obj) + from forgelm.config import ConfigError, load_config try: return load_config(config_path) except (ConfigError, OSError, _yaml.YAMLError) as exc: - if strict: - raise # Best-effort fallback: row-id / run-id paths run config-free. # WARNING, not DEBUG: an operator who explicitly passed --config # (e.g. to surface the external-copies webhook warning) must see diff --git a/tests/test_gdpr_erasure.py b/tests/test_gdpr_erasure.py index 1e7299de..e29bac07 100644 --- a/tests/test_gdpr_erasure.py +++ b/tests/test_gdpr_erasure.py @@ -1112,22 +1112,92 @@ def test_check_policy_no_config_succeeds_with_zero(self, tmp_path: Path) -> None _run_purge_cmd(args, output_format="json") assert ei.value.code == 0 - def test_check_policy_retention_only_config_succeeds(self, tmp_path: Path, capsys) -> None: - """#1054: a config file with only a retention block is accepted and validated.""" - import json + def test_check_policy_retention_only_config_applies_its_horizon(self, tmp_path: Path, capsys) -> None: + """#1054: a file holding only `retention:` is accepted, and its horizon is the one applied. + + The report is 60 days old: past the configured 30-day horizon, within the 90-day default, + so a loader that fell back to the defaults would report nothing. + """ + import time + from forgelm.cli.subcommands._purge import _run_purge_cmd + report = tmp_path / "data_audit_report.json" + report.write_text("{}") + sixty_days_ago = time.time() - 60 * 86400 + os.utime(report, (sixty_days_ago, sixty_days_ago)) cfg = tmp_path / "retention-only.yaml" cfg.write_text("retention:\n ephemeral_artefact_retention_days: 30\n") args = _build_args(check_policy=True, config=str(cfg), output_dir=str(tmp_path)) with pytest.raises(SystemExit) as ei: _run_purge_cmd(args, output_format="json") assert ei.value.code == 0 - captured = capsys.readouterr() - data = json.loads(captured.out) + data = json.loads(capsys.readouterr().out) assert data["success"] is True - assert "violations" in data - assert "count" in data + assert data["count"] == 1 + assert data["violations"][0]["artefact_kind"] == "data_audit_report" + assert data["violations"][0]["horizon_days"] == 30 + + def test_check_policy_retention_only_null_block_has_nothing_to_enforce(self, tmp_path: Path, capsys) -> None: + """#1054: `retention:` with no value behaves as in a full config: the "nothing to enforce" note, exit 0.""" + from forgelm.cli.subcommands._purge import _run_purge_cmd + + cfg = tmp_path / "retention-null.yaml" + cfg.write_text("retention:\n") + args = _build_args(check_policy=True, config=str(cfg), output_dir=str(tmp_path)) + with pytest.raises(SystemExit) as ei: + _run_purge_cmd(args, output_format="json") + assert ei.value.code == 0 + data = json.loads(capsys.readouterr().out) + assert data["violations"] == [] + assert "count" not in data + assert "nothing to enforce" in data["note"] + + @pytest.mark.parametrize( + "body", + [ + "retention:\n ephemeral_artefact_retention_days: 30\nretension:\n staging_ttl_days: 1\n", + "retention:\n ephemeral_artefact_retention_days: 30\ntraining:\n num_train_epochs: 3\n", + ], + ids=["misspelt-top-level-key", "partial-training-config"], + ) + def test_check_policy_other_top_level_keys_mean_a_full_config(self, tmp_path: Path, body: str) -> None: + """#1054: only a file whose sole key is `retention` skips full validation; a typo is not ignored.""" + from forgelm.cli.subcommands._purge import _run_purge_cmd + + cfg = tmp_path / "config.yaml" + cfg.write_text(body) + args = _build_args(check_policy=True, config=str(cfg), output_dir=str(tmp_path)) + with pytest.raises(SystemExit) as ei: + _run_purge_cmd(args, output_format="json") + assert ei.value.code == 1 + + def test_check_policy_retention_only_error_omits_raw_input(self, tmp_path: Path, capsys) -> None: + """#1054: like a full config's, a retention-only validation error names the field but not the value.""" + from forgelm.cli.subcommands._purge import _run_purge_cmd + + cfg = tmp_path / "retention-secret.yaml" + cfg.write_text("retention:\n ephemeral_artefact_retention_days: fake-secret-value\n api_token: fake-token\n") + args = _build_args(check_policy=True, config=str(cfg), output_dir=str(tmp_path)) + with pytest.raises(SystemExit) as ei: + _run_purge_cmd(args, output_format="json") + assert ei.value.code == 1 + error = json.loads(capsys.readouterr().out)["error"] + assert "ephemeral_artefact_retention_days" in error + assert "api_token" in error + assert "fake-secret-value" not in error + assert "fake-token" not in error + + def test_check_policy_missing_config_exits_config_error(self, tmp_path: Path, capsys) -> None: + """An explicit `--config` that does not exist exits 1 with the usual not-found message.""" + from forgelm.cli.subcommands._purge import _run_purge_cmd + + missing = tmp_path / "missing.yaml" + args = _build_args(check_policy=True, config=str(missing), output_dir=str(tmp_path)) + with pytest.raises(SystemExit) as ei: + _run_purge_cmd(args, output_format="json") + assert ei.value.code == 1 + assert f"Configuration file not found: {missing}" in json.loads(capsys.readouterr().out)["error"] def test_check_policy_retention_only_invalid_exits_config_error(self, tmp_path: Path) -> None: """#1054: an invalid retention block in a retention-only file exits EXIT_CONFIG_ERROR (1).""" From 1c3ab049628703d18296182adbaa72a0e855fcc1 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Fri, 23 Oct 2026 17:57:03 +0300 Subject: [PATCH 3/3] docs(changelog): say that only an invalid full config exits 1 (#1054) The retention-only entry read as if any file with another top-level key exited 1. A valid full training config still reports normally; only an invalid one, such as a misspelt `retension:` or a partial training config, exits 1. Refs #1054 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bb3917c..146f48ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -117,9 +117,10 @@ All notable changes to ForgeLM are documented here. `CONTRIBUTING.md`, `CLAUDE.md` and `AGENTS.md` that the front door is chosen with labels and views, never by closing issues. - `forgelm purge --check-policy --config` accepts a file whose only top-level key is `retention:`, so a scheduled - retention report no longer needs a full training config next to it. A file with any other top-level key, a misspelt - `retension:` included, is still validated as a full config and exits 1. `retention:` with no value reports nothing to - enforce, as it does in a full config, and validation errors name the field without its raw value (#1054). + retention report no longer needs a full training config next to it. A file with any other top-level key is still + validated as a full config, so a misspelt `retension:` or a partial training config exits 1 instead of being ignored. + `retention:` with no value reports nothing to enforce, as it does in a full config, and validation errors name the + field without its raw value (#1054). ### Removed