Full-project-review remediation: Waves 1–4 (H2→N1, 78 commits)#63
Merged
Conversation
…egenerate probes; refuse causal-LM-as-classifier (H2 / XP-06, F-P1-FAB-04,07,08, F-P3-FABLE-05,15,16,17)
Config (XP-06): reachable SafetyConfig/JudgeConfig/BenchmarkConfig states
silently disabled a configured auto-revert gate while the run still
reported passed=True. Add ge/le bounds to the five float threshold
fields (safety 0-1, judge 1-10, benchmark 0-1) and a SafetyConfig
model_validator that rejects min_safety_score under non-confidence_weighted
scoring, restricts severity_thresholds to the known severity vocabulary
with 0-1 values, and auto-enables track_categories when severity_thresholds
is set so the gate is actually enforced. Export SEVERITY_LEVELS as the
single source of truth shared with the runtime severity_dist counters.
Runtime fail-open closures:
- F-P3-FABLE-05/16: an existing-but-empty / all-blank / wrong-schema
probes file now fails CLOSED (passed=False, evaluation_completed=False),
symmetric with the missing-file path, instead of returning a vacuous
passed=True. _load_safety_prompts skips and counts rows that yield no
usable prompt instead of feeding empty-string probes.
- F-P3-FABLE-17: refuse a causal-LM checkpoint (e.g. the default
Llama-Guard-3-8B) loaded via pipeline('text-classification') when its
classification head is newly-initialized/absent (causal-LM architecture
+ placeholder LABEL_0/LABEL_1 labels) — every verdict would be garbage.
Generation-based Llama-Guard scoring is deferred; the minimal
refuse-with-actionable-error fix the finding offers is implemented.
Adds an evaluation_completed flag to SafetyResult so callers can tell
"could not evaluate" from "evaluated and failed the gate".
Regression: tests/test_config.py::TestSafetyGateValidation,
tests/test_safety_advanced.py::TestDegenerateProbeFailClosed,
::TestClassifierHeadValidation.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, enable track_categories, record total_count (H2 / F-P3-FABLE-12,13,16)
Standalone `forgelm safety-eval` (F-P3-FABLE-12/13):
- A classifier that never loaded (or a probes file with no usable
prompts) is a runtime/config problem, not a threshold failure. The
dispatcher now exits EXIT_TRAINING_ERROR (2) when run_safety_evaluation
flags the run with evaluation_completed=False, matching the documented
exit-code contract (was exit 3).
- Construct and pass an AuditLogger so the documented Article-15
audit.classifier_load_failed event can actually fire on this surface
(its emission guard previously always skipped).
- Pass SafetyEvalThresholds(track_categories=True) so the advertised
per-category breakdown is reachable instead of always {}.
Training gate (F-P3-FABLE-16): the safety.evaluation_completed audit
payload now carries total_count so a vacuous pass (zero probes
evaluated) is distinguishable from a real 100%-safe evaluation in the
append-only trail. Catalog row updated (EN + TR).
Regression: test_verification_toolbelt.py::TestSafetyEvalDispatcher
(classifier-load → exit 2; track_categories + audit_logger wired);
test_human_approval_gate.py::...::test_safety_audit_event_records_total_count.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…F-P3-FABLE-14, F-P7-OPUS-15) The safety-eval dispatcher explicitly refuses .gguf paths with a config error, but four user-facing surfaces still claimed GGUF support and the GGUF-export manual instructed operators to run a safety re-check against the quantised artefact (a command that exits 1). Align all surfaces: - the safety-eval subparser description and --model help no longer mention GGUF; - the top-level CLI epilog says "HF checkpoint"; - the EN + TR GGUF-export and deploy-targets manuals now tell operators to run safety-eval against the pre-export HuggingFace checkpoint and treat post-quant safety drift as a known limitation. No new behaviour; CLI ↔ docs help-text consistency restored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…H3 / F-P2-FAB-06, F-P3-FABLE-27, F-P2-FAB-42, F-P3-FABLE-63)
_math_reward_fn graded the FIRST "Answer:" occurrence via leftmost .search
while the format reward is end-anchored, so a self-correcting completion
("Answer: 5 … Answer: 7") earned format reward on its final answer but
correctness reward on an earlier discarded candidate — a reward-hackable
divergence. Grade the LAST marker (finditer → last) to match the format gate.
Unify the two divergent module-private _ANSWER_PATTERN constants: the
capturing extraction regex now lives once in grpo_rewards as the shared,
documented ANSWER_EXTRACT_PATTERN (imported by trainer); the format gate's
end-anchored pattern is renamed _ANSWER_END_PATTERN with its rstrip contract
documented, removing the same-name/different-contract collision.
Make combined_format_length_reward's zip strict=True, consistent with the
fail-loud strict zip in _math_reward_fn.
Regression tests: last-vs-first anchoring (correct-final + reward-hack→0),
trailing-prose last-marker grading, cross-module correctness/format agreement,
and combined-reward strict-zip-raises-on-mismatch. Bilingual alignment guide
updated (EN + TR) to state final-marker grading.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e (H4 / F-P2-FAB-03, F-P2-FAB-04) F-P2-FAB-03: after `forgelm approve` renames the gated staging dir to final_model/, `--resume-from` now recognises the stage as approved (staging gone + promoted path present), skips it, rewrites its output_model to the promoted path, and chains downstream from the approved weights — instead of re-training the gated stage. An unapproved gate (staging still on disk) still refuses per C3. F-P2-FAB-04: a `--resume-from` run now resets the prior run's terminal fields (final_status→in_progress, stopped_at/finished_at→None) so a resumed-successful run no longer carries a stale stopped_at_stage / gated_pending_approval status; `_finalise_pipeline` then rewrites and emits exactly one `pipeline.completed` with the corrected status. Regression tests cover: post-approval resume skips the gated stage and chains from final_model/ with one stage_started per stage_index; resume-to-success after a failure resets fields + emits corrected completion; resume-to-success after a gate finalises as completed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-OPUS-01, F-P6-OPUS-15) clean_string silently coerced non-string payloads (dict/list/int via str(), None/falsy -> "") into training data while the modern messages path (_process_messages_format) raises loudly on the same shapes. This baked Python repr strings and empty responses into the corpus with exit 0 and no error -- silent-wrong-training. Make clean_string raise ValueError on any non-str cell, symmetric with the messages path; wrap the text and User/Assistant formatters to add row-index context to the raised error. Policy: raise (not skip-with-count) -- matches the in-repo messages-path contract and the batched .map() has no per-row drop mechanism. Callers already omit genuinely-absent optional cells (missing system prompt) before reaching clean_string, so legitimate empties still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, atomic exports (H5 / F-P4-OPUS-03,04,05,06,10,11,13,15,33) - verify_audit_log refuses require_hmac=True with hmac_secret=None at the library boundary instead of returning valid=True after a presence-only check (fail-open closed). - verify-audit / verify-annex-iv exit-code docs (parser help, module docstring, usermanual EN+TR) reconciled to the shipped exit 1 contract. - Single-stage training manifest, human_approval.required event, and the training JSON envelope now carry config_hash (+run_id) via a shared compute_config_hash helper, mirroring the pipeline's config-binding. - export_compliance_artifacts writes the 5-artifact Annex IV bundle all-or-nothing (staging dir + os.replace promotion); export_evidence_bundle writes tmp+rename; the trainer emits compliance.artifacts_export_failed on export failure and fires compliance.artifacts_exported even when the secondary Article-10 governance report fails (carrying governance_ok). - New audit event compliance.artifacts_export_failed catalogued EN+TR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…romote_strategy drift (H5 / F-P4-OPUS-02,07,08,32; F-P5-OPUS-03; F-P7-OPUS-17) - All six EN+TR doc sites claiming the Windows AuditLogger uses msvcrt.locking now state the truth: no cross-process lock on Windows (advisory flock helper is a no-op); use a distinct output_dir per run. Code comment tightened from 'advisory-only' to 'no lock acquired'. - Webhook vocabulary corrected from 'five' to 'eight' everywhere (standard, audit_event_catalog EN+TR, webhooks usermanual EN+TR): the three pipeline.* events are now documented alongside the five lifecycle events, with the payload-schema enum and the canonical_webhook_events helper docstring updated to match the code's actual 8 events. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rip (H6 / F-P1-FAB-03,05,06) - DistributedConfig.strategy: Optional[str] -> Optional[Literal["deepspeed","fsdp"]] so unsupported strategies (horovod, ddp, "DeepSpeed") fail at config time instead of silently running single-GPU at trainer.py. - DataConfig: reject non-finite mix_ratio weights (NaN/inf) and add a cross-field model_validator requiring len(mix_ratio) == 1 + len(extra_datasets); data._merge_extra_datasets now raises loudly on a length mismatch instead of silently falling back to uniform mixing. - merge_pipeline_stage_config: dump with exclude_unset=True so unset defaults (evaluation.staging_ttl_days=7) no longer round-trip as explicitly-set and falsely raise ConfigError for pipeline + retention + evaluation configs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+ document include_eval_samples (H6 / F-P1-FAB-15,16,09) F-P1-FAB-15 (sample_packing): the documented-but-unconsumed field was a silent no-op. It now forwards to `packing` (TRL's single packing knob) at config time with a DeprecationWarning + logger.warning, so following the docs actually fires. Reconciled the contradictory "requires"/"mutually exclusive" descriptions across configuration.md (+TR), usermanuals/sft.md (+TR), troubleshooting.md (+TR), usage.md (+TR), config_template.yaml, roadmap/releases.md, design/blackwell_optimized.md — all now point at `packing`; removal scheduled v0.9.0. F-P1-FAB-16 / XP-16 (deprecation cadence): v0.7.0 shipped without the promised removals of evaluation.staging_ttl_days and --data-audit. Deferred both to v0.8.0 (the deferral was already recorded in _dispatch.py) and retargeted every stale "v0.7.0" stamp consistently: config.py warnings / ConfigError / docstrings, _parser.py --data-audit help, configuration.md (+TR) deprecation notes, usermanuals/cli.md (+TR), qms SOP (+TR), release.md worked example. Added an [Unreleased] CHANGELOG note recording the deferral; the ### Removed section lands in v0.8.0. F-P1-FAB-09 (include_eval_samples): documented the v0.7.0 privacy-by-default opt-in fields (evaluation.safety.include_eval_samples and evaluation.llm_judge.include_eval_samples) in configuration.md + TR mirror, plus a retroactive [0.7.0] ### Changed CHANGELOG note describing the artifact-content change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The training-mode `--dry-run --output-format json` envelope keyed only on `status: "valid"` with no `success` key, breaking the universal envelope contract (json-output.md "Common conventions": every envelope starts with `success`). A CI consumer reading `result["success"]` got a KeyError on the happy path — the most common CI pre-flight invocation. - Prepend `success: True` as the first key in `_build_dry_run_result` (keep `status: "valid"` for backward compat); skip both wrapper keys in the text-mode summary loop. - Document the dry-run config-validation envelope in json-output.md (+ TR mirror) with the full key set and exit-code mapping. - Regression test: test_dry_run_json_envelope_has_success_true. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s (H7 / F-P1-FAB-14) error-handling.md exit-code table row 0 claimed exit 0 means "all gates passed", but with the shipped default `evaluation.auto_revert: false` a failed benchmark/safety/judge gate is recorded (JSON gate block, `*_passed: false`) yet the model is still promoted and the run exits 0. The same false guarantee appeared across the user-facing surface. - error-handling.md row 0: qualify the exit-0 guarantee by `auto_revert`. - exit-codes.md (EN+TR): fix row 0, row 3 (was implying exit 3 fires without a revert), and the "What exit 0 actually guarantees" section. - judge.md (EN+TR): fix the inverted "otherwise exits non-zero" claim. - logging-observability.md + audit_event_catalog.md (EN+TR): reword the `training.success` / `pipeline.completed` "All gates passed" descriptions (the event also fires on the no-revert gate-failure path). - Regression tests pinning the load-bearing behaviour: a failed benchmark gate continues (returns True, records benchmark_passed=False) under auto_revert=false, and reverts+halts (returns False) under auto_revert=true. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…real one (H7 / F-P1-FAB-11) `tools/regenerate_config_doc.py` was cited as the Phase 16 config-drift control in the QMS change-management SOP (EN+TR §4.2), the ISO 27001 / SOC 2 design doc, check_field_descriptions.py's docstring, and a ci.yml comment — but the file never existed in git history and no config-doc diff-guard was ever wired. An auditor following the SOP would find a documented control that cannot be executed or evidenced. Option (b) from the finding (smaller honest diff): rewrite all four production-tree sites to describe the real control — `check_field_descriptions.py --strict` (CI) + manual doc review + `check_bilingual_parity.py --strict` — and record the correction in the append-only SOP review log. - Regression test (test_phantom_tool_citations.py): no tracked doc/code presents `regenerate_config_doc` as a live control; the real control file exists; the scanner docstring no longer cites the phantom companion. Note (out of H7 scope): other public-tree files still cite tools that do not exist (docs/design/library_api.md → check_api_compat.py / check_api_doc_completeness.py; docs/roadmap/* → check_webhook_event_vocabulary.py, check_test_naming.py, manual_smoke.py et al). A blanket "every cited tools/*.py must exist" guard belongs with the guard-apparatus work (H11); this test is deliberately targeted at the F-P1-FAB-11 phantom. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ort-error logs (H8 / F-P5-OPUS-01,06,11) safe_post/safe_get logged the raw requests/urllib3 exception string, which embeds the request URL path — webhook bearer tokens (Slack/Teams/Discord/ custom carry the secret in the path) leaked into the forgelm._http WARNING log on any transport failure. Add _redact_url_paths_in_text: an exact known-URL pass plus a bounded, ReDoS-safe 'url: <path>' token pass, applied after header masking in both helpers. webhook._mask was a second URL masker that appended the first path segment, leaking the token for custom receivers shaped https://host/<TOKEN>. Collapse it to a thin wrapper over the single _http._mask_netloc chokepoint so the masking policy lives in one place (no update-one-not-the-other drift). Regression tests: transport-error path-redaction for safe_post + safe_get + HEAD; _mask never leaks the secret across receiver shapes and equals _mask_netloc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ommonpath (H8 / F-P5-OPUS-02) forgelm purge --run-id <id> --kind staging took --run-id verbatim (free-form type=str, no charset constraint) into final_model.staging.<run_id>, gated only by Path.exists() before shutil.rmtree. A '..'-bearing run_id could resolve outside output_dir → arbitrary-directory delete — the exact commonpath defence-in-depth the design (gdpr_erasure.md §4.4) claims but never had. Add _path_inside_output_dir (mirrors _approve._staging_path_inside_output_dir: realpath + commonpath) and reject any resolved target that escapes output_dir before the dry-run report or deletion. Fail-closed (whole batch refused), EXIT_CONFIG_ERROR, with a data.erasure_failed event (error_class= PathTraversalRefused) — no new dotted audit event, so the catalog is unchanged. Regression tests: traversal run_id refused (victim untouched, exit 1, request→failed not completed); _path_inside_output_dir accept/reject unit coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… / F-P5-OPUS-04)
supply_chain_security.md (EN + TR) still described the pre-2026-05-24
UNKNOWN policy ('::warning::, nightly stays green' / 'yeşil kalır') after
check_pip_audit.py flipped to fail-closed (exit 1, ::error:: per finding).
pip-audit's JSON omits OSV severity so UNKNOWN is the dominant path — the
doc told deployers their gate was advisory when it actually fails the run.
Rewrite the UNKNOWN bullet in both languages to the fail-closed policy +
the opt-in ignore-file triage path.
Regression test: a doc-consistency guard asserting the UNKNOWN bullet says
'exit 1' and no longer declares 'stays green' / 'yeşil kalır' in either
language.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…H10 / F-P7-OPUS-05,06,07,08)
- _check_navigation_token now raises WizardCancel on cancel/q/c/quit so the
welcome banner's 'type cancel to exit cleanly' promise holds at every step
(F-P7-OPUS-05). The step machine and the quickstart/BYOD prelude convert it
to a clean WizardOutcome(None) -> EXIT_WIZARD_CANCELLED; the BYOD prelude
still treats cancel as 'fall back to the full wizard'.
- run_wizard_full wraps the prelude so back/reset raised before the step
machine no longer escape as an uncaught traceback (exit 1); they fall
through to the full wizard instead (F-P7-OPUS-08).
- _maybe_run_wizard refuses '--wizard --output-format json' with a proper
{success:false} envelope on stdout + exit 5, and routes the
--wizard-start-from guidance warning to stderr in JSON mode so it never
precedes the dispatcher envelope (F-P7-OPUS-06/07).
- Regression tests for cancel-in-step-machine, cancel-in-BYOD-fallback,
back/reset-in-prelude, JSON-mode refusal, and stderr/stdout warning routing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…0 / F-P7-OPUS-01,02,03,09,10,11,12,13,14,18) One CLI-docs sweep (EN+TR mirrors kept in spine sync): - Legacy --data-audit alias now passes enable_quality_filter=True so it matches the 'identical behaviour' claim vs the audit subcommand (-01). - cli.md exit-codes table: drop the phantom 130 row (Ctrl+C is clamped to 2) and disclose that argparse usage errors exit 2 while post-parse config validation exits 1 (-02/-03). - json-output.md checks[].status enum corrected to pass/warn/fail; a crashed probe surfaces as fail + extras.crashed (-09). - cache-tasks --output help advertises the Datasets cache chain it actually uses, not the Hub chain (-10). - cli.md doctor summary drops the non-existent audit-secret probe claim (-11). - json-output.md gains forgelm export + forgelm deploy envelope sections (-12). - cli.md stops advertising comma-separating the single-value --quant choices flag (-13); check_cli_help_consistency now flags comma-list prose claims on any choices flag. - chat --help + chat.py docstrings drop the unshipped per-response safety annotations claim (-14). - cli.md describes reject as PRESERVING staging for forensics, not 'discard staging' (-18). Regression tests: legacy-alias quality filter, chat-help no-safety, export comma-quant rejection, export/deploy envelope doc parity, cache-tasks help, doctor status-vocabulary + probe-list doc parity, exit-code/reject doc consistency, and four new prose-claim guard tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dules (H11 / F-P8-C-02, F-P8-C-03) model.py loading core (_resolve_bnb_compute_dtype ValueError, _device_map_for, _build_model_kwargs quantization gating, _load_tokenizer pad-token, _build_lora_config / _resolve_peft_flags) and data.py format-processing + column-validation raises (clean_string, _process_messages_format, _process_user_assistant_format, _validate_trainer_columns, _apply_mix_ratio, _merge_extra_datasets) previously ran only via lambda stubs or export-symbol assertions. Both now have direct no-GPU/no-network behavioural coverage of their raise paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (H11 / F-P8-C-08) The guard was wired into ci.yml but untested, and bypassable: the third pattern matched only ``sys.modules["torch"] = None`` while ``= object()`` / ``= SimpleNamespace()`` / ``= fake`` corrupt the session identically, and the per-physical-line scan missed the multi-line ``pop(\n "torch")`` split. - Broaden the assignment pattern to ``= <anything>`` (negative lookahead on ``==`` so equality comparisons stay clean). - Rejoin logical lines before matching so multi-line pop/del forms are caught. - Add tests/test_check_no_unguarded_sys_modules_pop.py covering each detected form, each sanctioned form, the live-repo clean pass, and CI wiring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… TR cross-link leaks (H11 / XP-13, F-P8-C-04, F-P8-C-07, F-P8-C-13) Guard apparatus had dead arms: of 19 tools/check_*.py guards, ten ran in no workflow and three gauntlet-listed guards ran only by human discipline. Wiring (ci.yml validate job): - check_library_api_doc, check_bilingual_code_blocks, check_yaml_snippets, check_site_chrome_parity (default mode — --strict gates the deferred i18n backlog), check_module_size, and the three gauntlet-only guards (check_wizard_defaults_sync, check_no_analysis_refs, update_site_version). - New check_tr_links_prefer_mirror guard (F-P8-C-04) wired into ci.yml AND the CLAUDE.md/AGENTS.md gauntlet. Self-enforcement: - tests/test_guard_wiring.py meta-test: every tools/check_*.py is wired OR allowlisted with a rationale; every gauntlet guard is in a workflow; the CLAUDE.md/AGENTS.md gauntlets stay in lockstep. Only check_doc_numerical_claims (W1/H5) and check_notebook_pins (W2/M7) remain allowlisted with owner notes. - Own-tests for the previously-untested guards: bilingual_code_blocks, library_api_doc, yaml_snippets, site_chrome_parity. TR cross-link sweep (F-P8-C-04): - 50 of 62 in-prose cross-links across 19 *-tr.md pages routed Turkish readers to the English sibling despite an existing -tr.md mirror; rewritten to the TR mirror (translated link text preserved). The **Ayna:** backlinks are exempt. - localization.md "Structural mirror rule" clarified to make the rule explicit. Co-located fixes folded in (house policy): - yaml-templates.md (EN+TR): severity_thresholds used invalid Llama-Guard category codes (S1/S5/S10) instead of severity levels — failed check_yaml_snippets; fixed to critical/high/medium/low. - check_no_analysis_refs: allowlist tests/test_phantom_tool_citations.py (functional path filter, not a content citation) so the now-wired guard is green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eps (M1 / P3-FABLE-22,23,24,25,26 + P2-FAB-09,15,20) - OOM rebuild now reuses the user-supplied callbacks captured in train() instead of the live callback_handler list (which already carries HF's instantiated defaults), so DefaultFlow/Progress callbacks are no longer duplicated and EarlyStopping patience no longer leaks across retries (F-P3-FABLE-22). - oom_recovery_min_batch_size gains a ge=1 Field bound + a defensive max(min_bs, 1) clamp in the loop, so a 0/negative floor can no longer drive new_bs to 0 → ZeroDivisionError; the clean "cannot recover" diagnostic fires instead (F-P3-FABLE-23). - execute_evaluation_checks decouples detection from reversion: a configured eval-loss threshold/baseline (and NaN/Inf divergence) is now always evaluated and logged even when auto_revert=false — previously the whole check was skipped, letting a diverged model ship with exit 0 and no signal (F-P3-FABLE-24). - Benchmark gate fails fast at the config preflight when lm-eval is missing (install hint, before training), and the three gate-runner import guards re-raise ImportError-with-hint instead of swallowing to None, so a configured gate can never silently degrade to a skip with exit 0 (F-P3-FABLE-25). - Safety infrastructure-failure results set safe_ratio=0.0 explicitly so the trainer-side metric / audit payload no longer report a perfect 1.0 safe ratio for an evaluation that classified zero responses (F-P3-FABLE-26). - train() moves _build_trainer inside the try so a construction failure still emits pipeline.failed + notify_failure after notify_start fired (F-P2-FAB-09); regression tests cover the OOM loop, gate import contract, and train() failure path (F-P2-FAB-15, F-P2-FAB-20). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…P2-FAB-07,11,12,13) - Re-executing a stage now clears every attempt-scoped field (error, skipped_reason, gate_decision, auto_revert_triggered, exit_code, finished_at, duration_seconds, output_model) loaded off the resumed state file, so a resumed-to-success stage no longer persists status="completed" next to a stale error="Trainer crashed: …" (F-P2-FAB-07). - `--stage <name>` no longer falsely refuses a stage that declares its own `model:` block: such a stage ignores the auto-chain seed, so the prev-output-on-disk demand is a spurious refusal (F-P2-FAB-11). - A `--stage` run on a pipeline that already ran preserves the prior run's completed stage history (status / output_model / metrics) via _init_state_preserving instead of clobbering pipeline_state.json — a later `--resume-from` then correctly skips stages whose final_model/ is on disk rather than re-training them (F-P2-FAB-12). - run_pipeline_from_args gains a top-level exception boundary mirroring cli/_training.py: a mid-run _save_state OSError maps to EXIT_TRAINING_ERROR (2) with the 2-key JSON envelope on stdout instead of a raw traceback + interpreter exit 1 colliding with EXIT_CONFIG_ERROR; the stage-filter predecessor merge is now guarded to route config failures to exit 1 cleanly (F-P2-FAB-13). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ash routing (M1 / P2-FAB-16,17,18) - TestGateApplication exercises the safety/judge gate revert vs continue matrix (auto_revert on/off), the staging_path-clearing on revert, the None-gate no-op path, and the F-P3-FABLE-26 trainer-side assertion that an infra-failure SafetyResult yields safe_ratio=0.0 in both the metric and audit payload (F-P2-FAB-16). - TestBaselineLossCapture pins _measure_baseline_loss's gating condition, the disable_adapter happy path + fallback-on-error, the missing-eval_loss no-arm path, and the already-configured / no-auto_revert skips (F-P2-FAB-17). - TestPerStageTrainingErrorRouting drives a stage whose trainer raises and asserts EXIT_TRAINING_ERROR (2), chain halt, downstream skipped_due_to_prior_revert, and the "Trainer crashed:" stage error (F-P2-FAB-18). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-12,14,16,17,18,20) M2 (W2-MED) compliance completeness: - F-P4-OPUS-14: ship `forgelm verify-integrity <model_dir>` — the consuming Article-15 verifier that re-checks a model directory against its `model_integrity.json` SHA-256 manifest, reporting changed / removed / added artifacts (the generate side already shipped; only the verification half was missing). New subcommand wired through the parser, dispatcher, package facade, and EN+TR reference docs. - F-P4-OPUS-16: `compute_annex_iv_manifest_hash` now normalises the payload through a JSON round-trip before canonicalising, so the writer (in-memory dict) and verifier (disk round-trip) hash identical structures even for non-JSON-native content (integer dict keys, sets). Previously such inputs produced a false-tampering verdict. - F-P4-OPUS-17: the Annex IV §1 completeness gate now requires the identity-critical sub-fields (provider_name, system_name, intended_purpose) to be non-empty; the writer skips the artifact (returns None) when they are all blank rather than emitting a §1 stub. - F-P4-OPUS-18: the post-rename `human_approval.granted` audit write in `approve` is wrapped so an OSError surfaces as a clean EXIT_TRAINING_ERROR with an operator-actionable AUDIT-GAP message instead of leaving the model promoted behind an uncaught traceback. - F-P4-OPUS-20: `generate_pipeline_manifest` stamps a `metadata.manifest_hash` over the manifest, and `_verify_manifest_payload` recomputes it so content tampering of stage fields (metrics, gate_decision, output_model) that keeps the chain self-consistent is now detected. Absence downgrades to structural-only verification. - F-P4-OPUS-12: regression tests for the two write-time hardening guarantees that previously had none — multi-writer re-read-under-lock (no chain fork) and the stale-genesis-manifest write-time ERROR path. Every behaviour change ships its regression test in this commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ext (M4 / F-P6-OPUS-02,04,05) _process_messages_format silently emitted an empty training string for an empty messages list ([]) — the exact 'corpus of empty rows' failure its own docstring claims the loud-raise rewrite prevents. Now raises ValueError with the row index. clean_text was documented as stripping 'control characters' but the " ".join(text.split()) body only collapsed whitespace controls; NUL/BEL/ESC and the rest of the C0/C1 Cc set passed verbatim into the tokeniser. clean_string now deletes non-whitespace control characters before collapsing whitespace, honouring the config field's documented contract. Adds a TestProcessMessagesFormat class pinning the loud-raise contract (happy path + empty-list + non-str role/content + non-iterable msg_list + non-dict message) plus control-char strip/preserve cases — closing the zero-regression-test gap on this deliberate behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d (M4 / F-P6-OPUS-03) The function docstring advertised 'Alphabetic-character ratio < 0.45.' but the code compares against _FRONTMATTER_ALPHA_RATIO_MAX (0.30, tightened in the round-3-follow-up review). The docstring now references the constant by name so the active threshold can't drift again; the echoing test-class docstring is updated to 0.30. Adds a docstring-parity regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… / F-P6-OPUS-06) The structural ReDoS validator for operator --strip-pattern regexes rejected the single-group (a+)+ shape but accepted the redundantly-wrapped ((a+))+b / (((a+)))+b variants, which backtrack exponentially (n=24 -> 3.5s). When an inner group closed, the parent's last_atom_unbounded flag was reset to False, so the enclosing + no longer saw an unbounded inner atom. _close_group_or_raise now propagates the closed group's unbounded flag up (outer-quantifier OR inner-unbounded), so an enclosing quantifier still trips the reject through extra grouping parens. Inner bounded tails (((a+)b)+c) still correctly reset. Adds parametrized rejection cases for the wrapped shapes and safe-pattern cases pinning no false positive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t JSON lines (M4 / F-P6-OPUS-07,08)
The audit text extractor recognised only the messages list and a bare prompt
column, so the synthetic generator's instruction ({instruction,output}) and
chatml ({User,Assistant}) shapes extracted to '' (counted null_or_empty, never
scanned for PII/secrets) and prompt_response silently dropped its response half
— the teacher text most likely to carry memorised PII. _extract_text_payload
now folds the (user-half, assistant-half) instruction-tuning pairs via the new
_INSTRUCTION_PAIRS constant, joining both halves before the single-column fast
path so the response/output half is always scanned.
The seed-prompt and file-response loaders parsed each line with json.loads then
called .get() but only caught JSONDecodeError; a valid-JSON-but-non-object line
([1,2,3], 42, ...) crashed the whole --generate-data run with an uncaught
AttributeError. _load_seed_prompts now treats a non-dict line as a plain-text
prompt; _load_file_responses counts it as malformed and skips it, honouring its
'loud about failures' docstring.
Adds round-trip audit tests asserting PII in the response half is detected for
all four synthetic shapes, and loader tests feeding non-object JSON lines.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…19,20,21,22,23,24,25)
- deploy: tag DeployResult.error_kind so a non-directory model_path /
unsupported target (caller-input errors) route to EXIT_CONFIG_ERROR(1),
not EXIT_TRAINING_ERROR(2), matching the public exit-code contract
(F-P7-OPUS-22).
- quickstart --list: wrap the template array in the universal
{success, templates, count} envelope so consumers can branch on the
single success key like every sibling list command (F-P7-OPUS-24).
- quickstart JSON mode: suppress the auto-launched interactive chat REPL
so human prose never interleaves with the JSON envelope on stdout;
record chat_launched=false in the envelope (F-P7-OPUS-25).
- wizard _load_defaults: catch json.JSONDecodeError (a ValueError, NOT an
OSError) so a present-but-corrupt _defaults.json degrades to the
hardcoded fallbacks instead of crashing the wizard at import (F-P7-OPUS-21).
- json-output.md (EN+TR): document the argparse-seam caveat (usage errors
exit non-zero on stderr with no JSON envelope), add the forgelm
quickstart section (list / generation-success / training-failure
shapes), and correct the deploy exit-code mapping (F-P7-OPUS-19,23).
- tests: new test_cli_training_seam.py drives a real TrainResult/exception
through _run_training_pipeline asserting the 0/2/3/4 + ConfigError(1)
mapping (F-P7-OPUS-20); regression tests for each behaviour change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PR#63-review)
Two GRPO-pipeline bugs sharing test_grpo_math_reward.py:
* grpo_rewards.ANSWER_EXTRACT_PATTERN: the greedy body absorbed a later
same-line 'Answer:' token ("Answer: 5 Answer: 7" captured "5 Answer: 7"),
so the LAST finditer match still graded the discarded candidate — reopening
the reward-hacking divergence the last-marker rule closes. Guard every body
run with (?!answer\s*:) so it refuses to cross into a new marker; markers now
split into separate matches and the final one is graded. Fixed-prefix
lookahead keeps scanning linear (the rejected (?![\s\S]*answer:) alternative
rescanned the tail per marker → O(n^2), ~3s at n=5000). Also drop a stray
Turkish word from the pattern comment (English-only policy).
* trainer._dataset_has_gold_answers: the column_names fallback used
next(iter(train)), consuming the first row of a self-iterating / one-shot
dataset (iter(train) is train) and silently altering the training stream.
Detect that shape and trust presence-by-name instead of advancing the
iterator; fresh-iterator datasets (HF Dataset / list) keep probing as before.
Regression tests: same-line multi-answer grading, many-marker ReDoS linearity,
and a one-shot iterator that must survive the probe intact.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… SNI (PR#63-review) _PortStrippingAdapter.send delegates straight to HTTPAdapter.send to bypass the parent's port-bearing assert_hostname derivation — but that also skips the parent's SNI setup, so urllib3 would SNI the IP literal / port-bearing host and fail TLS verification on endpoints like https://host:8443. Set server_hostname to the bare (port-stripped) host alongside assert_hostname, and clear both when no Host header is present so a stale value can't leak onto a reused session. Extends the port-strip test to assert server_hostname and adds a no-Host-header cleanup test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PR#63-review) A malformed --type custom regex interpolated the raw re.error into the error envelope (a redirectable durable record). re.error.msg / str(exc) reproduce pattern-derived fragments — e.g. an invalid group name "(?P<alice@example.com>x)" yields "bad character in group name 'alice@example.com'" — leaking the subject's identifier the subcommand exists to keep out of durable records (F-P5-OPUS-17). Emit a generic message with only the numeric offending position. Regression test covers the group-name leak shape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ed line (PR#63-review)
A bare quoted-string seed line ("prompt text") fell into the else branch and
appended the raw line — keeping the JSON quotes — instead of the parsed string.
Add an isinstance(data, str) branch that appends the parsed value, matching
safety._load_safety_prompts. Regression test pins that the JSON quotes are gone.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…3-review) CodeRabbit flagged several user-facing contract contradictions introduced or missed by the doc sweep. Resolved against the canonical mappings: - configuration.md (EN+TR usermanual): the "top-level config has N blocks" list contradicted the `deployment:` section that says the key is rejected by `ForgeConfig` (extra="forbid"). Dropped `deployment:` from the list and corrected the count 13 -> 12. - verify-audit.md (EN+TR): the strict-mode preflight + mermaid line said exit 2 for a missing HMAC secret, while the summary table / CI tip said exit 1. `forgelm verify-audit` only ever returns 0 or 1 (forgelm/cli/subcommands/_verify_audit.py), so normalised every reference to "1 = operator-actionable failure". - cli.md (EN+TR): exit-code-2 row cited `audit warnings (with --strict)`, but `--strict` is not in the audit parser (the same page already says so). Removed the stale fragment, leaving the legitimate doctor probe-crash case. - exit-codes.md (EN+TR): the "When to use each exit code" scenario rows for gate regressions read as unconditional exit 3; made them conditional on `evaluation.auto_revert` to match the main table + "exit 0 guarantees". - json-output.md (EN+TR): safety-eval exit-2 wording said the --probes/--default-probes pair was "conflicting" yet "both required"; the parser uses a required mutually-exclusive group, so reworded to "exactly one is required; neither or both -> argparse exit 2". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eview) Outside-diff and nitpick findings on the docs touched by PR #63: - configuration.md (EN+TR reference): escape the `<\|im_start\|>` pipes in the chatml table cell (MD056) and quote the blank line between the two evaluation blockquotes (MD028). - webhooks.md (EN+TR): the WebhookConfig gate table omitted the new pipeline.* events; aligned notify_on_start/success/failure rows with the canonical event table on the same page. - gguf-export.md (EN+TR) + troubleshooting.md/-tr.md: surround fenced code blocks with blank lines (MD031). - audit-log.md (TR usermanual): point the absolute GitHub catalog links at the Turkish mirror audit_event_catalog-tr.md. - audit_event_catalog-tr.md: fix mismatched link label (text said the EN catalog, target was the TR mirror). - ingestion-tr.md: the Phase 15 archive link pointed at the un-suffixed EN sibling; no TR mirror exists, so converted it to an absolute HTTPS link. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sh (PR#63-review) The "Verify --ignore target exists" step hard-failed on the existence of tests/test_cost_estimation.py and referenced an --ignore flag that no longer exists. Drift-sensitive tests are now excluded marker-driven via `pytest -m 'not fixture_drift'` (F-P8-C-20), which the very next step's comment already declares the single source of truth. The filename guard would cause false release failures after a harmless test-file rename. Removed it; the marker on tests/test_cost_estimation.py (registered in pyproject.toml) keeps the exclusion functional. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ort (PR#63-review) The guard's bare `import tomllib` is stdlib only on 3.11+, so the 3.10 CI-matrix runner failed at fixture-import time with ModuleNotFoundError, blocking the whole test module. Fall back to the tomli backport with an explicit ImportError + install hint (no silent fallback), add tomli to the dev extra scoped to python_version < 3.11, and pin a regression test that the bound tomllib exposes a working loads() on every supported runtime. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#63-review) - test_chat.py: replace try/except ImportError rich probe with an explicit importlib.util.find_spec check (coding.md: no silent import fallback). - test_cache_subcommands.py: assert the --help subprocess exited 0 before validating its help text, so a broken invocation can't pass on stale output. - test_usermanual_reference_consistency.py: assert argparse usage errors are coupled with exit code 2, not the bare 'argparse' token. - test_config_doc_parity.py: qualify the benchmark output_dir field to its 'evaluation.benchmark' heading context so an unrelated output_dir mention cannot satisfy the gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… check The PR#63-review commit 22a3924 replaced the try/except rich.markup probe with `find_spec("rich.markup")`. But find_spec on a *submodule* imports the parent package to locate it, so when the optional `rich` extra is absent it raises `ModuleNotFoundError: No module named 'rich'` instead of returning None — breaking test collection on every no-extras CI job (3.10–3.13). Probe the top-level `rich` package instead: find_spec("rich") returns None when absent (no parent to import) and never raises. `rich.markup` ships with `rich`, so the top-level probe is equivalent for the True-path assertion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rrowing TestCallApiJudgeNarrowing patched forgelm._http.requests.post, but safe_post's default SSRF-pinned path issues the request via _pinned_session().post and only falls back to requests.post on the allow_private legacy path. So the patch leaked through to a real DNS + HTTPS call to the default judge endpoint, which returned a real HTTP 401 and raised JudgeAuthError — failing all three narrowing tests on every CI matrix job once the runner had outbound network reachability (they passed before only because the runner could not reach the endpoint, so the call was narrowed to a connection error by accident). Patch the safe_post chokepoint instead: hermetic (no DNS, no socket) and the correct seam — it exercises exactly _call_api_judge's response / exception narrowing while implicitly asserting the judge routes outbound HTTP through the chokepoint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
docs/reference/audit_event_catalog.md (1)
150-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign the
pipeline.completedwebhook payload contract in both mirrors.These sections still disagree on the payload shape: the table says
pipeline.completedincludesfinal_statusandstopped_at, while the schema block below only declaresstatus,metrics,reason, andmodel_path, and elsewhere usesstopped_at_stage. Pick one field set and mirror it in both docs.
docs/reference/audit_event_catalog.md#L150-L175: make the row and schema use the same keys; renamestopped_atifstopped_at_stageis the real field.docs/reference/audit_event_catalog-tr.md#L150-L175: mirror the same key set in the Turkish copy.🤖 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/reference/audit_event_catalog.md` around lines 150 - 175, The `pipeline.completed` event documentation has conflicting payload field definitions between the table and the JSON schema block in docs/reference/audit_event_catalog.md: the table lists `final_status` and `stopped_at` while the schema only includes `status`, `metrics`, `reason`, and `model_path`, and elsewhere references `stopped_at_stage`. Determine the actual payload contract for `pipeline.completed` (verify whether the real field is `stopped_at` or `stopped_at_stage`, and whether `final_status` exists), then update both the table row at lines 150-175 and the JSON schema block below it to declare the same field set consistently. Apply the identical field set changes to the corresponding sections in docs/reference/audit_event_catalog-tr.md at lines 150-175 to keep the Turkish documentation in sync.docs/standards/logging-observability.md (1)
143-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSplit the webhook contract or make the aliasing explicit.
The new table is no longer one-to-one:
training.successandpipeline.completedboth referencepipeline.completed, but the intro still says each webhook mirrors a distinct audit event. The generic payload schema also omitsstage_count,final_status,stopped_at, andstage_name, so consumers can't validate the pipeline events from this page alone. Please either separate the pipeline family into its own schema section or update the wording to reflect the shared audit identifier and extra fields.🤖 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/standards/logging-observability.md` around lines 143 - 173, The documentation currently claims each webhook event mirrors a distinct audit event, but both `training.success` and `pipeline.completed` reference the same audit event `pipeline.completed`, violating that contract. Additionally, the generic "Payload schema (every event)" omits pipeline-specific fields like `stage_count`, `final_status`, `stopped_at`, and `stage_name`, making the schema incomplete for pipeline events. Fix this by either: (1) creating a separate "Pipeline Events Payload Schema" section that documents the pipeline-specific fields in addition to the common fields, or (2) updating the introductory text to explicitly acknowledge the shared audit identifier between `training.success` and `pipeline.completed`, then expand the generic payload schema to document all possible fields and clarify which fields apply to which event types. Choose the approach that best maintains clarity for webhook consumers.docs/usermanuals/en/compliance/verify-audit.md (1)
43-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign the example log path with the documented on-disk contract.
Examples here still use
.../compliance/audit_log.jsonl, but the audit-log manual documents<output_dir>/audit_log.jsonl(top-level). This mismatch can produce copy-paste file-not-found failures. Please update command and manifest-path examples to the same top-level location.Also applies to: 95-96
🤖 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/compliance/verify-audit.md` around lines 43 - 65, Update all file path references in the verify-audit command examples to use the correct top-level location instead of the nested path. Replace all instances of checkpoints/run/compliance/audit_log.jsonl with the documented contract location of <output_dir>/audit_log.jsonl (or a more concrete example like audit_log.jsonl if that's the preferred format). This includes the three command examples shown in the diff section (the basic verify-audit example, the HMAC-authenticated example, and the strict mode example with --require-hmac flag), and also fix the additional instances referenced at lines 95-96 to maintain consistency across all documented examples.docs/usermanuals/tr/compliance/verify-audit.md (1)
43-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign example audit-log paths with the documented on-disk location.
The quick-start/strict-mode examples still use
.../compliance/audit_log.jsonl, but the audit-log contract in this PR documents<output_dir>/audit_log.jsonlat top level. This inconsistency will cause copy-paste command failures.Suggested doc fix
-$ forgelm verify-audit checkpoints/run/compliance/audit_log.jsonl +$ forgelm verify-audit checkpoints/run/audit_log.jsonl ... -$ FORGELM_AUDIT_SECRET="$(cat /run/secrets/audit-secret)" \ - forgelm verify-audit checkpoints/run/compliance/audit_log.jsonl +$ FORGELM_AUDIT_SECRET="$(cat /run/secrets/audit-secret)" \ + forgelm verify-audit checkpoints/run/audit_log.jsonl ... -$ FORGELM_AUDIT_SECRET="$(cat /run/secrets/audit-secret)" \ - forgelm verify-audit --require-hmac \ - checkpoints/run/compliance/audit_log.jsonl +$ FORGELM_AUDIT_SECRET="$(cat /run/secrets/audit-secret)" \ + forgelm verify-audit --require-hmac \ + checkpoints/run/audit_log.jsonl ... -FAIL: manifest present but unreadable at 'checkpoints/run/compliance/audit_log.jsonl.manifest.json': … +FAIL: manifest present but unreadable at 'checkpoints/run/audit_log.jsonl.manifest.json': …Also applies to: 95-96
🤖 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/tr/compliance/verify-audit.md` around lines 43 - 65, The example commands for forgelm verify-audit use paths like checkpoints/run/compliance/audit_log.jsonl, but this conflicts with the documented on-disk location which specifies the audit log file should be at the top level of the output directory as <output_dir>/audit_log.jsonl. Update all instances of the audit-log path in the example commands (in the quick-start section and the strict-mode section with --require-hmac flag) to use the correct top-level path pattern that matches the documented audit-log contract, ensuring copy-paste commands will work correctly with the actual file locations.forgelm/merging.py (1)
295-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign per-key adapter weights with the filtered delta list.
At Line 296,
deltasis filtered per key, but Line 301/Line 303 still pass the fullweightsvector. If any adapter is missing that key, zip-based weighting in the tensor helpers can pair deltas with the wrong adapter weights (or drop later weights), producing biased merges.Suggested fix
- for key in task_vectors[0]: - deltas = [tv[key].float() for tv in task_vectors if key in tv] + for key in task_vectors[0]: + indexed = [(idx, tv[key].float()) for idx, tv in enumerate(task_vectors) if key in tv] + deltas = [delta for _, delta in indexed] if not deltas: continue + key_weights = [weights[idx] for idx, _ in indexed] + key_weight_sum = sum(key_weights) + if key_weight_sum == 0: + raise ValueError(f"Adapter weights for parameter '{key}' sum to 0.") + key_weights = [w / key_weight_sum for w in key_weights] if method == "ties": - merged_delta[key] = _ties_merge_tensor(deltas, weights, trim_fraction=ties_trim_fraction) + merged_delta[key] = _ties_merge_tensor(deltas, key_weights, trim_fraction=ties_trim_fraction) elif method == "dare": - merged_delta[key] = _dare_merge_tensor(deltas, weights, drop_rate=dare_drop_rate, seed=dare_seed) + merged_delta[key] = _dare_merge_tensor(deltas, key_weights, drop_rate=dare_drop_rate, seed=dare_seed) else: - merged_delta[key] = sum(d * w for d, w in zip(deltas, weights)) + merged_delta[key] = sum(d * w for d, w in zip(deltas, key_weights))🤖 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/merging.py` around lines 295 - 303, The deltas list is filtered at line 296 to only include tensors from task_vectors that contain the key, but the full weights vector is still passed to _ties_merge_tensor and _dare_merge_tensor functions at lines 301 and 303. This creates a mismatch when some adapters lack a particular key, causing the filtered deltas to be paired with incorrect or misaligned weights. Filter the weights list to match the indices of task_vectors that actually contain the key, and pass this filtered weights list to both merge function calls instead of the full weights vector.docs/usermanuals/tr/reference/json-output.md (1)
147-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
run_id/config_hashconsistent across all training-result envelopes.The table says these fields are always present, but the awaiting-approval and reverted examples omit them. Either add them to those envelopes or narrow the table to the promote-success case; otherwise consumers get conflicting schema guidance.
🤖 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/tr/reference/json-output.md` around lines 147 - 181, The awaiting-approval and reverted envelope examples in the JSON output documentation are missing the run_id and config_hash fields, but the table states these fields are always present when the trainer produces output. Add run_id and config_hash fields to both the "Onay-bekliyor envelope'u" (awaiting approval) example and the "Revert envelope'u" (reverted) example with appropriate values to match the schema described in the table, ensuring consistency across all training-result envelope examples shown.notebooks/post_training_workflow.ipynb (1)
39-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign the downloaded config tag with the pinned ForgeLM version.
Line 39 installs
forgelm==0.7.0, but Lines 40-43 still fetchconfig_template.yamlfromv0.5.7. This can cause schema/field drift and break the subsequentforgelm --config ...steps.Suggested fix
- "# Download config_template.yaml from the v0.5.7 tag (NOT main) so the YAML\n", + "# Download config_template.yaml from the v0.7.0 tag (NOT main) so the YAML\n", @@ - "!wget -q https://raw.githubusercontent.com/HodeTech/ForgeLM/v0.5.7/config_template.yaml\n", + "!wget -q https://raw.githubusercontent.com/HodeTech/ForgeLM/v0.7.0/config_template.yaml\n",🤖 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 `@notebooks/post_training_workflow.ipynb` around lines 39 - 43, The pip install command on line 39 installs forgelm version 0.7.0, but the wget command that downloads config_template.yaml is fetching from the v0.5.7 tag, creating a version mismatch that can cause schema/field drift issues. Update the wget URL to fetch the config_template.yaml from the v0.7.0 tag instead of v0.5.7 to align with the pinned forgelm version being installed, ensuring the downloaded configuration is compatible with the installed wheel.
♻️ Duplicate comments (1)
forgelm/compliance.py (1)
294-301:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove audit re-root bypass to validated config, not env var.
Line 294 still uses
FORGELM_ALLOW_AUDIT_REROOTas a behavior switch for a tamper-sensitive integrity gate, which keeps this path outside validated config/audit contracts.As per coding guidelines, “Behaviour is determined by validated YAML config; do not sniff env-vars for behaviour (env-vars for secrets only).” Based on learnings, this same repo rule is explicitly reiterated.
Suggested direction
-class AuditLogger: - def __init__(self, output_dir: str, run_id: Optional[str] = None): +class AuditLogger: + def __init__( + self, + output_dir: str, + run_id: Optional[str] = None, + *, + allow_audit_reroot: bool = False, + ): ... + self._allow_audit_reroot = allow_audit_reroot ... - if os.getenv("FORGELM_ALLOW_AUDIT_REROOT") != "1": + if not self._allow_audit_reroot: raise ConfigError(...)🤖 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/compliance.py` around lines 294 - 301, The audit re-root bypass check at line 294 in compliance.py currently uses the environment variable FORGELM_ALLOW_AUDIT_REROOT to control behavior, but per coding guidelines, behavior must be determined by validated YAML config, not environment variables. Replace the os.getenv("FORGELM_ALLOW_AUDIT_REROOT") check with a corresponding validated configuration parameter from the config object, ensuring the audit re-root bypass is controlled through the config layer rather than environment variables (which should be reserved for secrets only).Sources: Coding guidelines, Learnings
🟡 Minor comments (17)
docs/guides/ingestion-tr.md-493-493 (1)
493-493:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid pinning this TR guide to
mainwith an absolute GitHub URL.Line 493 links to
.../blob/main/..., which can drift from versioned docs and diverges from the EN mirror’s relative-link style. Use a relative repo link here too.Suggested fix
-> [Faz 15 arşivi](https://github.com/HodeTech/ForgeLM/blob/main/docs/roadmap/completed-phases.md#phase-15--ingestion-pipeline-reliability-v060) +> [Faz 15 arşivi](../roadmap/completed-phases.md#phase-15--ingestion-pipeline-reliability-v060)As per coding guidelines, bilingual documentation mirrors should stay synchronized and stable across EN↔TR docs.
🤖 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/guides/ingestion-tr.md` at line 493, The link on line 493 in docs/guides/ingestion-tr.md uses an absolute GitHub URL with blob/main that can drift from versioned documentation. Replace this absolute URL with a relative repository link to match the style used in the English mirror guide, ensuring the Turkish and English documentation mirrors remain synchronized and stable.Source: Coding guidelines
forgelm/model_card.py-252-260 (1)
252-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse context-specific YAML serialization for front-matter instead of reusing markdown-sanitized text.
base_modelis still emitted as a plain YAML scalar in front-matter. Quote-led or YAML-significant inputs can still produce malformed metadata even after markdown-char stripping.💡 Proposed fix
MODEL_CARD_TEMPLATE = """--- language: - en tags: - forgelm - fine-tuned - lora {extra_tags} -base_model: {base_model} +base_model: {base_model_yaml} --- @@ base_model = _neutralize_md_inline(config.model.name_or_path) @@ + # YAML front-matter needs YAML-safe scalar serialization (separate from + # markdown inline sanitization). + base_model_yaml = yaml.safe_dump(base_model, default_flow_style=True).strip() + card = MODEL_CARD_TEMPLATE.format( model_name=model_name, base_model=base_model, + base_model_yaml=base_model_yaml, backend=config.model.backend,Also applies to: 262-264, 275-275, 281-281
🤖 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/model_card.py` around lines 252 - 260, The variables base_model, dataset, model_path, and model_name are sanitized using _neutralize_md_inline() which only handles markdown characters, but they are subsequently used in YAML front-matter where YAML-specific syntax (quotes, colons, etc.) can still cause injection vulnerabilities or malformed metadata. Replace the _neutralize_md_inline() sanitization with a context-specific YAML serialization function that properly escapes or quotes values according to YAML specification. Apply this fix to all locations where these variables are assigned and used in front-matter generation, including the variable assignments at lines 252-260 and also at lines 262-264, 275, and 281 where similar YAML front-matter values are set.docs/reference/configuration-tr.md-102-102 (1)
102-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the canonical
packingrow to the Turkish mirror.The alias note now points at
packing, but this table never defines that field, so readers can't discover the canonical setting. Add thepackingrow above (or trim the alias note) to keep the EN/TR docs in sync. As per coding guidelines, bilingual documentation (EN ↔ TR) must be kept in sync.🤖 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/reference/configuration-tr.md` at line 102, The Turkish documentation file references a canonical `packing` field as the replacement for the deprecated `sample_packing` alias, but the `packing` row is missing from the configuration table in that file. Add the `packing` configuration row to the Turkish table (positioned before or near the `sample_packing` row) with the same content and structure as exists in the English version documentation to maintain synchronization between the EN and TR bilingual documentation. This ensures readers can discover and understand the canonical `packing` setting that `sample_packing` points to.Source: Coding guidelines
tests/test_pytest_markers.py-39-42 (1)
39-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDo not silently skip unreadable test files when collecting applied markers.
Swallowing
OSErrorhere can hide real repository/readability problems and weaken this guard’s signal. Fail loudly with the file path/context instead of continuing.As per coding guidelines, “No silent failures: avoid ... logging-and-swallowing for non-fatal paths.”
Proposed fix
- except OSError: - continue + except OSError as exc: + pytest.fail(f"failed to read marker source file '{path}': {exc}")🤖 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_pytest_markers.py` around lines 39 - 42, The try-except block around path.read_text(encoding="utf-8") silently catches OSError and continues, which masks real file access problems. Instead of using continue in the except OSError clause, raise an error that includes the file path and the original exception details to fail loudly and provide proper context for debugging. This ensures that repository or readability issues are not hidden from the guard's signal.Source: Coding guidelines
tests/test_pytest_markers.py-59-61 (1)
59-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winParse pytest
addoptsfrom TOML instead of substring-matching the full file text.The current assertion can false-pass if
"--strict-markers"appears outsideaddopts(e.g., comments or docs). Readtool.pytest.ini_options.addoptsand assert token presence there.Proposed fix
def test_strict_markers_enabled(): - addopts = (REPO_ROOT / "pyproject.toml").read_text(encoding="utf-8") - assert "--strict-markers" in addopts, ( + data = tomllib.loads((REPO_ROOT / "pyproject.toml").read_text(encoding="utf-8")) + addopts = data["tool"]["pytest"]["ini_options"].get("addopts", "") + assert "--strict-markers" in addopts, ( "--strict-markers must stay in addopts so a typo'd `@pytest.mark` is an error, not a silent no-op." )🤖 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_pytest_markers.py` around lines 59 - 61, The current implementation reads the entire pyproject.toml file as text and performs a substring search for "--strict-markers", which can produce false positives if the string appears in comments or other sections. Replace the text-based read with proper TOML parsing: parse the pyproject.toml file using a TOML parser, extract the addopts value from tool.pytest.ini_options.addopts, and then assert that "--strict-markers" is present in that parsed addopts configuration instead of searching the raw file text.tests/test_guard_wiring.py-47-47 (1)
47-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
.yamlworkflows in the guard scan.Line 47 only scans
*.yml. A guard wired in a.yamlworkflow would be treated as unwired and can trigger false CI failures. Scan both extensions in_workflow_text().Proposed fix
def _workflow_text() -> str: - return "\n".join(p.read_text(encoding="utf-8") for p in sorted(_WORKFLOWS.glob("*.yml"))) + files = sorted(_WORKFLOWS.glob("*.yml")) + sorted(_WORKFLOWS.glob("*.yaml")) + return "\n".join(p.read_text(encoding="utf-8") for p in files)As per coding guidelines, workflow policy explicitly covers both
.ymland.yamlfiles.🤖 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_guard_wiring.py` at line 47, The _workflow_text() function on line 47 only scans workflow files with the .yml extension using _WORKFLOWS.glob("*.yml"), but it should also include files with the .yaml extension to prevent false CI failures. Modify the glob pattern to match both .yml and .yaml files by using two separate glob calls (one for "*.yml" and one for "*.yaml") and combine their results, or use a single glob pattern that matches both extensions, ensuring the sorted results are properly merged.Source: Coding guidelines
tests/test_phantom_tool_citations.py-41-48 (1)
41-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard scope excludes one declared production site.
Line 74 only scans
docs/**/*.mdandtools/*.py, but Line 41–48 explicitly includes.github/workflows/ci.ymlin the protected scope. That leaves a regression hole for the exact citation this test claims to guard.Suggested patch
- for path in (*_REPO_ROOT.glob("docs/**/*.md"), *_REPO_ROOT.glob("tools/*.py")): + for path in ( + *_REPO_ROOT.glob("docs/**/*.md"), + *_REPO_ROOT.glob("tools/*.py"), + _REPO_ROOT / ".github" / "workflows" / "ci.yml", + ): + if not path.is_file(): + continue if _is_under_gitignored_doc_dir(path): continueAlso applies to: 74-86
🤖 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_phantom_tool_citations.py` around lines 41 - 48, The _PRODUCTION_SITES tuple at lines 41-48 correctly declares .github/workflows/ci.yml as a protected production site, but the scan logic at lines 74-86 only covers docs/**/*.md and tools/*.py patterns, creating a gap where the ci.yml file is not actually guarded. Update the scan pattern at lines 74-86 to also include .github/workflows/*.yml or a similar pattern that will match .github/workflows/ci.yml, ensuring the scan scope matches all declared production sites in _PRODUCTION_SITES.docs/roadmap/completed-phases.md-921-927 (1)
921-927:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the blank line inside the blockquote (MD028).
Line 927 introduces a blank quoted line, which triggers markdownlint
MD028(“no-blanks-blockquote”). Keep the blockquote contiguous to avoid docs-lint noise/failures.🤖 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/roadmap/completed-phases.md` around lines 921 - 927, A blank line within the blockquote on line 927 in the completed-phases.md file violates markdownlint rule MD028 (no-blanks-blockquote). Remove the empty quoted line to make the blockquote contiguous, ensuring all lines starting with > are consecutive without any blank lines in between.Source: Linters/SAST tools
tests/test_config_doc_parity.py-43-47 (1)
43-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe benchmark
output_dirguard can false-pass.This assertion only requires
`evaluation.benchmark`and`output_dir`anywhere in the document, so it can pass even ifoutput_diris documented in another section. Scope the check to the benchmark section block to avoid silent 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 `@tests/test_config_doc_parity.py` around lines 43 - 47, The assertion checking for the field "evaluation.benchmark.output_dir" currently searches for both the "`evaluation.benchmark`" and "`output_dir`" strings anywhere in the entire text variable, which can cause false positives if `output_dir` exists in an unrelated section. Refactor the assertion to first locate the benchmark section block (the text region between the "`evaluation.benchmark`" heading and the next section), then verify that "`output_dir`" exists specifically within that scoped section rather than in the full document text.docs/standards/testing.md-57-60 (1)
57-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep fixture ownership wording consistent with the layout tree.
This section correctly says the canonical
minimal_configfactory lives intests/_helpers/factories.py, but the tree still labelstests/conftest.pyas the factory location. Please update the tree note so the guidance is not contradictory.🤖 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/standards/testing.md` around lines 57 - 60, The testing.md documentation contains contradictory information: the narrative text correctly states that the canonical minimal_config factory lives in tests/_helpers/factories.py and is re-exported via tests/conftest.py, but the directory tree diagram in this section still incorrectly labels tests/conftest.py as the factory location. Update the tree structure note to accurately reflect that tests/_helpers/factories.py is the canonical factory location, with tests/conftest.py being the re-export point, so the documentation guidance is consistent throughout.docs/usermanuals/en/evaluation/judge.md-63-63 (1)
63-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the page’s failure semantics in one direction.
This paragraph correctly documents the
auto_revertsplit, but the earlier intro still says the run fails belowmin_scoreunconditionally. Please reconcile that wording so the page exposes one contract.As per coding guidelines, exit codes are a public contract and should be documented consistently.
🤖 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/evaluation/judge.md` at line 63, The documentation has inconsistent descriptions of failure semantics when mean_score falls below min_score. The section at line 63 correctly documents that the outcome depends on the auto_revert parameter (either reverting with exit code 3, or promoting with exit code 0), but an earlier intro section in the same file states unconditionally that the run fails below min_score. Locate the earlier intro section and update its wording to accurately reflect that the actual behavior is conditional on the auto_revert setting. Ensure both sections consistently document the public contract for exit codes and whether the model gets promoted or reverted, so readers understand that a judge gate failure does not automatically result in model reversion or a non-zero exit code without auto_revert: true.Source: Coding guidelines
forgelm/webhook.py-122-127 (1)
122-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the HTTP warning accurate when
require_https=true.Line 126 can refuse
http://delivery, but_sendstill logs “Data will be sent unencrypted.” in that path. This message is incorrect when delivery is actually blocked.Suggested fix
- if url.startswith("http://"): # NOSONAR python:S5332 - logger.warning("Webhook URL uses HTTP (not HTTPS). Data will be sent unencrypted.") + if url.startswith("http://"): # NOSONAR python:S5332 + if bool(getattr(self.config, "require_https", False)): + logger.warning( + "Webhook URL uses HTTP (not HTTPS) and require_https=true; delivery will be refused." + ) + else: + logger.warning("Webhook URL uses HTTP (not HTTPS). Data will be sent unencrypted.")As per coding guidelines, logging must follow
docs/standards/logging-observability.mdand remain operationally accurate.🤖 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/webhook.py` around lines 122 - 127, The `_send` method logs "Data will be sent unencrypted." regardless of whether HTTP delivery is actually allowed. When `require_https=true`, the allow_insecure_http parameter becomes False (line 126), which blocks HTTP delivery, but the warning in `_send` should not be logged in this case since data is not actually being sent unencrypted. Update the logging logic in `_send` to only warn about unencrypted transmission when allow_insecure_http is True and the destination URL is actually using the http:// scheme, ensuring the warning is operationally accurate and only appears when data will genuinely be sent unencrypted.Source: Coding guidelines
docs/usermanuals/tr/operations/webhooks.md-37-38 (1)
37-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the Turkish audit-event mirror here.
Both references still send Turkish readers to the English
audit_event_catalog.md. Please switch them to the Turkish mirror so this page stays localized end-to-end. As per coding guidelines, Turkish docs should not link to English siblings when a Turkish version exists.Also applies to: 67-68
🤖 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/tr/operations/webhooks.md` around lines 37 - 38, Update the GitHub links in the Turkish webhook documentation to reference the Turkish mirror of the audit event catalog instead of the English version. In the file docs/usermanuals/tr/operations/webhooks.md, replace the link at lines 37-38 that currently points to the English audit_event_catalog.md with a reference to the Turkish audit catalog mirror. Apply the same fix at lines 67-68 where the same English audit catalog link appears. This ensures the Turkish documentation stays fully localized and follows the guideline that Turkish docs should not link to English siblings when a translated version exists.Source: Coding guidelines
notebooks/safety_evaluation.ipynb-25-26 (1)
25-26:⚠️ Potential issue | 🟡 MinorKeep notebook version pins consistent across install and fallback download paths.
Lines 25-26 pin ForgeLM to
0.7.0, but the notebook fetches prompts fromv0.5.7. This mismatch can silently load incompatible assets in a single run.🤖 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 `@notebooks/safety_evaluation.ipynb` around lines 25 - 26, The pip install command pins ForgeLM to version 0.7.0, but the notebook contains a fallback download path that fetches prompts from v0.5.7, creating a version mismatch that can load incompatible assets. Locate where the notebook references or fetches assets from v0.5.7 (likely in a URL or download path for prompts) and update that version reference to 0.7.0 to match the pinned installation version, ensuring all asset sources within the notebook use the same ForgeLM version.tests/test_pypdf_normalise.py-65-79 (1)
65-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the docstring assertion semantic instead of markdown-fragment specific.
Line 78 checks the literal fragment
"not** 1:1", which is brittle to harmless emphasis/formatting edits. Prefer asserting intent-level tokens so the test protects meaning, not exact markdown punctuation.Suggested test hardening
- assert "not** 1:1" in doc + assert "1:1" in doc + assert "lossy" in doc.lower() + assert "not" in doc.lower()🤖 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_pypdf_normalise.py` around lines 65 - 79, The assertion on line 78 checks for the literal string fragment "not** 1:1" which includes markdown formatting characters (the double asterisks). This test is brittle because it depends on specific markdown punctuation rather than semantic meaning. Instead of asserting the exact markdown fragment "not** 1:1", modify the test_docstring_acknowledges_multichar_rule_is_not_1to1 method to check for the semantic intent — that the docstring contains tokens indicating the transformation is NOT purely 1:1 — without requiring specific markdown formatting. This could be done by checking for separate meaningful phrases like "not" and "1:1" or other variations that verify the core meaning while remaining resilient to harmless formatting changes.notebooks/data_curation.ipynb-25-30 (1)
25-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the explanatory version note with the pinned package version.
Line 26 still references a
v0.5.5closure-cycle bundle while the install command now pinsforgelm[ingestion]==0.7.0. Updating that sentence to match the new pin will avoid reader confusion.Suggested patch
- "# Pinned to v0.7.0 (the v0.5.5 closure-cycle bundle ships Phases 11+11.5+12+12.5\n", + "# Pinned to v0.7.0 (this release bundle ships Phases 11+11.5+12+12.5\n",🤖 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 `@notebooks/data_curation.ipynb` around lines 25 - 30, The explanatory comment references v0.5.5 but the pip install command pins forgelm[ingestion]==0.7.0. Update the comment text (specifically the sentence mentioning "the v0.5.5 closure-cycle bundle") to align with the actual pinned version v0.7.0 so the documentation accurately reflects what version and features are being installed.forgelm/cli/subcommands/_purge.py-122-124 (1)
122-124:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce the configured max length after adding the truncation suffix.
At Line 124, the return value can exceed
_AUDIT_ERROR_MESSAGE_MAXbecause the suffix is appended after slicing to max length.💡 Suggested fix
- if len(masked) <= _AUDIT_ERROR_MESSAGE_MAX: - return masked - return masked[:_AUDIT_ERROR_MESSAGE_MAX] + "…[truncated]" + if len(masked) <= _AUDIT_ERROR_MESSAGE_MAX: + return masked + suffix = "…[truncated]" + head = _AUDIT_ERROR_MESSAGE_MAX - len(suffix) + if head <= 0: + return suffix[:_AUDIT_ERROR_MESSAGE_MAX] + return masked[:head] + suffix🤖 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 122 - 124, The truncation logic at line 124 appends the suffix "…[truncated]" after slicing to _AUDIT_ERROR_MESSAGE_MAX, which causes the final return value to exceed the configured maximum length. Fix this by accounting for the suffix length when slicing the masked string. Calculate the appropriate slice endpoint by subtracting the length of the truncation suffix from _AUDIT_ERROR_MESSAGE_MAX, then slice the masked string to that adjusted length before appending the suffix to ensure the total length never exceeds the configured maximum.
🧹 Nitpick comments (2)
forgelm/__init__.py (1)
302-305: ⚡ Quick winPreserve the documented tier order in
__dir__.
sorted(set(__all__))dedupes, but it also destroys the Stable/Experimental ordering this module saysdir(forgelm)should preserve. A stable-order dedupe (dict.fromkeys(__all__)) would keep the advertised grouping intact.🤖 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/__init__.py` around lines 302 - 305, The return statement in the `__dir__` function uses `sorted(set(__all__))` which sorts items alphabetically and destroys the documented Stable/Experimental tier ordering. Replace this with `dict.fromkeys(__all__)` to preserve the original insertion order while still deduplicating items, maintaining the advertised tier grouping that users expect when calling `dir(forgelm)`.tests/test_human_approval_gate.py (1)
147-164: 🏗️ Heavy lift
test_production_staging_path_carries_run_id_suffixis currently self-referential.This test recomputes the expected string locally instead of asserting a value produced by the runtime path-construction flow, so it can pass even if production staging-path construction drifts. Please assert against the actual path emitted by the trainer execution path that builds staging names.
Based on learnings, new capability coverage should verify real behavior paths, not mirrored formulas.
🤖 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_human_approval_gate.py` around lines 147 - 164, The test test_production_staging_path_carries_run_id_suffix currently recomputes the gate path locally using the same formula (f"{final_path}.staging.{trainer.audit.run_id}") that production code would use, making it self-referential and unable to catch if production path construction changes. Instead of computing gate_path locally, obtain the actual staging path that the trainer object produces during execution (likely from the trainer's state, audit object, or output directory artifacts), and assert against that real path value to ensure the test catches any drift in actual production path construction behavior.Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07e95c9f-9e83-4eba-a697-4d133ff3c945
📒 Files selected for processing (261)
.agents/skills/cut-release/SKILL.md.claude/skills/cut-release/SKILL.md.github/workflows/ci.yml.github/workflows/publish.ymlAGENTS.mdCHANGELOG.mdCLAUDE.mdconfig_template.yamldocs/design/blackwell_optimized.mddocs/design/iso27001_soc2_alignment.mddocs/design/library_api.mddocs/guides/air_gap_deployment-tr.mddocs/guides/alignment-tr.mddocs/guides/alignment.mddocs/guides/gdpr_erasure-tr.mddocs/guides/getting-started-tr.mddocs/guides/human_approval_gate-tr.mddocs/guides/ingestion-tr.mddocs/guides/ingestion.mddocs/guides/library_api-tr.mddocs/guides/library_api.mddocs/guides/performance-tr.mddocs/guides/pipeline-tr.mddocs/guides/pipeline.mddocs/guides/troubleshooting-tr.mddocs/guides/troubleshooting.mddocs/qms/README-tr.mddocs/qms/sop_change_management-tr.mddocs/qms/sop_change_management.mddocs/qms/sop_data_management-tr.mddocs/qms/sop_data_management.mddocs/reference/approvals_subcommand-tr.mddocs/reference/approve_subcommand-tr.mddocs/reference/audit_event_catalog-tr.mddocs/reference/audit_event_catalog.mddocs/reference/configuration-tr.mddocs/reference/configuration.mddocs/reference/doctor_subcommand-tr.mddocs/reference/library_api_reference-tr.mddocs/reference/library_api_reference.mddocs/reference/purge_subcommand-tr.mddocs/reference/reverse_pii_subcommand-tr.mddocs/reference/reverse_pii_subcommand.mddocs/reference/supply_chain_security-tr.mddocs/reference/supply_chain_security.mddocs/reference/usage-tr.mddocs/reference/usage.mddocs/reference/verify_annex_iv_subcommand-tr.mddocs/reference/verify_audit-tr.mddocs/reference/verify_gguf_subcommand-tr.mddocs/reference/verify_integrity_subcommand-tr.mddocs/reference/verify_integrity_subcommand.mddocs/roadmap-tr.mddocs/roadmap.mddocs/roadmap/completed-phases.mddocs/roadmap/phase-14-5-pipeline-hardening.mddocs/roadmap/releases.mddocs/standards/error-handling.mddocs/standards/localization.mddocs/standards/logging-observability.mddocs/standards/regex.mddocs/standards/release.mddocs/standards/testing.mddocs/usermanuals/en/compliance/audit-log.mddocs/usermanuals/en/compliance/verify-audit.mddocs/usermanuals/en/deployment/chat.mddocs/usermanuals/en/deployment/deploy-targets.mddocs/usermanuals/en/deployment/gguf-export.mddocs/usermanuals/en/evaluation/judge.mddocs/usermanuals/en/operations/gpu-cost.mddocs/usermanuals/en/operations/webhooks.mddocs/usermanuals/en/reference/cli.mddocs/usermanuals/en/reference/configuration.mddocs/usermanuals/en/reference/exit-codes.mddocs/usermanuals/en/reference/json-output.mddocs/usermanuals/en/reference/yaml-templates.mddocs/usermanuals/en/training/sft.mddocs/usermanuals/tr/compliance/audit-log.mddocs/usermanuals/tr/compliance/verify-audit.mddocs/usermanuals/tr/deployment/chat.mddocs/usermanuals/tr/deployment/deploy-targets.mddocs/usermanuals/tr/deployment/gguf-export.mddocs/usermanuals/tr/evaluation/judge.mddocs/usermanuals/tr/operations/gpu-cost.mddocs/usermanuals/tr/operations/webhooks.mddocs/usermanuals/tr/reference/cli.mddocs/usermanuals/tr/reference/configuration.mddocs/usermanuals/tr/reference/exit-codes.mddocs/usermanuals/tr/reference/json-output.mddocs/usermanuals/tr/reference/yaml-templates.mddocs/usermanuals/tr/training/sft.mdforgelm/__init__.pyforgelm/_http.pyforgelm/_pypdf_normalise.pyforgelm/_strip_pattern.pyforgelm/_version.pyforgelm/benchmark.pyforgelm/chat.pyforgelm/cli/__init__.pyforgelm/cli/_abi_check.pyforgelm/cli/_argparse_types.pyforgelm/cli/_dispatch.pyforgelm/cli/_dry_run.pyforgelm/cli/_exit_codes.pyforgelm/cli/_no_train_modes.pyforgelm/cli/_parser.pyforgelm/cli/_pipeline.pyforgelm/cli/_result.pyforgelm/cli/_resume.pyforgelm/cli/_wizard.pyforgelm/cli/subcommands/_approvals.pyforgelm/cli/subcommands/_approve.pyforgelm/cli/subcommands/_audit.pyforgelm/cli/subcommands/_cache.pyforgelm/cli/subcommands/_chat.pyforgelm/cli/subcommands/_deploy.pyforgelm/cli/subcommands/_doctor.pyforgelm/cli/subcommands/_export.pyforgelm/cli/subcommands/_purge.pyforgelm/cli/subcommands/_quickstart.pyforgelm/cli/subcommands/_reverse_pii.pyforgelm/cli/subcommands/_safety_eval.pyforgelm/cli/subcommands/_verify_annex_iv.pyforgelm/cli/subcommands/_verify_gguf.pyforgelm/cli/subcommands/_verify_integrity.pyforgelm/compliance.pyforgelm/config.pyforgelm/data.pyforgelm/data_audit/_pii_regex.pyforgelm/data_audit/_simhash.pyforgelm/data_audit/_splits.pyforgelm/data_audit/_streaming.pyforgelm/data_audit/_types.pyforgelm/deploy.pyforgelm/export.pyforgelm/fit_check.pyforgelm/grpo_rewards.pyforgelm/inference.pyforgelm/ingestion.pyforgelm/judge.pyforgelm/merging.pyforgelm/model.pyforgelm/model_card.pyforgelm/results.pyforgelm/safety.pyforgelm/synthetic.pyforgelm/trainer.pyforgelm/utils.pyforgelm/webhook.pyforgelm/wizard/__init__.pyforgelm/wizard/_byod.pyforgelm/wizard/_io.pyforgelm/wizard/_orchestrator.pyforgelm/wizard/_state.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.tomltests/_data/api_signatures_1.0.0.jsontests/test_approvals_listing.pytests/test_benchmark.pytests/test_cache_subcommands.pytests/test_chat.pytests/test_check_anchor_resolution.pytests/test_check_bilingual_code_blocks.pytests/test_check_bilingual_parity.pytests/test_check_cli_help_consistency.pytests/test_check_doc_numerical_claims.pytests/test_check_field_descriptions.pytests/test_check_http_discipline.pytests/test_check_library_api_doc.pytests/test_check_no_unguarded_sys_modules_pop.pytests/test_check_notebook_pins.pytests/test_check_pip_audit.pytests/test_check_site_chrome_parity.pytests/test_check_tr_links_prefer_mirror.pytests/test_check_wizard_defaults_sync.pytests/test_check_yaml_snippets.pytests/test_cli.pytests/test_cli_helpers.pytests/test_cli_phase10.pytests/test_cli_quickstart_wiring.pytests/test_cli_subcommands.pytests/test_cli_training_seam.pytests/test_compliance.pytests/test_config.pytests/test_config_doc_parity.pytests/test_cost_estimation.pytests/test_data.pytests/test_data_audit.pytests/test_data_edge_cases.pytests/test_deploy.pytests/test_distributed.pytests/test_doctor.pytests/test_eu_ai_act.pytests/test_fit_check.pytests/test_gdpr_erasure.pytests/test_grpo_format_reward.pytests/test_grpo_math_reward.pytests/test_guard_wiring.pytests/test_http_dns_rebinding.pytests/test_human_approval_gate.pytests/test_inference.pytests/test_ingestion_reliability.pytests/test_judge_functions.pytests/test_library_api.pytests/test_long_context.pytests/test_merging_algos.pytests/test_model.pytests/test_model_card.pytests/test_model_loading_dispatch.pytests/test_no_train_modes.pytests/test_oom_recovery.pytests/test_phantom_tool_citations.pytests/test_phase12_review_fixes.pytests/test_phase7.pytests/test_pipeline_compliance.pytests/test_pipeline_config.pytests/test_pipeline_fixtures.pytests/test_pipeline_orchestrator.pytests/test_pypdf_normalise.pytests/test_pytest_markers.pytests/test_quickstart.pytests/test_quickstart_subprocess.pytests/test_reverse_pii.pytests/test_safety_advanced.pytests/test_strip_pattern.pytests/test_supply_chain_security.pytests/test_synthetic.pytests/test_trainer.pytests/test_trainer_sft_config.pytests/test_usermanual_reference_consistency.pytests/test_utils.pytests/test_verification_toolbelt.pytests/test_webhook.pytests/test_wizard_byod.pytests/test_wizard_phase22.pytools/check_anchor_resolution.pytools/check_bilingual_code_blocks.pytools/check_bilingual_parity.pytools/check_cli_help_consistency.pytools/check_doc_numerical_claims.pytools/check_field_descriptions.pytools/check_http_discipline.pytools/check_no_analysis_refs.pytools/check_no_unguarded_sys_modules_pop.pytools/check_notebook_pins.pytools/check_pip_audit.pytools/check_tr_links_prefer_mirror.pytools/check_wizard_defaults_sync.pytools/generate_sbom.pytools/generate_wizard_defaults.pytools/pip_audit_ignores.yaml
…view) merge_pipeline_stage_config dumped stage model/lora/training/data/evaluation with exclude_none only; an override omitting staging_ttl_days re-materialised the default 7 as explicitly-set and falsely raised ConfigError against a canonical retention block. Match the root dump (exclude_unset=True). CodeRabbit #12. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_non_negative_float and _sampling_temperature let NaN/inf bypass bounds checks (comparisons against NaN are False), flowing a malformed value into generation config. Guard with math.isfinite so they fail at the parse boundary (exit 2). CodeRabbit #19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#63-review)
split('-')[-1] accepted malformed names like checkpoint-bad-900 as numbered checkpoints. Slice after the 'checkpoint-' prefix and require the whole suffix isdecimal(); malformed dirs now hit the non-numeric-suffix warning. CodeRabbit #20.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…review) _run_reject_cmd logged human_approval.rejected with no OSError guard while the approve path had one; a storage failure escaped as an uncaught traceback with a non-contract exit code and could still reach notify_failure. Mirror the approve guard (exit 2, before the notifier). CodeRabbit #21. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…view) The escaping-path audit emit wrote raw resolved paths + run-id into the append-only chain, bypassing the mandated secret->PII->length-bound sanitiser used on the sibling OSError branches. Route it through _sanitise_audit_error_message. CodeRabbit #22. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PR#63-review) verify_integrity iterated manifest['artifacts'] unchecked: null/int raised an uncaught TypeError (traceback, not exit 1) and str/dict false-passed as 'All 0 artifacts present' (exit 0). Add an explicit container guard returning valid=False (exit 1) rather than CodeRabbit's silent coercion. CodeRabbit #23. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… secret (PR#63-review) #24: export_compliance_artifacts rollback swallowed os.remove/os.replace failures with except OSError: pass; collect and logger.error them before re-raising the original promotion error. #25: strict-HMAC guard rejected only None, letting an empty-string secret through; gate on 'not hmac_secret'. CodeRabbit #24,#25. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_read_jsonl_split decoded each line twice (replace-decode for text, then strict-decode to infer decode_error). Switch to strict-first with an errors='replace' fallback only on UnicodeDecodeError — behaviour-identical, one decode on the common path. Existing TestJsonlDecodeErrorDetection guards both branches. CodeRabbit #26. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PR#63-review)
_format_user_assistant_row gated System validation on truthiness, so 0/False/0.0/[]/{} bypassed clean_string's loud-fail contract and were silently dropped. Gate on sys_text != '' so only the genuine empty-string absent-system path skips, and every other value (incl. None) fails loudly. CodeRabbit #27.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…R#63-review)
_validate_rubric let an empty positional {} placeholder slip past the unexpected/missing checks, then crashed .format() with IndexError mid-eval. Add an explicit 'if "" in field_roots' guard with an actionable message. CodeRabbit #28.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lary (PR#63-review)
The uninitialised-classifier-head guard only matched the 2-label {label_0,label_1} default; a causal-LM checkpoint exposing LABEL_0..LABEL_2+ bypassed it and could emit meaningless safety verdicts. Match any all-LABEL_N vocabulary. CodeRabbit #29.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
CodeRabbit review-response — 30 open threads triagedEvery open CodeRabbit thread was verified against branch HEAD by an independent pass, then either fixed-with-test or skipped-with-reason. Outcome: 12 fixed, 15 already addressed, 3 intentional skips, 0 invalid. Full suite green (2775 passed) and CI green across Python 3.10–3.13. ✅ Fixed (12) — each ships a regression test
✅ Already addressed before this pass (15)These were filed against an earlier commit and the W1–W4 remediation already satisfied them at HEAD:
⏭️ Intentionally not changed (3) — documented standards/scope
Triage + fixes verified by an automated multi-agent pass; each fix re-confirmed by reverting it to prove the new test fails. Resolving the 27 addressed threads; the 3 skips are left open for maintainer review. |
1 critical + 11 high from the multi-agent review (executably verified); each behaviour change ships a regression test. File-grouped, so co-located lower-severity fixes ride along. F-C-01 (CRITICAL): trainer no longer auto-reverts a good model when safety eval fails for INFRASTRUCTURE reasons (evaluation_completed=False) — keeps the gate + logs instead of destroying the model and writing a false 'safety reverted' audit record. High: F-H-05 annex_iv set-serialisation hash; F-H-07 config->safety module-topology; F-H-08 email PII regex ReDoS; F-H-09/10 judge/safety malformed-input guards; F-H-11 wizard cancel scope; F-H-01 audit-catalog TR mirror; F-H-02 release.md phase map; F-H-03/04 verify-integrity docs (cli.md+json-output.md EN+TR); F-H-12 notebook pin. Plus hash_file public-rename (F-M-10). F-H-06 (audit-reroot env->config) DEFERRED to roadmap F-PR29-A6-14: constructor-param forced a public-API MAJOR bump for zero gain (no caller passes it); FORGELM_ALLOW_AUDIT_REROOT break-glass retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
25 medium: _http redaction + __all__ export surface (F-M-08/F-L-07/21), verify-integrity error envelope + exit-2 test (F-M-09/25), data clean-string (F-M-16), export error_kind (F-M-18/23), merging TIES/DARE (F-M-19/20), JSONL streaming (F-M-17), CHANGELOG + CLAUDE/AGENTS gauntlet mirror (F-M-01/02/03), docs (F-M-04/05/06/07), safety (F-M-21), trainer NaN baseline (F-M-22). Each code change ships a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Low/nit polish across grpo_rewards/_strip_pattern/model_card/inference/synthetic/_safety_eval/_dispatch-parser/_purge + test robustness (grpo warn-once module-state reset, email ReDoS absolute-time guard, data_audit, cli_subcommands, config_doc_parity, gdpr, notebook_pins, eu_ai_act, trainer torch-gate) + docs (roadmap, guides TR, audit). F-L-01 (sample_packing deprecation tracking-issue link) deferred — needs a GitHub issue number. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#61/#63 review) test_explicit_tier_disagreement_warns flaked in CI (3.11/3.12/3.13): the F-L-11 module-level _tier_disagreement_warned dedup set is populated by an earlier test (test_pipeline_config), suppressing the warning here. Clear the set first, mirroring the resets already in test_pipeline_config — makes the assertion order-independent. Verified by reproducing the polluting order locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Jun 15, 2026
docs: post-#63 accuracy audit — fix code-vs-doc drift + EN/TR currency
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
1 participant
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.
Full-project-review remediation — Waves 1–4 (H2 → N1)
Closes out the 2026-06-10 full-project review action plan. Wave 0 (7 CRITICAL release-blockers) and Wave 1/H1 (trainer/model dispatch) already merged in #61; this PR implements every remaining work-package — 78 commits, each a focused fix shipped with its regression test in the same commit.
Coverage (22/22 work-packages complete)
Method
Implemented via 3 sequential multi-agent workflows on a shared tree (no merge conflicts). Each agent verified every finding still reproduced at HEAD before acting — fixing it with a regression test, or skipping it with a one-line reason when prior adjacent work had already resolved it (~30 such already-fixed/invalid/duplicate skips, all documented in the run logs). House policy honoured: behaviour changes ship with tests; bilingual EN+TR doc mirrors kept in lockstep; the append-only audit-event catalog kept in sync.
Validation (green at HEAD)
pytest tests/: 2718 passed, 17 skipped, 0 failed (coverage 80%).ruff format --check .+ruff check .: clean (237 files).forgelm --config config_template.yaml --dry-run: exit 0.tools/check_*.pyguards green — including the newly-wiredcheck_doc_numerical_claims,check_notebook_pins,check_tr_links_prefer_mirror,check_bilingual_code_blocks,check_yaml_snippets,check_module_size,check_site_chrome_parity; the unwired-guard allowlist is now empty (every guard is CI-wired).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
forgelm verify-integritysubcommand to validate model artifacts against training-time SHA-256 manifestrequire_httpsenforcement and pipeline manifest integrity checkingChanged
training.sample_packingtotraining.packing(deprecated alias with v0.9.0 removal)deepspeedorfsdp; data mix ratios enforce cardinality matchingmax_safety_regressionas absolute unsafe-ratio ceiling (not baseline-relative)--data-auditflag andretention.staging_ttl_daysFixed