[cemililik]

Summary

• One-command bundled templates — forgelm quickstart customer-support renders a config the existing trainer accepts unchanged, copies a small seed dataset, runs training, and drops into chat with the result.
• 5 templates under forgelm/templates/ (all QLoRA 4-bit, rank=8, batch=1 — designed to fit on a 12 GB consumer GPU): customer-support, code-assistant, domain-expert (BYOD), medical-qa-tr (Turkish medical Q&A with safety disclaimers), grpo-math.
• GPU-aware auto-downsizing — picks primary 7B model when ≥10 GB VRAM available, fallback ≤2B otherwise; explicit no-gpu-detected reason without CUDA.
• Wizard integration — forgelm --wizard now opens with "Start from a template?" Yes routes to quickstart; No falls through to the existing 8-step interactive flow. Same code paths, same YAML schema — no bifurcation.
• CI smoke test — nightly runs quickstart --dry-run + --config <out> --dry-run for every bundled template; catches template drift the moment a config schema changes.
• pip install forgelm users get templates too via [tool.setuptools.package-data].

Closes Phase 10.5 of the roadmap. Bumps version to 0.4.5.

What changed

Area	File(s)	Note
New module	forgelm/quickstart.py	Template registry (frozen dataclass), auto_select_model(), run_quickstart() orchestrator
New assets	forgelm/templates/ (5 dirs + LICENSES.md)	Configs, seed JSONLs, attribution table (CC-BY-SA 4.0, author-original)
CLI	forgelm/cli.py	New quickstart subcommand: --list/--model/--dataset/--output/--dry-run/--no-chat
Wizard	forgelm/wizard.py	"Start from a template?" entry point routes to quickstart or falls through to existing flow
Tests	tests/test_quickstart.py (40 tests)	Includes regression test that loads every generated YAML through load_config() — strongest guard against template drift
CI	.github/workflows/nightly.yml	Per-template smoke test
Build	pyproject.toml	Version 0.4.5; package-data bundles templates into the wheel
Docs	README.md, CHANGELOG.md, docs/guides/quickstart.md, docs/roadmap.md/-tr.md, docs/roadmap/phase-12-quickstart.md, docs/roadmap/releases.md	Phase 10.5 marked Done

Test plan

• ruff format . && ruff check . — clean
• pytest tests/test_quickstart.py — 40/40 pass
• Full suite: 519 passed, 1 skipped (3 pre-existing TRL/torch FSDP failures in test_grpo_reward.py unrelated to this PR — verified via git stash)
• forgelm quickstart --list works
• forgelm quickstart customer-support --dry-run writes YAML and exits 0
• Generated YAML loads cleanly via forgelm.config.load_config
• Reviewer: spot-check one bundled JSONL for license-clean / well-formed entries
• Reviewer: confirm pip install -e . then forgelm quickstart --list works in a fresh venv (templates bundle correctly)

Notes for reviewer

• The 17 commits visible in this PR include unsquashed Phase 10 history that was already squash-merged to main via PR feat: Phase 10 post-training completion (v0.4.0) #7. The actual code diff (above) is clean and Phase 10.5-only — recommend squash merge to keep main history tidy.
• No breaking changes; all new surface is additive.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • One‑command quickstart: bundled templates, GPU‑aware model selection/auto‑downsizing, list/JSON/dry‑run, model/dataset/output overrides, optional post‑run chat, audit logging, and wizard integration.

• Bug Fixes

  • Chat save ensures output directories exist; stricter dataset/BYOD validation; GRPO max‑length accepts legacy key alias.

• Tests

  • Large expansion of unit, integration, packaging, and nightly wheel/install smoke tests.

• Packaging

  • Templates included in the wheel; project version bumped to v0.4.5.

[sourcery-ai]

Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters

[coderabbitai]

Warning

Rate limit exceeded

@cemililik has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 48 minutes and 24 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 24 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5dac36ba-8638-439f-bb3b-cf429cdf1267

📥 Commits

Reviewing files that changed from the base of the PR and between a933021 and 1319d57.

📒 Files selected for processing (4)

• forgelm/cli.py
• forgelm/grpo_rewards.py
• forgelm/quickstart.py
• tests/test_quickstart_hardening.py

📝 Walkthrough

Walkthrough

Adds a Quickstart Layer (v0.4.5): bundled one-command templates shipped in the wheel, forgelm quickstart CLI and wizard integration, VRAM-aware model selection and config materialization, GRPO reward fallbacks and trainer wiring, many templates/tests, and nightly/wheel-install CI smoke jobs.

Changes

Cohort / File(s)	Summary
Version & Packaging forgelm/__init__.py, pyproject.toml	Bump version 0.4.0→0.4.5 and include forgelm.templates package-data so template assets ship in built wheels.
Quickstart Core forgelm/quickstart.py	New immutable Template model and TEMPLATES registry; discovery APIs, auto_select_model (VRAM-aware), run_quickstart that materializes configs/datasets, writes provenance/audit JSONL, and helpers to format/summarize results.
CLI & Wizard forgelm/cli.py, forgelm/wizard.py	Add forgelm quickstart subcommand (--list/JSON, --model/--dataset/--output, --dry-run, --no-chat) with subprocess wiring and flag forwarding; wizard first attempts quickstart flow with BYOD validation and falls back to full wizard.
Templates & Assets forgelm/templates/..., forgelm/templates/LICENSES.md, forgelm/templates/__init__.py	Add five bundled templates (code-assistant, customer-support, domain-expert, grpo-math, medical-qa-tr) with configs/data/README and a licenses table; package docstring for reliable resource resolution.
Trainer / GRPO Rewards forgelm/grpo_rewards.py, forgelm/trainer.py	New GRPO reward fallbacks (format_match_reward, length_shaping_reward, combined_format_length_reward); trainer wiring always includes shaping reward and conditionally appends correctness reward when gold_answer exists; answer-normalization helpers and GRPO length field rename.
Config & Data forgelm/config.py, forgelm/data.py, config_template.yaml, docs/reference/configuration.md	Canonical grpo_max_completion_length with alias grpo_max_new_tokens via Field alias; tighten dataset pairing to zip(..., strict=True); docs/comments updated.
Inference / Chat forgelm/inference.py, forgelm/chat.py	Lazy-import torch, run model.generate inside torch.inference_mode() in background worker; ensure chat /save creates parent directory before writing.
Fit check / Utilities forgelm/fit_check.py	Refactor VRAM estimation into helpers: quant-scheme selection, component breakdown, CUDA probing, and verdict mapping; preserves public APIs.
Merging / Judge forgelm/merging.py, forgelm/judge.py	Minor merge tensor dim fix; refactor judge scoring into prompt-scoring and summarization helpers preserving result semantics.
Tests & Packaging QA tests/... (many new/updated tests)	Extensive test additions: quickstart registry/rendering, VRAM/no-GPU behavior, packaging/wheel asset presence, CLI subprocess forwarding, GRPO reward unit/integ tests, quickstart hardening/regression, wizard BYOD, and packaging smoke tests.
Docs & Roadmap README.md, CHANGELOG.md, docs/guides/quickstart.md, docs/roadmap*.md	Document quickstart usage and v0.4.5 release; update guides/roadmap to mark Phase 10.5 complete and describe templates, licensing, and CI.
CI .github/workflows/nightly.yml	Add template-level dry-run smoke in nightly job and a wheel-install-smoke job that builds/installs the wheel in an isolated venv and validates packaged templates; include it in failure notifications.

Sequence Diagram(s)

sequenceDiagram
    participant User as User / CLI
    participant CLI as forgelm.cli
    participant QS as forgelm.quickstart
    participant Train as Trainer Process
    participant Config as Config Loader

    User->>CLI: forgelm quickstart <template> [--dry-run/overrides]
    CLI->>QS: run_quickstart(template, overrides)
    QS->>QS: detect GPU VRAM / auto_select_model
    QS->>QS: load template config.yaml & patch model/data
    QS->>QS: copy bundled data.jsonl (if present)
    QS->>QS: write timestamped YAML + audit JSONL -> return QuickstartResult
    alt dry-run
        CLI-->>User: print summary / JSON
    else normal
        CLI->>Train: spawn subprocess: forgelm --config <generated.yaml>
        Train->>Config: load generated YAML
        Train-->>CLI: exit status
        CLI->>Train: optionally spawn: forgelm chat <model_dir>
        Train-->>User: interactive chat
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• feat: Phase 10 post-training completion (v0.4.0) #7: Overlapping changes affecting CLI/packaging/version bumps, inference/chat save adjustments, and GRPO wiring that are directly related to this quickstart and packaging work.

Poem

🐇 A hop, a template, a tiny plan,

Seeds and YAML sown by rabbit's hand.
VRAM-wise I pick the best, then run—
One command, one config, training begun.
Quickstarts bloom beneath the moonlit land.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 48.16% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title 'feat: Phase 10.5 Quickstart Layer (v0.4.5)' clearly and concisely summarizes the main change: introducing a quickstart layer feature for Phase 10.5 with version 0.4.5.
Description check	✅ Passed	The pull request description comprehensively covers Summary, Changes, Type, Testing, and Checklist sections. All required sections are present and filled out with appropriate detail.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch development

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codacy-production]

Not up to standards ⛔

🔴 Issues 2 critical · 1 minor

Alerts:
⚠ 3 issues (≤ 0 issues of at least minor severity)

Results:
3 new issues

Category	Results
BestPractice	1 minor
Security	2 critical

View in Codacy

🟢 Metrics 361 complexity · 20 duplication

Metric	Results
Complexity	361
Duplication	20

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[gemini-code-assist]

Code Review

This pull request introduces the Quickstart Layer and Post-Training toolchain, adding bundled templates, an interactive chat REPL, GGUF export, VRAM estimation, and deployment config generation. It also includes security hardening for webhooks and Markdown templates, along with refactoring for better modularity and OOM recovery. Feedback focuses on enhancing robustness, specifically by ensuring directory creation for saved chat history, handling interrupts in the quickstart CLI, guarding against missing configuration sections, enforcing strictness in dataset zipping, and ensuring inference mode is used in background generation threads.

I am having trouble creating individual review comments. Click here to see my feedback.

forgelm/chat.py (181)

The /save command will fail if the provided path includes non-existent parent directories. It's safer to ensure the directory structure exists before attempting to write the file.

            import os
            if os.path.dirname(path):
                os.makedirs(os.path.dirname(os.path.abspath(path)), exist_ok=True)
            with open(path, "w", encoding="utf-8") as f:

forgelm/cli.py (909-911)

The quickstart subcommand should handle KeyboardInterrupt gracefully, similar to the chat subcommand, to avoid showing a traceback when the user cancels the operation (especially during the long-running training phase).

    elif command == "quickstart":
        try:
            _run_quickstart_cmd(args, getattr(args, "output_format", "text"))
        except KeyboardInterrupt:
            pass
        sys.exit(EXIT_SUCCESS)

forgelm/config.py (472)

Accessing self.lora.r without checking if self.lora is None could lead to an AttributeError if the LoRA configuration section is omitted from the YAML. While other parts of the codebase might assume its presence, it's safer to guard this check.

        if self.lora and self.lora.r > 0:

forgelm/data.py (121)

Using zip without strict=True can lead to silent data truncation if the System, User, and Assistant columns have mismatched lengths. Enabling strict mode ensures that such inconsistencies are caught early.

        for s, u, a in zip(sys_texts, user_texts, asst_texts, strict=True)

forgelm/inference.py (288-290)

The background generation thread should run within torch.inference_mode() to disable gradient tracking and reduce memory overhead, consistent with the non-streaming generate implementation.

    def _gen_thread() -> None:
        import torch
        try:
            with torch.inference_mode():
                model.generate(**gen_kwargs)

[cemililik]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/roadmap-tr.md (1)

22-38: ⚠️ Potential issue | 🟡 Minor

Mermaid diagram is stale relative to the updated status table.

The table now references "Faz 10.5" (with target v0.4.5) and adds a new "Faz 14" (target v0.5.1), but the diagram still labels the quickstart node as P12 Quickstart Katmanı and omits Faz 14 entirely. Also, the AI summary mentions a styling change for P105 that is not present in the diagram code below.

Consider relabeling P12 → P105 (Faz 10.5) and adding a Faz 14 node so the visual matches the new "Bir bakışta durum" table.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/roadmap-tr.md` around lines 22 - 38, Update the Mermaid diagram so it
matches the status table: rename node P12 (label "Quickstart Katmanı") to P105
(Faz 10.5) and change its target tag to v0.4.5, add a new node for Faz 14 (e.g.,
P14 with label "Faz 14" and target v0.5.1) and wire the edges the same way P12
was connected (P10 → P105, P11 → P105, P105 → P13), and apply the intended style
change mentioned in the summary for P105 (the same fill/stroke scheme used for
other Faz 10.x nodes).

🧹 Nitpick comments (6)

forgelm/chat.py (1)

182-184: Directory pre-creation is fine; the if parent: guard is effectively dead.

Because os.path.abspath(path) always resolves to an absolute path, os.path.dirname(...) is never an empty string, so the guard never short-circuits. Not a bug — keep it for clarity if you prefer, or drop it. The surrounding OSError handler already covers makedirs failures, so error paths are correct.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/chat.py` around lines 182 - 184, The guard "if parent:" before
creating directories is dead because os.path.dirname(os.path.abspath(path))
never yields empty string; remove the conditional and call os.makedirs(parent,
exist_ok=True) unconditionally (or if you prefer keep it, replace it with an
explanatory comment) — update the block around where parent is computed (using
os.path.dirname and os.path.abspath) in forgelm/chat.py so only the necessary
os.makedirs(parent, exist_ok=True) remains and rely on the existing OSError
handling for failures.

forgelm/config.py (1)

472-479: The self.lora and guard is unreachable: lora is a required field on ForgeConfig.

ForgeConfig.lora: LoraConfigModel (line 373) is required, so self.lora is always truthy after validation — the new existence check never short-circuits. This is harmless, but if the goal was to handle a missing lora section then lora should be made Optional[LoraConfigModel] = None on ForgeConfig for the guard to actually do anything. Otherwise drop the redundant check.

♻️ Option A — drop the dead guard

-        if self.lora and self.lora.r > 0:
+        if self.lora.r > 0:
             logger.info(
                 "GaLore (gradient rank=%d) enabled alongside LoRA (adapter rank=%d). "

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/config.py` around lines 472 - 479, The conditional "if self.lora and
self.lora.r > 0:" contains a redundant existence check because ForgeConfig.lora
is required (type LoraConfigModel), so remove the dead "self.lora and" guard and
change the condition to "if self.lora.r > 0:" (keeping the logger message that
references self.training.galore_rank and self.lora.r); alternatively, if you
intentionally want lora to be optional, change the ForgeConfig.lora type to
Optional[LoraConfigModel] = None and keep the original guard—pick one approach
and apply it consistently.

forgelm/wizard.py (1)

318-378: Consider extracting the template-pick step.

_maybe_run_quickstart_template() interleaves the prompt, the digit/name parsing, the BYOD handling, and the orchestrator call in one function — and combined with the new branching in run_wizard() it pushes cognitive complexity over the SonarCloud threshold (flagged on line 381). Extracting the template-resolution block (lines 337‑354) into a small helper such as _pick_template_name(names) -> Optional[str] keeps each piece focused and quiets the static analysis warning without changing behavior.

Also, a small UX nit (no fix required): when the user types a digit that is out of range (e.g., "99"), the function silently falls through to the full 8‑step wizard rather than re-prompting. If that wasn't intentional, a single re-prompt would be friendlier than dumping into the long flow.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/wizard.py` around lines 318 - 378, Extract the template
selection/parsing logic inside _maybe_run_quickstart_template into a small
helper function named _pick_template_name(names) -> Optional[str] that accepts
the list of template names and returns the chosen name or None; move the block
that reads raw = _prompt(...), parses digits/indices and checks membership in
TEMPLATES into _pick_template_name and call it from
_maybe_run_quickstart_template (keep the same return semantics so behavior is
unchanged). Ensure the helper uses the same prompt text and default (names[0])
and preserves the current checks (raw.isdigit() -> index bounds -> names[idx-1],
elif raw in TEMPLATES -> raw, else None) so SonarCloud cognitive complexity is
reduced without changing UX; do not change BYOD handling or exception behavior.

forgelm/quickstart.py (3)

199-205: Address Sonar nits in _detect_available_vram_gb.

free is unused (Sonar python:S1481). Rename to _ to silence the warning and signal intent.

♻️ Proposed fix

-        free, total = torch.cuda.mem_get_info()
+        _, total = torch.cuda.mem_get_info()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/quickstart.py` around lines 199 - 205, In _detect_available_vram_gb,
rename the unused variable free returned by torch.cuda.mem_get_info() to _, so
the tuple unpacking becomes "_, total = torch.cuda.mem_get_info()" to silence
the Sonar python:S1481 unused-variable warning and signal intent while
preserving logic that uses total for the VRAM capacity calculation; keep the
existing exception handling and logger.debug intact.

---

318-330: Unused color parameter on format_template_list.

The color keyword-only argument is accepted but never read — Sonar python:S1172. Either drop it (no caller uses it today) or wire it up to ANSI rendering. Dropping is the lower-risk option since the public callers (forgelm/cli.py::_run_quickstart_cmd) invoke it without arguments.

♻️ Proposed fix

-def format_template_list(*, color: bool = False) -> str:
+def format_template_list() -> str:
     """Render the template registry as a human-readable list."""

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/quickstart.py` around lines 318 - 330, Remove the unused keyword-only
parameter `color` from the function signature of format_template_list and update
any internal references if present; ensure callers like _run_quickstart_cmd in
forgelm/cli.py continue to call format_template_list() without arguments (no
changes needed there because they don't pass color), and run tests/lint to
confirm the Sonar S1172 warning is resolved.

---

282-285: Defer config_target.parent.mkdir until after dataset resolution succeeds.

Today, calling run_quickstart("domain-expert") without dataset_override creates ./configs/ (or the user's --output parent directory) and only then raises ValueError("does not bundle a dataset"). The user is left with an empty directory they didn't ask for. Move the mkdir to just before the YAML write, or perform dataset resolution first.

♻️ Proposed reorder

     config_target = Path(output_path) if output_path else _default_output_path(template_name)
-    config_target.parent.mkdir(parents=True, exist_ok=True)
-
     dataset_path, dataset_notes = _resolve_dataset(template, dataset_override, config_target.parent)
 
     cfg = _materialize_config(template, chosen_model, dataset_path)
 
+    config_target.parent.mkdir(parents=True, exist_ok=True)
     with open(config_target, "w", encoding="utf-8") as f:
         yaml.safe_dump(cfg, f, default_flow_style=False, sort_keys=False)

Note: _resolve_dataset already calls scratch_dir.mkdir(...) itself before copying the bundled JSONL, so the bundled-data path keeps working.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/quickstart.py` around lines 282 - 285, The current code creates the
output parent directory via config_target.parent.mkdir(...) before calling
_resolve_dataset(...), which leaves an empty directory on failure; move the call
to config_target.parent.mkdir(parents=True, exist_ok=True) so it runs only after
dataset resolution succeeds (i.e., after dataset_path, dataset_notes =
_resolve_dataset(template, dataset_override, config_target.parent)) and
immediately before the YAML/config write step, or remove the early mkdir
entirely since _resolve_dataset() creates its own scratch dir when using bundled
data; update references to config_target, output_path, _default_output_path, and
dataset_override accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/nightly.yml:
- Around line 63-67: The loop in the nightly workflow intentionally omits the
BYOD template "domain-expert" because template_assets() (in
forgelm/quickstart.py) raises a ValueError for templates without bundled
datasets; add an inline comment inside the for tpl loop explaining that
domain-expert is BYOD and therefore excluded to prevent confusion, and also
update the PR summary text to accurately say "smoke tests 4 of 5 templates"
instead of "all 5 templates".

In `@CHANGELOG.md`:
- Line 7: The changelog has duplicate dates for release headings "## [0.4.5] —
2026-04-25" and "## [0.4.0] — 2026-04-25"; verify the correct release date for
the 0.4.5 entry and update the "## [0.4.5] — 2026-04-25" header to the correct
date (likely "2026-04-26") so it matches the PR/tag date, or correct the 0.4.0
date if that one is wrong, ensuring the header text for the affected version(s)
exactly matches the intended release date.

In `@docs/roadmap.md`:
- Around line 16-18: The roadmap has inconsistent release dates between the two
entries: the "v0.4.5" line shows 2026-04-25 while the "v0.4.0" line shows
2026-04-26, which is impossible; update the date for the "v0.4.5" entry (the
`v0.4.5` string in docs/roadmap.md) to the correct shipping date (likely
2026-04-26) so the timeline reads chronologically, or if  v0.4.0 is wrong update
the `v0.4.0` date instead—edit the text around the `v0.4.5` / `v0.4.0` entries
to make the dates consistent.

In `@docs/roadmap/releases.md`:
- Line 61: The release entry header "v0.4.5 — \"Quickstart Layer\" (2026-04-25)"
has a date earlier than the v0.4.0 entry and appears to be a typo; update the
header in releases.md so the date is consistent (e.g., change "(2026-04-25)" to
"(2026-04-26)" or set the correct published date), or if the version is wrong
adjust the version string to the intended one, ensuring the header "v0.4.5 —
\"Quickstart Layer\"" and its date match the PR/release timeline.

In `@forgelm/cli.py`:
- Around line 854-878: The spawned subprocesses (train_cmd and chat_cmd) don’t
inherit top-level CLI flags from args, so propagate relevant flags (e.g.
--output-format, --quiet, --log-level, --offline) into both train_cmd and
chat_cmd before calling subprocess.run; for each flag on args, append the
appropriate argv form (use flag-only for booleans like --quiet/--offline when
True, and flag + value for options like --output-format and --log-level) to the
lists so the child processes run with the same settings as the parent (locate
where train_cmd and chat_cmd are built near the call to
_load_config_for_quickstart_chat and update them accordingly).
- Around line 866-874: The code currently hardcodes "final_model" when building
final_model_dir; instead load the generated YAML via
_load_config_for_quickstart_chat(result.config_path) into a config dict and
derive the path using the config's output_dir and training.final_model_dir
(falling back to "final_model" if the field is missing). Replace the current
final_model_dir = Path(_load_config_for_quickstart_chat(result.config_path)) /
"final_model" with logic that reads config["output_dir"] and
config.get("training", {}).get("final_model_dir", "final_model") (mirroring how
_build_dry_run_result constructs the trainer output path) so auto-chat will find
the model when templates override final_model_dir.

In `@forgelm/templates/grpo-math/data.jsonl`:
- Line 26: Q26's prompt "Three numbers add to 60. The first is twice the second,
and the third is 5 more than the second..." yields a fractional second number
(13.75); change the constants so the answer is an integer — for example replace
"60" with "65" in the prompt so the equation 4x+5=65 gives x=15 and the final
line remains "Answer: <number>" for exact-match extraction.

In `@forgelm/templates/LICENSES.md`:
- Around line 30-31: The contributing checklist requires bundled template
configs to set gradient_checkpointing: true, but the added templates
(medical-qa-tr/config.yaml, code-assistant/config.yaml,
domain-expert/config.yaml, etc.) do not set training.gradient_checkpointing;
either add gradient_checkpointing: true under the training: section of each
bundled template config mentioned so the 12 GB-fit claim is accurate, or update
the checklist wording in LICENSES.md to remove the strict requirement (e.g., say
"rely on trainer default unless you need to override"); pick one approach and
apply it consistently across the listed templates and the checklist.

In `@forgelm/templates/medical-qa-tr/data.jsonl`:
- Line 12: Update the assistant message string in the JSON object that answers
"Yüksek ateş nasıl düşürülür?" to correct the typo "sertboyun" to two words
"sert boyun" so the model trains on the proper Turkish phrase; locate the
assistant content field inside the JSON entry (the object where
"role":"assistant" contains the response text) and replace the single-word
"sertboyun" with "sert boyun" preserving the rest of the sentence and
punctuation.
- Line 17: The assistant's phrase "üzerini temiz bir bez veya ileri tabakanın
yapışmadığı pansumanla örtün" is awkward and should be replaced with a concise
Turkish phrasing meaning "non‑stick dressing"; update the assistant message
content (the string starting with "Hafif (1. derece) yanıkta:") to read
something like "... üzerini temiz bir bez veya yara yüzeyine yapışmayan
pansumanla örtün." ensuring the rest of the sentence and the final safety
reminder ("Tıbbi acil durumlarda 112'yi arayın veya bir sağlık profesyoneline
danışın.") remain unchanged.

In `@forgelm/wizard.py`:
- Around line 357-361: The BYOD branch in _maybe_run_quickstart_template
currently treats an empty dataset_path as a silent fallback which causes
run_wizard to start the generic 8-step flow; change this to re-prompt the user
(or abort with a clear message) when template.bundled_dataset is false and the
user submits an empty path: replace the single _prompt call for dataset_path
with a loop that calls _prompt repeatedly (showing the same prompt and an
explicit "cancel" or empty-to-abort option) until a non-empty path is provided
or the user explicitly cancels, and if cancelled raise/return a clear sentinel
(or error) rather than returning None so run_wizard can handle it appropriately;
reference: template.bundled_dataset, chosen, _prompt,
_maybe_run_quickstart_template, run_wizard, and the BYOD template like
"domain-expert".

---

Outside diff comments:
In `@docs/roadmap-tr.md`:
- Around line 22-38: Update the Mermaid diagram so it matches the status table:
rename node P12 (label "Quickstart Katmanı") to P105 (Faz 10.5) and change its
target tag to v0.4.5, add a new node for Faz 14 (e.g., P14 with label "Faz 14"
and target v0.5.1) and wire the edges the same way P12 was connected (P10 →
P105, P11 → P105, P105 → P13), and apply the intended style change mentioned in
the summary for P105 (the same fill/stroke scheme used for other Faz 10.x
nodes).

---

Nitpick comments:
In `@forgelm/chat.py`:
- Around line 182-184: The guard "if parent:" before creating directories is
dead because os.path.dirname(os.path.abspath(path)) never yields empty string;
remove the conditional and call os.makedirs(parent, exist_ok=True)
unconditionally (or if you prefer keep it, replace it with an explanatory
comment) — update the block around where parent is computed (using
os.path.dirname and os.path.abspath) in forgelm/chat.py so only the necessary
os.makedirs(parent, exist_ok=True) remains and rely on the existing OSError
handling for failures.

In `@forgelm/config.py`:
- Around line 472-479: The conditional "if self.lora and self.lora.r > 0:"
contains a redundant existence check because ForgeConfig.lora is required (type
LoraConfigModel), so remove the dead "self.lora and" guard and change the
condition to "if self.lora.r > 0:" (keeping the logger message that references
self.training.galore_rank and self.lora.r); alternatively, if you intentionally
want lora to be optional, change the ForgeConfig.lora type to
Optional[LoraConfigModel] = None and keep the original guard—pick one approach
and apply it consistently.

In `@forgelm/quickstart.py`:
- Around line 199-205: In _detect_available_vram_gb, rename the unused variable
free returned by torch.cuda.mem_get_info() to _, so the tuple unpacking becomes
"_, total = torch.cuda.mem_get_info()" to silence the Sonar python:S1481
unused-variable warning and signal intent while preserving logic that uses total
for the VRAM capacity calculation; keep the existing exception handling and
logger.debug intact.
- Around line 318-330: Remove the unused keyword-only parameter `color` from the
function signature of format_template_list and update any internal references if
present; ensure callers like _run_quickstart_cmd in forgelm/cli.py continue to
call format_template_list() without arguments (no changes needed there because
they don't pass color), and run tests/lint to confirm the Sonar S1172 warning is
resolved.
- Around line 282-285: The current code creates the output parent directory via
config_target.parent.mkdir(...) before calling _resolve_dataset(...), which
leaves an empty directory on failure; move the call to
config_target.parent.mkdir(parents=True, exist_ok=True) so it runs only after
dataset resolution succeeds (i.e., after dataset_path, dataset_notes =
_resolve_dataset(template, dataset_override, config_target.parent)) and
immediately before the YAML/config write step, or remove the early mkdir
entirely since _resolve_dataset() creates its own scratch dir when using bundled
data; update references to config_target, output_path, _default_output_path, and
dataset_override accordingly.

In `@forgelm/wizard.py`:
- Around line 318-378: Extract the template selection/parsing logic inside
_maybe_run_quickstart_template into a small helper function named
_pick_template_name(names) -> Optional[str] that accepts the list of template
names and returns the chosen name or None; move the block that reads raw =
_prompt(...), parses digits/indices and checks membership in TEMPLATES into
_pick_template_name and call it from _maybe_run_quickstart_template (keep the
same return semantics so behavior is unchanged). Ensure the helper uses the same
prompt text and default (names[0]) and preserves the current checks
(raw.isdigit() -> index bounds -> names[idx-1], elif raw in TEMPLATES -> raw,
else None) so SonarCloud cognitive complexity is reduced without changing UX; do
not change BYOD handling or exception behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 83988864-811c-412d-b3ec-f7392d3a225d

📥 Commits

Reviewing files that changed from the base of the PR and between fd0838c and 993d840.

📒 Files selected for processing (31)

• .github/workflows/nightly.yml
• CHANGELOG.md
• README.md
• docs/guides/quickstart.md
• docs/roadmap-tr.md
• docs/roadmap.md
• docs/roadmap/phase-12-quickstart.md
• docs/roadmap/releases.md
• forgelm/__init__.py
• forgelm/chat.py
• forgelm/cli.py
• forgelm/config.py
• forgelm/data.py
• forgelm/inference.py
• forgelm/quickstart.py
• forgelm/templates/LICENSES.md
• forgelm/templates/__init__.py
• forgelm/templates/code-assistant/config.yaml
• forgelm/templates/code-assistant/data.jsonl
• forgelm/templates/customer-support/config.yaml
• forgelm/templates/customer-support/data.jsonl
• forgelm/templates/domain-expert/README.md
• forgelm/templates/domain-expert/config.yaml
• forgelm/templates/grpo-math/config.yaml
• forgelm/templates/grpo-math/data.jsonl
• forgelm/templates/medical-qa-tr/config.yaml
• forgelm/templates/medical-qa-tr/data.jsonl
• forgelm/wizard.py
• pyproject.toml
• tests/test_grpo_reward.py
• tests/test_quickstart.py

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (1)

forgelm/quickstart.py (1)

367-447: ⚠️ Potential issue | 🟡 Minor

dry_run still produces on-disk side effects (per-run dir, audit event, dataset copy, YAML).

Lines 396, 398-413, 415, 425-427 all execute regardless of dry_run. The flag is purely informational here and only gates the trainer subprocess in forgelm.cli. Either tighten the docstring to make that explicit (e.g. "Hint forwarded to the CLI; this function always materializes config + dataset"), or short-circuit _resolve_dataset/_materialize_config/the YAML write when dry_run=True so a true preview leaves no artifacts. Same concern as the prior review on this hunk — flagging again because the behavior is unchanged in this revision.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/quickstart.py` around lines 367 - 447, The quickstart function
currently performs filesystem side effects even when dry_run=True; change it to
short-circuit before any on-disk work: after deciding
chosen_model/selection_reason, if dry_run is True skip calling
_append_audit_event, skip creating config_target.parent, skip _resolve_dataset
and _materialize_config and the YAML write (and do not perform the dataset
copy), and return a QuickstartResult that reflects the
chosen_model/selection_reason with config_path and dataset_path set to None (or
a clearly non-written marker) and started_training=False; update any logfile
message to reflect dry-run state. Reference symbols: quickstart,
_append_audit_event, config_target, _resolve_dataset, _materialize_config,
QuickstartResult, and the YAML write block.

🧹 Nitpick comments (4)

forgelm/wizard.py (1)

326-378: Unused template_name parameter — drop it or surface it in prompts.

_resolve_byod_dataset_path accepts template_name but never uses it (SonarCloud confirmed). Either remove it from the signature and the caller, or thread it into the prompt/error messages so users see which template they're providing data for (e.g., "Path to your dataset JSONL for '{template_name}'…").

♻️ Suggested fix (drop the parameter)

-def _resolve_byod_dataset_path(template_name: str) -> Optional[str]:
+def _resolve_byod_dataset_path() -> Optional[str]:
     """Prompt the user for a BYOD dataset path and validate it.

-        dataset_path = _resolve_byod_dataset_path(chosen)
+        dataset_path = _resolve_byod_dataset_path()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/wizard.py` around lines 326 - 378, The function
_resolve_byod_dataset_path currently takes an unused parameter template_name;
remove this unused parameter from the function signature and its docstring and
update every caller to stop passing template_name (or, alternatively, if you
prefer to keep the context, thread template_name into the user-facing
strings—e.g., include it in the prompt and error messages inside
_resolve_byod_dataset_path—so the parameter is actually used). Ensure you update
the function definition, adjust all invocations to match the new signature (or
keep the same calls if you choose to use the parameter), and revise the
docstring to reflect the final behavior.

tests/test_cli_helpers.py (1)

35-44: Redundant filter assertions — the index check below is the real guard.

Lines 38-39 build a comprehension that filters by membership in ("--log-level", "DEBUG") and compares to the same list. This passes vacuously even if the two tokens were not adjacent (and would also pass if there were spurious copies). The contiguous-pair assertions at lines 41-44 already cover this correctly; consider dropping the comprehension lines to avoid the false sense of coverage.

♻️ Suggested simplification

 def test_inherited_flags_propagates_log_level_to_both() -> None:
     args = _ns(log_level="DEBUG")
     train_flags, chat_flags = _build_quickstart_inherited_flags(args)
-    assert ["--log-level", "DEBUG"] == [flag for flag in train_flags if flag in ("--log-level", "DEBUG")]
-    assert ["--log-level", "DEBUG"] == [flag for flag in chat_flags if flag in ("--log-level", "DEBUG")]
-    # Also assert the pair appears contiguously in both lists.
+    # Assert the --log-level/DEBUG pair appears contiguously in both lists.
     assert "--log-level" in train_flags
     assert train_flags[train_flags.index("--log-level") + 1] == "DEBUG"
     assert "--log-level" in chat_flags
     assert chat_flags[chat_flags.index("--log-level") + 1] == "DEBUG"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_cli_helpers.py` around lines 35 - 44, Remove the redundant
membership-comprehension assertions in
test_inherited_flags_propagates_log_level_to_both that check ["--log-level",
"DEBUG"] == [flag for flag in train_flags if flag in ("--log-level", "DEBUG")]
and the equivalent for chat_flags; they are vacuous and duplicate coverage
already provided by the contiguous-pair checks. Keep the calls to _ns and
_build_quickstart_inherited_flags and retain the explicit contiguous assertions
that verify train_flags and chat_flags contain "--log-level" followed
immediately by "DEBUG" (using train_flags.index("--log-level") + 1 and
chat_flags.index("--log-level") + 1). Ensure no other assertions are removed so
the test still validates adjacency and presence.

forgelm/trainer.py (2)

99-120: Minor: zip(..., strict=False) will silently drop completions when gold_answer length differs.

If TRL's per-sample column passthrough ever yields a gold_answer list shorter than completions (custom data collator, prompt-only fallback, etc.), the trailing completions silently get no reward signal instead of surfacing the mismatch. Given the contract is supposed to be aligned, strict=True would catch wiring regressions earlier without changing happy-path behavior.

♻️ Suggested change

-    for completion, gold in zip(completions, golds, strict=False):
+    for completion, gold in zip(completions, golds, strict=True):

Worth confirming TRL always passes per-sample columns aligned with completions. If it doesn't (e.g., for prompts without a gold_answer cell), strict=True would raise — in which case the current strict=False is the right call and this comment can be ignored.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/trainer.py` around lines 99 - 120, In _math_reward_fn the
zip(completions, golds, strict=False) call can silently drop completions when
gold_answer is shorter; change it to zip(completions, golds, strict=True) inside
_math_reward_fn so mismatched lengths raise immediately and surface wiring
regressions (keep the rest of the logic using _ANSWER_PATTERN, _normalize_answer
and _answers_match unchanged); if there is a valid case where golds may be
missing for some completions, instead explicitly handle that case before zipping
(e.g., fill missing golds or branch) rather than relying on strict=False.

---

45-81: Optional: bare "m" in _REWARD_STRIP_TOKENS can over-strip non-unit suffixes.

The bare "m" token combined with endswith/startswith will strip a trailing m from any answer string that happens to end with one (e.g., a hypothetical "Answer: them" collapses to "the"). It's safe for the curated grpo-math seed dataset but brittle if a future template reuses this normalizer with less constrained outputs.

Consider gating unit stripping behind a word-boundary or whitespace check (e.g., only strip when preceded by whitespace or a digit) so the helper can be reused safely outside the math template. Non-blocking for this PR.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/trainer.py` around lines 45 - 81, The token list _REWARD_STRIP_TOKENS
and function _normalize_answer can over-strip the bare "m" from arbitrary
strings; change the stripping logic to only remove "m" when it appears as a unit
token at a word boundary (e.g., preceded/followed by whitespace or a digit)
rather than any suffix/prefix match. Update _normalize_answer to check
surrounding characters (or use a simple regex word-boundary check) before
trimming a unit token like "m" so you still strip genuine units (e.g., "5 m" or
"5m") but avoid trimming letters inside words like "them"; keep the existing
two-pass loop and other tokens unchanged and only add the boundary check for
single-letter unit tokens.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@forgelm/cli.py`:
- Around line 831-848: The missing-TEMPLATE branch currently logs an error to
stderr and exits directly; update it to mirror the run_quickstart exception path
by checking the same output_format variable and, if output_format == "json",
print a JSON error envelope (e.g., {"success": False, "error": "<message>"} ) to
stdout instead of using logger.error, then call sys.exit(EXIT_CONFIG_ERROR);
modify the args.template check near the top of the quickstart invocation so it
uses the output_format-aware behavior, keeping the same error message text when
not in JSON mode.

In `@forgelm/templates/grpo-math/config.yaml`:
- Around line 46-47: Update the inline comment that currently says "Renamed from
grpo_max_new_tokens in v0.4.6" to reflect the shipped Quickstart Layer version
(v0.4.5) or make it version-agnostic; specifically edit the comment immediately
above the grpo_max_completion_length field (which replaced grpo_max_new_tokens)
to read either "Renamed from grpo_max_new_tokens in v0.4.5" or "Renamed from
grpo_max_new_tokens (v0.4.5+)" so the note matches the package/versioning in
pyproject.toml and docs/roadmap.md.

In `@tests/test_quickstart_subprocess.py`:
- Around line 105-159: The assertions that check for absolute paths use
POSIX-specific startswith("/") and will fail on Windows; update the two checks
(the cfg_value check in test_quickstart_subprocess.py and the model_path check
in test_chat_subprocess_uses_absolute_model_path) to use a platform-agnostic
absolute-path test such as os.path.isabs(cfg_value) or
pathlib.Path(cfg_value).is_absolute() (and similarly for model_path) so the
tests validate "absolute" on all OSes while still keeping the existing endswith
checks for the filename.

---

Duplicate comments:
In `@forgelm/quickstart.py`:
- Around line 367-447: The quickstart function currently performs filesystem
side effects even when dry_run=True; change it to short-circuit before any
on-disk work: after deciding chosen_model/selection_reason, if dry_run is True
skip calling _append_audit_event, skip creating config_target.parent, skip
_resolve_dataset and _materialize_config and the YAML write (and do not perform
the dataset copy), and return a QuickstartResult that reflects the
chosen_model/selection_reason with config_path and dataset_path set to None (or
a clearly non-written marker) and started_training=False; update any logfile
message to reflect dry-run state. Reference symbols: quickstart,
_append_audit_event, config_target, _resolve_dataset, _materialize_config,
QuickstartResult, and the YAML write block.

---

Nitpick comments:
In `@forgelm/trainer.py`:
- Around line 99-120: In _math_reward_fn the zip(completions, golds,
strict=False) call can silently drop completions when gold_answer is shorter;
change it to zip(completions, golds, strict=True) inside _math_reward_fn so
mismatched lengths raise immediately and surface wiring regressions (keep the
rest of the logic using _ANSWER_PATTERN, _normalize_answer and _answers_match
unchanged); if there is a valid case where golds may be missing for some
completions, instead explicitly handle that case before zipping (e.g., fill
missing golds or branch) rather than relying on strict=False.
- Around line 45-81: The token list _REWARD_STRIP_TOKENS and function
_normalize_answer can over-strip the bare "m" from arbitrary strings; change the
stripping logic to only remove "m" when it appears as a unit token at a word
boundary (e.g., preceded/followed by whitespace or a digit) rather than any
suffix/prefix match. Update _normalize_answer to check surrounding characters
(or use a simple regex word-boundary check) before trimming a unit token like
"m" so you still strip genuine units (e.g., "5 m" or "5m") but avoid trimming
letters inside words like "them"; keep the existing two-pass loop and other
tokens unchanged and only add the boundary check for single-letter unit tokens.

In `@forgelm/wizard.py`:
- Around line 326-378: The function _resolve_byod_dataset_path currently takes
an unused parameter template_name; remove this unused parameter from the
function signature and its docstring and update every caller to stop passing
template_name (or, alternatively, if you prefer to keep the context, thread
template_name into the user-facing strings—e.g., include it in the prompt and
error messages inside _resolve_byod_dataset_path—so the parameter is actually
used). Ensure you update the function definition, adjust all invocations to
match the new signature (or keep the same calls if you choose to use the
parameter), and revise the docstring to reflect the final behavior.

In `@tests/test_cli_helpers.py`:
- Around line 35-44: Remove the redundant membership-comprehension assertions in
test_inherited_flags_propagates_log_level_to_both that check ["--log-level",
"DEBUG"] == [flag for flag in train_flags if flag in ("--log-level", "DEBUG")]
and the equivalent for chat_flags; they are vacuous and duplicate coverage
already provided by the contiguous-pair checks. Keep the calls to _ns and
_build_quickstart_inherited_flags and retain the explicit contiguous assertions
that verify train_flags and chat_flags contain "--log-level" followed
immediately by "DEBUG" (using train_flags.index("--log-level") + 1 and
chat_flags.index("--log-level") + 1). Ensure no other assertions are removed so
the test still validates adjacency and presence.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 80595dc0-309c-476f-91cc-8faa22169936

📥 Commits

Reviewing files that changed from the base of the PR and between 02554cf and 56351b9.

📒 Files selected for processing (21)

• .github/workflows/nightly.yml
• config_template.yaml
• docs/guides/alignment.md
• docs/reference/configuration.md
• docs/roadmap-tr.md
• docs/roadmap.md
• docs/roadmap/phase-12-quickstart.md
• forgelm/cli.py
• forgelm/config.py
• forgelm/grpo_rewards.py
• forgelm/quickstart.py
• forgelm/templates/grpo-math/config.yaml
• forgelm/trainer.py
• forgelm/wizard.py
• tests/test_alignment.py
• tests/test_cli_helpers.py
• tests/test_grpo_math_reward.py
• tests/test_packaging.py
• tests/test_quickstart_compat.py
• tests/test_quickstart_subprocess.py
• tests/test_trainer_grpo_config.py

✅ Files skipped from review due to trivial changes (2)

• config_template.yaml
• docs/reference/configuration.md

🚧 Files skipped from review as they are similar to previous changes (1)

• .github/workflows/nightly.yml

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

forgelm/trainer.py (1)

162-180: Heads-up: bool(first["gold_answer"]) treats falsy column values as "no gold".

For string golds (the bundled templates), this is fine and test_empty_gold_answer_treated_as_missing even pins it. But if a future dataset stores numeric golds where the first row happens to be 0 (or 0.0, False, None), the math reward gets silently disabled even though the column exists. Since the rest of the pipeline normalizes everything via _normalize_answer(gold) and string-equality, you might prefer probing for column presence only:

♻️ Optional: presence-based probe

-        first = train[0]
-        if isinstance(first, dict):
-            return "gold_answer" in first and bool(first["gold_answer"])
+        first = train[0]
+        if isinstance(first, dict):
+            return "gold_answer" in first

You'd need to update test_empty_gold_answer_treated_as_missing accordingly, or keep the current "empty string = missing" semantics if that's the deliberate UX.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/trainer.py` around lines 162 - 180, The detection in
_dataset_has_gold_answers treats falsy gold values (e.g., 0, 0.0, False, None)
as "no gold" by using bool(first["gold_answer"]); change the probe to check for
key presence instead (e.g., if isinstance(first, dict): return "gold_answer" in
first) so existence is detected regardless of value, and then update or adjust
test_empty_gold_answer_treated_as_missing to reflect the new presence-based
semantics (or explicitly special-case empty-string-as-missing if you want to
keep the current UX).

forgelm/cli.py (1)

798-921: Dispatcher logic is solid — the JSON-mode missing-template error, abs-path argv, inherited flags, and YAML-driven final_model_dir lookup all line up with the prior review feedback.

SonarCloud reports cognitive complexity 19 (>15) on this function. Optional split: pull the train-and-chat tail (lines 876–921) into a _run_quickstart_train_then_chat(args, result, output_format) helper if you want to silence it without behavior change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/cli.py` around lines 798 - 921, Function _run_quickstart has high
cognitive complexity; extract the training-and-chat tail into a new helper to
simplify the dispatcher. Create a helper named
_run_quickstart_train_then_chat(args, result, output_format) that contains
everything after the dry_run check starting at the subprocess training
invocation through the final sys.exit(EXIT_SUCCESS), move the subprocess logic
(train_cmd, subprocess.run, train_rc handling), the args.no_chat check, the
_load_quickstart_train_paths/final_model_dir check, and the chat subprocess
logic into that helper, and call it from _run_quickstart with the same arguments
so behavior, exit codes, logging, and inherited flags (from
_build_quickstart_inherited_flags) remain identical; ensure you import
subprocess in the helper or keep the existing import and preserve the same
checks/comments and return/exit semantics.

tests/test_quickstart_subprocess.py (1)

121-160: Patching pathlib.Path.is_dir globally is broad but acceptable here — the assertions on argv shape and absolute-path semantics remain precise.

Worth noting if this test starts becoming flaky as the quickstart codepath grows: a more surgical patch.object(Path, "is_dir", ...) scoped to a constructed instance, or a real final_model directory under tmp_path, would reduce the blast radius. Not a blocker.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_quickstart_subprocess.py` around lines 121 - 160, The test
test_chat_subprocess_uses_absolute_model_path currently patches
pathlib.Path.is_dir globally via patch("pathlib.Path.is_dir",
return_value=True); to reduce blast radius and make the test less brittle,
change that to a more surgical approach by patching the Path class method via
patch.object(Path, "is_dir", return_value=True) or (alternatively) create an
actual final_model directory under tmp_path and point the patched
_load_quickstart_train_paths to that tmp_path-based directory; update references
in the test (the patch call and the mocked return_value for
_load_quickstart_train_paths used by main) so the Path.is_dir override is scoped
to Path and/or the created tmp_path/final_model instance rather than globally
patching pathlib.Path.is_dir.

forgelm/wizard.py (1)

326-378: Path resolution doesn't match the docstring — .expanduser() alone keeps relative paths relative.

Lines 329–330 promise "local paths are returned as resolved absolute paths," but line 360 only does Path(dataset_path).expanduser(). A user input of ./mydata.jsonl is validated and then returned as mydata.jsonl — still relative. Today the train subprocess inherits the parent cwd so this works by accident, but anything that later changes cwd between argv build and exec (or any non-cli consumer of the generated YAML) will silently miss the dataset.

Either tighten the impl to actually return absolute, or weaken the docstring.

♻️ Suggested fix (resolve to absolute, matching the docstring)

-        resolved = Path(dataset_path).expanduser()
+        resolved = Path(dataset_path).expanduser().resolve()
         if not resolved.is_file():

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/wizard.py` around lines 326 - 378, The docstring promises absolute
paths but the code only calls Path(dataset_path).expanduser() and returns that
(still relative); update _resolve_byod_dataset_path so after expanding user (~)
you check existence on that Path, then call resolved = resolved.resolve() (or
resolved.resolve(strict=True)) to convert it to an absolute path before
returning; i.e., change the block around resolved = Path(...).expanduser() to
first validate resolved.is_file(), then replace the returned value with
str(resolved.resolve()) so callers always get an absolute path.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@forgelm/cli.py`:
- Around line 893-895: The code currently propagates the raw child return code
(train_rc) via logger.error and sys.exit, which can leak signal-derived codes
(e.g., 137, 139); change the logic around the train_rc check so that if train_rc
!= 0 you log the code but map/clamp any value outside the allowed contract
(0,1,2,3,4) to the canonical EXIT_TRAINING_ERROR constant (likely named
EXIT_TRAINING_ERROR or value 2) before calling sys.exit; update the block that
references train_rc, logger.error and sys.exit to compute an exit_code =
train_rc if train_rc in (0,1,2,3,4) else EXIT_TRAINING_ERROR, log accordingly,
and call sys.exit(exit_code).

---

Nitpick comments:
In `@forgelm/cli.py`:
- Around line 798-921: Function _run_quickstart has high cognitive complexity;
extract the training-and-chat tail into a new helper to simplify the dispatcher.
Create a helper named _run_quickstart_train_then_chat(args, result,
output_format) that contains everything after the dry_run check starting at the
subprocess training invocation through the final sys.exit(EXIT_SUCCESS), move
the subprocess logic (train_cmd, subprocess.run, train_rc handling), the
args.no_chat check, the _load_quickstart_train_paths/final_model_dir check, and
the chat subprocess logic into that helper, and call it from _run_quickstart
with the same arguments so behavior, exit codes, logging, and inherited flags
(from _build_quickstart_inherited_flags) remain identical; ensure you import
subprocess in the helper or keep the existing import and preserve the same
checks/comments and return/exit semantics.

In `@forgelm/trainer.py`:
- Around line 162-180: The detection in _dataset_has_gold_answers treats falsy
gold values (e.g., 0, 0.0, False, None) as "no gold" by using
bool(first["gold_answer"]); change the probe to check for key presence instead
(e.g., if isinstance(first, dict): return "gold_answer" in first) so existence
is detected regardless of value, and then update or adjust
test_empty_gold_answer_treated_as_missing to reflect the new presence-based
semantics (or explicitly special-case empty-string-as-missing if you want to
keep the current UX).

In `@forgelm/wizard.py`:
- Around line 326-378: The docstring promises absolute paths but the code only
calls Path(dataset_path).expanduser() and returns that (still relative); update
_resolve_byod_dataset_path so after expanding user (~) you check existence on
that Path, then call resolved = resolved.resolve() (or
resolved.resolve(strict=True)) to convert it to an absolute path before
returning; i.e., change the block around resolved = Path(...).expanduser() to
first validate resolved.is_file(), then replace the returned value with
str(resolved.resolve()) so callers always get an absolute path.

In `@tests/test_quickstart_subprocess.py`:
- Around line 121-160: The test test_chat_subprocess_uses_absolute_model_path
currently patches pathlib.Path.is_dir globally via patch("pathlib.Path.is_dir",
return_value=True); to reduce blast radius and make the test less brittle,
change that to a more surgical approach by patching the Path class method via
patch.object(Path, "is_dir", return_value=True) or (alternatively) create an
actual final_model directory under tmp_path and point the patched
_load_quickstart_train_paths to that tmp_path-based directory; update references
in the test (the patch call and the mocked return_value for
_load_quickstart_train_paths used by main) so the Path.is_dir override is scoped
to Path and/or the created tmp_path/final_model instance rather than globally
patching pathlib.Path.is_dir.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: efae9881-392d-4e08-9828-7f6c07d08d7e

📥 Commits

Reviewing files that changed from the base of the PR and between 56351b9 and 2b0df78.

📒 Files selected for processing (7)

• forgelm/cli.py
• forgelm/templates/grpo-math/config.yaml
• forgelm/trainer.py
• forgelm/wizard.py
• tests/test_cli_helpers.py
• tests/test_grpo_math_reward.py
• tests/test_quickstart_subprocess.py

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/test_cli_helpers.py
• forgelm/templates/grpo-math/config.yaml

[cemililik]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (1)

forgelm/quickstart.py (1)

401-433: ⚠️ Potential issue | 🟡 Minor

Reminder: dry_run=True still materializes the dataset and writes config.

run_quickstart(..., dry_run=True) still calls config_target.parent.mkdir(...) and _resolve_dataset(...) (which copies the seed JSONL). The flag effectively only flips a field on QuickstartResult for the CLI to consult later — the docstring at line 384 ("Generate the YAML without invoking training") suggests no on-disk side effects beyond the YAML.

This was raised in a prior review and not yet addressed; flagging again so it isn't lost. Either tighten the docstring or skip dataset materialization on dry_run=True.

🧹 Nitpick comments (2)

forgelm/trainer.py (1)

47-50: Redundant \n inside the negated \s class.

SonarCloud is right here: in [^\s.!?\n], \s already covers \n (along with \t, \r, etc.), so the trailing \n is dead. The second class [^.!?\n] does need the explicit \n because there's no \s there — keep that one.

♻️ Drop the duplicate

 _ANSWER_PATTERN = re.compile(
-    r"answer\s*:\s*([^\s.!?\n][^.!?\n]*(?:[.!?](?!\s|$)[^.!?\n]*)*)",
+    r"answer\s*:\s*([^\s.!?][^.!?\n]*(?:[.!?](?!\s|$)[^.!?\n]*)*)",
     re.IGNORECASE,
 )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/trainer.py` around lines 47 - 50, The regex in _ANSWER_PATTERN
contains a redundant "\n" inside the first negated character class [^\s.!?\n];
remove the "\n" from that class so it becomes [^\s.!?] while leaving the second
class [^.!?\n] unchanged; update the _ANSWER_PATTERN definition accordingly to
keep semantics identical but eliminate the duplicate newline exclusion.

tests/test_grpo_math_reward.py (1)

116-229: Consider adding a non-string gold_answer case to TestMathRewardFn.

TestDatasetHasGoldAnswers covers 0, 0.0, and False as present labels, but every case in TestMathRewardFn uses string gold_answer (e.g. ["7"], ["2/5"]). That gap is what hides the _normalize_answer AttributeError flagged in forgelm/trainer.py — adding a single parametrized test like _math_reward_fn(["Answer: 0"], gold_answer=[0]) would catch it and lock in the contract that the gating function advertises.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_grpo_math_reward.py` around lines 116 - 229, Add a test to
TestMathRewardFn that supplies a non-string gold_answer to exercise type
handling (e.g., call _math_reward_fn(["Answer: 0"], gold_answer=[0]) and assert
reward 1.0), so the reward pipeline (_math_reward_fn and underlying
_normalize_answer in forgelm/trainer.py) handles numeric/boolean labels without
raising AttributeError; add this as a new unit (or parametrize existing tests)
alongside existing string cases to lock in the contract that gold_answer may be
non-string.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@forgelm/cli.py`:
- Around line 940-953: The except block around run_quickstart currently catches
(FileNotFoundError, ValueError) but not FileExistsError raised by
forgelm.quickstart._resolve_dataset; update the error handling in the try/except
that invokes run_quickstart to also catch FileExistsError (either add it to the
tuple or add a separate except FileExistsError as e), and ensure the handler
uses the same JSON/text reporting logic (printing {"success": False, "error":
str(e)} for output_format == "json" and logger.error("Quickstart failed: %s", e)
otherwise) before calling sys.exit(EXIT_CONFIG_ERROR).

In `@forgelm/trainer.py`:
- Around line 101-123: _normalize_answer currently calls s.strip() and will
crash for non-string gold values (ints/bools) which _dataset_has_gold_answers
allows; to fix, coerce non-None inputs to str at the top of _normalize_answer
(e.g., treat None as "" and otherwise replace s with str(s)) so subsequent
.strip/.rstrip calls are safe, and add a regression test in TestMathRewardFn
that calls _math_reward_fn(["Answer: 0"], gold_answer=[0]) and asserts rewards
== [1.0] (also include a boolean case like gold_answer=[False]) to ensure
numeric/bool gold labels don't crash.

In `@forgelm/wizard.py`:
- Around line 369-389: The HF Hub regex branch misclassifies relative local
paths (like "data/train.jsonl"); change the control flow in the input loop in
wizard.py so you validate the path as a local JSONL first (call
_validate_local_jsonl(dataset_path) immediately after basic checks) and only if
that returns None then test _HF_HUB_ID_RE.match(dataset_path) and treat it as a
Hub ID; keep the existing messages but ensure _validate_local_jsonl is attempted
before the _HF_HUB_ID_RE check so local files are accepted and Hub IDs are a
fallback.

---

Nitpick comments:
In `@forgelm/trainer.py`:
- Around line 47-50: The regex in _ANSWER_PATTERN contains a redundant "\n"
inside the first negated character class [^\s.!?\n]; remove the "\n" from that
class so it becomes [^\s.!?] while leaving the second class [^.!?\n] unchanged;
update the _ANSWER_PATTERN definition accordingly to keep semantics identical
but eliminate the duplicate newline exclusion.

In `@tests/test_grpo_math_reward.py`:
- Around line 116-229: Add a test to TestMathRewardFn that supplies a non-string
gold_answer to exercise type handling (e.g., call _math_reward_fn(["Answer: 0"],
gold_answer=[0]) and assert reward 1.0), so the reward pipeline (_math_reward_fn
and underlying _normalize_answer in forgelm/trainer.py) handles numeric/boolean
labels without raising AttributeError; add this as a new unit (or parametrize
existing tests) alongside existing string cases to lock in the contract that
gold_answer may be non-string.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d63ae65a-270a-42c1-bab9-b9e05ffe1acb

📥 Commits

Reviewing files that changed from the base of the PR and between 2b0df78 and dfeb6e5.

📒 Files selected for processing (9)

• forgelm/cli.py
• forgelm/quickstart.py
• forgelm/trainer.py
• forgelm/wizard.py
• tests/test_cli_quickstart_wiring.py
• tests/test_grpo_format_reward.py
• tests/test_grpo_math_reward.py
• tests/test_quickstart_compat.py
• tests/test_quickstart_subprocess.py

✅ Files skipped from review due to trivial changes (1)

• tests/test_cli_quickstart_wiring.py

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

forgelm/judge.py (1)

308-312: ⚠️ Potential issue | 🟡 Minor

Non-numeric judge score values can crash the entire eval run.

float(raw_score) will raise ValueError/TypeError if the judge returns a non-numeric score (e.g., "score": "high", "score": "8/10", "score": [8]). Because this happens inside the per-prompt loop with no try/except, a single malformed verdict aborts the whole run_judge_evaluation and bypasses the failure_count / score=None sentinel path that the rest of the refactor carefully preserves.

🛡️ Suggested guard

-        raw_score = result.get("score")
-        score = _clip_judge_score(float(raw_score) if raw_score is not None else None)
+        raw_score = result.get("score")
+        try:
+            numeric_score = float(raw_score) if raw_score is not None else None
+        except (TypeError, ValueError):
+            logger.warning("Judge returned non-numeric score %r; treating as failure", raw_score)
+            numeric_score = None
+        score = _clip_judge_score(numeric_score)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/judge.py` around lines 308 - 312, When converting per-judge raw_score
to float inside run_judge_evaluation, guard the conversion to avoid crashing on
non-numeric values: wrap float(raw_score) in a try/except (catch ValueError and
TypeError), and on exception set score = None, increment failure_count, and
proceed to append score to scores (so the existing _clip_judge_score(None) /
sentinel path is preserved); use the same symbols raw_score, score,
_clip_judge_score, failure_count, scores and result to locate and update the
logic.

🧹 Nitpick comments (5)

forgelm/judge.py (1)

326-332: Minor: pass len(eval_prompts) instead of the full list.

_summarize_judge_scores only uses eval_prompts for its length in log/error messages. Passing an int (e.g., num_prompts) tightens the contract and avoids implying the function inspects prompt content.

♻️ Optional refactor

 def _summarize_judge_scores(
     *,
     scores: List[Optional[float]],
     failure_count: int,
-    eval_prompts: List[str],
+    num_prompts: int,
     min_score: float,
 ) -> tuple[float, bool, Optional[str]]:

…and update the two len(eval_prompts) references plus the call site at line 265.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/judge.py` around lines 326 - 332, Change _summarize_judge_scores so
it accepts num_prompts: int instead of eval_prompts: List[str] (update the
signature and type annotation) and replace its internal uses of
len(eval_prompts) with num_prompts in log/error messages; then update every call
that currently passes eval_prompts to pass len(eval_prompts)) instead (ensure
the call site(s) use the same symbol names and types). Ensure the return tuple
and behavior remain unchanged and update any docstring/comments referring to
eval_prompts.

tests/test_grpo_format_reward.py (1)

205-248: Classifier-reward wiring test is correct, but the assertion is weaker than the docstring claims.

The docstring on line 244 says "calling it should invoke our mock_rw_model exactly once," but line 248 only asserts mock_rw_model.called (boolean). Consider tightening to assert mock_rw_model.call_count == 1 to actually enforce the "exactly once" contract — otherwise a future regression that double-invokes the reward model per call would slip through.

♻️ Tighten the call-count assertion

-    assert mock_rw_model.called, "classifier-backed reward closure must call the loaded reward model"
+    assert mock_rw_model.call_count == 1, (
+        "classifier-backed reward closure must call the loaded reward model exactly once"
+    )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_grpo_format_reward.py` around lines 205 - 248, The test
test_reward_func_uses_classifier_when_configured currently only asserts
mock_rw_model.called; change this to assert the exact call count to match the
docstring by replacing that boolean check with a precise check such as assert
mock_rw_model.call_count == 1 so the closure invocation is enforced to call the
loaded reward model exactly once (refer to mock_rw_model and reward_funcs in the
test for locating the assertion to update).

tests/test_quickstart_subprocess.py (1)

49-168: LGTM — covers the high-value subprocess argv invariants without over-coupling.

The actionable-recovery message check (line 64-65) and the os.path.isabs portability fix on lines 112/167 hit the right level of specificity. Materializing a real tmp_path/checkpoints/final_model instead of monkey-patching Path.is_dir() globally is the right call — the past foot-gun is gone.

Minor optional polish: the two main()-driven tests share ~80% of their setup (recorder, argv, patches, final_model_dir creation). A small fixture (e.g. _invoke_quickstart) would let each test focus on the one assertion that distinguishes them, though it isn't blocking.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_quickstart_subprocess.py` around lines 49 - 168, Tests
test_train_subprocess_uses_absolute_config_path and
test_chat_subprocess_uses_absolute_model_path duplicate ~80% setup; extract a
small helper (e.g. _invoke_quickstart or fixture) that builds argv, creates
final_model_dir, constructs the _RunRecorder, applies the same patches
(forgelm.quickstart._detect_available_vram_gb,
forgelm.cli._load_quickstart_train_paths, subprocess.run, sys.argv), runs main()
and returns the recorder and exit code so each test only asserts its specific
argv details (use symbols _RunRecorder, main, and the two test names to locate
callers to refactor).

tests/test_cli_quickstart_wiring.py (1)

32-52: Optional: _RunRecorder is now duplicated verbatim with tests/test_quickstart_subprocess.py.

Both files explicitly note the duplication is intentional, but a tiny shared tests/conftest.py (or tests/_quickstart_helpers.py) holding _RunRecorder and the fake_train_paths/_invoke_main helpers would let future quickstart-wiring tests start with one import instead of copy-pasting the harness. Non-blocking — flag to revisit when a third file in the same shape lands.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_cli_quickstart_wiring.py` around lines 32 - 52, Extract the
duplicated test helper class _RunRecorder and the helpers fake_train_paths and
_invoke_main into a single shared test helper module (e.g., a tests conftest or
helpers module) and update both test files to import them instead of
duplicating; specifically, move the _RunRecorder implementation (preserving its
constructor signature, __call__ behavior and internal _Completed class) and the
fake_train_paths/_invoke_main helper functions into the shared module, then
replace the verbatim copies in tests/test_cli_quickstart_wiring.py and
tests/test_quickstart_subprocess.py with imports from that shared module.

forgelm/fit_check.py (1)

253-256: Optional: the m.config.torch_dtype fallback is unreachable against the current ModelConfig schema.

ForgeConfig.model is a Pydantic ModelConfig with extra="forbid" and no torch_dtype (or config) field — so both getattr(m, "torch_dtype", None) and getattr(getattr(m, "config", None), "torch_dtype", None) always evaluate to None, and the function always falls through to the bf16 default. If that's intentional defensive code for a future field, fine; if you want to actually honor a user-declared dtype, the field needs to exist on ModelConfig first.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/fit_check.py` around lines 253 - 256, The fallback to
m.config.torch_dtype in fit_check.py is unreachable because ForgeConfig.model
uses a Pydantic ModelConfig that lacks a torch_dtype (and config) field; add an
optional torch_dtype field to the ModelConfig schema (e.g., Optional[str] or
Optional[torch.dtype]) with appropriate validation/default so getattr(m,
"torch_dtype", None) returns the user-declared dtype, and keep the existing
lookup in fit_check.py unchanged; update the ModelConfig class definition to
include the new torch_dtype attribute and tests/docs where ModelConfig is
constructed.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@forgelm/trainer.py`:
- Around line 633-651: The _build_classifier_reward function needs two fixes:
add a pad_token guard before tokenization by setting _rw_tok.pad_token =
_rw_tok.pad_token or _rw_tok.eos_token to avoid "asking to pad" errors for
tokenizers without an explicit pad token (e.g., GPT-2), and update the function
docstring to state the expected reward-head shape (e.g., "expects a
sequence-classification head with output_size=1; logits[:, 0] returns the scalar
reward; 2-class heads will yield class logits and are incompatible without
post-processing"). Make these changes inside the _build_classifier_reward
staticmethod (affecting symbols _rw_tok and _reward_fn and clarifying use of
logits[:, 0]).

---

Outside diff comments:
In `@forgelm/judge.py`:
- Around line 308-312: When converting per-judge raw_score to float inside
run_judge_evaluation, guard the conversion to avoid crashing on non-numeric
values: wrap float(raw_score) in a try/except (catch ValueError and TypeError),
and on exception set score = None, increment failure_count, and proceed to
append score to scores (so the existing _clip_judge_score(None) / sentinel path
is preserved); use the same symbols raw_score, score, _clip_judge_score,
failure_count, scores and result to locate and update the logic.

---

Nitpick comments:
In `@forgelm/fit_check.py`:
- Around line 253-256: The fallback to m.config.torch_dtype in fit_check.py is
unreachable because ForgeConfig.model uses a Pydantic ModelConfig that lacks a
torch_dtype (and config) field; add an optional torch_dtype field to the
ModelConfig schema (e.g., Optional[str] or Optional[torch.dtype]) with
appropriate validation/default so getattr(m, "torch_dtype", None) returns the
user-declared dtype, and keep the existing lookup in fit_check.py unchanged;
update the ModelConfig class definition to include the new torch_dtype attribute
and tests/docs where ModelConfig is constructed.

In `@forgelm/judge.py`:
- Around line 326-332: Change _summarize_judge_scores so it accepts num_prompts:
int instead of eval_prompts: List[str] (update the signature and type
annotation) and replace its internal uses of len(eval_prompts) with num_prompts
in log/error messages; then update every call that currently passes eval_prompts
to pass len(eval_prompts)) instead (ensure the call site(s) use the same symbol
names and types). Ensure the return tuple and behavior remain unchanged and
update any docstring/comments referring to eval_prompts.

In `@tests/test_cli_quickstart_wiring.py`:
- Around line 32-52: Extract the duplicated test helper class _RunRecorder and
the helpers fake_train_paths and _invoke_main into a single shared test helper
module (e.g., a tests conftest or helpers module) and update both test files to
import them instead of duplicating; specifically, move the _RunRecorder
implementation (preserving its constructor signature, __call__ behavior and
internal _Completed class) and the fake_train_paths/_invoke_main helper
functions into the shared module, then replace the verbatim copies in
tests/test_cli_quickstart_wiring.py and tests/test_quickstart_subprocess.py with
imports from that shared module.

In `@tests/test_grpo_format_reward.py`:
- Around line 205-248: The test test_reward_func_uses_classifier_when_configured
currently only asserts mock_rw_model.called; change this to assert the exact
call count to match the docstring by replacing that boolean check with a precise
check such as assert mock_rw_model.call_count == 1 so the closure invocation is
enforced to call the loaded reward model exactly once (refer to mock_rw_model
and reward_funcs in the test for locating the assertion to update).

In `@tests/test_quickstart_subprocess.py`:
- Around line 49-168: Tests test_train_subprocess_uses_absolute_config_path and
test_chat_subprocess_uses_absolute_model_path duplicate ~80% setup; extract a
small helper (e.g. _invoke_quickstart or fixture) that builds argv, creates
final_model_dir, constructs the _RunRecorder, applies the same patches
(forgelm.quickstart._detect_available_vram_gb,
forgelm.cli._load_quickstart_train_paths, subprocess.run, sys.argv), runs main()
and returns the recorder and exit code so each test only asserts its specific
argv details (use symbols _RunRecorder, main, and the two test names to locate
callers to refactor).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4cbe8d1a-2832-49ce-a694-d73acb3787a4

📥 Commits

Reviewing files that changed from the base of the PR and between 2b0df78 and a933021.

📒 Files selected for processing (15)

• forgelm/cli.py
• forgelm/fit_check.py
• forgelm/judge.py
• forgelm/merging.py
• forgelm/quickstart.py
• forgelm/trainer.py
• forgelm/wizard.py
• notebooks/kto_binary_feedback.ipynb
• tests/runtime_smoke.py
• tests/test_cli_quickstart_wiring.py
• tests/test_grpo_format_reward.py
• tests/test_grpo_math_reward.py
• tests/test_quickstart_compat.py
• tests/test_quickstart_subprocess.py
• tests/test_wizard_byod.py

✅ Files skipped from review due to trivial changes (1)

• forgelm/merging.py

🚧 Files skipped from review as they are similar to previous changes (3)

• tests/test_grpo_math_reward.py
• tests/test_quickstart_compat.py
• forgelm/wizard.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if accelerate is in install_requires / dependencies
fd -t f 'pyproject.toml|setup.py|setup.cfg|requirements*.txt' --max-depth 3 \
  --exec rg -n -i 'accelerate' {} \;

Repository: cemililik/ForgeLM

Length of output: 116

---

🏁 Script executed:

#!/bin/bash
# Find and examine the dependency declaration files
fd -t f 'pyproject.toml|setup.py' --max-depth 3 -x cat -n {} \; | head -100

Repository: cemililik/ForgeLM

Length of output: 3589

---

🏁 Script executed:

#!/bin/bash
# Check for pad_token handling in the trainer.py file
rg -n 'pad_token' forgelm/trainer.py -A 2 -B 2

Repository: cemililik/ForgeLM

Length of output: 43

---

🏁 Script executed:

#!/bin/bash
# Look for any documentation or docstring mentioning reward model shape expectations
rg -n 'reward.*model|reward.*shape|scalar.*reward' forgelm/trainer.py -A 3 -B 1

Repository: cemililik/ForgeLM

Length of output: 2458

---

Add pad_token guard and document reward-model shape assumption in docstring.

The _build_classifier_reward method has two issues that should be addressed:

1. _rw_tok(completions, ..., padding=True, ...) will raise ValueError: Asking to pad but the tokenizer does not have a padding token for tokenizers without an explicit padding token (e.g., some GPT-2-based reward models). Add _rw_tok.pad_token = _rw_tok.pad_token or _rw_tok.eos_token before tokenizing to handle this edge case.

2. The docstring does not document that logits[:, 0] assumes a scalar (1-output) reward head. For 2-class classifiers, this returns the negative class logit. Document the expected reward-model shape (e.g., "expects a sequence-classification head with output_size=1") so users configure compatible models.

Note: accelerate>=0.27.0 is already a declared runtime dependency, so device_map="auto" support is guaranteed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@forgelm/trainer.py` around lines 633 - 651, The _build_classifier_reward
function needs two fixes: add a pad_token guard before tokenization by setting
_rw_tok.pad_token = _rw_tok.pad_token or _rw_tok.eos_token to avoid "asking to
pad" errors for tokenizers without an explicit pad token (e.g., GPT-2), and
update the function docstring to state the expected reward-head shape (e.g.,
"expects a sequence-classification head with output_size=1; logits[:, 0] returns
the scalar reward; 2-class heads will yield class logits and are incompatible
without post-processing"). Make these changes inside the
_build_classifier_reward staticmethod (affecting symbols _rw_tok and _reward_fn
and clarifying use of logits[:, 0]).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud