Repository navigation
feat(wave-2b): Phase 16 + 19 + 21 + 35 + 36 consolidated integration - #30
Merged
Merged
Conversation
Five-phase consolidated integration covering the dependency-free batch
unblocked by the Wave 2a merge. All work lives on
closure/wave2b-integration as a single reviewable PR.
PHASE 21 — GDPR Article 17 right-to-erasure (`forgelm purge`)
- `forgelm/cli/subcommands/_purge.py` — three-mode dispatcher:
* --row-id <id> --corpus <path> — atomic JSONL row erasure, hashed
target_id (SHA-256(salt + value)), persistent per-output-dir salt
at <output_dir>/.forgelm_audit_salt (mode 0600).
* --run-id <id> --kind {staging,artefacts} — run-scoped artefact
erasure (staging dir or compliance bundle).
* --check-policy — read-only retention-policy violation report
(always exits 0; report-not-gate per design §10 Q5).
- forgelm/config.py — RetentionConfig (4 horizons + enforce mode) +
ForgeConfig.retention field. EvaluationConfig.staging_ttl_days
deprecation alias-forwards to retention.staging_ttl_days with
DeprecationWarning; conflicting values raise ConfigError.
- 6 new audit events (data.erasure_*) catalogued bilingually.
- Bilingual operator guide: docs/guides/gdpr_erasure(.md|-tr.md).
- 28 regression tests covering every design §7 acceptance row.
PHASE 35 — Air-gap pre-cache subcommands
- forgelm/cli/subcommands/_cache.py:
* cache-models --model M [--safety S] [--output DIR] — repeatable
--model; huggingface_hub.snapshot_download populates HF cache.
Resolution: --output > HF_HUB_CACHE > HF_HOME/hub > default.
* cache-tasks --tasks CSV — lm_eval.tasks.get_task_dict + dataset
download_and_prepare; requires [eval] extra (clear install hint
on missing import).
- 6 new cache.populate_* audit events catalogued.
- 14 regression tests with mocked huggingface_hub + lm_eval (suite
stays network-free + extra-free).
PHASE 36 — Compliance verification toolbelt
- verify-annex-iv <path> — Annex IV §1-9 field check + manifest hash
recompute (canonical-JSON SHA-256).
- safety-eval --model <path> {--probes <jsonl> | --default-probes} —
standalone safety eval; supports HF + GGUF (GGUF requires [export]).
- verify-gguf <path> — magic header + metadata parse + optional
SHA-256 sidecar comparison.
- New bundled probe set: forgelm/safety_prompts/default_probes.jsonl
(50 prompts × 14 harm categories).
- Public library functions exposed: verify_annex_iv_artifact,
verify_gguf, with VerifyAnnexIVResult / VerifyGgufResult types.
- 21 regression tests.
PHASE 16 — Pydantic description= migration with CI guard
- tools/check_field_descriptions.py — AST-based scanner;
--strict mode exits 1 on any field missing description=.
- .github/workflows/ci.yml — new "Pydantic description= guard"
step in lint job runs the scanner in strict mode.
- All 174 fields across 19 Pydantic config classes migrated to
Field(default=..., description=...) form.
PHASE 19 — Library API support (Implementation)
- forgelm/__init__.py rewritten as strict lazy-import facade:
* PEP 562 __getattr__ resolves stable symbols on first access via
_LAZY_SYMBOLS registry; results cached in globals() so
subsequent accesses are zero-cost.
* __dir__ lists full public surface for IDE autocomplete +
help() discovery before any attribute access.
* TYPE_CHECKING block for mypy --strict / pyright consumers.
- forgelm/_version.py — __api_version__ separate from __version__;
anchored at 1.0.0.
- forgelm/py.typed (PEP 561 marker) shipped via pyproject.toml
package-data block.
- __all__ expanded with 25 stable symbols covering every Phase 19
documented surface (training, data, audit, compliance, Phase 36
verification toolbelt, webhook).
- 13 integration tests including subprocess-based lazy-import
discipline assertions.
VERIFICATION
- ruff check + format clean
- pytest 1237 passed, 14 skipped (was 1160 baseline; +77 new)
- All 5 new subcommands surface in `forgelm --help` + epilog
- forgelm --config config_template.yaml --dry-run green
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR consolidates five implementation phases (16, 19, 21, 35, 36) of the Wave 2b closure plan. It adds GDPR erasure ( ChangesLibrary API Facade & Versioning Foundation
Configuration & Compliance Foundations
GDPR Article 17 Right-to-Erasure (Phase 21)
Verification Toolbelt (Phase 36)
Cache Operations (Phase 35)
Release Management & Metadata
Sequence DiagramsequenceDiagram
actor Op as Operator
participant CLI as forgelm CLI
participant Audit as AuditLogger
participant FS as Filesystem
participant Config as Config Loader
rect rgba(100, 150, 200, 0.5)
note over Op,Config: Row Erasure Mode (--row-id)
Op->>CLI: forgelm purge --row-id ABC --corpus data.jsonl
CLI->>Config: load config for audit context
CLI->>FS: resolve salt from .forgelm_audit_salt
CLI->>Audit: log data.erasure_requested
CLI->>FS: scan JSONL for matching rows
alt matches found
CLI->>FS: atomic rewrite (drop matched lines)
FS->>FS: fsync → os.replace
CLI->>Audit: log data.erasure_completed
CLI->>Op: print success envelope
else no matches
CLI->>Audit: log data.erasure_failed
CLI->>Op: exit with error
end
end
rect rgba(150, 100, 200, 0.5)
note over Op,Config: Retention Policy Check (--check-policy)
Op->>CLI: forgelm purge --check-policy --config config.yml
CLI->>Config: strictly load retention block
CLI->>FS: scan artefact horizons (audit log, staging, etc.)
CLI->>Audit: compute age from first timestamp per run
CLI->>Op: report violations (age > horizon), exit 0
end
rect rgba(200, 150, 100, 0.5)
note over Op,Config: Verification & Caching
Op->>CLI: forgelm verify-annex-iv artifact.json
CLI->>FS: load + validate required fields
CLI->>FS: recompute manifest hash (optional)
CLI->>Op: print valid/invalid + reason
Op->>CLI: forgelm cache-models --model llama2-7b
CLI->>FS: resolve HF cache directory
CLI->>Audit: log cache.populate_models_requested
par model download
CLI->>FS: huggingface_hub.snapshot_download(...)
FS-->>CLI: cached model path + size
end
CLI->>Audit: log cache.populate_models_completed
CLI->>Op: print success envelope
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR spans five independent implementation phases with substantial new modules (purge.py, _cache.py, verify*.py), refactored library API mechanics (lazy imports + versioning), config-level deprecation reconciliation, and corresponding comprehensive test suites. While individual features are relatively self-contained, the interaction between library versioning, config deprecation logic, and multiple CLI subcommands introduces moderate density. Heterogeneous changes across audit integration, file I/O atomicity, environment variable isolation, and JSON envelope schemas demand separate reasoning per layer. Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 5 high |
| CodeStyle | 95 minor |
🟢 Metrics 636 complexity · 36 duplication
Metric Results Complexity 636 Duplication 36
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new features and improvements, including a GDPR Article 17 erasure tool, air-gap pre-caching for models and tasks, a compliance verification toolbelt, and a library API facade. It also migrates Pydantic field definitions to include descriptions with a CI guard. The review feedback highlights that several broad exception catches (using Exception) in forgelm/cli/subcommands/_purge.py are too permissive and should be narrowed to specific exception types to improve error handling and debugging.
| os.write(fd, secrets.token_bytes(_SALT_BYTES)) | ||
| finally: | ||
| os.close(fd) | ||
| with open(salt_path, "rb") as fh: |
There was a problem hiding this comment.
The broad exception catch Exception here is too permissive and could hide unexpected bugs during the audit logging process. It is better to catch specific exceptions related to logging or I/O operations.
Collaborator
Author
There was a problem hiding this comment.
Skipped with rationale (already documented in CHANGELOG round-1 absorption notes). The broad except Exception is intentional: it is a best-effort recovery path where any unexpected exception class must funnel into the same data.erasure_failed event so a forensic reviewer sees the unfinished erasure. Narrowing would silently mask a regression class. The # noqa: BLE001 comment justifies the breadth per project standard.
| if os.path.exists(tmp_path): | ||
| try: | ||
| os.unlink(tmp_path) | ||
| except OSError: # pragma: no cover — defensive |
There was a problem hiding this comment.
Catching Exception is too broad and can mask programming errors. Consider catching specific exceptions like OSError or PermissionError that are expected during file operations.
Collaborator
Author
There was a problem hiding this comment.
Same rationale as the L150 sibling reply: best-effort recovery path; narrowing would silently mask a regression class. See CHANGELOG round-1 absorption notes.
| from forgelm._http import _mask_netloc | ||
|
|
||
| targets.append(_mask_netloc(url)) | ||
| except Exception: # noqa: BLE001 — best-effort redaction |
There was a problem hiding this comment.
Catching Exception is too broad. It is safer to catch specific exceptions that might be raised by the redaction logic.
Collaborator
Author
There was a problem hiding this comment.
Same rationale: best-effort recovery path; broad except justified per project standard with # noqa: BLE001.
| from forgelm.config import load_config | ||
|
|
||
| return load_config(config_path) | ||
| except Exception as exc: # noqa: BLE001 — best-effort: config is optional |
There was a problem hiding this comment.
Catching Exception is too broad here. Consider catching specific exceptions related to configuration loading to avoid hiding other potential issues.
Collaborator
Author
There was a problem hiding this comment.
Same rationale: best-effort recovery path; broad except justified per project standard.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
forgelm/cli/subcommands/_cache.py (1)
344-364: 💤 Low valueUnused
output_formatparameter.The
output_formatparameter is never used in_maybe_audit_logger. Since the function only returns an AuditLogger or None (not rendering output), the parameter can be removed.♻️ Proposed fix
-def _maybe_audit_logger(audit_dir: str, output_format: str): +def _maybe_audit_logger(audit_dir: str):And update the call sites at lines 142 and 256:
- audit = _maybe_audit_logger(getattr(args, "audit_dir", None) or cache_dir, output_format) + audit = _maybe_audit_logger(getattr(args, "audit_dir", None) or cache_dir)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/cli/subcommands/_cache.py` around lines 344 - 364, Remove the unused output_format parameter from the function signature of _maybe_audit_logger and its docstring; update the implementation to only accept audit_dir and return AuditLogger|None as before. Then update all callers that currently pass an output_format argument (the places invoking _maybe_audit_logger) to call it with only the audit_dir value. Ensure any type hints or tests referencing _maybe_audit_logger are adjusted to the new single-argument signature.forgelm/cli/subcommands/_purge.py (2)
805-835: ⚖️ Poor tradeoffConsider using a strategy pattern or lookup table for output rendering.
SonarCloud flags cognitive complexity of 24 (threshold 15). The nested conditionals for mode/dry_run combinations could be simplified with a dispatch table or by extracting per-mode renderers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/cli/subcommands/_purge.py` around lines 805 - 835, The _emit_purge_success function has high cognitive complexity due to nested mode/dry_run branching; refactor it into a dispatch table or per-mode renderer functions (e.g., render_row_success, render_run_success) and replace the nested if/else with a lookup keyed by payload.get("mode") (and optionally payload.get("dry_run") for separate dry-run handlers). Each renderer should accept the payload and handle both dry_run and real-run output using the same keys currently referenced (matches, first_line, row_id_hash, salt_source, warnings, would_delete, deleted, kind, run_id, bytes_freed), and the top-level _emit_purge_success should only select and call the appropriate renderer or fallback to JSON printing when output_format == "json".
733-763: ⚖️ Poor tradeoffConsider extracting timestamp parsing to reduce cognitive complexity.
SonarCloud flags cognitive complexity of 17 (threshold 15). The nested try/except blocks and conditionals could be simplified by extracting the ISO timestamp parsing into a helper function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/cli/subcommands/_purge.py` around lines 733 - 763, _refactor suggestion: The _age_from_audit_log function has high cognitive complexity due to nested parsing/try/except/conditionals; extract the ISO timestamp parsing into a small helper (e.g., _parse_iso_timestamp(ts: str) -> Optional[float]) that encapsulates datetime.fromisoformat replacement of "Z", timezone handling, and ValueError handling and returns a POSIX timestamp or None; then simplify _age_from_audit_log to iterate lines, json.loads each line, check event dict and ts existence, call _parse_iso_timestamp to get when_ts and return now - when_ts if present, using early continues to remove nested blocks and reduce complexity.forgelm/cli/subcommands/_safety_eval.py (1)
138-204: ⚖️ Poor tradeoffConsider extracting output rendering to reduce cognitive complexity.
SonarCloud flags cognitive complexity of 19 (threshold 15). The function mixes model loading, evaluation invocation, and output formatting. Extracting the output rendering (lines 180-204) into a separate helper would improve readability and testability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/cli/subcommands/_safety_eval.py` around lines 138 - 204, The _run_safety_eval_cmd function mixes evaluation logic and CLI rendering which raises cognitive complexity; extract the block that builds prints JSON/plain output and the final sys.exit into a new helper (e.g., _render_safety_eval_output(output_format, payload, model_path) or render_safety_eval_output returning an exit code) and replace the inline rendering (currently building marker, printing safety_score/safe_ratio/harm_categories/failure_reason and calling sys.exit) with a single call to that helper; ensure the helper accepts the same payload shape, handles both "json" and human-readable paths, sorts/prints harm_categories, and returns or calls sys.exit consistently so _run_safety_eval_cmd only prepares payload and delegates rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/guides/gdpr_erasure-tr.md`:
- Line 71: The exit-code descriptions in the exit-code summary contradict the
earlier description of the `--check-policy` mode: the docs state
`--check-policy` is "report-not-gate" but the table/summary treats it as
"gate-not-report"; update the exit-code summary and the related entries
(including the block referenced around the later section with the same mismatch)
so they consistently use the "report-not-gate" wording for `--check-policy` and
flip any gate/report labels and corresponding exit-code meanings to match that
semantic across the document.
In `@docs/guides/gdpr_erasure.md`:
- Line 71: Update the exit-code table entries in docs/guides/gdpr_erasure.md so
the mode wording matches the prose: replace any "gate-not-report" phrasing with
"report-not-gate" and ensure the flag name `--check-policy` is used consistently
in those rows; specifically search for occurrences of "gate-not-report" and the
exit-code table blocks (including the second occurrence around lines 118-122)
and make the table cells match the prose's "report-not-gate" / `--check-policy`
wording.
In `@forgelm/cli/subcommands/_purge.py`:
- Around line 343-352: The loop in _purge.py is checking non-existent
WebhookConfig fields (url_success, url_failure, url_safety_alert,
url_compliance_alert) so getattr always returns None; update the code to check
the actual WebhookConfig attributes used in forgelm/config.py (at minimum "url"
and "url_env") by iterating over ("url", "url_env"), and for "url_env" resolve
the environment variable value (e.g., os.environ.get(webhook.url_env)) before
passing to _mask_netloc; keep the existing try/except redaction logic and fall
back to "<webhook-url-redacted>" if masking/resolution fails so webhook URLs
appear in targets when configured.
In `@forgelm/cli/subcommands/_safety_eval.py`:
- Line 188: The code is accessing a non-existent SafetyResult field
"harm_categories", causing empty output; change the mapping to use the correct
dataclass field "category_distribution" (e.g., replace getattr(result,
"harm_categories", {}) with getattr(result, "category_distribution", {}) and
wrap with dict(... or {})). Also update the subsequent output rendering logic
that references harm categories (the block that reads/renders harm_categories)
to instead read from category_distribution so the reported categories reflect
the real SafetyResult data.
In `@forgelm/cli/subcommands/_verify_gguf.py`:
- Around line 103-119: The code currently treats empty or non-hex sidecar
contents as OK; update the sidecar handling in the block that reads sidecar_path
(referencing _file_sha256, expected_text, expected, checks, and
VerifyGgufResult) to validate that the extracted expected token is exactly a
64‑character hex string (e.g. len==64 and all chars in 0-9a-fA-F or use a
regex); if it's missing or malformed, set checks["sidecar_present"]=True,
checks["sha256_expected"]=expected_text (or ""), set
checks["sidecar_match"]=False and return a failing VerifyGgufResult with a clear
reason like "Malformed SHA-256 sidecar: expected 64-hex digest" so malformed
sidecars fail closed instead of being ignored.
- Around line 89-97: The metadata parse branch is silently skipped when the
optional gguf dependency is missing; update the import/handling so missing gguf
raises an ImportError with install instructions instead of returning a harmless
metadata_check (apply to the code paths around _maybe_parse_metadata and
verify_gguf): detect the ImportError raised when attempting to import or use
gguf, and re-raise a new ImportError that includes a short install hint like
"install forgelm[gguf]" (or appropriate extra name) so callers of verify_gguf
fail loudly; ensure VerifyGgufResult is not returned in that case and keep the
existing metadata_parse error handling unchanged.
In `@tests/test_cache_subcommands.py`:
- Around line 190-208: The test
test_cache_tasks_missing_extra_emits_install_hint must remove any preloaded
lm_eval entries from sys.modules before patching __import__; update the test to
wrap the import-blocking patch with a patch.dict on sys.modules that pops
'lm_eval' and 'lm_eval.tasks' (set them to None or remove) so that importing
lm_eval inside _run_cache_tasks_cmd goes through the patched builtins.__import__
handler; keep the existing _block_lm_eval and call to _run_cache_tasks_cmd
unchanged.
In `@tests/test_verification_toolbelt.py`:
- Around line 152-154: The minimal GGUF fixture created by _make_minimal_gguf is
not parseable by GGUFReader when the optional gguf package is installed, causing
verify_gguf to fail; to fix the tests (e.g., test_valid_magic_passes and
test_sha256_sidecar_match_passes) mock the metadata parsing instead of changing
file bytes: in those success-path tests patch the module function that tries to
parse GGUF metadata (the function named _maybe_parse_metadata or the GGUFReader
parse call) to return a benign value (e.g., None or a minimal metadata dict) so
GGUFReader/verify_gguf will treat the fixture as valid; ensure the
monkeypatch/patch is applied before invoking verify_gguf and revert after the
test.
---
Nitpick comments:
In `@forgelm/cli/subcommands/_cache.py`:
- Around line 344-364: Remove the unused output_format parameter from the
function signature of _maybe_audit_logger and its docstring; update the
implementation to only accept audit_dir and return AuditLogger|None as before.
Then update all callers that currently pass an output_format argument (the
places invoking _maybe_audit_logger) to call it with only the audit_dir value.
Ensure any type hints or tests referencing _maybe_audit_logger are adjusted to
the new single-argument signature.
In `@forgelm/cli/subcommands/_purge.py`:
- Around line 805-835: The _emit_purge_success function has high cognitive
complexity due to nested mode/dry_run branching; refactor it into a dispatch
table or per-mode renderer functions (e.g., render_row_success,
render_run_success) and replace the nested if/else with a lookup keyed by
payload.get("mode") (and optionally payload.get("dry_run") for separate dry-run
handlers). Each renderer should accept the payload and handle both dry_run and
real-run output using the same keys currently referenced (matches, first_line,
row_id_hash, salt_source, warnings, would_delete, deleted, kind, run_id,
bytes_freed), and the top-level _emit_purge_success should only select and call
the appropriate renderer or fallback to JSON printing when output_format ==
"json".
- Around line 733-763: _refactor suggestion: The _age_from_audit_log function
has high cognitive complexity due to nested parsing/try/except/conditionals;
extract the ISO timestamp parsing into a small helper (e.g.,
_parse_iso_timestamp(ts: str) -> Optional[float]) that encapsulates
datetime.fromisoformat replacement of "Z", timezone handling, and ValueError
handling and returns a POSIX timestamp or None; then simplify
_age_from_audit_log to iterate lines, json.loads each line, check event dict and
ts existence, call _parse_iso_timestamp to get when_ts and return now - when_ts
if present, using early continues to remove nested blocks and reduce complexity.
In `@forgelm/cli/subcommands/_safety_eval.py`:
- Around line 138-204: The _run_safety_eval_cmd function mixes evaluation logic
and CLI rendering which raises cognitive complexity; extract the block that
builds prints JSON/plain output and the final sys.exit into a new helper (e.g.,
_render_safety_eval_output(output_format, payload, model_path) or
render_safety_eval_output returning an exit code) and replace the inline
rendering (currently building marker, printing
safety_score/safe_ratio/harm_categories/failure_reason and calling sys.exit)
with a single call to that helper; ensure the helper accepts the same payload
shape, handles both "json" and human-readable paths, sorts/prints
harm_categories, and returns or calls sys.exit consistently so
_run_safety_eval_cmd only prepares payload and delegates rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7aaf4e15-5b9a-4b69-b0d0-c2dfd25af3af
📒 Files selected for processing (25)
.github/workflows/ci.ymlCHANGELOG.mddocs/guides/gdpr_erasure-tr.mddocs/guides/gdpr_erasure.mddocs/reference/audit_event_catalog-tr.mddocs/reference/audit_event_catalog.mdforgelm/__init__.pyforgelm/_version.pyforgelm/cli/__init__.pyforgelm/cli/_dispatch.pyforgelm/cli/_parser.pyforgelm/cli/subcommands/_cache.pyforgelm/cli/subcommands/_purge.pyforgelm/cli/subcommands/_safety_eval.pyforgelm/cli/subcommands/_verify_annex_iv.pyforgelm/cli/subcommands/_verify_gguf.pyforgelm/config.pyforgelm/py.typedforgelm/safety_prompts/default_probes.jsonlpyproject.tomltests/test_cache_subcommands.pytests/test_gdpr_erasure.pytests/test_library_api.pytests/test_verification_toolbelt.pytools/check_field_descriptions.py
Comment on lines
+89
to
+97
| metadata_check = _maybe_parse_metadata(path) | ||
| checks["metadata_parsed"] = metadata_check["parsed"] | ||
| if metadata_check.get("error"): | ||
| # Metadata corruption is a real integrity problem. | ||
| return VerifyGgufResult( | ||
| valid=False, | ||
| reason=f"GGUF metadata block could not be parsed: {metadata_check['error']}", | ||
| checks=checks, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "_verify_gguf.py" -o -name "verify_gguf.py" 2>/dev/null | head -20Repository: cemililik/ForgeLM
Length of output: 43
🏁 Script executed:
git ls-files | grep -E "verify_gguf|gguf" | head -20Repository: cemililik/ForgeLM
Length of output: 193
🏁 Script executed:
cat -n forgelm/cli/subcommands/_verify_gguf.py | head -150Repository: cemililik/ForgeLM
Length of output: 6906
Raise ImportError with install hints when gguf is missing instead of silently skipping metadata verification.
The code catches the ImportError for the optional gguf dependency (line 140) and returns a silent no-error state (error: None) rather than raising with an install hint. This allows verify_gguf() to report success even when the advertised metadata verification phase never ran because the optional dependency was absent, violating the coding guideline: "Optional dependencies must raise ImportError with install hints when missing instead of silent import fallbacks."
This masks a missing extra from operators and weakens the integrity check. The guideline applies to all forgelm/**/*.py files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forgelm/cli/subcommands/_verify_gguf.py` around lines 89 - 97, The metadata
parse branch is silently skipped when the optional gguf dependency is missing;
update the import/handling so missing gguf raises an ImportError with install
instructions instead of returning a harmless metadata_check (apply to the code
paths around _maybe_parse_metadata and verify_gguf): detect the ImportError
raised when attempting to import or use gguf, and re-raise a new ImportError
that includes a short install hint like "install forgelm[gguf]" (or appropriate
extra name) so callers of verify_gguf fail loudly; ensure VerifyGgufResult is
not returned in that case and keep the existing metadata_parse error handling
unchanged.
Collaborator
Author
There was a problem hiding this comment.
Fixed in commit 3fc77bf (Wave 2b Round-1). Malformed SHA-256 sidecars (empty file, "TODO" placeholder, truncated paste, non-hex digest, wrong-length digest) now fail closed with a clear "Malformed SHA-256 sidecar" reason instead of being silently accepted. Sidecar tokens validated against ^[0-9a-fA-F]{64}$ regex before any digest comparison. Four new parametrised regression cases in tests/test_verification_toolbelt.py::TestVerifyGguf::test_malformed_sidecar_fails_closed.
Contributor
There was a problem hiding this comment.
@cemililik, thanks for the detailed description — the sidecar hardening is a solid fix and aligns with what the PR objectives already noted. However, the fix described addresses sidecar token validation, while the original review concern was specifically about the gguf import fallback in _maybe_parse_metadata silently returning a harmless no-error result when the gguf package is not installed.
That said, looking at this more carefully against the project's established convention (using a _HAS_<DEP> + _require_<dep>() lazy-import pattern where optional features are truly optional), the silent skip in _maybe_parse_metadata appears to be by design — the verify_gguf docstring explicitly marks the metadata block check as "optional, when the gguf package is installed". That aligns with how the project treats other optional extras like datasketch, xxhash, etc. So my original concern was likely misapplied in this context.
To summarize:
- ✅ Sidecar hardening (your fix): malformed sidecars now fail closed via
^[0-9a-fA-F]{64}$validation — this is a real improvement. - ℹ️ Metadata skip when
ggufabsent: per the project's opt-in lazy-import convention, this appears intentional — metadata parsing is a best-effort optional layer, not a hard gate.
✏️ Learnings added
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 30
File: forgelm/cli/subcommands/_verify_gguf.py:96-104
Timestamp: 2026-05-04T20:11:00.034Z
Learning: In cemililik/ForgeLM, `verify_gguf` in `forgelm/cli/subcommands/_verify_gguf.py` intentionally skips the GGUF metadata block parse when the optional `gguf` Python package is not installed (via `_maybe_parse_metadata`). This is by design: metadata parsing is marked as "optional, when the `gguf` package is installed" in the docstring. Do not flag the silent import fallback in `_maybe_parse_metadata` as a violation — it follows the project's established opt-in lazy-import pattern (consistent with learning 756299d4-4493-48cb-bd2a-633c8a537c1a).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 13
File: forgelm/data_audit.py:71-79
Timestamp: 2026-04-28T11:08:48.337Z
Learning: In this project (cemililik/ForgeLM), treat heavy/optional dependencies (e.g., datasketch, xxhash, unsloth, llama_cpp) as opt-in extras: use a lazy-import pattern instead of failing eagerly at module import time. Concretely: do a top-level `try/except ImportError` that sets a `_HAS_<DEP>` boolean and assigns the missing module name to `None`, then gate feature code behind a `_require_<dep>()` helper that raises `ImportError` with an install hint only when the feature is actually invoked. Do not use a convention where importing the module always fails due to optional extras; this is intentional to keep base-install users from being forced to install optional dependencies just to use other features.
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 19
File: forgelm/judge.py:213-240
Timestamp: 2026-04-30T17:54:35.571Z
Learning: In cemililik/ForgeLM, YAML-driven numeric config fields (e.g., `JudgeConfig.batch_size`) are already validated at config-parse time using the Pydantic constraints on the corresponding config dataclass (e.g., `Field(ge=1)`). During code review, avoid flagging internal helper code (such as batching logic in functions like `_generate_responses_batched` in `forgelm/judge.py`) for missing additional runtime checks for those same YAML-derived parameters, since the Pydantic validation is the authoritative boundary gate. Only add/require runtime validation when entering true public library-API boundaries (e.g., functions like `run_safety_evaluation`) where callers may bypass the YAML/Pydantic config parsing path.
Inline review on the Wave 2b consolidation surfaced 7 valid defects + 5
non-actionable suggestions. All fixes minimal, behaviour-preserving
where possible, and validated against the full 1241-test suite.
Code fixes:
- _purge.py::_extract_webhook_targets: iterated over fictitious
WebhookConfig fields (url_success / url_failure / etc.) so the
data.erasure_warning_external_copies event always carried an empty
webhook_targets list. Now reads the real schema (webhook.url
literal + webhook.url_env resolved via os.environ). Caught
exceptions narrowed from bare Exception to specific classes.
- _safety_eval.py:188: payload key harm_categories →
category_distribution to match the real SafetyResult dataclass
field. Previous name meant getattr always returned empty dict.
- _verify_gguf.py::verify_gguf: malformed SHA-256 sidecars (empty,
non-hex, wrong-length, "TODO" placeholder) now fail closed with a
clear reason instead of being silently accepted. Sidecar tokens
validated against ^[0-9a-fA-F]{64}$ before digest comparison.
Doc fixes:
- gdpr_erasure.md:118 + gdpr_erasure-tr.md:118: exit-code table cell
gate-not-report → report-not-gate (matches the prose at L71 and
the design spec; the tables previously contradicted the paragraph
immediately above them).
Test fixes:
- test_cache_subcommands.py::test_cache_tasks_missing_extra_emits_install_hint:
explicitly pops preloaded lm_eval entries from sys.modules before
patching builtins.__import__. Without the pop, a sibling test
that imports lm_eval first leaves the cached module and Python
short-circuits the import, bypassing the patched handler.
- test_verification_toolbelt.py::TestVerifyGguf success paths gained
a _stub_metadata_parse(monkeypatch) helper that patches
_maybe_parse_metadata to a benign no-op so the minimal-fixture
tests stay green when the optional gguf package is installed.
- 4 new parametrised regression cases for malformed-sidecar fail-
closed behaviour.
Skipped (rationale in CHANGELOG):
- "Raise ImportError on missing gguf package": conflicts with
optional-extras project standard. Magic + SHA-256 checks remain
useful without gguf installed.
- "Refactor _emit_purge_success / _age_from_audit_log /
_run_safety_eval_cmd for cognitive complexity": Sonar gate already
passes; refactor would trade complexity for indirection.
- "Drop unused output_format param from _maybe_audit_logger": real
nit but signature touch invalidates 4 call sites for stylistic gain.
- "Narrow remaining 3 except Exception blocks": intentionally broad;
each is a best-effort failure path where any class must funnel
into the same operator-facing failure. # noqa: BLE001 comments
already justify per project standard.
Verification: ruff check + format clean, pytest 1241 passed, 14
skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
forgelm/cli/subcommands/_verify_gguf.py (1)
171-174:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRaise an install-hint
ImportErrorwhenggufis missing (don’t silently skip metadata verification).Line 173/174 currently downgrades a missing
ggufdependency into a “no error” path, which letsverify_ggufcontinue as if metadata verification were optional-success. For this codebase, missing optional deps inforgelm/**/*.pyshould fail loudly with an install hint.Suggested patch
def _maybe_parse_metadata(path: str) -> Dict[str, Any]: @@ try: from gguf import GGUFReader # type: ignore[import-untyped] - except ImportError: - return {"parsed": False, "error": None, "tensor_count": None} + except ImportError as exc: + raise ImportError( + "Optional dependency 'gguf' is required for metadata verification. " + "Install with: pip install 'forgelm[export]'" + ) from exc @@ def _run_verify_gguf_cmd(args, output_format: str) -> None: @@ try: result = verify_gguf(path) + except ImportError as exc: + _output_error_and_exit( + output_format, + str(exc), + EXIT_CONFIG_ERROR, + ) except OSError as exc: _output_error_and_exit( output_format, f"Could not read GGUF file {path!r}: {exc}.", EXIT_TRAINING_ERROR, )#!/bin/bash # Verify the silent optional-dependency fallback and missing ImportError handling path. set -euo pipefail echo "1) Locate gguf ImportError handling:" rg -n -C3 'from gguf import GGUFReader|except ImportError' forgelm/cli/subcommands/_verify_gguf.py echo echo "2) Check dispatcher exception branches:" rg -n -C3 'try:|except ImportError|except OSError|verify_gguf\(path\)' forgelm/cli/subcommands/_verify_gguf.pyAs per coding guidelines: “
forgelm/**/*.py: Optional dependencies must raise ImportError with install hints when missing instead of silent import fallbacks”.Also applies to: 207-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/cli/subcommands/_verify_gguf.py` around lines 171 - 174, The try/except around "from gguf import GGUFReader" silently returns a success-like dict when gguf is missing; change it to raise an ImportError with a clear install hint so metadata verification fails loudly. Replace the except branch in the import block used by verify_gguf to raise ImportError("gguf is required for GGUF verification; install with pip install gguf") (or similar), and apply the same pattern to the other import-handling section referenced around the GGUFReader usage so both import-failure paths provide the install hint instead of returning {"parsed": False, "error": None, ...}.
🧹 Nitpick comments (3)
tests/test_cache_subcommands.py (2)
143-144: ⚡ Quick winAdd an exit-code assertion to anchor the audit-chain test.
The
pytest.raises(SystemExit)call captureseibut never assertsei.value.code == 0. If the command exits with a non-zero code (e.g., due to a config issue with the fake), therequestedevent may still be written whilecompletedis absent — the ordering assertion at line 156 would catch that pair relationship, but the test silently permits unexpected failure paths.✏️ Proposed fix
- with pytest.raises(SystemExit): + with pytest.raises(SystemExit) as ei: _cache._run_cache_models_cmd(args, output_format="json") + assert ei.value.code == 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cache_subcommands.py` around lines 143 - 144, The test currently uses pytest.raises(SystemExit) without asserting the exit code; update the with block that calls _cache._run_cache_models_cmd(args, output_format="json") to capture the exception (e.g., "as ei") and add an assertion that ei.value.code == 0 to ensure the command exited successfully; locate the with pytest.raises(SystemExit) block around the _run_cache_models_cmd call and append the exit-code assertion immediately after the call to anchor the audit-chain test.
212-212: ⚡ Quick winRemove the redundant
list()wrapping_sys.modules.The list comprehension iterates over
_sys.moduleswithout modifying it, so there is no "dictionary changed during iteration" risk. The innerlist()makes an unnecessary full copy of all module keys.✏️ Proposed fix
- for cached in [k for k in list(_sys.modules) if k == "lm_eval" or k.startswith("lm_eval.")]: + for cached in [k for k in _sys.modules if k == "lm_eval" or k.startswith("lm_eval.")]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cache_subcommands.py` at line 212, The list comprehension currently copies _sys.modules unnecessarily; change the iteration in the test loop that builds cached modules (the comprehension using list(_sys.modules)) to iterate directly over _sys.modules (i.e., remove the list() wrapper) so the for cached in [...] comprehension uses _sys.modules and preserves the same filter logic (k == "lm_eval" or k.startswith("lm_eval.")) without making a full copy.forgelm/cli/subcommands/_safety_eval.py (1)
107-107: ⚡ Quick winAdd
# NOSONARto resolve SonarCloud suppression-comment syntax warnings.SonarCloud now interprets
# noqa:comments, butBLE001is a ruff rule ID that SonarCloud does not recognise, so it flags the comment syntax as invalid. All three# noqa: BLE001comments (lines 107, 130, 173) trigger this warning. Appending# NOSONARkeeps the ruff suppression intact while satisfying SonarCloud's format:♻️ Proposed fix for all three occurrences
- except Exception as exc: # noqa: BLE001 — best-effort: GGUF loaders surface a wide failure surface (file truncation, magic mismatch, llama-cpp-python missing). + except Exception as exc: # noqa: BLE001 # NOSONAR — best-effort: GGUF loaders surface a wide failure surface (file truncation, magic mismatch, llama-cpp-python missing).- except Exception as exc: # noqa: BLE001 — broad surface from HF loaders (FileNotFoundError, OSError, ValueError, KeyError, etc.). + except Exception as exc: # noqa: BLE001 # NOSONAR — broad surface from HF loaders (FileNotFoundError, OSError, ValueError, KeyError, etc.).- except Exception as exc: # noqa: BLE001 — broad surface: classifier load failure, OOM, generation crash all funnel into one operator-facing failure path. + except Exception as exc: # noqa: BLE001 # NOSONAR — broad surface: classifier load failure, OOM, generation crash all funnel into one operator-facing failure path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/cli/subcommands/_safety_eval.py` at line 107, Append " # NOSONAR" to the existing ruff suppression comments on the broad except clauses in forgelm/cli/subcommands/_safety_eval.py (the lines that read "except Exception as exc: # noqa: BLE001 — best-effort: ..."). Update all three occurrences so each comment becomes " # noqa: BLE001 ... # NOSONAR" to preserve the ruff suppression while satisfying SonarCloud's suppression-comment syntax.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/guides/gdpr_erasure-tr.md`:
- Around line 103-108: The docs list a non-implemented payload field and a bogus
target kind: remove `policy_check` from the `target_kind` set (the CLI
`--check-policy` does not emit erasure audit events) and remove the
`recommendation` column from the `data.erasure_warning_*` rows (the
implementation in `_purge.py` only adds `affected_run_ids`, `synthetic_files`,
and `webhook_targets` to warning events); update the table rows for
`data.erasure_warning_memorisation`,
`data.erasure_warning_synthetic_data_present`, and
`data.erasure_warning_external_copies` to mirror `_purge.py`'s actual payloads
and ensure `data.erasure_failed`/`data.erasure_completed` rows stay consistent
with their implemented fields.
In `@docs/guides/gdpr_erasure.md`:
- Around line 103-108: The table claims audit events that the code doesn't emit;
update the docs to match current behavior by removing `policy_check` from
`target_kind` (since _run_purge_check_policy() does not log erasure audit
events) and remove the `recommendation` field from all three warning rows;
instead document only the actual extra fields added by the warnings in
_purge.py: `data.erasure_warning_memorisation` includes `affected_run_ids`,
`data.erasure_warning_synthetic_data_present` includes `synthetic_files`, and
`data.erasure_warning_external_copies` includes `webhook_targets`; ensure
`data.erasure_requested`, `data.erasure_completed`, and `data.erasure_failed`
rows remain accurate (including `error_class`/`error_message` for failed events)
and that `target_id` hashing and other noted fields remain unchanged.
In `@forgelm/cli/subcommands/_purge.py`:
- Around line 724-733: The current horizons list used by the --check-policy scan
omits the per-run staging layout and the raw_documents retention horizon; update
the horizons tuple construction (the variable named horizons in _purge.py) to
include entries for the resolved run-kind staging targets (i.e., the
final_model.staging.<run_id> layout that _resolve_run_kind_targets() recognizes)
and for the raw documents retention (use retention.raw_documents_retention_days
and the path(s) that correspond to the raw_documents location), so the policy
checker inspects per-run staging directories and raw_documents artefacts when
computing overdue artefacts.
- Around line 645-648: The current check "if run_id in fname" is too loose and
can delete files whose names merely contain the run_id substring; tighten the
match in the loop that iterates over compliance_dir (the block using
compliance_dir, fname, fpath, run_id, paths) by only appending files whose
filename contains the run id as a discrete token or matches supported filename
shapes. Replace the substring check with either (a) a regex that enforces
separators/boundaries around run_id (e.g. require start/end or delimiter
characters like _, -, . around run_id) or (b) parse the run id from the filename
by splitting on known delimiters and compare equality, then append fpath only
when that strict match succeeds.
- Around line 805-821: The current _maybe_load_config silences all config load
errors which lets an invalid --config be treated as "no retention" downstream;
change _maybe_load_config to accept a strict: bool flag (default False) and, on
exception from load_config, re-raise the exception when strict is True (keep the
current debug/log+None behavior when strict is False). Then update callers that
perform strict validation (specifically _run_purge_check_policy) to call
_maybe_load_config(config_path, strict=True) so real ConfigError/load failures
surface for --check-policy, while keeping the best-effort fallback for other
non-critical callers.
In `@forgelm/cli/subcommands/_safety_eval.py`:
- Around line 138-210: The function _run_safety_eval_cmd exceeds allowed
cognitive complexity due to the output rendering block; extract that rendering
logic into a new helper function (e.g., _emit_safety_result) that accepts the
payload and output_format and performs the JSON vs human-printing and
category/failure printing, then replace the existing rendering lines in
_run_safety_eval_cmd with a single call to _emit_safety_result(payload,
output_format). Ensure the helper is unit-testable, imports json/sys as needed,
and preserve behavior including EXIT selection via sys.exit after the helper
returns.
- Around line 105-106: The import error arises because load_gguf_model is
referenced from forgelm.inference but is not implemented; either implement
load_gguf_model in the inference module to return a (model, tokenizer) tuple (to
match the unpacking used by the safety-eval command) and update its
docstring/typing, or remove GGUF detection/handling from the safety-eval
model-path logic so it no longer tries to import load_gguf_model; also update
the exception message that currently suggests installing [export] to accurately
reflect that GGUF loading is unimplemented. Ensure you touch the symbols
load_gguf_model, the safety-eval model-path detection/unpacking, and the
ImportError handling to keep behavior and logs consistent.
---
Duplicate comments:
In `@forgelm/cli/subcommands/_verify_gguf.py`:
- Around line 171-174: The try/except around "from gguf import GGUFReader"
silently returns a success-like dict when gguf is missing; change it to raise an
ImportError with a clear install hint so metadata verification fails loudly.
Replace the except branch in the import block used by verify_gguf to raise
ImportError("gguf is required for GGUF verification; install with pip install
gguf") (or similar), and apply the same pattern to the other import-handling
section referenced around the GGUFReader usage so both import-failure paths
provide the install hint instead of returning {"parsed": False, "error": None,
...}.
---
Nitpick comments:
In `@forgelm/cli/subcommands/_safety_eval.py`:
- Line 107: Append " # NOSONAR" to the existing ruff suppression comments on the
broad except clauses in forgelm/cli/subcommands/_safety_eval.py (the lines that
read "except Exception as exc: # noqa: BLE001 — best-effort: ..."). Update all
three occurrences so each comment becomes " # noqa: BLE001 ... # NOSONAR" to
preserve the ruff suppression while satisfying SonarCloud's suppression-comment
syntax.
In `@tests/test_cache_subcommands.py`:
- Around line 143-144: The test currently uses pytest.raises(SystemExit) without
asserting the exit code; update the with block that calls
_cache._run_cache_models_cmd(args, output_format="json") to capture the
exception (e.g., "as ei") and add an assertion that ei.value.code == 0 to ensure
the command exited successfully; locate the with pytest.raises(SystemExit) block
around the _run_cache_models_cmd call and append the exit-code assertion
immediately after the call to anchor the audit-chain test.
- Line 212: The list comprehension currently copies _sys.modules unnecessarily;
change the iteration in the test loop that builds cached modules (the
comprehension using list(_sys.modules)) to iterate directly over _sys.modules
(i.e., remove the list() wrapper) so the for cached in [...] comprehension uses
_sys.modules and preserves the same filter logic (k == "lm_eval" or
k.startswith("lm_eval.")) without making a full copy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fda9cab-fd57-4a50-81dc-196231d29b3f
📒 Files selected for processing (8)
CHANGELOG.mddocs/guides/gdpr_erasure-tr.mddocs/guides/gdpr_erasure.mdforgelm/cli/subcommands/_purge.pyforgelm/cli/subcommands/_safety_eval.pyforgelm/cli/subcommands/_verify_gguf.pytests/test_cache_subcommands.pytests/test_verification_toolbelt.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_verification_toolbelt.py
…red) Two pre-merge agent reviews of PR #29 (`development → main` release-PR consolidating Wave 1 + Wave 2a) — `opusreview-pr29.md` (Opus 4.7, correctness + cohesion + CI bütünlük) and `kimireview-pr29.md` (Kimi, code-correctness focus) — surfaced one HIGH that was load-bearing across the approve / reject family plus seven MEDIUM / LOW / NIT defensive-coding gaps. 10 of 11 findings shipped fixes; F-PR29-03 (cohesion: `_assert_audit_log_readable_or_exit` in `_approve.py` rather than `_audit_log_reader.py`) deferred to Phase 21 with rationale. Code: - `forgelm/compliance.py::generate_training_manifest` — webhook config now persisted into the manifest (and therefore into the co-located `compliance_report.json` the approve / reject dispatchers read). Without this stanza `_build_approval_notifier` saw `webhook_cfg=None` and silently no-op'd `notify_success` / `notify_failure`; an operator with a valid Slack / Teams webhook in the training YAML got the "awaiting approval" notification but **never** the follow-up success / rejection notification. (Kimi F-37-01 — HIGH) - `forgelm/cli/_result.py::_output_result` — `default=str` on `json.dumps` so a `TrainResult.resource_usage` containing a `Path` / `datetime` / numpy scalar cannot crash with `TypeError`. Mirrors Wave 2a Round-5 F-R5-06. (Kimi F-TRAIN-01 — MEDIUM) - `forgelm/cli/_dispatch.py::main` — extracted body to `_main_inner` + top-level `try/except KeyboardInterrupt → sys.exit(EXIT_TRAINING_ERROR)`. Ctrl-C during `parse_args()` / wizard / config-load no longer bubbles up as shell-shaped 130. (Kimi F-CLI-01 — LOW) - `forgelm/cli/subcommands/_doctor.py::_render_json` — `ensure_ascii=False` so Turkish operator names + Unicode paths render as literal characters, not `\uXXXX` escapes. (Kimi F-34-01 — LOW) - `forgelm/cli/subcommands/_doctor.py::_run_doctor_cmd` — env-var scan now also checks `HF_DATASETS_OFFLINE=1`. Mirrors what `_apply_offline_flag` already sets at training time. (Kimi F-XPR-01 — LOW) - `forgelm/cli/subcommands/_doctor.py::_walk_hf_cache_bounded` — log message prints `?` (not literal `None`) when `OSError.filename` is unset. (Opus F-PR29-06 — LOW) - `forgelm/cli/subcommands/_approvals.py::_emit_show_json` and `_emit_pending_json` — `default=str` + `ensure_ascii=False`. (Kimi F-37-02 — NIT) - `tests/test_grpo_reward.py::test_reward_funcs_is_callable_list` — patch targets corrected from `forgelm.trainer.SFTTrainer` / `forgelm.trainer.SFTConfig` to `trl.SFTTrainer` / `trl.SFTConfig` (lazy-import contract: not module-level on `forgelm.trainer`). Fix unblocks PR #29 CI matrix on all four Python versions. (Opus F-PR29-01.b — HIGH; CI matrix unblocker) - `tests/test_faz27_narrow_exceptions.py::test_english_payload_returns_en` — `pytest.importorskip("langdetect")`. CI matrix uses `[dev]` only; `langdetect` is under the optional `[ingestion]` extra. Fix unblocks PR #29 CI matrix. (Opus F-PR29-01.a — HIGH; CI matrix unblocker) Docs: - `docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md` §5.1 event catalog row for `data.erasure_requested` now lists `salt_source` (row mode only). Wave 2a Round-5 F-R5-05 introduced the field in §5.4 but did not propagate it to §5.1; Phase 21 implementer following §5.1 alone would have missed the salt-source persistence and silently regressed F-R5-05's hash-discontinuity detection property. (Opus F-PR29-02 — HIGH) Verification: ruff format + check clean; pytest 1161 passed, 14 skipped, 0 failed (was 1160 passed, 2 failed on PR #29 CI matrix); forgelm --config config_template.yaml --dry-run exits 0. Deferred with rationale (documented in CHANGELOG): - Opus F-PR29-03 (cohesion of `_assert_audit_log_readable_or_exit`) - Opus F-PR29-04 (collapse triple `os.path.isfile` into enum helper) - Opus F-PR29-05 (test-injection env var harden) - Opus F-PR29-07 (verify-audit option-error code 2 vs 1 ambiguity) - Opus F-PR29-09 (orchestrator test-injection pragma: no cover) - Kimi F-INFRA-01 (`_get_version` duplication) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…kip) Round 2 of inline review on Wave 2b consolidation. Six valid defects + three actionable nits fixed; one duplicate (gguf ImportError) skipped with rationale. All fixes minimal, behaviour-preserving where possible. Code fixes: - _purge.py::_resolve_run_kind_targets: tightened compliance-bundle filename match. Previous `run_id in fname` could delete files belonging to a different run when the short run-id was a substring. Now uses token-boundary helper _filename_contains_run_id requiring flanking _ / - / . or string edges. - _purge.py::_scan_retention_violations: expanded horizons tuple to include per-run staging dirs (final_model.staging.<run_id>/ — what the trainer creates since Phase 9 v2) and raw_documents horizon (covers raw_documents/ + legacy ingestion_output/). New helpers _discover_per_run_staging_horizons + _discover_raw_documents_horizons. - _purge.py::_maybe_load_config: added strict=False param. --check-policy now invokes with strict=True so malformed YAML / Pydantic validation surfaces as EXIT_CONFIG_ERROR; row-id / run-id paths keep silent-degrade-to-None (config-agnostic by design). - _safety_eval.py::_load_model_for_safety: removed phantom GGUF branch. forgelm.inference has no load_gguf_model function; operators see honest "not yet supported" message naming the Phase 36+ shim instead of misleading [export]-extra hint. - _safety_eval.py: extracted _emit_safety_result + _build_safety_eval_payload helpers to drop Sonar S3776 cognitive complexity. Behaviour-preserving; helpers in __all__ for tests. - _safety_eval.py: appended # NOSONAR to broad-except suppressions. Doc fixes: - gdpr_erasure(.md|-tr.md):103-108 + audit_event_catalog(.md|-tr.md):61 — removed `policy_check` from target_kind (--check-policy emits no events) + removed `recommendation` column from warning rows (_detect_warning_conditions doesn't emit it). Tables now mirror what the code actually emits. Test fixes: - test_cache_subcommands.py audit-chain test: added ei.value.code == 0 anchor so the chain assertions don't silently pass on a code-1 / code-2 run. - test_cache_subcommands.py lm_eval pop: dropped redundant list() wrapper around _sys.modules. Skipped (duplicate of Round-1): - "Raise ImportError on missing gguf package": optional-extras project standard; magic + SHA-256 checks remain useful without it. Verification: ruff check + format clean, pytest 1241 passed, 14 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
11 SonarCloud findings on PR #30 absorbed. All fixes minimal, behaviour-preserving, validated against the full 1241-test suite. - S5713 _purge.py:720: dropped redundant `except Exception` fallback on the strict config-load path. The (ConfigError, OSError) tuple already covers every documented failure mode of `load_config`; any other class would be a contract violation in the loader and should propagate so we fix the root cause rather than mask it. - S1192 __init__.py: extracted six submodule-path constants (_M_DATA_AUDIT, _M_COMPLIANCE, _M_VERIFY_ANNEX_IV, _M_VERIFY_GGUF, _M_UTILS, _M_BENCHMARK) so the _LAZY_SYMBOLS dict no longer duplicates the same string 7+ times. Single-source rename target. - S7632 _cache.py:158, 281 + _verify_gguf.py:179: appended ` # NOSONAR` to the broad-except suppression comments so SonarCloud's suppression-comment syntax is satisfied alongside ruff's `# noqa: BLE001`. - S1172 _cache.py:344: dropped the unused `output_format` parameter from `_maybe_audit_logger`; updated both call sites. - S3776 _purge.py x4 (complexity 16/17/17/24): refactored: * `_age_from_audit_log` (24 → ~6) split into `_parse_iso_timestamp_to_posix` + `_genesis_timestamp_from_lines` helpers. * `_emit_purge_success` (17 → ~5) replaced if/elif chain with a `_TEXT_RENDERERS` dict + per-mode `_render_*_success` helpers. Adding a new purge mode is now a one-row + one-helper edit. * `_run_purge_row_id` (17 → ~6) extracted `_validate_match_count_or_fail`, `_emit_row_dry_run`, and `_perform_row_erasure_and_audit` so the dispatcher reads top-down (validate → branch dry-run → real erasure). * `_run_purge_check_policy` (16 → 14) — already partially fixed by the S5713 broad-except removal; remaining branches now stay under the ceiling. All refactors are behaviour-preserving — no test changes needed. Verification: ruff check + format clean, pytest 1241 passed, 14 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SonarCloud S3776 still flagged _resolve_run_kind_targets at complexity 17 after the round-3 refactor (the staging + artefacts branches each carried their own nested checks). Replaced the if/elif chain with a _RUN_KIND_RESOLVERS dispatch dict + two per-kind helpers (_staging_targets_for_run, _artefact_targets_for_run). Adding a new --kind value is now a one-row + one-helper edit. Behaviour-preserving (28 Phase 21 tests still green; full suite stays at 1241 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
F-W2B-01 (HIGH) + F-W2B-05: reconcile Annex IV writer/verifier shape drift. ``compliance.export_compliance_artifacts`` previously wrote ``manifest["annex_iv"]`` verbatim (the operator-supplied 7-key provider block); ``verify-annex-iv`` then rejected it because the verifier checks Annex IV §1-9 categories (system_identification, system_components, data_governance, …). Added ``build_annex_iv_artifact(manifest)`` which synthesises the §1-9 layout from manifest sub-blocks while preserving the operator block under ``provider_metadata``. Added ``compute_annex_iv_manifest_hash(artifact)`` shared by writer + verifier so canonicalisation cannot drift byte-for-byte. Writer now stamps ``metadata.manifest_hash`` so the verifier's tampering branch fires on round-trip. ``_verify_annex_iv._compute_manifest_hash`` delegates to the shared helper. Regression tests cover writer→ verifier round-trip + tampering rejection. F-W2B-02 (HIGH): switch RetentionConfig migration heuristic from value-comparison to Pydantic v2 ``model_fields_set``. The previous "legacy != default" check could not distinguish "operator omitted the legacy field" from "operator wrote the default value explicitly", so a YAML that set only the canonical block could be silently overridden by Pydantic-filled legacy defaults. Regression test asserts canonical block wins when YAML omits the legacy key. F-W2B-03: ``_atomic_rewrite_dropping_lines`` now ``flush() + os.fsync()`` before ``os.replace()`` so a crash mid-purge cannot leave a half-written audit log on disk. Regression test spies on fsync + replace to assert ordering. F-W2B-04: ``forgelm cache models|tasks --output <dir>`` now warns when ``<dir>`` diverges from the env-resolved cache (HF_HUB_CACHE > HF_HOME/hub > ~/.cache/huggingface/hub) so operators don't end up warming a directory ``forgelm doctor`` later flags as misconfigured. F-W2B-06: ``_scan_run_ids_with_final_model`` falls back to walking the audit log for ``human_approval.granted`` events when no staging directories survive (already-promoted runs) so the listing remains complete after promotion. F-W2B-07: ``tools/check_field_descriptions.py`` now recognises the Pydantic v2 ``Annotated[T, Field(..., description=...)]`` form so the CI gate stops false-flagging valid v2 idioms. F-W2B-08: ``_verify_gguf.py:113`` sidecar read now uses an explicit context manager (file handle was previously left to GC). F-W2B-09 (skip): PR description refresh — not a code change. Round-4 absorption keeps the existing 1245-test suite green and ships the writer/verifier reconciliation that the verifier toolbelt needed to be self-consistent on a freshly exported artefact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_grpo_reward.py (1)
290-322:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
TestGrpoClassifierTrustRemoteCodeis missing thetorch_availableskip guard.The file-level comment on line 12 states "ForgeTrainer requires torch — skip all tests if not available", and
TestGrpoRewardCallablecarries that guard.TestGrpoClassifierTrustRemoteCodeimportsForgeTrainerat line 306 inside the test but has no@pytest.mark.skipifon the class. In an environment wheretorchis absent andforgelm/trainer.pyimportstorchat module scope, this test will error withImportErrorrather than being skipped cleanly.🛡️ Proposed fix
+@pytest.mark.skipif(not torch_available, reason="torch not installed") class TestGrpoClassifierTrustRemoteCode:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_grpo_reward.py` around lines 290 - 322, Add the pytest skip guard to the TestGrpoClassifierTrustRemoteCode class so the whole class is skipped when torch isn't available: wrap the class with the same pytest.mark.skipif condition used elsewhere (torch_available) or annotate the class with `@pytest.mark.skipif`(not torch_available, reason="ForgeTrainer requires torch") so importing ForgeTrainer inside test_classifier_reward_passes_trust_remote_code does not raise ImportError when torch is absent.
🧹 Nitpick comments (1)
tests/test_faz27_narrow_exceptions.py (1)
43-63: 💤 Low valueLGTM —
pytest.importorskipguard is correctly placed and thereasonkwarg is valid.The
reasonkeyword argument topytest.importorskipis fully supported and will surface the provided string as the skip message whenlangdetectcannot be imported.One minor note: the return value of
pytest.importorskipis the imported module, and the docs recommend it "should be assigned to its canonical name." Since the module itself isn't used in the test body (only_detect_languageis needed), the discarded return value is harmless — but assigning it avoids any future linter noise:🔧 Optional: assign the return value
- pytest.importorskip( + langdetect = pytest.importorskip( # noqa: F841 "langdetect", reason=( "_detect_language is constant 'unknown' without the optional " "[ingestion] extra; this happy-path assertion only meaningful " "when langdetect is installed." ), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_faz27_narrow_exceptions.py` around lines 43 - 63, The pytest.importorskip call currently discards its return value; assign it to the canonical module name to avoid linter noise and follow docs: change the call to capture the result (e.g. langdetect = pytest.importorskip(...)) while keeping the same reason string and leaving the rest of the test using _detect_language unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 50-53: Change the Markdown code span that currently contains a
leading space (written as ` ` # NOSONAR` `) to remove the extra space so it
reads `` `# NOSONAR` ``; update the CHANGELOG.md line referencing the filename
and adjust the inline code span around # NOSONAR to eliminate the inner space so
markdownlint MD038 no longer flags it.
In `@docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md`:
- Line 242: The docs table entry for data.erasure_requested incorrectly lists
`policy_check` as a possible value of `target_kind`; remove `policy_check` from
the `target_kind` set in the `data.erasure_requested` row so the allowed values
read `{row, staging, artefacts, policy_check}` → `{row, staging, artefacts}`
(and ensure any accompanying mention of `policy_check` in that cell is deleted),
since `--check-policy` is read-only and should not generate an erasure request.
In `@docs/guides/gdpr_erasure-tr.md`:
- Around line 90-93: Update the "legacy-only" bullet to match the actual parser
behavior: explain that _reconcile_staging_ttl_days consults model_fields_set, so
any explicitly set key (e.g., evaluation.staging_ttl_days: 7) is treated as
"set" and will trigger alias-forwarding to retention.staging_ttl_days and emit a
DeprecationWarning; keep the existing descriptions for the canonical-only,
identical-values (DeprecationWarning, canonical wins), and differing-values
(raise ConfigError) cases but clarify that "set" is determined by
model_fields_set rather than "non-default value" wording.
In `@docs/guides/gdpr_erasure.md`:
- Around line 69-71: Update the guide text around the --check-policy behavior to
clarify that it does not unconditionally exit 0: change the "Always exits 0"
statement to say "Successful policy reports exit 0; config-load failures exit
with EXIT_CONFIG_ERROR (non-zero)" and mention that explicit --config
missing/invalid/unreadable returns EXIT_CONFIG_ERROR; apply the same
clarification to the other occurrence referenced (lines 118-122) so the docs
match the implementation that returns a config error instead of unconditional 0.
In `@forgelm/cli/subcommands/_cache.py`:
- Around line 308-359: The command resolves cache_dir but never hands it to the
datasets layer; update the plumbing so the chosen cache_dir is actually used:
change _prepare_one_task(name, task_obj) to accept a cache_dir parameter and
thread that value into whatever code calls datasets.load_dataset(...,
cache_dir=cache_dir) or dataset.download_and_prepare(cache_dir=cache_dir);
alternatively set os.environ["HF_DATASETS_CACHE"]=cache_dir before
resolving/preparing each task in _prepare_one_task; ensure the same change is
applied to the other cache-tasks code path that emits success (the other call
site of _prepare_one_task/_emit_cache_success) so the audit/logged cache_dir
matches where artifacts are stored.
In `@forgelm/cli/subcommands/_purge.py`:
- Around line 927-950: The scan is reusing a single audit_age_seconds for all
artefacts; change it to resolve age per artefact by consulting the audit log for
that artefact's run_id instead of a single genesis timestamp: for each horizon
entry produced by _discover_per_run_staging_horizons and
_discover_raw_documents_horizons, extract the run_id (or otherwise identify the
owning run), call a per-run lookup (extend _age_from_audit_log or add a helper
like _age_from_audit_log_for_run) to obtain that run's audit age, and only if
that lookup fails fall back to mtime via _resolve_artefact_age; update the loop
that calls _resolve_artefact_age to pass the per-artifact audit age (or allow
_resolve_artefact_age to accept a run_id and perform the lookup internally) so
each artefact uses its correct run-specific audit timestamp.
In `@forgelm/cli/subcommands/_safety_eval.py`:
- Around line 18-25: Update the exit handling and header comment so a model that
completes evaluation but fails the safety gate uses the evaluation-failure exit
code; specifically, change any exit that currently returns EXIT_CONFIG_ERROR for
the non-passing safety path to EXIT_EVAL_FAILURE (search for uses of
EXIT_CONFIG_ERROR in the safety-eval logic, including the non-passing branch
where the command completes but produces a negative safety verdict and the
occurrences referenced around the current lines ~190-191), and update the header
comment block that lists exit codes to reflect "1 — evaluation completed but
safety thresholds exceeded (operator-actionable; the model failed the gate)"
maps to EXIT_EVAL_FAILURE. Ensure you only change the error code constant and
the comment text, leaving runtime error exits (EXIT_RUNTIME_ERROR) and
config-error semantics unchanged.
In `@forgelm/compliance.py`:
- Around line 718-755: The code currently persists the entire webhook model (via
webhook_cfg.model_dump(mode="json")) which can include a raw credential in the
"url" field; instead, when building manifest["webhook_config"] only persist
non-secret/env-backed fields (e.g. "url_env", "secret_env", "notify_on_success",
"notify_on_failure", "notify_on_revert", "notify_on_awaiting_approval",
"timeout_seconds", "retry_count", "retry_backoff_seconds") and explicitly omit
or redact "url"; update both the model_dump path (call model_dump then drop or
replace the "url" key) and the AttributeError fallback (do not include
getattr(webhook_cfg, "url", ...) there) so no literal webhook URL is written to
the compliance artifact.
In `@forgelm/config.py`:
- Around line 963-1007: The code treats the retention default (e.g. 7) as an
explicit canonical value, causing false conflicts; fix by only treating
retention.staging_ttl_days as canonical when it was explicitly provided. Detect
explicitness the same way you do for evaluation: add
canonical_was_explicitly_set = bool(self.retention is not None and
"staging_ttl_days" in self.retention.model_fields_set) and set canonical =
self.retention.staging_ttl_days if canonical_was_explicitly_set else None, then
use that canonical (and its explicitness) in the subsequent branches instead of
assuming any retention object implies an explicit staging_ttl_days.
---
Outside diff comments:
In `@tests/test_grpo_reward.py`:
- Around line 290-322: Add the pytest skip guard to the
TestGrpoClassifierTrustRemoteCode class so the whole class is skipped when torch
isn't available: wrap the class with the same pytest.mark.skipif condition used
elsewhere (torch_available) or annotate the class with `@pytest.mark.skipif`(not
torch_available, reason="ForgeTrainer requires torch") so importing ForgeTrainer
inside test_classifier_reward_passes_trust_remote_code does not raise
ImportError when torch is absent.
---
Nitpick comments:
In `@tests/test_faz27_narrow_exceptions.py`:
- Around line 43-63: The pytest.importorskip call currently discards its return
value; assign it to the canonical module name to avoid linter noise and follow
docs: change the call to capture the result (e.g. langdetect =
pytest.importorskip(...)) while keeping the same reason string and leaving the
rest of the test using _detect_language unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b203deb-50c8-4bbd-894b-42cbd39a124f
📒 Files selected for processing (25)
CHANGELOG.mddocs/analysis/code_reviews/gdpr-erasure-design-202605021414.mddocs/guides/gdpr_erasure-tr.mddocs/guides/gdpr_erasure.mddocs/reference/audit_event_catalog-tr.mddocs/reference/audit_event_catalog.mdforgelm/__init__.pyforgelm/cli/_dispatch.pyforgelm/cli/_result.pyforgelm/cli/subcommands/_approvals.pyforgelm/cli/subcommands/_cache.pyforgelm/cli/subcommands/_doctor.pyforgelm/cli/subcommands/_purge.pyforgelm/cli/subcommands/_safety_eval.pyforgelm/cli/subcommands/_verify_annex_iv.pyforgelm/cli/subcommands/_verify_gguf.pyforgelm/compliance.pyforgelm/config.pytests/test_cache_subcommands.pytests/test_faz27_narrow_exceptions.pytests/test_gdpr_erasure.pytests/test_grpo_reward.pytests/test_integration_smoke.pytests/test_verification_toolbelt.pytools/check_field_descriptions.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_cache_subcommands.py
- docs/reference/audit_event_catalog.md
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/reference/audit_event_catalog-tr.md
- forgelm/cli/subcommands/_verify_annex_iv.py
- forgelm/cli/subcommands/_verify_gguf.py
- tests/test_verification_toolbelt.py
- forgelm/init.py
- tests/test_gdpr_erasure.py
Code (8 fixes):
- ``forgelm/cli/subcommands/_cache.py`` — ``cache-tasks`` now stamps
``HF_DATASETS_CACHE=<cache_dir>`` before the loop AND threads
``cache_dir`` into ``_prepare_one_task`` so the underlying
``datasets.download_and_prepare()`` writes parquet shards under the
resolved cache_dir instead of silently landing under
``~/.cache/huggingface/datasets``. Without this the JSON-envelope
``cache_dir`` claim and the on-disk artefact location diverged,
defeating the air-gap workflow.
- ``forgelm/cli/subcommands/_purge.py`` — retention scan now ages each
artefact against its OWN audit timestamp. Builds
``{None: genesis_age, run_id: age_for_run}`` once per scan; the two
discovery helpers (``_discover_per_run_staging_horizons`` /
``_discover_raw_documents_horizons``) emit 4-tuples carrying the
owning ``run_id``; ``_resolve_artefact_age(path, audit_ages, run_id,
now)`` looks up per-run age first, falls back to genesis, then
mtime. Previously every per-run staging dir was aged from the
merged-log genesis timestamp — wildly wrong for runs created weeks
apart. Removed the now-orphaned ``_age_from_audit_log`` +
``_genesis_timestamp_from_lines`` helpers.
- ``forgelm/cli/subcommands/_safety_eval.py`` — non-passing safety
branch now exits ``EXIT_EVAL_FAILURE`` (3) instead of
``EXIT_CONFIG_ERROR`` (1). A model that completes evaluation but
fails the gate is an evaluation failure, not a config error;
regulated CI pipelines can now distinguish "the gate said no" from
"the run never started because the config was rejected". Header
exit-code list updated to add the new code-3 entry.
- ``forgelm/compliance.py`` — ``manifest["webhook_config"]`` no
longer carries the literal ``url`` field. The Slack/Teams URL can
embed a webhook secret in its path; persisting it verbatim into a
plain-JSON compliance artefact (typically committed to the
auditor's evidence bundle) leaks the credential. We now persist
only env-backed indirection fields (``url_env``, ``secret_env``,
``notify_on_*``, ``timeout_seconds``, ``retry_*``) so the approve/
reject dispatchers can re-resolve the webhook at runtime without
any credential reaching disk. Both the ``model_dump`` path and
the AttributeError fallback are updated.
- ``forgelm/config.py`` — retention reconcile now applies the same
``model_fields_set`` test to the canonical block as it does to the
legacy block. An operator who writes ``retention:
{audit_log_retention_days: 1825}`` (no staging key) leaves
``staging_ttl_days`` at its default of 7; treating that 7 as an
explicit canonical value would spuriously raise ``ConfigError``
when paired with ``evaluation.staging_ttl_days: 14``. The
canonical value is now ``None`` unless the operator actually wrote
the field.
Tests (2 fixes):
- ``tests/test_grpo_reward.py`` — ``TestGrpoClassifierTrustRemoteCode``
now carries a class-level ``@pytest.mark.skipif(not torch_available)``
so importing ``ForgeTrainer`` inside the test does not raise
ImportError on torch-less matrix builds.
- ``tests/test_faz27_narrow_exceptions.py`` — capture
``langdetect = pytest.importorskip(...)`` (with ``# noqa: F841``)
per pytest docs so the binding documents the optional dep
relationship.
Docs (4 fixes):
- ``CHANGELOG.md`` — drop the leading space inside the
```# NOSONAR``` code span (markdownlint MD038).
- ``docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md``
— remove ``policy_check`` from the ``data.erasure_requested``
``target_kind`` set; ``--check-policy`` is read-only and emits no
audit events.
- ``docs/guides/gdpr_erasure.md`` + ``-tr.md`` — clarify that the
deprecation reconciler keys on Pydantic v2 ``model_fields_set``
(operator-explicit) rather than "non-default value", and that
``--check-policy`` exits ``EXIT_CONFIG_ERROR`` (non-zero) when the
supplied ``--config`` is missing/unreadable/invalid (no longer
"always exits 0"). Exit-code table updated for parity in both
language mirrors.
Skips:
- F-W2B-09 (PR description refresh) — tracked separately, not a code
change.
Validation: ``ruff format`` clean, ``ruff check`` clean, ``pytest``
1245 passed / 14 skipped, ``forgelm --dry-run config_template.yaml``
green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit follow-ups on Round-5 (4d636c6): - ``forgelm/config.py`` — alias-forward path now uses ``retention.model_copy(update={"staging_ttl_days": legacy})`` instead of ``RetentionConfig(staging_ttl_days=legacy)`` so an operator who set ``retention: {audit_log_retention_days: 1825}`` plus ``evaluation.staging_ttl_days: 14`` does not lose the explicit ``audit_log_retention_days`` value when the deprecation reconciler fires. - ``forgelm/cli/subcommands/_cache.py`` — ``cache-tasks`` now resolves via the *Datasets* env chain (``HF_DATASETS_CACHE > HF_HOME/datasets > ~/.cache/huggingface/datasets``) instead of the Hub chain. HF treats hub vs datasets caches as separate roots; the previous code set ``HF_DATASETS_CACHE`` to the hub-shaped path so parquet shards landed in ``hub/`` instead of ``datasets/``. Added ``_resolve_env_datasets_cache_dir`` + ``_resolve_datasets_cache_dir`` helpers; ``_warn_on_cache_dir_divergence`` now takes a ``kind`` argument so the warning names the right env var per subcommand. SonarCloud Quality Gate failures (5 issues): - ``forgelm/config.py:966+979`` (S2259, None-narrowing) — bind ``self.evaluation`` / ``self.retention`` to local variables before the ``model_fields_set`` check + attribute access, so the static analyser sees the same narrowing the runtime ``is not None`` guard enforces. - ``forgelm/cli/subcommands/_purge.py:1056`` (S3776, complexity 18 > 15) — extracted ``_absorb_audit_line`` from ``_build_audit_age_lookup`` to drop the per-line guard chain out of the file-walk loop. - ``tools/check_field_descriptions.py:117`` (S3776, complexity 17 > 15) — extracted ``_scan_field_stmt`` from ``_scan_class`` so the per-stmt branch chain lives in a single-purpose helper. - ``tests/test_faz27_narrow_exceptions.py:52`` (S1481, unused local ``langdetect``) — bind the importorskip result and consume it via a ``hasattr`` sanity check (also documents the API surface the downstream consumer relies on). Validation: ``ruff format`` clean, ``ruff check`` clean, ``pytest`` 1245 passed / 14 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
forgelm/config.py (1)
930-1030: ⚡ Quick winSonarCloud quality gate failure: extract helper to reduce cognitive complexity to ≤15.
Cognitive complexity of
_reconcile_staging_ttl_daysexceeds the SonarCloud S3776 threshold (15). The five-branch state machine is clearly structured, but the alias-forwarding branch (lines 991–1010) and the comparison branch (lines 1011–1029) can be extracted without changing behaviour.♻️ Refactoring sketch — extract `_emit_legacy_alias_forward_warning`
+ `@staticmethod` + def _emit_legacy_alias_forward_warning() -> None: + warnings.warn( + "`evaluation.staging_ttl_days` is deprecated and forwards to " + "`retention.staging_ttl_days` for the v0.5.5 → v0.6.x window. " + "Move the value under the new top-level `retention:` block; the " + "deprecated field is removed in v0.7.0.", + DeprecationWarning, + stacklevel=4, + ) + def _reconcile_staging_ttl_days(self) -> None: ... if legacy is not None and canonical is None: - if retention is not None: - self.retention = retention.model_copy(update={"staging_ttl_days": legacy}) - else: - self.retention = RetentionConfig(staging_ttl_days=legacy) - warnings.warn(...) + self.retention = ( + retention.model_copy(update={"staging_ttl_days": legacy}) + if retention is not None + else RetentionConfig(staging_ttl_days=legacy) + ) + self._emit_legacy_alias_forward_warning() return(Note:
stacklevel=4is required in the extracted method to preserve the original warning attribution to the caller of_reconcile_staging_ttl_days.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/config.py` around lines 930 - 1030, The method _reconcile_staging_ttl_days is too complex; extract the alias-forwarding branch and the "both set and equal" warning branch into small helpers to reduce cognitive complexity. Create e.g. _apply_legacy_alias_forward(self, legacy, retention) which updates self.retention using retention.model_copy(update=...) or RetentionConfig(...) and emits the DeprecationWarning with stacklevel=4, and _emit_legacy_match_warning(self) which emits the DeprecationWarning (also stacklevel=4). Replace the inline blocks in _reconcile_staging_ttl_days with calls to these helpers, keeping all existing messages, use the existing symbols (RetentionConfig, self.retention, legacy, canonical) and preserve behavior and return paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@forgelm/cli/subcommands/_safety_eval.py`:
- Around line 131-136: The ImportError handler currently tells users to "pip
install forgelm", which is misleading because transformers is a required base
dependency; update the except ImportError block that calls
_output_error_and_exit (in _safety_eval.py) to provide an actionable hint:
report the actual exception (exc) and suggest verifying the current installation
and environment (e.g., ensure the package was installed into the active
virtualenv or container) and, if needed, reinstall or explicitly install the
missing package with a command such as "pip install transformers" or "pip
install --force-reinstall forgelm", instead of instructing only "pip install
forgelm". Ensure the message remains concise and includes the exception text.
---
Nitpick comments:
In `@forgelm/config.py`:
- Around line 930-1030: The method _reconcile_staging_ttl_days is too complex;
extract the alias-forwarding branch and the "both set and equal" warning branch
into small helpers to reduce cognitive complexity. Create e.g.
_apply_legacy_alias_forward(self, legacy, retention) which updates
self.retention using retention.model_copy(update=...) or RetentionConfig(...)
and emits the DeprecationWarning with stacklevel=4, and
_emit_legacy_match_warning(self) which emits the DeprecationWarning (also
stacklevel=4). Replace the inline blocks in _reconcile_staging_ttl_days with
calls to these helpers, keeping all existing messages, use the existing symbols
(RetentionConfig, self.retention, legacy, canonical) and preserve behavior and
return paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 016bb4fd-17b6-4c72-9bca-21417a18310e
📒 Files selected for processing (12)
CHANGELOG.mddocs/analysis/code_reviews/gdpr-erasure-design-202605021414.mddocs/guides/gdpr_erasure-tr.mddocs/guides/gdpr_erasure.mdforgelm/cli/subcommands/_cache.pyforgelm/cli/subcommands/_purge.pyforgelm/cli/subcommands/_safety_eval.pyforgelm/compliance.pyforgelm/config.pytests/test_faz27_narrow_exceptions.pytests/test_grpo_reward.pytools/check_field_descriptions.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_faz27_narrow_exceptions.py
- forgelm/cli/subcommands/_purge.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_grpo_reward.py
- forgelm/compliance.py
- tools/check_field_descriptions.py
- forgelm/cli/subcommands/_cache.py
- ``forgelm/cli/subcommands/_safety_eval.py`` — ``transformers`` ImportError handler now reports the actual exception text and suggests environment verification (`python -c 'import transformers'`) plus a reinstall command. The previous "pip install forgelm" hint was misleading because ``transformers`` is a *core* dependency, not an optional extra; if the import fails, the operator's environment is broken (wrong virtualenv / corrupted install) and a fresh ``pip install forgelm`` would re-pull the same broken set. - ``forgelm/config.py`` — extracted ``_apply_legacy_alias_forward`` and ``_emit_legacy_match_warning`` from ``_reconcile_staging_ttl_days`` so the validator method stays under the SonarCloud cognitive-complexity ceiling. Behaviour preserved: ``model_copy(update=...)`` still mirrors the alias onto the canonical block while preserving any other ``retention.*`` fields, and both DeprecationWarnings now use ``stacklevel=4`` so the warning surfaces at the operator's ``ForgeConfig(...)`` call frame rather than inside the Pydantic ``@model_validator`` machinery. Validation: ``ruff format`` clean, ``ruff check`` clean, ``pytest`` 1245 passed / 14 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
forgelm/config.py (1)
1008-1049: 💤 Low valueAdd a unit test to verify
stacklevel=4points the warning at the operator's call site, not Pydantic internals.Existing tests in
tests/test_gdpr_erasure.pyverify that theDeprecationWarningis raised with the correct message, but they don't check where the warning points (filename:lineno). The call stack fromwarnings.warn()upwards includes Pydantic's validator dispatch layer, whose depth may vary across Pydantic versions. A test assertingwarning.filenameresolves to the test file (notpydantic/...) would pin the stacklevel down and catch future regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/config.py` around lines 1008 - 1049, Add a unit test that instantiates ForgeConfig (triggering _reconcile_staging_ttl_days → _apply_legacy_alias_forward or _emit_legacy_match_warning) inside a warnings.catch_warnings(record=True) block with warnings.simplefilter("always"), capture the emitted DeprecationWarning(s), and assert the warning message matches the expected text and that warning.filename (or str(w.filename)) refers to the test file (i.e. does not contain "pydantic" or other third-party paths) to ensure stacklevel=4 surfaces at the operator call site; reference _apply_legacy_alias_forward and _emit_legacy_match_warning in the test so it covers both forward and match-warning paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@forgelm/config.py`:
- Around line 1008-1049: Add a unit test that instantiates ForgeConfig
(triggering _reconcile_staging_ttl_days → _apply_legacy_alias_forward or
_emit_legacy_match_warning) inside a warnings.catch_warnings(record=True) block
with warnings.simplefilter("always"), capture the emitted DeprecationWarning(s),
and assert the warning message matches the expected text and that
warning.filename (or str(w.filename)) refers to the test file (i.e. does not
contain "pydantic" or other third-party paths) to ensure stacklevel=4 surfaces
at the operator call site; reference _apply_legacy_alias_forward and
_emit_legacy_match_warning in the test so it covers both forward and
match-warning paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65e86624-7ce0-4c11-8ac4-bad9463d6764
📒 Files selected for processing (2)
forgelm/cli/subcommands/_safety_eval.pyforgelm/config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- forgelm/cli/subcommands/_safety_eval.py
- ``tests/test_gdpr_erasure.py`` — new ``test_deprecation_warnings_attribute_to_operator_call_site`` exercises both deprecation paths (``_apply_legacy_alias_forward`` for legacy-only, ``_emit_legacy_match_warning`` for both-set-equal) inside a ``warnings.catch_warnings(record=True)`` block and asserts that the captured ``WarningMessage.filename`` is this test file (not a path under ``pydantic/``). Locks in the stacklevel tuning so a future Pydantic upgrade that changes the validator wrapper depth fails the test before the misleading attribution leaks into operators' logs. - ``forgelm/config.py`` — bumped both deprecation warnings from ``stacklevel=4`` to ``stacklevel=5``. Empirically Pydantic v2's ``@model_validator(mode="after")`` wrapper adds one more frame than I'd budgeted, so stacklevel=4 was landing at ``pydantic/main.py:<init line>`` instead of the operator's ``ForgeConfig(...)`` call site. Confirmed by the new regression test. Validation: ``ruff format`` clean, ``ruff check`` clean, ``pytest`` 1246 passed / 14 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Self-contained multi-agent review prompt targeting Wave 2b's five phases (16, 19, 21, 35, 36) at HEAD ``36afbc0``. Tuned for a ship-or-block decision after five rounds of absorption have already landed: - Areas-to-scrutinise section is keyed off the surfaces where Rounds 1-5 made the biggest changes (atomic rewrite ordering, per-run audit-age lookup, Annex IV writer/verifier round-trip, Hub vs. Datasets cache resolver split, RetentionConfig ``model_copy(update=...)`` preservation). - Severity bar narrowed for the final round: ``CRITICAL`` / ``HIGH`` reserved for genuine merge blockers; ``LOW`` / ``NIT`` items will be tracked as follow-up issues, not absorbed. - Cross-phase checks XPR-01 through XPR-09 cover audit-event vocabulary stability, exit-code contract, JSON envelope schema, doc/code drift, vacuous tests, webhook-URL redaction, Sonar gate, backwards compatibility, and ``model_copy`` deprecation-forward call sites. - Required deliverable structure includes a per-agent ``Merge recommendation`` paragraph so the maintainer can compare agents head-to-head. Mirrors the format of the Wave 2a Round-2 prompt (``wave2a-round2-review-prompt.md``); same skeleton so prior reviewers can pick up cold. ``.gitignore`` allowlist extended to re-include the new prompt under the otherwise-internal ``docs/analysis/code_reviews/`` tree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Final pre-merge absorption of the multi-agent review on PR #30. Skips 12 LOW/NIT items with documented reasons; absorbs 24 across tests, code, and docs. CODE EDITS (5): - _safety_eval.py F-36-02: transformers ImportError -> EXIT_TRAINING_ERROR (2) - _cache.py F-XPR-01: huggingface_hub ImportError -> EXIT_TRAINING_ERROR (2) - _cache.py F-35-01: HF_DATASETS_CACHE try/finally restore (no env leak) - _purge.py F-XPR-02: _maybe_load_config narrow exception tuple - __init__.py F-19-02: __dir__ filters single-underscore privates TEST ADDITIONS (52 new tests, suite 1246 -> 1298): - test_gdpr_erasure.py +11: F-21-T-01..07 (truncated salt, prefix matcher, per-run audit-age x3, --check-policy strict-load x3, model_copy preservation, fsync fd pinning) - test_cache_subcommands.py +10: F-35-T-01..03 (Hub/Datasets resolver split + cross-bleed, env stamp + leak defence + restore, mid-batch failure ordering) - test_verification_toolbelt.py +11: F-36-T-01 (safety-eval exit-3), F-36-03 (parametrised tampering x10) - test_library_api.py +4: F-19-T-01 (forbid forgelm.* submodules), F-19-T-02 (__dir__ no imports), F-19-02 dir-leak regression, __api_version__ rule pin - test_check_field_descriptions.py NEW (17): F-16-01 scanner own test suite (recognition branches, model_config skip, multi-class discrimination, --strict CLI exit codes, canonical config gate) DOC EDITS (4): - json-output.md (EN+TR) F-XPR-04 (HIGH): 6 new subcommand sections with success-envelope schemas + per-key tables + exit-code maps - safety.md (EN+TR) F-XPR-04b: "200-prompt" -> "50-prompt" reconciled with bundled default_probes.jsonl + parser --help - reference/cli.md (EN+TR) F-XPR-04c: 6 rows added to subcommand table - cut-release SKILL + release.md F-19-01: __api_version__ bump policy documented (Step 3.5 + parallel standard section) SKIPPED (12 documented): - F-21-04, F-21-05 (defensive only / pre-existing pattern) - F-36-04 (design preference, documented contract) - F-XPR-04d, F-XPR-01-stds (pre-existing in main, OOS) - F-XPR-T-04 (release-time manual) - F-XPR-T-03 (cosmetic rename) - F-16-02 (extension hook intentional) - F-XPR-T-01, F-XPR-T-02 (covered by dispatcher tests / no-finding) - F-36-T-02, F-36-T-03 (minor edge / covered transitively) VALIDATION - ruff format clean - ruff check clean - pytest 1298 passed / 14 skipped (+52 new) - forgelm --dry-run config_template.yaml green Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
10 faz status update: Wave 2a (PR #28, merged 2026-05-04): - Faz 17 (audit --workers N) ⏳ → ✅ - Faz 18 (Library API design) ⏳ → ✅ - Faz 20 (GDPR design) ⏳ → ✅ - Faz 34 (forgelm doctor) ⏳ → ✅ - Faz 37 (forgelm approvals) ⏳ → ✅ Wave 2b (PR #30, ready to merge 2026-05-05): - Faz 16 (Pydantic description= migration + AST guard) ⏳ → ✅ - Faz 19 (Library API impl: lazy facade + py.typed) ⏳ → ✅ - Faz 21 (GDPR purge + RetentionConfig + 6 audit events) ⏳ → ✅ - Faz 35 (air-gap cache-models / cache-tasks) ⏳ → ✅ - Faz 36 (verify-annex-iv / verify-gguf / safety-eval) ⏳ → ✅ Plus: status legend entries for Wave 2a + 2b, kapanış özet paragraflarının eklenmesi, version log v2.6 (Wave 2a) + v2.7 (Wave 2b) entry'leri. Kalan iş (Wave 3+): 7 faz + 1 release. - Faz 22 (ISO 27001/SOC 2 design) - Faz 23 (ISO 27001/SOC 2 impl) - Faz 24 (bilingual TR mirror sweep) - Faz 26 (QMS bilingual mirror) - Faz 28 (Major+Minor cleanup) - Faz 30 (final docs + site finalization) - Faz 38 (forgelm reverse-pii GDPR Art. 15) - Faz 33 (v0.5.5 PyPI publish — POST-WAVE) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/cut-release/SKILL.md:
- Around line 45-47: Remove the stale sentence that states `__version__` is the
single source of truth (the duplicate line "Current version in `pyproject.toml`
line 7. Single source of truth.") and replace it with a single consistent
statement that `__version__` and `__api_version__` each have their own source of
truth (i.e., `__version__` in pyproject.toml and `__api_version__` in
forgelm/_version.py), ensuring the doc no longer contradicts the dual-contract
guidance; edit the SKILL.md paragraph containing `__version__` and
`__api_version__` to remove the conflicting sentence and keep one clear line
describing each variable's source of truth.
- Line 43: The docs currently advise using string comparison like assert
forgelm.__api_version__ >= "X.Y.0", which is incorrect for semantic versions;
update the three spots that recommend that exact assertion to instruct using
packaging.version.Version parsing instead: show importing Version from
packaging.version and using assert Version(forgelm.__api_version__) >=
Version("X.Y.0") for feature detection (i.e., replace the string-comparison
example with the Version(...) comparison and a short note to import
packaging.version.Version).
In `@docs/analysis/code_reviews/wave2b-final-merge-review-prompt.md`:
- Line 19: The PR base branch is wrongly stated as `main` in the line "**PR:**
[`#30`](https://github.com/cemililik/ForgeLM/pull/30) → base `main`"; update that
text to read `development` so it matches the PR objectives
("closure/wave2b-integration → development"). Edit the line in the
docs/analysis/code_reviews/wave2b-final-merge-review-prompt.md file to replace
`main` with `development` (keeping the PR link and formatting intact) so
reviewers see the correct target branch.
In `@docs/usermanuals/en/reference/json-output.md`:
- Around line 379-383: Update the description of checks.metadata_parsed so it no
longer implies that a missing optional parser makes the whole artifact invalid:
change the prose for `checks.metadata_parsed` to state it is true when the
`gguf` metadata block was successfully parsed, and false when the metadata block
is corrupted or when the optional `gguf` package is absent/skipped (and
explicitly note that a false `checks.metadata_parsed` does not by itself force
`valid` to be false).
- Around line 176-235: The docs and implementation diverge: update
forgelm/cli/subcommands/_purge.py to emit the documented JSON envelope shape —
in row mode replace the current "matches" and "first_line" keys with
"match_count" and "pre_erasure_line_number" and ensure "row_id_hash" is a
sha256: prefixed hex digest (never plain 64-hex); in run mode replace
"would_delete"/"deleted" with "files_modified" (list) and "bytes_freed" (int)
and include "run_id" and "kind"; for --check-policy ensure the success response
uses "violations" (list of objects with
artefact_kind/path/age_days/horizon_days/age_source) and "count" ==
len(violations) (instead of emitting "note" only), and always include "success"
and "dry_run" where applicable; locate and modify the response-building code
(functions/variables in _purge.py that assemble the envelope dict) to rename
keys, add missing fields (mode, salt_source, corpus_path when applicable), and
format row_id_hash with the "sha256:" prefix before JSON serialization.
In `@docs/usermanuals/tr/reference/json-output.md`:
- Around line 379-384: The documentation currently implies that `valid: true`
requires `checks.metadata_parsed` to be true; update the `valid` field
description so it explicitly states that `valid: true` means the magic
header/metadata block/sidecar (if present) passed verification but does not
require `checks.metadata_parsed` to be true (i.e., metadata may be skipped or
unparsed yet overall validation can still be true per tests), and make the
`checks.metadata_parsed` entry remain as-is to indicate parsing status; mention
that tests (e.g., `tests/test_verification_toolbelt.py`) allow
`checks.metadata_parsed == false` while `valid == true`.
- Around line 176-235: The docs JSON schema in docs/usermanuals/... does not
match the runtime payload emitted by the purge implementation in _purge.py;
update the documented examples and key table to exactly match the actual keys:
in row mode use "matches" and "first_line" (not
"match_count"/"pre_erasure_line_number") and document that "row_id_hash" is a
plain 64-hex string (no "sha256:" prefix); in run mode list "would_delete" /
"deleted" instead of "files_modified"; for --check-policy show that successful
responses include "note" and may omit a "retention" block and that "count" may
be absent (or set to len(violations) only if implementation emits it) — adjust
the table rows for `violations`, `count`, and `dry_run` accordingly and ensure
examples and descriptions reference the actual keys produced by _purge.py and
the --check-policy flag.
In `@forgelm/cli/subcommands/_purge.py`:
- Around line 757-765: _staging_targets_for_run currently always includes the
unscoped legacy path "final_model.staging" which allows purging unrelated runs;
update _staging_targets_for_run to only include base/"final_model.staging" when
ownership can be proved (e.g. inspect a marker/metadata file inside
final_model.staging and verify it contains the given run_id) and otherwise skip
that candidate (or require an explicit opt‑in/force flag), leaving the explicit
scoped path base/"final_model.staging.<run_id>" unchanged.
In `@tests/test_cache_subcommands.py`:
- Around line 460-487: Update the two tests that currently use plain with
pytest.raises(SystemExit): to capture the exception context and assert a
successful exit code; specifically wrap the _cache._run_cache_tasks_cmd call
with with pytest.raises(SystemExit) as ei: and after the call add assert
ei.value.code == 0. Apply this change in both tests shown (the one that checks
HF_DATASETS_CACHE is not leaked and
test_cache_tasks_restores_prior_hf_datasets_cache_env) so they validate the
happy-path exit rather than any SystemExit.
In `@tests/test_check_field_descriptions.py`:
- Around line 180-184: Update the regression comment that contradicts the test
expectation: change the wording around _annotation_has_described_field and
"Field-in-Annotated" to state that a bare Field inside typing.Annotated without
a description is detected/flagged (the test asserts len(missing) == 1), not
"missed"; reference the existing symbols _annotation_has_described_field and
Field-in-Annotated so future readers know the comment applies to that behavior
and note that if future refactors change recursion into kwargs the assertion
should be updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60f44696-4f26-4a34-8d38-400c703a94f7
📒 Files selected for processing (20)
.claude/skills/cut-release/SKILL.md.gitignoredocs/analysis/code_reviews/wave2b-final-merge-review-prompt.mddocs/standards/release.mddocs/usermanuals/en/evaluation/safety.mddocs/usermanuals/en/reference/cli.mddocs/usermanuals/en/reference/json-output.mddocs/usermanuals/tr/evaluation/safety.mddocs/usermanuals/tr/reference/cli.mddocs/usermanuals/tr/reference/json-output.mdforgelm/__init__.pyforgelm/cli/subcommands/_cache.pyforgelm/cli/subcommands/_purge.pyforgelm/cli/subcommands/_safety_eval.pyforgelm/config.pytests/test_cache_subcommands.pytests/test_check_field_descriptions.pytests/test_gdpr_erasure.pytests/test_library_api.pytests/test_verification_toolbelt.py
✅ Files skipped from review due to trivial changes (5)
- docs/usermanuals/en/reference/cli.md
- docs/standards/release.md
- .gitignore
- docs/usermanuals/tr/reference/cli.md
- forgelm/cli/subcommands/_safety_eval.py
🚧 Files skipped from review as they are similar to previous changes (4)
- forgelm/cli/subcommands/_cache.py
- tests/test_library_api.py
- forgelm/init.py
- forgelm/config.py
| | Stable library symbol added to `forgelm.__all__` (e.g. new `verify_*` function) | MINOR | MINOR | | ||
| | Stable library symbol removed or signature changed | MAJOR | MAJOR | | ||
|
|
||
| `__version__` (in [`pyproject.toml`](../../../pyproject.toml) line 7) tracks the *CLI / YAML / behavioural* contract. `__api_version__` (in [`forgelm/_version.py`](../../../forgelm/_version.py)) tracks the *Python library* contract — operators who pin `assert forgelm.__api_version__ >= "X.Y.0"` 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. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify locations where direct string comparison guidance appears
rg -n -C2 '__api_version__\s*>=' .claude/skills/cut-release/SKILL.md docs/standards/release.mdRepository: cemililik/ForgeLM
Length of output: 2460
🏁 Script executed:
#!/bin/bash
# Check if packaging library is already used in the codebase
rg -l "packaging" --type py
# Also check what version comparison patterns exist in actual code
rg "from.*version|import.*version" --type py forgelm/ | head -20Repository: cemililik/ForgeLM
Length of output: 1163
🏁 Script executed:
#!/bin/bash
# Check docs/standards/release.md line 39 context
sed -n '35,42p' docs/standards/release.mdRepository: cemililik/ForgeLM
Length of output: 723
🏁 Script executed:
#!/bin/bash
# Check if __api_version__ is actually used anywhere with comparisons
rg "__api_version__" --type py -A 2 -B 2 forgelm/Repository: cemililik/ForgeLM
Length of output: 1876
🏁 Script executed:
#!/bin/bash
# Demonstrate the lexicographic comparison issue
python3 <<'EOF'
# Show the problem with string comparison for semver
test_cases = [
("1.10.0", "1.2.0"),
("2.0.0", "10.0.0"),
("1.9.9", "1.10.0"),
]
print("String comparison (WRONG for semver):")
for v1, v2 in test_cases:
result = v1 >= v2
print(f' "{v1}" >= "{v2}": {result}')
print("\nParsed comparison (correct for semver):")
try:
from packaging.version import Version
for v1, v2 in test_cases:
result = Version(v1) >= Version(v2)
print(f' Version("{v1}") >= Version("{v2}"): {result}')
except ImportError:
print(" packaging not installed, but string comparison clearly fails")
EOFRepository: cemililik/ForgeLM
Length of output: 365
Use semantic version parsing instead of string comparison for feature detection guidance
The current guidance to use assert forgelm.__api_version__ >= "X.Y.0" for version checks is incorrect. Python string comparison is lexicographic, not semantic-aware: "1.10.0" >= "1.2.0" evaluates to False (wrong for semver). Operators following this guidance will have broken feature detection.
Update three locations (.claude/skills/cut-release/SKILL.md lines 43 and 103; docs/standards/release.md line 39) to recommend packaging.version.Version parsing:
Proposed doc fix
- operators who pin `assert forgelm.__api_version__ >= "X.Y.0"` for feature detection
+ operators should parse versions for feature detection: `from packaging.version import Version; assert Version(forgelm.__api_version__) >= Version("X.Y.0")`
- Library consumers can then pin `assert forgelm.__api_version__ >= "X.Y.0"` for feature detection.
+ Library consumers can then gate on parsed versions: `from packaging.version import Version; assert Version(forgelm.__api_version__) >= Version("X.Y.0")`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/cut-release/SKILL.md at line 43, The docs currently advise
using string comparison like assert forgelm.__api_version__ >= "X.Y.0", which is
incorrect for semantic versions; update the three spots that recommend that
exact assertion to instruct using packaging.version.Version parsing instead:
show importing Version from packaging.version and using assert
Version(forgelm.__api_version__) >= Version("X.Y.0") for feature detection
(i.e., replace the string-comparison example with the Version(...) comparison
and a short note to import packaging.version.Version).
Comment on lines
+176
to
+235
| **Row-erasure success envelope** (`forgelm purge --row-id ROW --corpus PATH`): | ||
|
|
||
| ```json | ||
| { | ||
| "success": true, | ||
| "mode": "row", | ||
| "row_id_hash": "sha256:abc123...", | ||
| "salt_source": "per_dir", | ||
| "corpus_path": "/work/train.jsonl", | ||
| "files_modified": ["/work/train.jsonl"], | ||
| "bytes_freed": 142, | ||
| "pre_erasure_line_number": 42, | ||
| "match_count": 1, | ||
| "dry_run": false | ||
| } | ||
| ``` | ||
|
|
||
| **Run-erasure success envelope** (`--run-id RUN --kind {staging,artefacts}`): | ||
|
|
||
| ```json | ||
| { | ||
| "success": true, | ||
| "mode": "run", | ||
| "run_id": "fg-abc123", | ||
| "kind": "staging", | ||
| "files_modified": ["/work/output/final_model.staging.fg-abc123"], | ||
| "bytes_freed": 1048576, | ||
| "dry_run": false | ||
| } | ||
| ``` | ||
|
|
||
| **Check-policy success envelope** (`--check-policy [--config PATH]`): | ||
|
|
||
| ```json | ||
| { | ||
| "success": true, | ||
| "violations": [ | ||
| { | ||
| "artefact_kind": "staging_dir[fg-abc123]", | ||
| "path": "/work/output/final_model.staging.fg-abc123", | ||
| "age_days": 14.7, | ||
| "horizon_days": 7, | ||
| "age_source": "audit" | ||
| } | ||
| ], | ||
| "count": 1 | ||
| } | ||
| ``` | ||
|
|
||
| | Key | Type | Notes | | ||
| |---|---|---| | ||
| | `success` | bool | `true` for a successful operation; `false` (with `error`) on config / runtime failure. | | ||
| | `mode` (row/run) | str | Discriminator; `"row"` or `"run"`. Absent on `--check-policy`. | | ||
| | `row_id_hash` | str | `sha256:`-prefixed hex digest of the salted row id. Cleartext value never appears in the envelope. | | ||
| | `salt_source` | str | `"per_dir"` or `"env_var"` per `FORGELM_AUDIT_SECRET` toggle. | | ||
| | `violations` | list[object] | `--check-policy` only. `artefact_kind` is one of `audit_log`, `staging_dir`, `staging_dir[<run_id>]`, `compliance_bundle`, `data_audit_report`, `raw_documents[...]`. `age_source` ∈ `{audit, mtime}`. | | ||
| | `count` | int | `--check-policy` only; equals `len(violations)`. | | ||
| | `dry_run` | bool | Mirrors the `--dry-run` flag. | | ||
|
|
||
| **Exit code mapping:** `0` = success or successful policy report; `1` = config error (unknown row, missing corpus, conflicting flags, malformed `--check-policy --config`); `2` = runtime error (I/O, atomic rename failure). |
Contributor
There was a problem hiding this comment.
The documented purge envelopes don't match the implementation.
forgelm/cli/subcommands/_purge.py currently emits a different shape here: row mode uses matches / first_line and a plain 64-hex row_id_hash, run mode uses would_delete / deleted, and the successful --check-policy path without a retention: block includes note while omitting count. Because this page is presented as the canonical JSON contract, CI consumers pinning to it will parse the wrong schema.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/usermanuals/en/reference/json-output.md` around lines 176 - 235, The
docs and implementation diverge: update forgelm/cli/subcommands/_purge.py to
emit the documented JSON envelope shape — in row mode replace the current
"matches" and "first_line" keys with "match_count" and "pre_erasure_line_number"
and ensure "row_id_hash" is a sha256: prefixed hex digest (never plain 64-hex);
in run mode replace "would_delete"/"deleted" with "files_modified" (list) and
"bytes_freed" (int) and include "run_id" and "kind"; for --check-policy ensure
the success response uses "violations" (list of objects with
artefact_kind/path/age_days/horizon_days/age_source) and "count" ==
len(violations) (instead of emitting "note" only), and always include "success"
and "dry_run" where applicable; locate and modify the response-building code
(functions/variables in _purge.py that assemble the envelope dict) to rename
keys, add missing fields (mode, salt_source, corpus_path when applicable), and
format row_id_hash with the "sha256:" prefix before JSON serialization.
Code (1): - _purge.py F-21-STAGING: _staging_targets_for_run no longer unconditionally includes the unscoped legacy "final_model.staging/" path. The legacy candidate is now only purged when a marker file "staging_run_id" inside the directory matches the operator's --run-id; absent / unreadable / mismatching markers cause the legacy candidate to be skipped (operators with v0.5.0-or-earlier workspaces remove the dir manually). Defends against silent cross-run staging deletion. - _purge.py: row-mode JSON envelopes now carry "corpus_path" (additive; no key renames). _emit_row_dry_run signature gains corpus_path: str so the dry-run path doesn't bleed args into helpers that don't take it. Docs (5): - .claude/skills/cut-release/SKILL.md F-19-DOC: dropped the duplicate "Single source of truth" sentence; replaced string-comparison example with packaging.version.Version-based comparison (avoids the lexicographic "1.10.0" < "1.2.0" pitfall). Mirrored the same change at the Step-3.5 checklist row. - docs/standards/release.md: same Version() comparison block. - docs/analysis/code_reviews/wave2b-final-merge-review-prompt.md: PR base branch corrected from "main" to "development" (PR #30 actually targets development; gh pr view confirms). Goal paragraph mirrored. - docs/usermanuals/{en,tr}/reference/json-output.md verify-gguf table: clarified that checks.metadata_parsed=false does NOT force valid=false when the optional gguf package is absent; only an attempted-and-failed check (corruption) rejects. Cited tests/test_verification_toolbelt.py as the contract pin. - docs/usermanuals/{en,tr}/reference/json-output.md purge schema: rewrote the EN + TR sections to match the actual _purge.py envelopes (matches/first_line/would_delete/deleted) instead of the aspirational shape (match_count/pre_erasure_line_number/ files_modified) that the Wave 2b absorption commit had documented. Documented that row_id_hash is plain 64-hex (no "sha256:" prefix), the dry-vs-wet shape difference for run mode, and the no-retention-block branch's "note" key in --check-policy. Reality > aspiration. Tests (2): - tests/test_cache_subcommands.py: two HF_DATASETS_CACHE leak / restore tests now assert ei.value.code == 0 so the leak-defence claim is anchored on the happy path (a code-1 / code-2 early-exit that never reaches the env-stamp restore would otherwise silently pass). - tests/test_check_field_descriptions.py: the stale comment in test_annotated_field_without_description_flagged claimed bare Field-in-Annotated without description IS missed; the actual assertion (and shipped contract) is that it IS flagged. Rewrote the comment to match the assertion + cite the scanner helper. SKIPPED (1): - _purge.py rename "matches"/"first_line" → "match_count"/ "pre_erasure_line_number" + "would_delete"/"deleted" → "files_modified" + "sha256:" prefix on row_id_hash. Renaming the JSON envelope wire contract immediately after Wave 2b ships would break consumers; the doc-direction (fix docs to match impl) is the right call. Additive corpus_path enhancement applied instead. VALIDATION - ruff format clean - ruff check clean - pytest 1298 passed / 14 skipped - forgelm --dry-run config_template.yaml green Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_check_field_descriptions.py (2)
233-260: ⚡ Quick winExpand CLI exit-code contract tests beyond 0/1 (Line 233-Line 260).
These tests lock strict/non-strict success semantics well, but they don’t pin the remaining public exit-code paths. Add targeted cases for the other contract codes so CI behavior can’t drift silently.
As per coding guidelines, “Error handling must follow docs/standards/error-handling.md standards including exit codes (0/1/2/3/4) as public contracts for CI/CD pipelines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_check_field_descriptions.py` around lines 233 - 260, Add tests to assert the full public exit-code contract (0/1/2/3/4) by expanding TestCliStrictMode: create three new test methods that call check_field_descriptions.main and assert expected codes for error scenarios beyond strict/non-strict: (1) a test that passes an invalid/unknown CLI flag to produce the argument/usage error and assert return code 2, (2) a test that supplies a non-existent file path (or unreadable file) to simulate a CLI I/O or validation failure and assert return code 3, and (3) a test that forces an internal failure (e.g., monkeypatch/patch a helper used by check_field_descriptions.main to raise an unexpected Exception) and assert return code 4; reference the check_field_descriptions.main entrypoint and the TestCliStrictMode test class/ tmp_path fixture when adding these new test methods so CI asserts all public exit-code paths.
23-30: ⚡ Quick winAlign import pattern with existing project guidance by using
importlib.util(Line 26-29).Tests in this repository already follow a pattern of importing tools without modifying
sys.path(seetests/test_check_site_claims.pyline 28, which states "Import tools/check_site_claims.py as a module without modifying sys.path"). The current code mutates the globalsys.path, which contradicts the established project pattern and the explicit guidance intest_check_site_claims.pythat the "test suite stays decoupled from any sys.path manipulation" (line 12). Useimportlib.util.spec_from_file_locationto match the existing pattern and align with Python best practices.Proposed patch
-from __future__ import annotations - -import sys +from __future__ import annotations + +import importlib.util from pathlib import Path @@ -# Import the scanner via its file path — avoids a project-side -# ``setup.py`` install just to make ``tools/`` importable. -_TOOLS_DIR = Path(__file__).parent.parent / "tools" -if str(_TOOLS_DIR) not in sys.path: - sys.path.insert(0, str(_TOOLS_DIR)) - -import check_field_descriptions # noqa: E402 — depends on the path patch above. +# Import scanner directly from file path without modifying sys.path. +_TOOLS_FILE = Path(__file__).parent.parent / "tools" / "check_field_descriptions.py" +_SPEC = importlib.util.spec_from_file_location("check_field_descriptions", _TOOLS_FILE) +assert _SPEC is not None and _SPEC.loader is not None +check_field_descriptions = importlib.util.module_from_spec(_SPEC) +_SPEC.loader.exec_module(check_field_descriptions)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_check_field_descriptions.py` around lines 23 - 30, Replace the sys.path mutation import pattern in tests/test_check_field_descriptions.py: stop inserting _TOOLS_DIR into sys.path and instead load the tools/check_field_descriptions.py file as a module using importlib.util.spec_from_file_location and importlib.util.module_from_spec; use the existing _TOOLS_DIR variable to build the file path and the module name "check_field_descriptions", call spec.loader.exec_module(spec_module) (or equivalent) to execute it, and assign the loaded module to the name check_field_descriptions so the rest of the test can reference it without altering sys.path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/analysis/code_reviews/wave2b-final-merge-review-prompt.md`:
- Line 21: Replace residual references to "main" with "development" in the
document so the base branch is consistent: update the diff summary line that
currently reads "**Diff scope vs main:**" to "**Diff scope vs development:**",
change the phrase "will be merged to `main`" to "will be merged to
`development`", and scan for any other occurrences of "vs main" or "`main`"
(notably the block around the summary and the paragraph near "merged to `main`")
to ensure all mentions match the PR objective (closure/wave2b-integration →
development).
In `@forgelm/cli/subcommands/_purge.py`:
- Around line 235-279: The atomic rewrite in _atomic_rewrite_dropping_lines
currently flushes and fsyncs the temp file but omits fsyncing the parent
directory after os.replace, so add a directory fsync immediately after
os.replace(tmp_path, corpus_path): open the parent directory (using os.open with
O_DIRECTORY), call os.fsync on that directory FD, then close it (ensuring this
is in a try/finally or similar to always close); keep the existing temp-file
cleanup/exception behavior, and apply the same directory-fsync change in
forgelm/data_audit/_orchestrator.py::_atomic_write_json.
---
Nitpick comments:
In `@tests/test_check_field_descriptions.py`:
- Around line 233-260: Add tests to assert the full public exit-code contract
(0/1/2/3/4) by expanding TestCliStrictMode: create three new test methods that
call check_field_descriptions.main and assert expected codes for error scenarios
beyond strict/non-strict: (1) a test that passes an invalid/unknown CLI flag to
produce the argument/usage error and assert return code 2, (2) a test that
supplies a non-existent file path (or unreadable file) to simulate a CLI I/O or
validation failure and assert return code 3, and (3) a test that forces an
internal failure (e.g., monkeypatch/patch a helper used by
check_field_descriptions.main to raise an unexpected Exception) and assert
return code 4; reference the check_field_descriptions.main entrypoint and the
TestCliStrictMode test class/ tmp_path fixture when adding these new test
methods so CI asserts all public exit-code paths.
- Around line 23-30: Replace the sys.path mutation import pattern in
tests/test_check_field_descriptions.py: stop inserting _TOOLS_DIR into sys.path
and instead load the tools/check_field_descriptions.py file as a module using
importlib.util.spec_from_file_location and importlib.util.module_from_spec; use
the existing _TOOLS_DIR variable to build the file path and the module name
"check_field_descriptions", call spec.loader.exec_module(spec_module) (or
equivalent) to execute it, and assign the loaded module to the name
check_field_descriptions so the rest of the test can reference it without
altering sys.path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c50f3e0-8e59-496b-b062-19875f7462a9
📒 Files selected for processing (9)
.claude/skills/cut-release/SKILL.mddocs/analysis/code_reviews/closure-plan-202604300906.mddocs/analysis/code_reviews/wave2b-final-merge-review-prompt.mddocs/standards/release.mddocs/usermanuals/en/reference/json-output.mddocs/usermanuals/tr/reference/json-output.mdforgelm/cli/subcommands/_purge.pytests/test_cache_subcommands.pytests/test_check_field_descriptions.py
✅ Files skipped from review due to trivial changes (2)
- docs/analysis/code_reviews/closure-plan-202604300906.md
- docs/standards/release.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .claude/skills/cut-release/SKILL.md
- docs/usermanuals/en/reference/json-output.md
- tests/test_cache_subcommands.py
| - **Branch under review:** `closure/wave2b-integration` | ||
| - **PR:** [#30](https://github.com/cemililik/ForgeLM/pull/30) → base `development` | ||
| - **HEAD SHA at prompt freeze:** `36afbc0` | ||
| - **Diff scope vs main:** 195 files, ~32 300 insertions, ~6 300 deletions. |
Contributor
There was a problem hiding this comment.
Residual main references contradict the corrected base branch.
Line 19 was fixed to development in a prior round, but line 21 still describes the diff as "vs main", and line 382 still says the branch will be "merged to main". This contradicts line 10 ("merged to development") and the PR objectives (closure/wave2b-integration → development), and may mislead reviewers about which baseline the ~32 300/~6 300 line counts were computed against.
📝 Proposed fix
-- **Diff scope vs main:** 195 files, ~32 300 insertions, ~6 300 deletions.
+- **Diff scope vs development:** 195 files, ~32 300 insertions, ~6 300 deletions. - After this round, surviving findings will be addressed in a single
final absorption commit (or skipped with a documented reason), and
- the branch will be merged to `main`. There is no Round-7.
+ the branch will be merged to `development`. There is no Round-7.Also applies to: 380-383
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/analysis/code_reviews/wave2b-final-merge-review-prompt.md` at line 21,
Replace residual references to "main" with "development" in the document so the
base branch is consistent: update the diff summary line that currently reads
"**Diff scope vs main:**" to "**Diff scope vs development:**", change the phrase
"will be merged to `main`" to "will be merged to `development`", and scan for
any other occurrences of "vs main" or "`main`" (notably the block around the
summary and the paragraph near "merged to `main`") to ensure all mentions match
the PR objective (closure/wave2b-integration → development).
Comment on lines
+235
to
+279
| def _atomic_rewrite_dropping_lines(corpus_path: str, line_numbers_to_drop: List[int]) -> int: | ||
| """Rewrite ``corpus_path`` excluding the given 1-based line numbers. | ||
|
|
||
| Uses a temp file in the same directory + ``os.replace`` for atomic | ||
| swap — operators get either the full pre-erasure file or the full | ||
| post-erasure file, never a partial state. Returns the byte count | ||
| freed. | ||
|
|
||
| Raises :class:`OSError` on any I/O failure; the caller is responsible | ||
| for emitting ``data.erasure_failed`` and surfacing | ||
| ``EXIT_TRAINING_ERROR``. | ||
| """ | ||
| drop_set = set(line_numbers_to_drop) | ||
| pre_size = os.path.getsize(corpus_path) | ||
| parent = os.path.dirname(os.path.abspath(corpus_path)) or "." | ||
| fd, tmp_path = tempfile.mkstemp(prefix=".forgelm_purge_", suffix=".tmp", dir=parent) | ||
| try: | ||
| with os.fdopen(fd, "w", encoding="utf-8") as out: | ||
| with open(corpus_path, "r", encoding="utf-8") as src: | ||
| for line_no, line in enumerate(src, start=1): | ||
| if line_no in drop_set: | ||
| continue | ||
| out.write(line) | ||
| # Wave 2b Round-4 review F-W2B-03: flush + fsync the temp | ||
| # file's data blocks to disk BEFORE the namespace swap. | ||
| # ``os.replace`` is rename-atomic on POSIX, but without an | ||
| # explicit fsync the temp file's *contents* may still be | ||
| # buffered in the page cache. A power loss between the | ||
| # swap and the cache flush would leave the corpus inode | ||
| # pointing at the new file with its data blocks unwritten | ||
| # — i.e. an empty corpus. Mirrors the discipline in | ||
| # `forgelm/data_audit/_orchestrator.py::_atomic_write_json`. | ||
| out.flush() | ||
| os.fsync(out.fileno()) | ||
| os.replace(tmp_path, corpus_path) | ||
| except OSError: | ||
| # Best-effort cleanup of the temp file if the swap failed. | ||
| if os.path.exists(tmp_path): | ||
| try: | ||
| os.unlink(tmp_path) | ||
| except OSError: # pragma: no cover — defensive | ||
| logger.debug("Could not clean up purge temp file %s", tmp_path) | ||
| raise | ||
| post_size = os.path.getsize(corpus_path) | ||
| return pre_size - post_size |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On POSIX, is fsync on the parent directory required after os.replace/rename to make the rename durable across power loss?
💡 Result:
Yes, on POSIX systems, fsync on the parent directory (or directories, if source and destination parents differ) is required after rename (or os.replace, which uses rename) to make the rename durable across power loss. POSIX rename is atomic with respect to other filesystem operations but provides no durability guarantees against crashes; directory entries are data in the parent directory, so fsync on the file does not persist the name change.[1][2][3][4] The Linux fsync man page explicitly states: "Calling fsync does not necessarily ensure that the entry in the directory containing the file has also reached disk. For that an explicit fsync on a file descriptor for the directory is also needed."[2][1] The Austin Group (POSIX maintainers) defect report confirms: "Although directory operations are atomic and serializable, they are not necessarily durable. An application that requires a directory modification to be durable should use fdatasync or fsync on the directory... Two calls to fsync may be needed after a rename to ensure its durability."[3][4] If oldpath and newpath are in the same directory, fsync on that single parent suffices; otherwise, fsync both parents.[5][6]
Citations:
- 1: Question: fsync on file and/or directory google/renameio#11
- 2: http://www.man7.org/linux/man-pages/man2/fsync.2.html
- 3: https://austingroupbugs.net/view.php?id=672
- 4: https://www.austingroupbugs.net/view.php?id=672
- 5: https://stackoverflow.com/questions/3764822/how-to-durably-rename-a-file-in-posix
- 6: Add fsync on parent directory after file creation and rename Telcoin-Association/telcoin-network#551
🏁 Script executed:
find . -name "_orchestrator.py" -path "*/data_audit/*" | head -5Repository: cemililik/ForgeLM
Length of output: 99
🏁 Script executed:
rg -A 30 "def _atomic_write_json" ./forgelm/data_audit/_orchestrator.pyRepository: cemililik/ForgeLM
Length of output: 1456
Parent directory fsync is missing after os.replace to make the rename durable across power loss.
The current discipline (flush() → fsync(temp) → os.replace) guarantees the new file's data blocks reach disk, but on POSIX the rename operation (the directory entry change that flips corpus_path to the temp inode) lives in the parent directory inode and is not made durable until the directory itself is fsynced. After a power loss between the rename and the directory cache flush, the corpus path can revert to the pre-erasure inode — the very scenario the docstring claims to defend against. The same gap exists in forgelm/data_audit/_orchestrator.py::_atomic_write_json.
🛡️ Proposed fix: fsync parent directory after replace
out.flush()
os.fsync(out.fileno())
os.replace(tmp_path, corpus_path)
+ # Make the rename itself durable: without this, a power loss
+ # between the namespace swap and the directory-entry flush can
+ # leave the corpus path pointing back at the pre-erasure inode.
+ try:
+ dir_fd = os.open(parent, os.O_DIRECTORY)
+ except (OSError, AttributeError):
+ # Windows / no O_DIRECTORY support — skip; os.replace is
+ # already crash-safe on NTFS.
+ pass
+ else:
+ try:
+ os.fsync(dir_fd)
+ finally:
+ os.close(dir_fd)
except OSError:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forgelm/cli/subcommands/_purge.py` around lines 235 - 279, The atomic rewrite
in _atomic_rewrite_dropping_lines currently flushes and fsyncs the temp file but
omits fsyncing the parent directory after os.replace, so add a directory fsync
immediately after os.replace(tmp_path, corpus_path): open the parent directory
(using os.open with O_DIRECTORY), call os.fsync on that directory FD, then close
it (ensuring this is in a try/finally or similar to always close); keep the
existing temp-file cleanup/exception behavior, and apply the same
directory-fsync change in
forgelm/data_audit/_orchestrator.py::_atomic_write_json.
Code (1): - _purge.py + data_audit/_orchestrator.py F-21-04: parent-dir fsync added after os.replace. POSIX rename atomicity is necessary but not sufficient on non-journaled filesystems (FAT, certain tmpfs); fsyncing the parent directory after the rename ensures the directory entry's metadata is on disk so a power loss between the rename and the metadata flush cannot lose the entry. ``O_DIRECTORY`` is unsupported on Windows; the open is wrapped in ``try/except (AttributeError, OSError)`` and the fd is always closed in a ``finally``. Both atomic-rewrite sites get the same discipline so the two helpers don't fork their durability story. Docs (1): - docs/analysis/code_reviews/wave2b-final-merge-review-prompt.md: L21 ``Diff scope vs main`` → ``vs development`` (last residual ``main`` reference; PR base + goal text already corrected in prior round). Tests (1): - tests/test_check_field_descriptions.py: switched from ``sys.path`` mutation to ``importlib.util.spec_from_file_location`` + ``module_from_spec`` + sys.modules registration + ``exec_module``. Avoids leaking ``tools/`` onto the import path for the rest of the pytest invocation. ``sys.modules`` registration is load-bearing because the scanner uses ``@dataclass(frozen=True)`` which resolves its parent module via ``sys.modules[cls.__module__]`` at class-creation time — the comment explains the failure mode so a future refactor doesn't break the registration. Skipped (1, documented): - ``test_check_field_descriptions.py`` exit-code 0/1/2/3/4 contract tests — the scanner is a CI-only tool, not a forgelm subcommand. The 0/1/2/3/4 public contract per ``docs/standards/error-handling.md`` applies to the ``forgelm`` binary's subcommands; ``tools/check_field_descriptions.py`` returns 0 (clean) or 1 (strict + missing) only. Asserting non-existent contracts (3/4) would pin behaviour the code never promised. VALIDATION - ruff format clean - ruff check clean - pytest 1298 passed / 14 skipped - forgelm --dry-run config_template.yaml green Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
May 5, 2026
PR #30 merged to development at commit b05edb5 (2026-05-05). Closure plan updated to reflect post-merge state: - Status legend: target branch corrected from "main" → "development" for PRs #21, #28, #30 (the project merges into development first; development → main happens at release time). - Wave 2b kapanış özeti: merge commit SHA recorded; suite count delta noted (1160 → 1298, +138 net tests across the wave). - Version log v2.7: rewritten from "merge öncesi" → "merge sonrası" with merge commit SHA + Wave 3 immediate-batch readiness note (Faz 22 + 24 + 38 dependency-free). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks done
6 tasks done
This was referenced May 8, 2026
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five-phase consolidated integration covering the dependency-free batch unblocked by the Wave 2a merge. All work lives on closure/wave2b-integration as a single reviewable PR.
PHASE 21 — GDPR Article 17 right-to-erasure (
forgelm purge)forgelm/cli/subcommands/_purge.py— three-mode dispatcher:PHASE 35 — Air-gap pre-cache subcommands
PHASE 36 — Compliance verification toolbelt
PHASE 16 — Pydantic description= migration with CI guard
PHASE 19 — Library API support (Implementation)
VERIFICATION
forgelm --help+ epilogSummary by CodeRabbit
New Features
Documentation
Chores