Repository navigation
chore: migrate to HodeTech org + ignore new nightly CVE - #61
Merged
Merged
Conversation
The project moved to the HodeTech GitHub organization (github.com/HodeTech/ForgeLM). Update every reference project-wide: - Repo URLs: github.com/cemililik/ForgeLM -> github.com/HodeTech/ForgeLM (README badges, pyproject [project.urls], docs, user manuals EN+TR, notebooks, site/, CHANGELOG, CONTRIBUTING, issue-template config, model_card.py, standards, roadmap). - GHCR image: ghcr.io/cemililik/forgelm -> ghcr.io/hodetech/forgelm (lowercase namespace, per GHCR owner naming). - Contact email (CODE_OF_CONDUCT.md): ai-team@cemililik.com -> forgelm@hodetech.io. - SonarCloud (sonar-project.properties): organization=hodetech, projectKey=HodeTech_ForgeLM. 271 references across 99 files, 1:1 replacements (no other edits). Local git remote origin re-pointed to the HodeTech URL. Doc/site guards re-run green: anchor resolution, bilingual parity, user-manual self-contained, CLI help consistency, site-version, site-claims. model_card tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…5-24) The nightly on main is green through the new --ignores mechanism (the run log shows CVE-2026-1839 correctly suppressed), but a new no-fix advisory dropped: transformers PYSEC-2025-217 (CVE-2025-14929), X-CLIP checkpoint-conversion deserialization RCE. CVSS is AV:L/UI:R (local + user must open a malicious checkpoint), affected through 5.0.0rc0 with no fix in the pinned 4.x line. Same threat-model carve-out as the existing transformers/torch deserialization ignores: operators are documented as required to load only trusted checkpoints, and ForgeLM does not invoke X-CLIP checkpoint conversion. With the Phase-refactored ignore file this is a one-line data change, no workflow edit — exactly the maintainability win the PR #59 review asked for. Refs: #58 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
changed the title
Collaborator
Author
|
@coderabbitai review all |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
|
Warning Review limit reached
More reviews will be available in 1 hour, 5 minutes, and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates repository and registry references across documentation and site assets, adds audit-event catalog drift checks and pipeline lifecycle catalog entries, and changes training, model-loading, webhook, and audit-log behavior with matching tests. It also adds a pip-audit ignore entry and refreshes related contract documentation. ChangesRepository and documentation migration
Pipeline, audit, and training behavior
Security policy and vulnerability ignore
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 3
🧹 Nitpick comments (1)
docs/usermanuals/en/deployment/chat.md (1)
110-110: ⚡ Quick winMisleading equivalence between chat-compare and benchmark-only.
The note states "Today the same comparison runs through
forgelm --benchmark-only", but the--benchmark-onlymode runs lm-evaluation-harness benchmark tasks (forgelm/cli/_no_train_modes.py:1-140), which is fundamentally different from the side-by-side chat prompt comparison shown in lines 113-119. Consider revising to acknowledge that no direct equivalent exists today, or clarify that operators must manually run benchmarks separately on each model and compare the judge results themselves (which is a different workflow from the planned dedicated UX).🤖 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/deployment/chat.md` at line 110, The note incorrectly implies feature parity between the planned chat-compare subcommand and the existing --benchmark-only mode; update the text to state that chat-compare provides an interactive side-by-side chat prompt comparison while --benchmark-only (see forgelm/cli/_no_train_modes.py) runs lm-evaluation-harness benchmarks and requires manually running benchmarks per checkpoint and aggregating judge results, so operators must currently run and compare results themselves rather than getting the planned dedicated UX.
🤖 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/usermanuals/en/deployment/chat.md`:
- Around line 66-68: Resolve two issues: pick one consistent flag form (e.g.,
use "--safety" or "--safety on" everywhere) and update the paragraph to use that
chosen form; and clarify the current v0.5.5 behavior by either (A) describing
the exact binding workflow (pointing readers to the chat subcommand parser in
forgelm/cli/_parser.py and specifying which config file/key to set and how the
runner must load it) or (B) explicitly state that on v0.5.5 the CLI does not
support per-turn safety screening and that the YAML safety: enabled: true entry
is only read by certain runners (name them) — do not claim a binding mechanism
exists unless you spell out the exact steps referencing forgelm/cli/_parser.py.
In `@site/js/translations.js`:
- Line 4345: The privacy copy currently hardcodes "pip install forgelm==0.5.5"
in the "privacy.body" locale string (and the same hardcoded version appears in
the sibling privacy locale entries noted in the comment); change this to avoid a
stale pinned version by either removing the "==0.5.5" (use "pip install
forgelm") or replace it with a template/variable (e.g., "{FORGELM_PIP}" or
"{siteVersion}") that the site build injects so the install line is maintained
site-wide and updated from a single source; locate and update the string key
"privacy.body" (and the other privacy locale keys mentioned) to use the
non-hardcoded form.
In `@tools/pip_audit_ignores.yaml`:
- Around line 51-69: Update the PYSEC-2025-217 ignore entry (id: PYSEC-2025-217,
package: transformers) to match advisory wording: state CVE-2025-14929 with CVSS
vector including AV:L and UI:R, remove the unverified phrase "affected through
5.0.0rc0" and instead say "no fixed version announced for the 4.x line" (or
mirror the advisory language about lack of a fix), and adjust the
reevaluate_after criteria accordingly; additionally perform and document a
codebase check to confirm the threat-model claim by searching for usages of
X-CLIP checkpoint conversion paths (search for identifiers/strings such as
"X-CLIP", "checkpoint conversion", "pt2 loader", and any direct "torch.load"
checkpoint-loading sites) and record the result in the entry.
---
Nitpick comments:
In `@docs/usermanuals/en/deployment/chat.md`:
- Line 110: The note incorrectly implies feature parity between the planned
chat-compare subcommand and the existing --benchmark-only mode; update the text
to state that chat-compare provides an interactive side-by-side chat prompt
comparison while --benchmark-only (see forgelm/cli/_no_train_modes.py) runs
lm-evaluation-harness benchmarks and requires manually running benchmarks per
checkpoint and aggregating judge results, so operators must currently run and
compare results themselves rather than getting the planned dedicated UX.
🪄 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: 3115152d-2066-4049-a433-0504b4e6c23f
📒 Files selected for processing (100)
.agents/skills/cut-release/SKILL.md.agents/skills/sync-bilingual-docs/SKILL.md.claude/skills/cut-release/SKILL.md.claude/skills/sync-bilingual-docs/SKILL.md.github/ISSUE_TEMPLATE/config.yml.github/workflows/nightly.ymlCHANGELOG.mdCODE_OF_CONDUCT.mdCONTRIBUTING.mdREADME.mddocs/guides/quickstart-tr.mddocs/guides/quickstart.mddocs/guides/troubleshooting-tr.mddocs/guides/troubleshooting.mddocs/reference/usage-tr.mddocs/reference/usage.mddocs/roadmap/releases.mddocs/roadmap/risks-and-decisions.mddocs/standards/code-review.mddocs/standards/documentation.mddocs/usermanuals/en/compliance/annex-iv.mddocs/usermanuals/en/compliance/audit-log.mddocs/usermanuals/en/compliance/gdpr-erasure.mddocs/usermanuals/en/compliance/gdpr.mddocs/usermanuals/en/compliance/human-approval-gate.mddocs/usermanuals/en/compliance/overview.mddocs/usermanuals/en/compliance/verify-audit.mddocs/usermanuals/en/data/deduplication.mddocs/usermanuals/en/data/pii-masking.mddocs/usermanuals/en/deployment/chat.mddocs/usermanuals/en/deployment/model-merging.mddocs/usermanuals/en/deployment/verify-gguf.mddocs/usermanuals/en/evaluation/trend-tracking.mddocs/usermanuals/en/getting-started/installation.mddocs/usermanuals/en/operations/docker.mddocs/usermanuals/en/operations/experiment-tracking.mddocs/usermanuals/en/operations/gpu-cost.mddocs/usermanuals/en/operations/iso-soc2-deployer.mddocs/usermanuals/en/operations/supply-chain.mddocs/usermanuals/en/operations/troubleshooting.mddocs/usermanuals/en/operations/webhooks.mddocs/usermanuals/en/reference/configuration.mddocs/usermanuals/en/reference/exit-codes.mddocs/usermanuals/en/reference/json-output.mddocs/usermanuals/en/reference/library-api.mddocs/usermanuals/en/reference/performance.mddocs/usermanuals/en/reference/yaml-templates.mddocs/usermanuals/tr/compliance/annex-iv.mddocs/usermanuals/tr/compliance/audit-log.mddocs/usermanuals/tr/compliance/gdpr-erasure.mddocs/usermanuals/tr/compliance/gdpr.mddocs/usermanuals/tr/compliance/human-approval-gate.mddocs/usermanuals/tr/compliance/overview.mddocs/usermanuals/tr/compliance/verify-audit.mddocs/usermanuals/tr/data/deduplication.mddocs/usermanuals/tr/data/pii-masking.mddocs/usermanuals/tr/deployment/chat.mddocs/usermanuals/tr/deployment/model-merging.mddocs/usermanuals/tr/deployment/verify-gguf.mddocs/usermanuals/tr/evaluation/trend-tracking.mddocs/usermanuals/tr/getting-started/installation.mddocs/usermanuals/tr/operations/docker.mddocs/usermanuals/tr/operations/experiment-tracking.mddocs/usermanuals/tr/operations/gpu-cost.mddocs/usermanuals/tr/operations/iso-soc2-deployer.mddocs/usermanuals/tr/operations/supply-chain.mddocs/usermanuals/tr/operations/troubleshooting.mddocs/usermanuals/tr/operations/webhooks.mddocs/usermanuals/tr/reference/configuration.mddocs/usermanuals/tr/reference/exit-codes.mddocs/usermanuals/tr/reference/json-output.mddocs/usermanuals/tr/reference/library-api.mddocs/usermanuals/tr/reference/performance.mddocs/usermanuals/tr/reference/yaml-templates.mdforgelm/model_card.pynotebooks/data_curation.ipynbnotebooks/dpo_alignment.ipynbnotebooks/galore_memory_optimization.ipynbnotebooks/grpo_reasoning.ipynbnotebooks/ingestion_playground.ipynbnotebooks/kto_binary_feedback.ipynbnotebooks/multi_dataset.ipynbnotebooks/post_training_workflow.ipynbnotebooks/quickstart_sft.ipynbnotebooks/safety_evaluation.ipynbnotebooks/synthetic_data_training.ipynbpyproject.tomlsite/README.mdsite/compliance.htmlsite/contact.htmlsite/features.htmlsite/guide.htmlsite/index.htmlsite/js/guide.jssite/js/translations.jssite/privacy.htmlsite/quickstart.htmlsite/terms.htmlsonar-project.propertiestools/pip_audit_ignores.yaml
…acy pin, PYSEC-2025-217) All four findings verified against current code; all still valid. chat.md (EN+TR) — safety routing (L66-68): Verified forgelm/cli/_parser.py: `forgelm chat` has no --safety flag and loads no config; run_chat/ChatSession take no safety arg. The old copy claimed a YAML `safety:` binding into the chat session — false, and the key path was wrong. Rewritten to state the current release does NOT screen per-turn in the REPL, and that `evaluation.safety.enabled: true` is consumed only by the training auto-revert gate (forgelm/trainer.py) and `forgelm safety-eval`. Collapsed the inconsistent `--safety` / `--safety on` forms to a single `--safety`. chat.md (EN+TR) — chat-compare note (L110): Verified forgelm/cli/_no_train_modes.py: --benchmark-only runs lm-evaluation-harness on a SINGLE checkpoint, no side-by-side, no judge. Removed the implied parity; now says operators must run --benchmark-only per checkpoint, run the judge separately, and aggregate themselves until the planned chat-compare UX ships. site/js/translations.js — privacy.body (6 locales): Dropped the stale hard-coded `pip install forgelm==0.5.5` pin (real release is 0.7.0; this string is outside update_site_version.py's scope so it drifted). Now `pip install forgelm` — no version to drift, consistent with the site's no-build-step constraint. tools/pip_audit_ignores.yaml — PYSEC-2025-217: Reworded to advisory language: full CVSS vector (AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H), "no fixed version for the 4.x line" instead of "affected through 5.0.0rc0", reevaluate_after no longer assumes a fixed 5.x exists. Documented the codebase check (git grep over forgelm/): no X-CLIP usage, no checkpoint-conversion path besides HF->GGUF export, zero direct torch.load sites — all loading via from_pretrained, so the vulnerable path is unreachable. Validated: bilingual parity, anchor resolution, user-manual self-contained, site-version, site-claims, site chrome parity, CLI help consistency, ignore-file schema; 23 check_pip_audit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on validation split (C1/XP-01) GRPO trains on generation-based rewards, not validation loss, so `_build_grpo_trainer` pops the `eval_dataset`. But the GRPO branch of `_get_training_args_for_type` left `eval_strategy="steps"` in place — the value `_get_common_training_kwargs` sets whenever a validation split exists, which is the *default* pipeline path (auto-created split). A GRPOConfig with `eval_strategy="steps"` and no eval_dataset makes HF/TRL raise `ValueError: ... you didn't pass an eval_dataset` at GRPOTrainer construction. Force `eval_strategy="no"` and clear `eval_steps` in lockstep with the popped eval_dataset (the best-model args were already popped). Regression test `test_grpo_config_disables_eval_when_validation_split_present` asserts ForgeLM's arg-reconciliation invariant (not TRL's raise): with a validation split present, the GRPOConfig kwargs carry `eval_strategy="no"`, no `eval_steps`, and none of the eval-coupled best-model args. Guarded on `GRPOConfig` importability (the actual precondition) rather than `GRPOTrainer`. Findings: F-P2-FAB-05, F-P3-FABLE-01 (XP-01). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ebhook persist-field drift (C4/XP-03) `_WEBHOOK_PERSIST_FIELDS` had drifted from the real `WebhookConfig` schema: it persisted six fields that don't exist (`notify_on_revert`, `notify_on_awaiting_approval`, `secret_env`, `timeout_seconds`, `retry_count`, `retry_backoff_seconds`) and dropped the real `url`/`notify_on_start`/`timeout`. When `forgelm approve`/`reject` rebuilt a WebhookNotifier from the co-located compliance manifest via a SimpleNamespace, `_resolve_url` read `self.config.url` and raised AttributeError — *after* the model was already promoted and the `human_approval.granted` audit event committed, crashing a successful enterprise run and violating the public exit-code contract. Three-part fix: 1. compliance.py — derive `_WEBHOOK_PERSIST_FIELDS` from `WebhookConfig.model_fields` (single source of truth, secret-bearing `url` excluded) so the persisted shape can never drift from the model again. 2. webhook.py — `_resolve_url` reads `getattr(self.config, "url", None)` so a rebuilt namespace missing `url` resolves cleanly instead of crashing. 3. _approve.py — wrap the post-promotion notifier emit (approve + reject) so a webhook hiccup is non-fatal (a notification is not a gate). Regression tests (TestWebhookPersistRoundTrip): field-set parity vs the live schema; `_resolve_url` tolerates a namespace with no `url`; inline `url` secret never reaches the manifest while `url_env` survives; and an end-to-end rebuild via the real `_build_approval_notifier` fires `notify_success` without crashing. Findings: F-P1-FAB-02, F-P4-OPUS-09 (XP-03). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d awaiting_approval discriminator (C2/XP-02) A reverted approval-gated stage was misreported as "awaiting human approval" (exit 4) instead of "eval-failure" (exit 3): when require_human_approval was on, the pipeline eagerly set staging_path before the post-train gates ran, but the benchmark/safety/judge revert branches set success=False without clearing it — and _run_single_stage checked `if result.staging_path` (→ exit 4) BEFORE `if not result.success` (→ exit 3). The revert had already deleted the staging dir, so the run was reported as awaiting approval against a non-existent artefact. TrainResult / the JSON envelope had no field distinguishing the two. Fix: - results.py: add `awaiting_approval: bool` — the authoritative discriminator (set True only when the gate genuinely fires alongside success=True). - trainer.py: the gate sets `awaiting_approval=True`; the three gate revert branches clear `staging_path` (revert deletes the dir). - _pipeline.py: route on `success` first (exit 3), then on the explicit `awaiting_approval` flag (exit 4) — not `bool(staging_path)`, which can survive a revert. - _training.py: single-run path routes on the result discriminator, not the bare config flag. - _result.py: JSON envelope carries `awaiting_approval` (always) + `staging_path` (when staged). - json-output.md (EN+TR): correct the false `success: true ⟺ exit 0` claim and document the full training result envelope (success/awaiting/reverted). Tests: pipeline reverted-gated → exit 3; genuine gate → exit 4 via discriminator; stale-staging-on-failure defends to exit 3; the gate sets the flag and revert clears staging_path; envelope discriminates all three outcomes. Fixed the masking orchestrator test that encoded the success=False-as-gated bug. Findings: F-P1-FAB-12, F-P1-FAB-13, F-P1-FAB-30, F-P2-FAB-02, F-P2-FAB-14 (XP-02). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(C3/P2-FAB-01)
The resume skip-list decided "already done" from disk presence:
``status in {completed, gated_pending_approval} and os.path.isdir(output_model)``.
A ``gated_pending_approval`` stage's ``output_model`` is its UNAPPROVED staging
directory (``final_model.staging.<run_id>``), which exists on disk — so
``--resume-from <later_stage>`` silently skipped the gated stage and chained
downstream training from a model no human had signed off, defeating the
Article 14 human-approval gate.
Decide skip-eligibility on the state-file status, not disk presence: only a
``completed`` stage (with its output_model on disk) is skipped. A
``gated_pending_approval`` stage in the resume range is now refused with
EXIT_CONFIG_ERROR and an actionable message telling the operator to approve or
reject first — never silently bypassed. (The approval workflow that flips an
approved stage to a skippable status is handled in the Wave-1 resume/terminal
package; until then resuming past an un-approved gate is correctly blocked.)
Regression test: a gated first stage + ``--resume-from dpo_stage`` without
approval is refused (EXIT_CONFIG_ERROR) and instantiates zero downstream
trainers. Replaces the prior test that asserted the insecure skip.
Findings: F-P2-FAB-01.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…enesis manifest (C6/P4-OPUS-01) `verify_audit_log` early-returned `VerifyResult(valid=True, entries_count=0)` for an empty log BEFORE the genesis-manifest cross-check ran — so an attacker who truncated the audit log to zero entries passed verification clean, even though the genesis manifest sidecar pins a real non-empty first entry. This is the exact truncation attack the manifest exists to detect; the in-chain hash-continuity walk can't catch it because there are no lines left to walk. - `_verify_genesis_manifest`: when the manifest pins a first entry but entries_count == 0, return valid=False with a truncate-to-empty reason. - `verify_audit_log`: consult the genesis manifest before reporting a clean empty chain; a manifest-less empty log stays legitimately valid. Tests: truncate-to-empty with manifest → valid=False; empty log without a manifest → valid=True (benign first-run); and a re-root tamper (wipe body, keep write-once manifest, write a fresh valid chain) → manifest-mismatch valid=False (P4-OPUS-22 genesis-mismatch coverage that was previously untested). Findings: F-P4-OPUS-01 (+ F-P4-OPUS-22 test coverage). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+ test the audit-catalog guard (C7/XP-04) `tools/check_audit_event_catalog.py` exited 1 at HEAD, was wired into no workflow, and had no own test — so six genuine `pipeline.*` stage/decision events (`started`, `force_resume`, `stage_started`, `stage_completed`, `stage_gated`, `stage_reverted`) drifted into the code uncatalogued with zero CI tripwire on a public EU AI Act Art. 12 contract (append-only audit log). - docs/reference/audit_event_catalog.md (+ -tr mirror): add the 6 pipeline.* rows with accurate when-emitted / payload / Article columns. - check_audit_event_catalog.py: tighten the emission detector to match only real emission contexts (log_event/_audit_event first-arg, event= kwarg, "event": JSON value, _EVT_* constant declarations) instead of any quoted dotted string — kills the `training.output_dir` config-path false positive (F-P8-C-12) while dropping zero genuine events (verified empirically). - ci.yml + CLAUDE.md/AGENTS.md gauntlet: wire the guard with --strict so the drift can never recur silently. - tests/test_check_audit_event_catalog.py: happy path, undocumented-event and ghost-row failures, the config-path-not-flagged regression, constant/keyword emission detection, real-repo-in-sync, and meta-assertions that the guard is wired into CI and the gauntlet. Findings: F-P8-C-01 (XP-04: F-P4-OPUS-19 + F-P8-OPUS-03/06), F-P8-C-12. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…enerated README (C5/P1-FAB-01)
`SyntheticConfig.model_dump` overrides redaction of `api_key`, but Pydantic v2's
parent `ForgeConfig.model_dump()` recurses via its OWN serializer and never
calls the child's Python-level override — so the redaction was dead on the
parent-serialization path. The model-card exclude set
(`{auth, webhook, monitoring}`) also omitted `synthetic`, so a populated
`synthetic.api_key` landed in the generated `README.md` in plaintext.
Two-layer defence in model_card.py:
1. Add `synthetic` to the section `exclude` (matches auth/webhook/monitoring).
2. `_redact_secrets`: recursively mask any secret-keyed value (`api_key`,
`token`, `secret`, `password`, ...) in the serialized config, while keeping
`*_env` env-var-NAME references intact. Catches residual secrets in any
non-excluded section, forward-compatibly.
Chose the model-card-side fix over a config field_serializer to avoid the
`merge_pipeline_stage_config` round-trip hazard (a redaction literal becoming
the live value — F-P1-FAB-03 / H6 territory).
Tests: `synthetic.api_key` never appears in the rendered card; the recursive
masker redacts `judge_api_key`/`password` but keeps `*_env` names.
Findings: F-P1-FAB-01.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…set judge API key env (H1a / F-P3-FABLE-03,18) F-P3-FABLE-03: only the SFT branch passed model.max_length to TRL; DPO/ORPO/ SimPO/KTO silently fell back to TRL's 512/1024 max_length defaults and GRPO to its 512 max_prompt_length default, while the config field, configuration.md, and the Article 11 compliance manifest all claimed model.max_length applied — silent truncation plus a false statement in an audit artefact. Added `_apply_max_length` and wired it into the four preference branches (max_length) and GRPO (max_prompt_length). A future TRL rename now surfaces as a loud TypeError, never silent truncation. F-P3-FABLE-18: a configured `judge_api_key_env` that is unset silently flipped the judge to local mode (judge.py treats api_key=None as "local"), running a different evaluator than configured; with auto_revert=true a failed local load deletes the trained adapters over a misdiagnosed env-var problem. The trainer now raises ConfigError naming the variable BEFORE invoking the judge, so the model is preserved (auto-revert never fires) and the error is actionable. Tests: per-trainer max_length application (DPO/GRPO verified; ORPO/KTO/SimPO skip where trl can't import the config in the local env); judge env unset → ConfigError + run_judge_evaluation never called; env set → API mode runs. Findings: F-P3-FABLE-03, F-P3-FABLE-18. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…method, loud non-CUDA 4-bit, VLM pad guard (H1b / F-P3-FABLE-07,09,10,11) F-P3-FABLE-07: `_apply_moe_config` gated MoE behaviour on `hasattr(config, "num_local_experts")`, which only Mixtral/Phi expose — Qwen3-MoE (`num_experts`) and DeepSeek-V3 (`n_routed_experts`) silently no-op'd despite the README advertising them. Resolve the expert count via an attribute registry and WARN loudly when moe is configured but no expert attribute is found. F-P3-FABLE-09: the unsloth backend passed only the deprecated boolean shortcuts, silently downgrading `method: dora`/`rslora` to plain LoRA and dropping `method: pissa` — identical YAML trained a different algorithm than the transformers backend. Both backends now derive flags from the shared `_resolve_peft_flags`; pissa+unsloth raises ConfigError instead of silently degrading. F-P3-FABLE-10: `load_in_4bit=true` on a non-CUDA host loaded full precision with zero log output while compliance artefacts still claimed "4-bit NF4". Added a loud WARNING that the run is not quantized and artefacts will overstate it. F-P3-FABLE-11: the multimodal path read `tokenizer.pad_token_id` unguarded, but an AutoProcessor exposes pad_token only on its inner `.tokenizer` (AttributeError crash), and loaded VLMs via AutoModelForCausalLM (most are registered under AutoModelForImageTextToText). Resolve the inner tokenizer for pad handling and select the image-text-to-text auto-class when multimodal is enabled. Findings: F-P3-FABLE-07, F-P3-FABLE-09, F-P3-FABLE-10, F-P3-FABLE-11. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…S merge works on real models (H1c / F-P3-FABLE-19) `_ties_merge_tensor` computed the trim threshold with `torch.quantile`, which hard-fails above 2^24 (16,777,216) elements (PyTorch #64947). Every real-model weight tensor exceeds this — a 7B mlp.gate_proj is ~45M elements — so the DEFAULT MergeConfig method (`ties`) crashed on any non-toy model with the cryptic "quantile() input tensor is too large", swallowed into MergeResult and surfaced as a CLI exit 2 that gave the operator no clue the feature was broken. `kthvalue` has no size limit and yields the same trim threshold: the k-th smallest magnitude, k = trim_fraction · n. DARE was unaffected (no quantile). Regression test: `_ties_merge_tensor` on a tensor just past 2^24 elements returns the same shape without raising and trims ~20% to zero. Findings: F-P3-FABLE-19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… exception-safe restore (H1d / F-P3-FABLE-04) OOM recovery mutates config.training.per_device_train_batch_size in place. The compliance manifest restored the configured (pre-OOM) value while the model card read the effective (post-OOM) value, so the two Article-11 artefacts silently contradicted (manifest=16, README=4) with nothing explaining the discrepancy. The save/restore was also not exception-safe: a manifest-build error skipped the restore, leaving config holding the configured values under the outer BLE001 catch. - Record BOTH values + an explicit `oom_recovery` block (applied, configured_*, effective_*) in the manifest when OOM occurred, so an auditor sees the discrepancy explained rather than two artefacts that just disagree. - Wrap the restore in try/finally so the effective values are always restored. The manifest's primary batch_size keeps the configured value (existing contract + test unchanged); the new block is additive. Tests: oom_recovery block present with both values after a simulated OOM; absent when no OOM; config restored to effective values when manifest generation raises. Findings: F-P3-FABLE-04. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…he native merge (H1e / F-P3-FABLE-20)
mergekit was declared in the `merging` extra, advertised in README/usage,
probed by `forgelm doctor`, and named in config field descriptions + the
architecture standard — but never imported anywhere. Model merging is fully
native (peft + torch on state dicts), as docs/reference/architecture.md already
stated ("no mergekit dependency required"). The unused extra installed a heavy
package with its own tight transformers/torch pins that could downgrade a
training env for zero benefit, and `forgelm doctor` warned about a "missing
merge backend" on systems where merge works perfectly.
Aligned all surfaces to the native implementation: removed mergekit from the
`merging` extra (kept empty for `pip install forgelm[merging]` backward-compat),
dropped the doctor probe row, reworded config.py descriptions, README, usage.md
(+ TR), the architecture standard's heavy-deps list + extras matrix, and the
merging.py BLE001 rationale.
Regression tests: no extra declares mergekit; the `merging` extra is an empty
no-op.
Findings: F-P3-FABLE-20.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…model (H1f / F-P3-FABLE-02,08) Both MoE expert operations ran BEFORE get_peft_model, so neither did anything the operator asked for: F-P3-FABLE-02: `_freeze_unselected_experts` set requires_grad=False on base expert weights one line before get_peft_model, which then re-freezes every base parameter and makes only the injected `lora_*` adapters trainable — so the post-PEFT trainable set was identical whether experts_to_train was set or not, while the log asserted a selective-training outcome that never materialised. F-P3-FABLE-08: `_apply_moe_expert_quantization` ran before any freezing, so its "recast frozen expert weights" sweep always found nothing (permanent no-op on the non-4bit path); on the default 4-bit path it recast packed Params4bit uint8 storage to bf16, corrupting the quant_state (crash or garbage at first forward). Restructured the flow: `_detect_moe_experts` resolves the count pre-PEFT; `_apply_moe_post_peft` runs selection + quantization AFTER get_peft_model. `_freeze_unselected_experts` now freezes the LoRA adapters of unselected experts (and warns when target_modules cover no experts, so selection has no effect). Quantization skips entirely when the model is 4-bit-loaded and never casts packed uint8/int8 storage; per-tensor recast failures now surface at WARNING. Tests: selection now constrains the adapter set (differs from "all"); no-expert- adapter case warns; 4-bit model skips quantization (uint8 unchanged); detection recognises Qwen3/DeepSeek shapes. Findings: F-P3-FABLE-02, F-P3-FABLE-08. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Heads-up: this branch now also carries the full-project review Wave 0 + Wave 1/H1 fixesBeyond this PR's original HodeTech-migration scope, Wave 0 — 7 CRITICAL release-blockers ✅
Wave 1 / H1 — Trainer/model dispatch (11/11) ✅
Still to come (separate follow-up work)W1 H2–H11 (~60 findings) and W2–W4 (~120 findings). Tracker: 🤖 Generated with Claude Code |
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 `@docs/reference/audit_event_catalog.md`:
- Line 41: The documentation for the `pipeline.stage_completed` event
incorrectly lists `auto_revert_triggered` as an unconditional field in the
payload. Since the implementation in `forgelm/cli/_pipeline.py` only emits this
field on failure paths (not on success), update the payload documentation to
mark `auto_revert_triggered` as conditional or optional, clearly indicating it
is only present when pipeline stages fail. This ensures the audit event catalog
accurately reflects what the actual code emits.
In `@docs/standards/architecture.md`:
- Line 130: The documentation in docs/standards/architecture.md at line 130
states that heavy dependencies are declared with "specific version bounds," but
pyproject.toml does not enforce upper bounds for all of them—wandb, unsloth, and
deepspeed have only lower bounds. Update the wording to either use "version
constraints" as a more general term that covers both bounded and lower-bounded
cases, or explicitly call out that some dependencies have only lower bounds
while others have specific bounds. This will align the architectural standard
documentation with the actual dependency declarations in pyproject.toml.
In `@forgelm/cli/_pipeline.py`:
- Around line 390-400: The resume gate that checks for prior_state.status ==
"gated_pending_approval" refuses the operation by logging an error and returning
EXIT_CONFIG_ERROR, but fails to emit a structured audit event to the append-only
audit log before returning. Add a structured audit event emission before the
return statement that captures this decision gate event with at least three
pieces of information: the requested stage (resume_from parameter), the blocking
stage (prior_state.name), and the blocking status (the "gated_pending_approval"
status value). This ensures the append-only audit log has a record of the
blocked resume attempt as required by Article 14 decision gate guidelines.
In `@forgelm/cli/subcommands/_approve.py`:
- Around line 492-499: The webhook notification error handling is logging the
raw exception object directly, which can expose sensitive data like tokens or
URLs in webhook-related exceptions. For both the approve command notification
error handler at lines 492-499 (after the notifier.notify_success call) and the
similar reject command notification error handler at lines 572-577 (after what
is likely a notifier.notify_failure call), replace the exception object in the
logger.warning call with a generic message or exception type name that does not
include the raw exception details. This prevents leaking secret-bearing
information while still logging that the webhook notification failed.
In `@forgelm/model.py`:
- Around line 524-533: The code at line 524 attempts to extract target_modules
using nested getattr calls that fail when peft_config is a dictionary (keyed by
adapter name per PEFT's multi-adapter design), causing it to default to "?"
instead of showing actionable target_modules information. Modify the
target_modules extraction logic to first check if peft_config is a dictionary,
and if so, retrieve the first adapter's configuration object before accessing
the target_modules attribute. This ensures the warning message displays the
actual target_modules information regardless of whether peft_config is a simple
object or a multi-adapter dictionary.
In `@forgelm/trainer.py`:
- Around line 910-913: The `_revert_model()` function currently clears only
`train_result.staging_path` when reverting, but does not clear
`train_result.final_model_path`, which can point to a `gate_path` directory that
gets deleted. This causes the CLI to advertise a model path that no longer
exists. Set `train_result.final_model_path = None` alongside the existing
`train_result.staging_path = None` assignment in all three revert branches to
ensure the result does not report stale artifact paths.
- Around line 1402-1408: The ConfigError raised when judge_api_key_env is not
properly configured is currently being caught by generic Exception handlers in
two entrypoints and returning EXIT_TRAINING_ERROR (exit 2) instead of the
correct EXIT_CONFIG_ERROR (exit 1). Add dedicated exception handling for
ConfigError in forgelm/cli/_training.py (around lines 156-163) and
forgelm/cli/_pipeline.py (around lines 1295-1298), placing the ConfigError
handler before the generic Exception handler so that ConfigError exceptions are
caught specifically and return EXIT_CONFIG_ERROR to properly reflect the
configuration error exit code contract.
In `@tests/test_human_approval_gate.py`:
- Around line 216-217: The test does not properly validate the reset path for
the awaiting_approval flag because it starts with the default value of False.
Before executing the revert operation (around the TrainResult creation),
explicitly seed awaiting_approval to True in the test setup. This ensures that
when _apply_safety_result is called during revert, the test can properly verify
that the flag is actually being cleared rather than just remaining False by
default.
In `@tests/test_packaging.py`:
- Around line 120-125: The _load_optional_dependencies function currently only
attempts to import tomllib and skips the test if unavailable, but since the
project supports Python 3.10 where tomllib is not available, the test will
always skip on Python 3.10 despite having coverage available through tomli.
Align this function with the existing fallback pattern already present elsewhere
in the file (around lines 93-96) by attempting to import tomllib first, then
falling back to tomli if tomllib is not found, and only calling pytest.skip if
neither module is available. This ensures test coverage across all supported
Python versions including Python 3.10.
In `@tests/test_trainer_max_length.py`:
- Around line 72-86: The test_grpo_passes_max_length_as_prompt_cap function
lacks the importability guard that exists in the preference trainer test above
it. Before attempting to patch trl.GRPOConfig in the function, add a guard using
getattr(trl, "GRPOConfig") followed by a pytest.skip() call to gracefully skip
the test if trl.GRPOConfig cannot be imported due to torch/trl/vllm
compatibility issues, rather than allowing the test to hard-fail when the import
fails.
🪄 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: 4f5e1a2a-559e-4874-b4ff-1984b1d8fa46
📒 Files selected for processing (41)
.github/workflows/ci.ymlAGENTS.mdCLAUDE.mdREADME.mddocs/reference/audit_event_catalog-tr.mddocs/reference/audit_event_catalog.mddocs/reference/usage-tr.mddocs/reference/usage.mddocs/standards/architecture.mddocs/usermanuals/en/reference/json-output.mddocs/usermanuals/tr/reference/json-output.mdforgelm/cli/_pipeline.pyforgelm/cli/_result.pyforgelm/cli/_training.pyforgelm/cli/subcommands/_approve.pyforgelm/cli/subcommands/_doctor.pyforgelm/compliance.pyforgelm/config.pyforgelm/merging.pyforgelm/model.pyforgelm/model_card.pyforgelm/results.pyforgelm/trainer.pyforgelm/webhook.pypyproject.tomltests/test_check_audit_event_catalog.pytests/test_compliance.pytests/test_cost_estimation.pytests/test_human_approval_gate.pytests/test_merging_algos.pytests/test_model_card.pytests/test_model_loading_dispatch.pytests/test_moe_functions.pytests/test_oom_recovery.pytests/test_packaging.pytests/test_pipeline_orchestrator.pytests/test_trainer_grpo_config.pytests/test_trainer_judge_env.pytests/test_trainer_max_length.pytests/test_webhook.pytools/check_audit_event_catalog.py
✅ Files skipped from review due to trivial changes (3)
- docs/reference/audit_event_catalog-tr.md
- README.md
- forgelm/config.py
…ert paths, audit/doc accuracy) CI: `ruff format` config.py (the failing `ruff format --check .` gate). Bandit B615 (from_pretrained without revision pinning) is left as-is — a pre-existing MEDIUM-severity pattern shared by all four from_pretrained calls (no revision pinning anywhere in the project); bandit fails only on HIGH. Code: - trainer.py: `_mark_reverted` clears `final_model_path` and `awaiting_approval` (not just `staging_path`) on every gate-revert branch — the revert deletes the model, so the CLI/JSON envelope must not advertise a stale path, and a reverted run is never awaiting approval. - _training.py + _pipeline.py: route `ConfigError` to EXIT_CONFIG_ERROR (1) before the generic catch, so the judge-env-unset ConfigError (H1a) maps to the config- error exit code instead of EXIT_TRAINING_ERROR (2) in both entrypoints. - _pipeline.py: emit a `pipeline.resume_refused` audit event (requested/blocking stage + status) before refusing a resume across an unapproved gate (Article 14 decision-gate record) — catalogued EN+TR. - model.py: resolve `peft_config` dict (PEFT keys by adapter name) before reading `target_modules` in the no-expert-adapters warning, so it shows the real value. - _approve.py: webhook-failure handlers log the exception TYPE, not the raw object (a transport error can embed the secret-bearing URL/token). Docs: - audit_event_catalog (EN+TR): mark `stage_completed`'s `metrics` (success only) and `auto_revert_triggered` (failure path only) as conditional; add the new `pipeline.resume_refused` row. - architecture standard: "specific version bounds" → "version constraints (a lower bound each; some also carry an upper bound)" to match pyproject. Tests: - ConfigError→exit-1 (pipeline stage + single-run CLI); resume-refusal emits the audit event; revert clears final_model_path + awaiting_approval (seeded True); target_modules surfaces from a dict peft_config; packaging tomli fallback for Python 3.10; GRPOConfig importability guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forgelm/trainer.py (1)
1409-1415:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail this judge-secret validation before training starts.
This check currently runs only in the post-train judge stage, so a missing
judge_api_key_envcan burn the entire training job, write staged artifacts, and only then exit with a config error. Move the validation into_validate_evaluation_config()(or another preflight reached from__init__) and emit a dedicated audit event before raising so the refusal is fail-fast and recorded as its own decision, rather than only surfacing later via the genericpipeline.failedcatch.As per coding guidelines, every decision gate in
forgelm/must emit a structured audit event to the append-only audit log.🤖 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/trainer.py` around lines 1409 - 1415, The judge API key validation check currently runs too late in the pipeline (post-train judge stage), allowing the training job to proceed and consume resources before failing. Move the validation block that checks judge_cfg.judge_api_key_env and raises ConfigError into the _validate_evaluation_config() method (or another preflight validation called from __init__) to fail fast before training begins. Additionally, emit a structured audit event before raising the ConfigError to record this decision gate, ensuring compliance with coding guidelines that require all decision gates in forgelm/ to emit audit events to the append-only audit log.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@forgelm/trainer.py`:
- Around line 1409-1415: The judge API key validation check currently runs too
late in the pipeline (post-train judge stage), allowing the training job to
proceed and consume resources before failing. Move the validation block that
checks judge_cfg.judge_api_key_env and raises ConfigError into the
_validate_evaluation_config() method (or another preflight validation called
from __init__) to fail fast before training begins. Additionally, emit a
structured audit event before raising the ConfigError to record this decision
gate, ensuring compliance with coding guidelines that require all decision gates
in forgelm/ to emit audit events to the append-only audit log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85f4e859-0619-4f07-9e2f-e6f2ad2f4cac
📒 Files selected for processing (15)
docs/reference/audit_event_catalog-tr.mddocs/reference/audit_event_catalog.mddocs/standards/architecture.mdforgelm/cli/_pipeline.pyforgelm/cli/_training.pyforgelm/cli/subcommands/_approve.pyforgelm/config.pyforgelm/model.pyforgelm/trainer.pytests/test_human_approval_gate.pytests/test_moe_functions.pytests/test_packaging.pytests/test_pipeline_orchestrator.pytests/test_trainer_judge_env.pytests/test_trainer_max_length.py
✅ Files skipped from review due to trivial changes (3)
- docs/standards/architecture.md
- docs/reference/audit_event_catalog.md
- forgelm/config.py
🚧 Files skipped from review as they are similar to previous changes (10)
- docs/reference/audit_event_catalog-tr.md
- forgelm/cli/subcommands/_approve.py
- tests/test_trainer_judge_env.py
- tests/test_moe_functions.py
- forgelm/cli/_pipeline.py
- tests/test_packaging.py
- tests/test_human_approval_gate.py
- tests/test_trainer_max_length.py
- tests/test_pipeline_orchestrator.py
- forgelm/model.py
…fore training (review) The judge_api_key_env check (F-P3-FABLE-18: an API judge whose env var is unset must not silently flip to local mode) ran only at the post-train judge stage, so a misconfigured run consumed the whole training loop before failing. Move the check into a `_validate_judge_config` preflight called from `__init__` (alongside `_validate_evaluation_config`), so construction raises ConfigError before training starts. `_run_judge_if_configured` now calls the same shared helper (defence-in-depth for the direct entry point), so there is no duplicated logic and the existing judge-time tests stay valid. The CLI ConfigError→exit-1 routing already wraps trainer construction in both entrypoints, so the earlier raise still maps to EXIT_CONFIG_ERROR. Not done — the suggested audit event for this refusal is intentionally skipped: a preflight config-validation error is not a runtime decision gate (the audit-event requirement targets safety/judge/benchmark/approval gate *outcomes* with scores); the sibling preflight `_validate_evaluation_config` likewise raises/warns without audit events; and adding one would introduce a new catalogued event for a non-gate config error before any run-level audit context exists. The ConfigError message + the exit-1 contract are the actionable record. Test: constructing ForgeTrainer with an unset judge_api_key_env raises ConfigError at __init__ (preflight), not only at the judge stage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review all |
Contributor
|
✅ Action performedFull review finished. |
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.
Summary
Two related housekeeping changes that both target
mainviadevelopment:github.com/HodeTech/ForgeLM). Updates every reference project-wide (271 refs / 99 files, 1:1 string replacements).transformers PYSEC-2025-217to the project ignore list so the daily nightly goes green again.1. Org migration
github.com/cemililik/ForgeLM→github.com/HodeTech/ForgeLM(README badges,pyproject.toml[project.urls], docs, user manuals EN+TR, notebooks,site/, CHANGELOG, CONTRIBUTING, issue-template,forgelm/model_card.py, standards, roadmap)ghcr.io/cemililik/forgelm→ghcr.io/hodetech/forgelm(lowercase namespace)ai-team@cemililik.com→forgelm@hodetech.io(CODE_OF_CONDUCT.md)organization=hodetech,projectKey=HodeTech_ForgeLM(sonar-project.properties)git grep cemililik→ 0 remaining. Guards green: anchor resolution, bilingual parity, user-manual self-contained, CLI help consistency, site-version, site-claims.ruffclean;model_cardtests pass.2. Nightly CVE ignore (transformers PYSEC-2025-217)
The new
--ignores tools/pip_audit_ignores.yamlmechanism works onmain(the run log showsCVE-2026-1839correctly suppressed), but a fresh no-fix advisory dropped:AV:L/UI:R(local + user must open a malicious checkpoint), affected through5.0.0rc0, no fix in the pinned 4.x line.The ignore-file loader self-validates (
tools/check_pip_audit.py --ignores); 23 tests pass.Repo-side follow-ups (not in this diff)
hodetechorg with keyHodeTech_ForgeLM; re-addSONAR_TOKENsecret if not inherited (repo currently lists no Actions secrets).Closes #58
🤖 Generated with Claude Code
Summary by CodeRabbit
Security
Bug Fixes
Documentation
Chores
mergingextra a no-op (native merging).