[cemililik]

Summary by CodeRabbit

• New Features

  • Human-approval staging with approve/reject commands; expanded CLI entrypoints and a packaged dataset-audit (PII/secrets, near-duplicate, Croissant export).

• Documentation

  • Updated human-oversight, audit-event, localization, release, onboarding docs; refreshed CHANGELOG/README and site SEO/locales.

• Configuration

  • Stricter enum validation for several config fields; added staging TTL for approvals.

• Chores

  • CI: strict site-claim validation, release pipeline reworked to tag-triggered cross-OS/Python matrix with per-combo SBOMs; optional local pre-commit; removed unused optional extra.

• Tests

  • Many new/updated tests for CLI, audit, human-approval, config validation, and site-claim checks.

[qodo-code-review]

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

[sourcery-ai]

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

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 570a1b7e-ff0b-44d9-ac55-460956c25b42

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Restructures the CLI from a monolithic module into a package architecture, introduces a comprehensive data-audit framework with PII/secrets/duplicate detection, implements human-approval staging workflow with CLI operators commands, narrows exception handling across multiple modules, adds configuration constraints via Literal types, updates CI/CD pipelines with cross-OS testing and SBOM generation, and establishes site-claim validation tooling.

Changes

Cohort / File(s)	Summary
CLI Package Restructuring forgelm/cli.py (deleted), forgelm/cli/__init__.py, forgelm/cli/__main__.py, forgelm/cli/_*.py, forgelm/cli/subcommands/_*.py	Refactors monolithic CLI into a structured package: deletes original cli.py (~1861 lines), creates package hierarchy with modular components for argument parsing (_parser.py), subcommand dispatching (_dispatch.py), logging/versioning (_logging.py), config loading (_config_load.py), training orchestration (_training.py), result formatting (_result.py), and per-command handlers (chat, export, deploy, quickstart, ingest, audit, verify-audit, approve, reject) under subcommands/. Adds exit code constants and preserves public import surface via package __init__.py.
Data Audit Package (New) forgelm/data_audit/__init__.py, forgelm/data_audit/_*.py	Introduces comprehensive forgelm.data_audit package (~2800 lines) with modular analysis pipeline: PII detection/masking (_pii_regex.py, _pii_ml.py), secrets detection (_secrets.py), near-duplicate finding via simhash/MinHash LSH (_simhash.py, _minhash.py), streaming JSONL ingestion (_streaming.py), per-split auditing (_aggregator.py, _splits.py), Croissant 1.0 metadata generation (_croissant.py), quality heuristics (_quality.py), cross-split leakage analysis (_orchestrator.py), and human-readable reporting (_summary.py, _types.py). Includes optional-dependency gating (_optional.py) for datasketch, presidio, xxhash, numpy.
Human Approval Workflow forgelm/cli/subcommands/_approve.py, forgelm/trainer.py, forgelm/webhook.py, forgelm/results.py	Adds staged deployment model: trainer saves adapters to final_model.staging when evaluation.require_human_approval=true, emits human_approval.required audit events with staging_path/run_id, and invokes webhook notifier. New CLI subcommands forgelm approve <run_id> and forgelm reject <run_id> perform atomic rename promotion, emit compliance events, and trigger success/failure webhooks. TrainResult gains staging_path field. Updated webhook notification includes operator instructions.
Configuration Enhancements forgelm/config.py	Converts seven config fields from unconstrained str to Literal[...] constraints (LoRA bias, FSDP backward prefetch/state dict type, safety scoring, compliance risk classification, Galore optimizer/projection type), shifting validation to parse time. Adds EvaluationConfig.staging_ttl_days field (int ≥0) for compliance retention metadata.
Exception Handling Narrowing forgelm/compliance.py, forgelm/judge.py, forgelm/model.py, forgelm/safety.py, forgelm/trainer.py	Replaces broad except Exception with targeted exception tuples across generation/classification/utility functions: API calls catch RequestException+shape issues (KeyError, IndexError, TypeError, ValueError); generation/tokenization catch RuntimeError, ValueError, TypeError, IndexError, KeyError; model operations catch specific OSError/TypeError/ValueError subsets. Adds lint suppression where best-effort audit/resource logging must not mask primary failures.
Model Expert Name Resolution forgelm/model.py	Replaces brute-force substring matching for MoE expert indices with centralized regex-pattern registry (_EXPERT_NAME_PATTERNS), enabling architecture-specific parsing (Mixtral, Qwen3, DeepSeek-V3, Phi-MoE) with suppressed duplicate-log sentinel tracking for unrecognized patterns.
Wizard Output Testability forgelm/wizard.py	Wraps all wizard output via new _print(...) helper routing through builtins.print, enabling test fixture interception and testable stdout contracts without monkeypatching builtins directly.
Test Fixtures & Factory Consolidation tests/_helpers/factories.py, tests/conftest.py, tests/test_*.py (multiple)	Introduces shared minimal_config(**overrides) factory in _helpers/factories.py, replaces local test helpers/hardcoded dicts with pytest fixture injection across 20+ test modules, consolidates config construction patterns, and ensures test isolation via deep-copy independent structures.
New Test Coverage tests/test_check_site_claims.py, tests/test_human_approval_gate.py, tests/test_faz27_narrow_exceptions.py, tests/test_model_moe_naming.py, tests/test_phase12_5.py, tests/test_wizard_phase11.py, tests/test_config_literal_sweep.py	Adds comprehensive test suites: site-claim validation contract (pyproject version, GPU pricing, artifact filenames, templates), human-approval gate trainer/CLI flows (staging promotion, audit events, webhook dispatch), narrowed exception paths (langdetect fallback, API errors, classifier robustness), MoE naming pattern matching and observability, Presidio/MinHash optional-extra behavior, wizard output contract, and Literal type validation across config enums.
CI/CD Pipelines & Tools .github/workflows/ci.yml, .github/workflows/publish.yml, .github/workflows/site-deploy.yml, .github/workflows/usermanuals-validate.yml, tools/check_site_claims.py, tools/generate_sbom.py, tools/build_usermanuals.py	Adds site-claim validation step in validate job; refactors publish.yml to trigger on v* tag push (not GitHub Releases), removes TestPyPI, adds cross-os-tests matrix (3 OS × 4 Python, install wheel, run pytest subset, generate per-combo CycloneDX SBOM); narrows site-deploy verification to only language bundles in SUPPORTED_LANGUAGES; adds strict orphan-markdown detection in usermanuals-validate; introduces check_site_claims.py (AST+HTML parsing, 4-check contract verification, --strict exit-code gating); introduces generate_sbom.py (CycloneDX 1.5 from pip list JSON); updates build_usermanuals.py to skip deferred languages and detect stale bundles.
Pre-commit Configuration .pre-commit-config.yaml, CONTRIBUTING.md, docs/standards/code-review.md	Introduces optional local developer pre-commit hook config (ruff lint/format, gitleaks secret scanning, trailing-whitespace/EOF fixes, YAML/TOML validation, merge-conflict detection). Documents installation/usage in CONTRIBUTING.md with SKIP=... override guidance and clarifies CI remains enforcement. Updates code-review docs to tie pre-commit to existing CI checks.
Site & SEO Updates site/*.html, site/js/guide.js	Updates <head> metadata on 9 HTML pages: replaces YOUR_DOMAIN placeholder with https://forgelm.dev, adds og:locale:alternate and per-language hreflang links (en, tr, x-default), updates canonical URLs. Adds locale-aware toLocaleLowerCase(state.lang) for search matching/highlighting in guide.js. Updates compliance page artifact list with 4 new output files.
Documentation & CHANGELOG CHANGELOG.md, README.md, docs/**, docs/usermanuals/*	CHANGELOG documents Wave 1 closure (human-approval workflow, staging/promote/reject, new audit events, wizard refactoring, test consolidation, deprecation path for --data-audit, Literal validation, CI/tool changes, new gate tests); updates product strategy (en/tr) to detail staging deployment and operator control flow; expands audit event catalog (human\_approval.\* payload fields, legacy CLI deprecation); formalizes localization policy (EN+TR manually authored, DE/FR/ES/ZH UI-only with fallback); updates release procedures documentation; documents optional-dependency removals; updates user manual CLI examples to positional run_id and --output-dir flags.
Localization Narrowing .github/workflows/site-deploy.yml, docs/standards/localization.md, tools/build_usermanuals.py	Formalizes supported language policy: only EN and TR receive authored user manuals with strict mirror requirements, DE/FR/ES/ZH are site-UI translation only with i18n fallback to English. Removes ingestion-secrets extra from docs/packaging (detect-secrets moved into audit CLI, no longer optional).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

• feat: Phase 10.5 Quickstart Layer (v0.4.5) #9: Both PRs modify quickstart implementation and wiring across forgelm/quickstart, CLI/wizard integration, templates, and related tests/pyproject versioning at the code level.
• feat: Phase 10 post-training completion (v0.4.0) #7: Both PRs refactor CLI entrypoint and subcommand dispatch, restructuring control flow and subcommand wiring.
• feat(closure): foundation bundle — Faz 1-8 (8 phase consolidation) #19: Both PRs make overlapping changes to CI jobs (validate steps, tools/build\_usermanuals, site-claim checks), data\_audit/cli refactors, webhook approval notifications, and related tests/helpers.

Poem

🐰 Hops into code with joy so clear,
CLI blooms in packages dear,
Humans judge before we ship,
Audit guards each data quip,
Testing strong with fresh-spun thread—
Wave One closes, main() fed! 🎉

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch closure/wave1-faz9-32

[codacy-production]

Not up to standards ⛔

🔴 Issues 3 critical · 22 high · 1 minor

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

Results:
26 new issues

Category	Results
ErrorProne	22 high
Security	3 critical 1 minor

View in Codacy

🟢 Metrics 1027 complexity · 15 duplication

Metric	Results
Complexity	1027
Duplication	15

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 implements the Article 14 human-approval gate for training runs, introducing staging directories, new CLI subcommands (approve/reject), and associated audit logging and webhook notifications. It also adds optional pre-commit hooks, updates the release pipeline with a cross-OS test matrix, and tightens configuration validation using Pydantic Literals. My feedback suggests optimizing memory usage in the audit log parser and using os.path.lexists for safer destination checks during model promotion.

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

tools/generate_sbom.py (1)

46-47: ⚡ Quick win

Include timeout stderr for improved diagnostics.

TimeoutExpired also captures stderr and stdout from the pip subprocess, which can help diagnose why the command timed out (e.g., hung prompts, network issues).

💡 Proposed enhancement

-    except (subprocess.TimeoutExpired, FileNotFoundError) as exc:
-        raise RuntimeError(f"pip list failed: {exc}") from exc
+    except subprocess.TimeoutExpired as exc:
+        stderr = (exc.stderr or "").strip() if isinstance(exc.stderr, str) else ""
+        detail = f" — stderr: {stderr}" if stderr else ""
+        raise RuntimeError(f"pip list timed out after {exc.timeout}s{detail}") from exc
+    except FileNotFoundError as exc:
+        raise RuntimeError(f"pip list failed: {exc}") from exc

As per coding guidelines, "Anything with output must follow docs/standards/logging-observability.md standards."

🤖 Prompt for AI Agents

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

In `@tools/generate_sbom.py` around lines 46 - 47, The except block currently
catches both subprocess.TimeoutExpired and FileNotFoundError and raises a
generic RuntimeError; split or branch the handling so that when
subprocess.TimeoutExpired is caught you include exc.stdout and exc.stderr (or at
least exc.stderr) in the RuntimeError message for better diagnostics, e.g.,
detect isinstance(exc, subprocess.TimeoutExpired) and raise RuntimeError(f"pip
list timed out: {exc}; stdout={exc.stdout}; stderr={exc.stderr}") from exc,
while preserving the existing message for FileNotFoundError; update the except
handling around the subprocess call in generate_sbom.py where you raise
RuntimeError to include these fields.

🤖 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/subcommands/_approve.py`:
- Around line 135-160: The code passes the raw dict from json.load (webhook_cfg)
into WebhookNotifier but WebhookNotifier expects attribute access (e.g.,
self.config.notify_on_success), so wrap/convert the dict into an object with
attribute-style access before constructing WebhookNotifier; update the _Carrier
class (used in _build_approval_notifier) to accept either None or a dict and set
attributes (notify_on_success, notify_on_failure, url, etc.) from the dict (or
use types.SimpleNamespace/dataclass) so attribute lookups won't raise
AttributeError, and apply the same conversion for the other locations that pass
webhook_cfg into WebhookNotifier (the other _Carrier/constructor usages around
the approve/reject helpers).

In `@forgelm/trainer.py`:
- Around line 993-1006: When require_human_approval is true the code saves the
model to gate_path (final_path + ".staging") but leaves
TrainResult.final_model_path pointing at the intended final location, which
misreports the saved path; change the assignment so that after calling
self.save_final_model(gate_path) you set train_result.final_model_path =
gate_path (and still set train_result.staging_path = gate_path) so downstream
reporters see the actual on-disk location; apply the same adjustment to the
analogous block around the code at lines 1027-1028 that handles
approval-required saving.

In `@tests/test_model_moe_naming.py`:
- Around line 107-119: The test's dedup logging check is unreliable because the
second call runs outside the caplog.at_level context and the initial count is
never asserted; wrap both calls to _expert_index_in_name inside the same with
caplog.at_level(logging.INFO, logger="forgelm.model") block (use the existing
caplog context around calls to _expert_index_in_name), capture the first
occurrence and assert caplog.text.count("Unrecognized MoE expert parameter
naming") == 1 to validate "exactly once", then call caplog.clear() still inside
that context, invoke _expert_index_in_name a second time, and assert that
"Unrecognized MoE expert parameter naming" is not present (count == 0) to verify
the dedup behavior; reference the function _expert_index_in_name and the caplog
logger "forgelm.model" when locating the code to change.

In `@tools/generate_sbom.py`:
- Around line 60-69: The _component function currently builds purls from
possibly missing fields; validate that pkg contains non-empty "name" and
"version" before calling _purl: if either is missing or blank, raise a clear,
standardized error (e.g., ValueError) that includes the offending pkg dict so
callers can handle it per the project's error-handling standards; update
_component to perform this check and raise the error rather than producing
malformed purls.

---

Nitpick comments:
In `@tools/generate_sbom.py`:
- Around line 46-47: The except block currently catches both
subprocess.TimeoutExpired and FileNotFoundError and raises a generic
RuntimeError; split or branch the handling so that when
subprocess.TimeoutExpired is caught you include exc.stdout and exc.stderr (or at
least exc.stderr) in the RuntimeError message for better diagnostics, e.g.,
detect isinstance(exc, subprocess.TimeoutExpired) and raise RuntimeError(f"pip
list timed out: {exc}; stdout={exc.stdout}; stderr={exc.stderr}") from exc,
while preserving the existing message for FileNotFoundError; update the except
handling around the subprocess call in generate_sbom.py where you raise
RuntimeError to include these fields.

🪄 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: d2fdeac0-c93f-4ef6-a045-25d395273259

📥 Commits

Reviewing files that changed from the base of the PR and between 536be49 and 806f1fc.

📒 Files selected for processing (11)

• forgelm/cli/_dispatch.py
• forgelm/cli/_training.py
• forgelm/cli/subcommands/_approve.py
• forgelm/cli/subcommands/_ingest.py
• forgelm/cli/subcommands/_quickstart.py
• forgelm/data_audit/_croissant.py
• forgelm/trainer.py
• tests/test_cli_helpers.py
• tests/test_cli_quickstart_wiring.py
• tests/test_model_moe_naming.py
• tools/generate_sbom.py

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

• forgelm/data_audit/_croissant.py

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

• forgelm/cli/_dispatch.py

[coderabbitai]

Actionable comments posted: 4

♻️ Duplicate comments (2)

forgelm/cli/_dispatch.py (1)

150-155: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep the missing-config JSON error on stdout, and match the normal envelope.

Every other --output-format json error path in the provided CLI files emits {"success": false, "error": ...} to stdout. This branch switches both the stream and the payload shape, so callers that parse stdout get no machine-readable error on one of the earliest validation failures.

Suggested fix

     if not args.config:
         if getattr(args, "output_format", "text") == "json":
-            print(json.dumps({"error": "--config is required.", "code": EXIT_CONFIG_ERROR}), file=sys.stderr)
+            print(json.dumps({"success": False, "error": "--config is required. Use --help for usage."}))
         else:
             print("Error: --config is required. Use --help for usage.", file=sys.stderr)
         sys.exit(EXIT_CONFIG_ERROR)

🤖 Prompt for AI Agents

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

In `@forgelm/cli/_dispatch.py` around lines 150 - 155, The missing-config branch
currently prints a non-enveloped message to stderr when args.config is absent;
change it to print the JSON envelope {"success": false, "error": "...", "code":
EXIT_CONFIG_ERROR} to stdout when getattr(args, "output_format", "text") ==
"json" (matching other CLI error paths), and keep the human-readable message to
stderr for non-json output; update the print call that checks
args.config/output_format to write the enveloped JSON to stdout and preserve
EXIT_CONFIG_ERROR for sys.exit.

forgelm/trainer.py (1)

1002-1005: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Don't recycle a shared .staging directory across runs.

gate_path is stable for a given output_dir. Deleting it here lets a later gated run replace an earlier run's staged artifacts, so forgelm approve <old run_id> can end up promoting the newer model from the same path. That breaks the human-approval audit chain even though the directory is “clean.”

Suggested fix

             gate_path = final_path + ".staging"
-            if os.path.isdir(gate_path):
-                shutil.rmtree(gate_path)
+            if os.path.lexists(gate_path):
+                raise FileExistsError(
+                    f"Refusing to reuse existing staging directory: {gate_path}. "
+                    "Approve, reject, or remove the previous staged run first."
+                )
             self.save_final_model(gate_path)

🤖 Prompt for AI Agents

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

In `@forgelm/trainer.py` around lines 1002 - 1005, The current code deletes a
shared gate_path (final_path + ".staging") which allows later runs to overwrite
earlier runs' staged artifacts; instead generate a unique per-run staging
directory and avoid removing the shared ".staging". Change the gate_path
computation to include a run-unique suffix (e.g., run_id, timestamp, or uuid4)
so gate_path = final_path + ".staging.<run_id_or_uuid>", write the staged model
with self.save_final_model(gate_path), and remove the unconditional
shutil.rmtree(gate_path) call; if you need cleanup, only remove the specific
run-unique staging directory and never a global ".staging". Ensure any
approval/promotion logic references the same unique staging path for that run.

🤖 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/subcommands/_approve.py`:
- Around line 193-207: After locating required_event via
_cli_facade._find_human_approval_required_event, also check the audit log for
any terminal decision events for the same run (e.g. "human_approval.rejected" or
"human_approval.granted"); if any such decision event exists, call
_output_error_and_exit to refuse promotion and report the conflicting terminal
decision for run_id. Implement this by adding a lookup (or new helper on
_cli_facade like _find_human_approval_decision_event) immediately after
required_event is found and before computing staging_path/final_path, and use a
clear error message referencing the found decision event.

In `@forgelm/cli/subcommands/_quickstart.py`:
- Around line 125-129: Wrap the subprocess.run(train_cmd, check=False) call in a
try/except that catches OSError (and optionally subprocess.SubprocessError) so
executable launch failures don't propagate; on exception log the error including
exception details, map the resulting failure to the public exit-code contract
using the same _PUBLIC_EXIT_CODES/EXIT_TRAINING_ERROR logic (e.g., set exit_code
= EXIT_TRAINING_ERROR unless the exception provides a code present in
_PUBLIC_EXIT_CODES) and then call sys.exit(exit_code); keep the existing
behavior for non-exceptional runs by preserving train_rc and the current mapping
to _PUBLIC_EXIT_CODES/EXIT_TRAINING_ERROR.

In `@forgelm/data_audit/_croissant.py`:
- Around line 64-198: The metadata builder emits raw path-derived identifiers
(file_id, split_name, column, url_safe) which can contain spaces, path
separators, or '.' and break JSON-LD ids/URLs; add a small normalization step
inside _build_croissant_metadata: implement a sanitize/slug function (e.g.
re.sub to replace/strip unsafe chars, collapse whitespace to '_', remove path
separators, and map "."/empty to a sensible fallback) and apply it to file_id,
split_name (when used for _JSONLD_ID_KEY), each column when building
_JSONLD_ID_KEY and contentUrl, and to url_safe before insertion; keep column
JSONPath escaping as-is but use the sanitized column for ids/urls so all emitted
`@id/contentUrl/url` values are safe and deterministic.

In `@tools/check_site_claims.py`:
- Around line 398-402: The current condition accepts any older released version
because it allows m_t[3] == 0 and m_t <= code_t, so sites on v0.1.0 pass when
pyproject is 0.6.0; change the logic in the branch that builds bad (the if using
m_t and code_t) to only accept released versions that match the release identity
of the code (e.g., require the release tuple equality), e.g., replace the
condition with one that checks m_t[3] == 0 and m_t[:3] == code_t[:3] (or
otherwise require m_t == code_t) so only the same released version is treated as
valid. Ensure you update the if that currently reads "if m_t[3] == 0 and m_t <=
code_t:" to the stricter check using m_t[:3] and code_t[:3].

---

Duplicate comments:
In `@forgelm/cli/_dispatch.py`:
- Around line 150-155: The missing-config branch currently prints a
non-enveloped message to stderr when args.config is absent; change it to print
the JSON envelope {"success": false, "error": "...", "code": EXIT_CONFIG_ERROR}
to stdout when getattr(args, "output_format", "text") == "json" (matching other
CLI error paths), and keep the human-readable message to stderr for non-json
output; update the print call that checks args.config/output_format to write the
enveloped JSON to stdout and preserve EXIT_CONFIG_ERROR for sys.exit.

In `@forgelm/trainer.py`:
- Around line 1002-1005: The current code deletes a shared gate_path (final_path
+ ".staging") which allows later runs to overwrite earlier runs' staged
artifacts; instead generate a unique per-run staging directory and avoid
removing the shared ".staging". Change the gate_path computation to include a
run-unique suffix (e.g., run_id, timestamp, or uuid4) so gate_path = final_path
+ ".staging.<run_id_or_uuid>", write the staged model with
self.save_final_model(gate_path), and remove the unconditional
shutil.rmtree(gate_path) call; if you need cleanup, only remove the specific
run-unique staging directory and never a global ".staging". Ensure any
approval/promotion logic references the same unique staging path for that run.

🪄 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: d9f784e3-7aee-46d0-9458-be5dd230cada

📥 Commits

Reviewing files that changed from the base of the PR and between 536be49 and bea7cb9.

📒 Files selected for processing (13)

• forgelm/cli/_dispatch.py
• forgelm/cli/_training.py
• forgelm/cli/subcommands/_approve.py
• forgelm/cli/subcommands/_ingest.py
• forgelm/cli/subcommands/_quickstart.py
• forgelm/data_audit/_croissant.py
• forgelm/trainer.py
• tests/test_cli_helpers.py
• tests/test_cli_quickstart_wiring.py
• tests/test_faz27_narrow_exceptions.py
• tests/test_model_moe_naming.py
• tools/check_site_claims.py
• tools/generate_sbom.py

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

• tests/test_faz27_narrow_exceptions.py
• tests/test_model_moe_naming.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

This still accepts arbitrarily stale released versions.

The m_t[3] == 0 and m_t <= code_t branch treats any older release as valid, so a site stuck on something like v0.1.0 still passes when pyproject.toml is 0.6.0. That makes --strict too weak to catch real version drift.

🤖 Prompt for AI Agents

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

In `@tools/check_site_claims.py` around lines 398 - 402, The current condition
accepts any older released version because it allows m_t[3] == 0 and m_t <=
code_t, so sites on v0.1.0 pass when pyproject is 0.6.0; change the logic in the
branch that builds bad (the if using m_t and code_t) to only accept released
versions that match the release identity of the code (e.g., require the release
tuple equality), e.g., replace the condition with one that checks m_t[3] == 0
and m_t[:3] == code_t[:3] (or otherwise require m_t == code_t) so only the same
released version is treated as valid. Ensure you update the if that currently
reads "if m_t[3] == 0 and m_t <= code_t:" to the stricter check using m_t[:3]
and code_t[:3].

[cemililik]

By design — not changing. The m_t <= code_t condition intentionally allows the site to display the last stable release badge (e.g. v0.5.0) while pyproject is on a dev-cycle pre-release (e.g. 0.5.1rc1). Changing to >= would flag v0.5.0 as stale the moment a dev-cycle bump is committed, producing false positives on every release cycle.

[coderabbitai]

Rate Limit Exceeded

@cemililik have exceeded the limit for the number of chat messages per hour. Please wait 59 minutes and 15 seconds before sending another message.

[coderabbitai]

Actionable comments posted: 6

🤖 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/subcommands/_approve.py`:
- Around line 247-252: The current final_path logic can wrongly remove only
".staging" inside names like "<final>.staging.<run_id>"; instead, detect and
strip the full staged suffix by locating the last occurrence of _STAGING_SUFFIX
in staging_path (e.g., using rfind) and slicing staging_path up to that index to
remove ".staging" plus any runtime suffix (".<run_id>"); if no trailing runtime
suffix exists fall back to removing the exact _STAGING_SUFFIX when
staging_path.endswith(_STAGING_SUFFIX), otherwise leave staging_path unchanged.
Update the computation around required_event.get("staging_path") / staging_path
and final_path to use this rfind-based strip first, then the legacy
endswith-based strip as a fallback.
- Around line 197-204: The loaded JSON may not be a dict and webhook_cfg may not
be a dict, which can cause attribute errors down the line; after json.load() in
the block that reads config_path, first check isinstance(report, dict) before
calling report.get("webhook_config"), and then ensure webhook_cfg is a dict with
isinstance(webhook_cfg, dict) (otherwise set webhook_cfg = None or an empty
dict) so that the call return WebhookNotifier(_Carrier(webhook_cfg)) always
receives a safe mapping or a None/empty value and degrades to a no-op notifier;
update the code around json.load, report, webhook_cfg, _Carrier, and
WebhookNotifier accordingly and keep the existing exception logging of
logger.debug.

In `@forgelm/cli/subcommands/_quickstart.py`:
- Around line 87-101: The helper _load_quickstart_train_paths is rebuilding the
model path from the seed YAML (training.output_dir + training.final_model_dir)
but trainer now emits runtime-resolved paths (e.g., staging_path and final path
that include run_id), so change _load_quickstart_train_paths to read the
trainer-emitted artifact that contains the resolved model path (instead of
reconstructing it from training.output_dir/final_model_dir): locate the trainer
artifact key (e.g., the emitted "staged_model_path" or "final_model_path")
produced by the training step, open and parse that artifact (yaml/json) to
extract the runtime-resolved path, and return that path (and any fallback) so
quickstart uses the actual runtime model directory rather than a reconstructed
one.
- Around line 154-160: subprocess.run(chat_cmd, check=False) can raise OSError
and currently will abort the quickstart flow; wrap the call in a try/except
OSError block around the chat launch in _quickstart.py (the code building
chat_cmd and calling subprocess.run), catch OSError as e, call logger.warning
with a concise message including the exception (e) and the chat_cmd context, and
then return from the surrounding function instead of letting the exception
propagate so the quickstart remains a best-effort that succeeds even if the REPL
fails to launch.

In `@tools/generate_sbom.py`:
- Around line 52-55: After parsing result.stdout (the json.loads(result.stdout)
call) validate the decoded payload is the expected shape (root is a list and
each item is a dict with required keys such as "name" and "version"); if
validation fails, raise a RuntimeError (the same exception type used now) with a
clear message describing the observed type/shape and include the raw/partial
payload or parsing error for context, and ensure the raised error follows the
repo's error-handling/exit-code standards (use the project helper or map to the
appropriate exit code per docs/standards/error-handling.md).
- Around line 126-128: The catch-all except in main() currently returns 1 for
both RuntimeError and ValueError; update main() (the exception handling block
that catches (RuntimeError, ValueError) as exc) to translate each failure to the
documented exit-code contract (0/1/2/3/4) instead of always returning 1:
distinguish the exception types (e.g., map ValueError to the "invalid input"
exit code and RuntimeError to the "runtime failure" exit code per
docs/standards/error-handling.md), include the error text in the stderr print,
and return or call sys.exit() with the appropriate numeric code. Ensure the
mapping is explicit in the except handlers or via a small helper inside main()
so future exceptions can be mapped consistently.

🪄 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: b37dbc08-e1cc-4efe-9e42-c8ccd1ce15f5

📥 Commits

Reviewing files that changed from the base of the PR and between 536be49 and bea9747.

📒 Files selected for processing (15)

• forgelm/cli/__init__.py
• forgelm/cli/_dispatch.py
• forgelm/cli/_training.py
• forgelm/cli/subcommands/_approve.py
• forgelm/cli/subcommands/_ingest.py
• forgelm/cli/subcommands/_quickstart.py
• forgelm/data_audit/_croissant.py
• forgelm/trainer.py
• tests/test_cli_helpers.py
• tests/test_cli_quickstart_wiring.py
• tests/test_faz27_narrow_exceptions.py
• tests/test_model_moe_naming.py
• tests/test_phase12_5.py
• tools/check_site_claims.py
• tools/generate_sbom.py

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

• tests/test_faz27_narrow_exceptions.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Resolve the chat target from runtime artifacts, not the seed config.

This helper rebuilds the model path from training.output_dir + training.final_model_dir, but the trainer-side flow now emits runtime-specific locations (staging_path is f"{final_path}.staging.{run_id}", and this PR also changes final paths to include run_id). That leaves quickstart looking in the wrong directory after a successful run, so auto-chat can be skipped even when training produced a usable model. Read the runtime-resolved model path from a trainer-emitted artifact instead of reconstructing it from the YAML.

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_quickstart.py` around lines 87 - 101, The helper
_load_quickstart_train_paths is rebuilding the model path from the seed YAML
(training.output_dir + training.final_model_dir) but trainer now emits
runtime-resolved paths (e.g., staging_path and final path that include run_id),
so change _load_quickstart_train_paths to read the trainer-emitted artifact that
contains the resolved model path (instead of reconstructing it from
training.output_dir/final_model_dir): locate the trainer artifact key (e.g., the
emitted "staged_model_path" or "final_model_path") produced by the training
step, open and parse that artifact (yaml/json) to extract the runtime-resolved
path, and return that path (and any fallback) so quickstart uses the actual
runtime model directory rather than a reconstructed one.

[cemililik]

Not applicable. When require_human_approval=True the training subprocess exits with EXIT_AWAITING_APPROVAL (4) which is in _PUBLIC_EXIT_CODES, so _run_quickstart_train_subprocess propagates that exit and the chat launch is never reached. When require_human_approval=False, gate_path = os.path.join(checkpoint_dir, config.training.final_model_dir) — exactly what _load_quickstart_train_paths reconstructs from the YAML.

[coderabbitai]

Rate Limit Exceeded

@cemililik have exceeded the limit for the number of chat messages per hour. Please wait 59 minutes and 11 seconds before sending another message.

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (1)

tools/check_site_claims.py (1)

398-401: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

--strict still permits arbitrarily stale release badges.

This branch accepts any older stable version, so a page stuck on v0.1.0 still passes when pyproject.toml is 0.6.0. That makes the version guard too weak to catch the staleness it claims to enforce.

🤖 Prompt for AI Agents

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

In `@tools/check_site_claims.py` around lines 398 - 401, The current check (if
m_t[3] == 0 and m_t <= code_t: continue) incorrectly allows older stable release
badges; change the comparison so released versions are only accepted if they are
at least as new as the project version. In tools/check_site_claims.py update the
branch that inspects m_t (the parsed badge version tuple) and code_t (pyproject
version tuple) to require m_t >= code_t for m_t[3] == 0 (i.e., use >= instead of
<=) so stale released badges are rejected when strict mode is enabled.

🧹 Nitpick comments (2)

forgelm/cli/subcommands/_ingest.py (1)

17-19: 💤 Low value

Potential AttributeError if pii_mask is missing from args.

Line 18 accesses args.pii_mask directly without getattr(), but line 19 uses getattr(args, "secrets_mask", False). If the parser doesn't register --pii-mask for some path, this would raise AttributeError.

♻️ Suggested fix for consistency

     all_mask = getattr(args, "all_mask", False)
-    pii_mask = bool(args.pii_mask) or all_mask
+    pii_mask = bool(getattr(args, "pii_mask", False)) or all_mask
     secrets_mask = bool(getattr(args, "secrets_mask", False)) or all_mask

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_ingest.py` around lines 17 - 19, The code reading
CLI flags in _ingest.py uses direct attribute access args.pii_mask which can
raise AttributeError; change it to use getattr(args, "pii_mask", False) and then
compute pii_mask = bool(getattr(args, "pii_mask", False)) or all_mask (mirroring
secrets_mask and all_mask logic) so that missing parser flags do not crash the
handler (look for variables all_mask, pii_mask, secrets_mask in the ingest
subcommand handler).

forgelm/cli/subcommands/_quickstart.py (1)

87-101: ⚖️ Poor tradeoff

Past review concern remains applicable: train paths derived from seed config.

The past review noted that _load_quickstart_train_paths() reads from the seed YAML (training.output_dir + training.final_model_dir), but the trainer now emits runtime-specific paths (with run_id in staging/final directories). This can cause quickstart to look in the wrong directory for auto-chat.

The current implementation works for the common case where config paths match runtime paths, but the design gap remains. Consider reading from a trainer-emitted artifact in a future iteration.

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_quickstart.py` around lines 87 - 101,
_load_quickstart_train_paths currently reads seed YAML keys (training.output_dir
and training.final_model_dir) which can differ from runtime paths that include
run-specific suffixes (e.g., run_id). Modify _load_quickstart_train_paths to
first look for a trainer-emitted artifact/metadata (example: a small JSON or
YAML file the trainer writes containing actual runtime paths such as
"output_dir" and "final_model_dir"); if that artifact exists, parse and return
those runtime paths, otherwise fall back to reading the seed YAML as it does
now. Update the function to check for the presence of this artifact and document
the fallback behavior so quickstart reliably finds the trained checkpoint
directory.

🤖 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/subcommands/_approve.py`:
- Around line 239-297: The approve handler does a read-then-act without locking,
so concurrent operators can both see decision_event is None and emit conflicting
terminal events; wrap the entire critical section (from calling
_cli_facade._find_human_approval_decision_event through the state-changing
operations: _cli_facade._atomic_rename_or_move, audit.log_event and
notifier.notify_success) in a mutual-exclusion lock and hold it until after the
audit log write completes. Implement the same locking around the reject handler
(_run_reject_cmd) so both paths serialize access to the audit log; acquire the
lock before calling _find_human_approval_decision_event and release it only
after audit.log_event (and any filesystem moves) succeed or on error, ensuring
the check-and-mutate sequence is atomic.
- Around line 158-170: The _load_metrics_from_manifest function is looking for
"compliance/training_manifest.yaml" but the trainer writes
"compliance/training_manifest.json", so update _load_metrics_from_manifest to
check for and read the JSON file first (e.g., "training_manifest.json") and fall
back to the YAML path; use json.load when reading the JSON and keep the existing
yaml.safe_load fallback and exception handling, returning an empty dict on
errors and ensuring metrics = manifest.get("final_metrics", {}) and returning it
only if it's a dict.

In `@forgelm/trainer.py`:
- Around line 953-960: The log message passed to logger.info incorrectly states
that `forgelm reject` will “discard” staged artifacts; update the message string
used in the `logger.info` call (the log that references `self.checkpoint_dir`
and `self.audit.run_id`) so it correctly describes that `forgelm reject`
preserves the staged artifacts for forensic review (e.g., replace “discard” with
“preserve for forensic review” or similar clear wording).

In `@tests/test_model_moe_naming.py`:
- Around line 90-96: The test test_unicode_digit_is_not_matched leaves the
module-level sentinel _LOGGED_UNKNOWN_EXPERT_NAMES modified by calling
_expert_index_in_name, so wrap the call and assertion in a try/finally and in
the finally block clear _LOGGED_UNKNOWN_EXPERT_NAMES (same cleanup used in
test_unfamiliar_expert_name_logs_info) to ensure the sentinel is reset after the
test; locate test_unicode_digit_is_not_matched and add the try/finally around
the assertion that calls forgelm.model._expert_index_in_name and reset
forgelm.model._LOGGED_UNKNOWN_EXPERT_NAMES in finally.

In `@tools/generate_sbom.py`:
- Around line 64-67: The _purl(name: str, ver: str) helper currently emits
non-canonical PyPI purls because it doesn't normalize package names per PEP 503;
update _purl to normalize the package name before building the purl (e.g., call
packaging.utils.canonicalize_name(name) or apply re.sub(r"[-_.]+", "-",
name).lower()) so runs of dots/underscores/hyphens and case are handled; ensure
the function returns f"pkg:pypi/{normalized_name}@{ver}" and add the packaging
import if using canonicalize_name.

---

Duplicate comments:
In `@tools/check_site_claims.py`:
- Around line 398-401: The current check (if m_t[3] == 0 and m_t <= code_t:
continue) incorrectly allows older stable release badges; change the comparison
so released versions are only accepted if they are at least as new as the
project version. In tools/check_site_claims.py update the branch that inspects
m_t (the parsed badge version tuple) and code_t (pyproject version tuple) to
require m_t >= code_t for m_t[3] == 0 (i.e., use >= instead of <=) so stale
released badges are rejected when strict mode is enabled.

---

Nitpick comments:
In `@forgelm/cli/subcommands/_ingest.py`:
- Around line 17-19: The code reading CLI flags in _ingest.py uses direct
attribute access args.pii_mask which can raise AttributeError; change it to use
getattr(args, "pii_mask", False) and then compute pii_mask = bool(getattr(args,
"pii_mask", False)) or all_mask (mirroring secrets_mask and all_mask logic) so
that missing parser flags do not crash the handler (look for variables all_mask,
pii_mask, secrets_mask in the ingest subcommand handler).

In `@forgelm/cli/subcommands/_quickstart.py`:
- Around line 87-101: _load_quickstart_train_paths currently reads seed YAML
keys (training.output_dir and training.final_model_dir) which can differ from
runtime paths that include run-specific suffixes (e.g., run_id). Modify
_load_quickstart_train_paths to first look for a trainer-emitted
artifact/metadata (example: a small JSON or YAML file the trainer writes
containing actual runtime paths such as "output_dir" and "final_model_dir"); if
that artifact exists, parse and return those runtime paths, otherwise fall back
to reading the seed YAML as it does now. Update the function to check for the
presence of this artifact and document the fallback behavior so quickstart
reliably finds the trained checkpoint directory.

🪄 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: 8ccfff0c-5c7e-431e-8ad7-1fc5590bd5ed

📥 Commits

Reviewing files that changed from the base of the PR and between 536be49 and bc1e783.

📒 Files selected for processing (15)

• forgelm/cli/__init__.py
• forgelm/cli/_dispatch.py
• forgelm/cli/_training.py
• forgelm/cli/subcommands/_approve.py
• forgelm/cli/subcommands/_ingest.py
• forgelm/cli/subcommands/_quickstart.py
• forgelm/data_audit/_croissant.py
• forgelm/trainer.py
• tests/test_cli_helpers.py
• tests/test_cli_quickstart_wiring.py
• tests/test_faz27_narrow_exceptions.py
• tests/test_model_moe_naming.py
• tests/test_phase12_5.py
• tools/check_site_claims.py
• tools/generate_sbom.py

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

• forgelm/cli/_training.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Load the manifest file the trainer actually writes.

The trainer exports compliance/training_manifest.json, so this .yaml lookup always misses and approve notifies success with an empty metrics payload.

💡 Suggested fix

 def _load_metrics_from_manifest(output_dir: str) -> dict:
-    """Read final metrics from ``compliance/training_manifest.yaml`` if present."""
-    manifest_path = os.path.join(output_dir, "compliance", "training_manifest.yaml")
+    """Read final metrics from ``compliance/training_manifest.json`` if present."""
+    manifest_path = os.path.join(output_dir, "compliance", "training_manifest.json")
     if not os.path.isfile(manifest_path):
         return {}
     try:
         with open(manifest_path, "r", encoding="utf-8") as fh:
-            manifest = yaml.safe_load(fh) or {}
-    except (OSError, yaml.YAMLError) as exc:
+            manifest = json.load(fh)
+    except (OSError, json.JSONDecodeError) as exc:
         logger.warning("Could not read training manifest at %s: %s", manifest_path, exc)
         return {}
-    metrics = manifest.get("final_metrics", {})
+    metrics = manifest.get("final_metrics", {}) if isinstance(manifest, dict) else {}
     return metrics if isinstance(metrics, dict) else {}

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_approve.py` around lines 158 - 170, The
_load_metrics_from_manifest function is looking for
"compliance/training_manifest.yaml" but the trainer writes
"compliance/training_manifest.json", so update _load_metrics_from_manifest to
check for and read the JSON file first (e.g., "training_manifest.json") and fall
back to the YAML path; use json.load when reading the JSON and keep the existing
yaml.safe_load fallback and exception handling, returning an empty dict on
errors and ensuring metrics = manifest.get("final_metrics", {}) and returning it
only if it's a dict.

[cemililik]

Not applicable. compliance.py line 876 writes training_manifest.yaml (YAML, via yaml.dump). _load_metrics_from_manifest correctly reads that file. The training_manifest.json mention in trainer.py's docstring is a stale comment error — the actual on-disk artifact is .yaml.

[coderabbitai]

Rate Limit Exceeded

@cemililik have exceeded the limit for the number of chat messages per hour. Please wait 59 minutes and 5 seconds before sending another message.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

Serialize terminal decisions before mutating state.

Both handlers do a read-then-act check against audit_log.jsonl, but neither takes a lock before renaming/logging. Two operators can still pass decision_event is None concurrently, and an approve/reject race can emit conflicting terminal events for the same run_id.

💡 Suggested direction

+import contextlib
 ...
+def _acquire_human_approval_lock(output_dir: str, run_id: str) -> str:
+    lock_path = os.path.join(output_dir, f".human_approval.{run_id}.lock")
+    fd = os.open(lock_path, os.O_CREAT | os.O_EXCL | os.O_WRONLY)
+    os.close(fd)
+    return lock_path
 ...
-    decision_event = _cli_facade._find_human_approval_decision_event(audit_log_path, run_id)
-    if decision_event is not None:
-        ...
+    try:
+        lock_path = _acquire_human_approval_lock(output_dir, run_id)
+    except FileExistsError:
+        _output_error_and_exit(
+            output_format,
+            f"Another approve/reject operation is already in progress for run {run_id!r}.",
+            EXIT_CONFIG_ERROR,
+        )
+
+    try:
+        decision_event = _cli_facade._find_human_approval_decision_event(audit_log_path, run_id)
+        if decision_event is not None:
+            ...
+        # keep the lock held through rename/log_event
+    finally:
+        with contextlib.suppress(OSError):
+            os.unlink(lock_path)

Apply the same lock around both _run_approve_cmd() and _run_reject_cmd().

Also applies to: 333-368

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_approve.py` around lines 239 - 297, The approve
handler does a read-then-act without locking, so concurrent operators can both
see decision_event is None and emit conflicting terminal events; wrap the entire
critical section (from calling _cli_facade._find_human_approval_decision_event
through the state-changing operations: _cli_facade._atomic_rename_or_move,
audit.log_event and notifier.notify_success) in a mutual-exclusion lock and hold
it until after the audit log write completes. Implement the same locking around
the reject handler (_run_reject_cmd) so both paths serialize access to the audit
log; acquire the lock before calling _find_human_approval_decision_event and
release it only after audit.log_event (and any filesystem moves) succeed or on
error, ensuring the check-and-mutate sequence is atomic.

[cemililik]

Acknowledged — the race window is real, but a threading.Lock has no effect across separate OS processes (two operators running forgelm approve simultaneously). Proper serialization needs a cross-process file lock: fcntl.flock is Unix-only and filelock is not in stdlib. The primary guard is the atomicity of os.rename on POSIX — the second concurrent rename gets ENOENT and routes to EXIT_TRAINING_ERROR. Deferring the file-lock mechanism to a dedicated ticket to evaluate the right cross-platform approach.

[coderabbitai]

Rate Limit Exceeded

@cemililik have exceeded the limit for the number of chat messages per hour. Please wait 59 minutes and 4 seconds before sending another message.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (2)

forgelm/cli/subcommands/_approve.py (2)

158-170: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Load the trainer’s JSON manifest before falling back to YAML.

ForgeTrainer._export_compliance_if_needed() documents compliance/training_manifest.json as the emitted artifact, so this YAML-only lookup returns {} on the normal approve path and notify_success() drops the final metrics payload. Prefer JSON first and keep YAML only as a backward-compatible fallback.

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_approve.py` around lines 158 - 170, The
_load_metrics_from_manifest function currently only reads
training_manifest.yaml, causing final metrics to be lost; modify
_load_metrics_from_manifest to first try loading JSON from
"compliance/training_manifest.json" (use json.load with proper encoding and
except (OSError, json.JSONDecodeError) handling and a warning log similar to the
existing one), and if that yields no dict or file is missing/fails, fall back to
reading "compliance/training_manifest.yaml" exactly as currently implemented;
always extract manifest.get("final_metrics", {}) and return it only if
isinstance(metrics, dict) else {} and preserve the existing logger warnings for
both JSON and YAML read failures.

---

239-297: ⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

Serialize approve/reject decisions before mutating state.

Both handlers still do a read-then-act check against audit_log.jsonl without a shared lock. An approve/reject race can have both processes observe “no terminal decision yet” and then append conflicting human_approval.granted and human_approval.rejected events for the same run_id.

Also applies to: 333-368

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_approve.py` around lines 239 - 297, The
approve/reject flow does a racey read-then-act against the audit log (via
_find_human_approval_decision_event and later audit.log_event), allowing
concurrent processes to both decide; serialize by taking a shared exclusive lock
spanning the decision check, any promotion/mutation, and the append of the
terminal event. Concretely: wrap the sequence that calls
_find_human_approval_decision_event, checks/stops on an existing decision,
performs the promotion (_atomic_rename_or_move) and then audit.log_event in a
single exclusive file lock on the audit log (audit_log.jsonl) so only one
process can read+append; if your AuditLogger exposes a lock method use that
(e.g. with AuditLogger(...).acquire_lock(): ...), otherwise add a small helper
that acquires an OS file lock (flock/portalocker) on audit.audit_log_path and
use it around the check+mutate+log; ensure the lock is released in finally and
convert the earlier separate checks (including the other handler at the other
location) to the same locked pattern.

🤖 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/subcommands/_quickstart.py`:
- Around line 232-237: The code currently calls _emit_quickstart_result(result,
output_format) before running _run_quickstart_train_then_chat(...), which emits
a final-success JSON even if the subsequent training fails; move the
JSON-success emission so it runs only after _run_quickstart_train_then_chat
completes (i.e., call _emit_quickstart_result(...) after
_run_quickstart_train_then_chat when result.dry_run is false), or alternatively
change the pre-training call to emit an explicit intermediate payload (e.g.,
include an "intermediate": true field) instead of "success": true; update the
logic around result.dry_run and output_format "json" accordingly so callers
never receive a final success envelope before training finishes.

In `@forgelm/trainer.py`:
- Around line 1002-1008: gate_path can be relative (inherited from
training.output_dir) which breaks approval logging and CLI lookup; before
calling save_final_model and before assigning train_result.final_model_path and
train_result.staging_path, canonicalize to an absolute/normalized path (e.g.,
resolve with os.path.abspath or pathlib.Path(...).resolve()) so the value stored
and later read by _handle_human_approval_gate() and
required_event["staging_path"] is absolute; update the code around gate_path,
save_final_model, and the assignments to train_result to use the normalized
absolute path.

In `@tools/generate_sbom.py`:
- Around line 36-42: The subprocess.run call using sys.executable is flagged by
Codacy/Bandit despite being safe; update the subprocess.run invocation (the one
calling [sys.executable, "-m", "pip", ...]) to include a Bandit/Codacy
suppression (e.g., append "# nosec" to that line) and add a short inline comment
explaining why this use of sys.executable is safe (all args are literals and
controlled). This targets the subprocess.run call and sys.executable reference
so the static checker will be satisfied while preserving the current behavior.

---

Duplicate comments:
In `@forgelm/cli/subcommands/_approve.py`:
- Around line 158-170: The _load_metrics_from_manifest function currently only
reads training_manifest.yaml, causing final metrics to be lost; modify
_load_metrics_from_manifest to first try loading JSON from
"compliance/training_manifest.json" (use json.load with proper encoding and
except (OSError, json.JSONDecodeError) handling and a warning log similar to the
existing one), and if that yields no dict or file is missing/fails, fall back to
reading "compliance/training_manifest.yaml" exactly as currently implemented;
always extract manifest.get("final_metrics", {}) and return it only if
isinstance(metrics, dict) else {} and preserve the existing logger warnings for
both JSON and YAML read failures.
- Around line 239-297: The approve/reject flow does a racey read-then-act
against the audit log (via _find_human_approval_decision_event and later
audit.log_event), allowing concurrent processes to both decide; serialize by
taking a shared exclusive lock spanning the decision check, any
promotion/mutation, and the append of the terminal event. Concretely: wrap the
sequence that calls _find_human_approval_decision_event, checks/stops on an
existing decision, performs the promotion (_atomic_rename_or_move) and then
audit.log_event in a single exclusive file lock on the audit log
(audit_log.jsonl) so only one process can read+append; if your AuditLogger
exposes a lock method use that (e.g. with AuditLogger(...).acquire_lock(): ...),
otherwise add a small helper that acquires an OS file lock (flock/portalocker)
on audit.audit_log_path and use it around the check+mutate+log; ensure the lock
is released in finally and convert the earlier separate checks (including the
other handler at the other location) to the same locked pattern.

🪄 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: ffc9a9b9-95c1-4922-826c-695ea12ce24b

📥 Commits

Reviewing files that changed from the base of the PR and between 536be49 and 53ca318.

📒 Files selected for processing (15)

• forgelm/cli/__init__.py
• forgelm/cli/_dispatch.py
• forgelm/cli/_training.py
• forgelm/cli/subcommands/_approve.py
• forgelm/cli/subcommands/_ingest.py
• forgelm/cli/subcommands/_quickstart.py
• forgelm/data_audit/_croissant.py
• forgelm/trainer.py
• tests/test_cli_helpers.py
• tests/test_cli_quickstart_wiring.py
• tests/test_faz27_narrow_exceptions.py
• tests/test_model_moe_naming.py
• tests/test_phase12_5.py
• tools/check_site_claims.py
• tools/generate_sbom.py

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

• forgelm/cli/subcommands/_ingest.py
• tests/test_model_moe_naming.py
• tests/test_faz27_narrow_exceptions.py

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

• forgelm/cli/_training.py
• forgelm/data_audit/_croissant.py
• tools/check_site_claims.py

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (1)

forgelm/cli/subcommands/_approve.py (1)

92-92: ⚡ Quick win

Extract event-name constants to eliminate the "human_approval.rejected" / "human_approval.granted" literal duplication (SonarCloud finding).

The literals appear three times for "human_approval.rejected" (lines 92, 383, 392) and twice for "human_approval.granted" (lines 92, 305). A typo in any occurrence would silently produce a wrong event name.

♻️ Proposed refactor — introduce module-level event-name constants

+_EVT_HUMAN_APPROVAL_GRANTED = "human_approval.granted"
+_EVT_HUMAN_APPROVAL_REJECTED = "human_approval.rejected"

-_TERMINAL_DECISION_EVENTS = frozenset({"human_approval.granted", "human_approval.rejected"})
+_TERMINAL_DECISION_EVENTS = frozenset({_EVT_HUMAN_APPROVAL_GRANTED, _EVT_HUMAN_APPROVAL_REJECTED})

Then replace the raw strings at the three remaining call sites:

-    audit.log_event(
-        "human_approval.granted",
+    audit.log_event(
+        _EVT_HUMAN_APPROVAL_GRANTED,

-    audit.log_event(
-        "human_approval.rejected",
+    audit.log_event(
+        _EVT_HUMAN_APPROVAL_REJECTED,

-    reason = f"human_approval.rejected: {args.comment}" if args.comment else "human_approval.rejected"
+    reason = f"{_EVT_HUMAN_APPROVAL_REJECTED}: {args.comment}" if args.comment else _EVT_HUMAN_APPROVAL_REJECTED

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_approve.py` at line 92, Introduce module-level
constants for the two event names (e.g., HUMAN_APPROVAL_GRANTED =
"human_approval.granted" and HUMAN_APPROVAL_REJECTED =
"human_approval.rejected"), update the existing _TERMINAL_DECISION_EVENTS to use
these constants instead of raw strings, and replace all remaining raw
occurrences of those literals with the new constants (the other call sites
referencing "human_approval.granted" and "human_approval.rejected" in this
module). Ensure you reference the new constants where code currently uses the
string literals so any future typo will be avoided.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/analysis/code_reviews/closure-plan-202604300906.md`:
- Line 110: The Phase 33 prose is inconsistent with the updated table: update
the Phase 33 section body (the paragraph that currently reads "Tüm önceki fazlar
(1-32)") to match the table entry by changing it to "Tüm önceki fazlar (1-32,
34-38)"; locate the Phase 33 heading/section in the doc and replace the old
dependency string so the prose and the table are identical.
- Around line 952-968: The docs reference an absent companion file
ghost-features-analysis-20260502.md causing dead links; fix by either
adding/committing that missing file with the full 28-entry ghost-feature
analysis (matching the summary cited in closure-plan-202604300906.md) or update
the references in closure-plan-202604300906.md (and the other occurrences at the
same reference points) to inline the summarized content or point to an existing
alternative source; ensure the chosen resolution keeps the same anchor text and
any cross-reference IDs used by the document so links and the table of ghost
entries remain valid.

In `@forgelm/cli/subcommands/_approve.py`:
- Around line 95-124: The function _find_human_approval_decision_event currently
ignores malformed audit log lines without tracking them; add a skipped_lines
counter (like in _find_human_approval_required_event), increment it whenever a
json.JSONDecodeError is caught and whenever an event parsed is not a dict, and
after looping emit a logger.warning if skipped_lines > 0 that reports the number
of skipped lines and the audit_log_path; keep existing behavior of returning
latest_decision but ensure skipped_lines is initialized before reading and
updated in both error branches.
- Around line 276-296: The AuditLogger is being constructed after calling
_cli_facade._atomic_rename_or_move, so if AuditLogger.__init__ raises
ConfigError the model is already promoted without any audit entry; move the
AuditLogger initialization (create audit = AuditLogger(output_dir,
run_id=run_id)) to before calling _atomic_rename_or_move, keep the same
ConfigError handling that calls _output_error_and_exit on failure, and only
proceed to call _cli_facade._atomic_rename_or_move(staging_path, final_path) if
AuditLogger was successfully constructed so promotion and audit logging are
consistent.

In `@forgelm/cli/subcommands/_quickstart.py`:
- Around line 215-232: The try/except around run_quickstart only catches
FileNotFoundError/FileExistsError so other filesystem-related OSErrors (e.g.,
PermissionError, IsADirectoryError or generic OSError) escape; update the
exception clause in the block that calls run_quickstart to catch OSError (or
include PermissionError/IsADirectoryError) so all filesystem errors from
run_quickstart and helpers like _resolve_dataset are handled the same way,
returning the existing error path that prints JSON when output_format == "json"
or logs via logger.error and exits with EXIT_CONFIG_ERROR.

In `@tools/generate_sbom.py`:
- Around line 110-116: The SBOM currently emits metadata.tools as an array (the
deprecated CycloneDX ≤1.4 format); update the generator in generate_sbom.py so
that metadata["tools"] is an object using the CycloneDX 1.5 shape (e.g.,
metadata["tools"] = {"components": [ { "vendor": "ForgeLM", "name":
"generate_sbom.py", "version": "1.0.0" } ] } or use "services" if appropriate)
instead of a plain array, ensuring specVersion remains "1.5" and preserving the
same tool fields inside the components entries.

---

Nitpick comments:
In `@forgelm/cli/subcommands/_approve.py`:
- Line 92: Introduce module-level constants for the two event names (e.g.,
HUMAN_APPROVAL_GRANTED = "human_approval.granted" and HUMAN_APPROVAL_REJECTED =
"human_approval.rejected"), update the existing _TERMINAL_DECISION_EVENTS to use
these constants instead of raw strings, and replace all remaining raw
occurrences of those literals with the new constants (the other call sites
referencing "human_approval.granted" and "human_approval.rejected" in this
module). Ensure you reference the new constants where code currently uses the
string literals so any future typo will be avoided.

🪄 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: f9fb7e28-c971-41f8-8eb3-96c600babbde

📥 Commits

Reviewing files that changed from the base of the PR and between 536be49 and be7d656.

📒 Files selected for processing (18)

• docs/analysis/code_reviews/closure-plan-202604300906.md
• forgelm/cli/__init__.py
• forgelm/cli/_dispatch.py
• forgelm/cli/_training.py
• forgelm/cli/subcommands/_approve.py
• forgelm/cli/subcommands/_ingest.py
• forgelm/cli/subcommands/_quickstart.py
• forgelm/data_audit/_croissant.py
• forgelm/model.py
• forgelm/trainer.py
• tests/test_cli_helpers.py
• tests/test_cli_quickstart_wiring.py
• tests/test_faz27_narrow_exceptions.py
• tests/test_human_approval_gate.py
• tests/test_model_moe_naming.py
• tests/test_phase12_5.py
• tools/check_site_claims.py
• tools/generate_sbom.py

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

• tests/test_model_moe_naming.py

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

• forgelm/cli/subcommands/_ingest.py
• forgelm/cli/_training.py
• tests/test_human_approval_gate.py
• tests/test_faz27_narrow_exceptions.py
• tools/check_site_claims.py
• forgelm/cli/_dispatch.py
• forgelm/trainer.py

[coderabbitai]

Actionable comments posted: 8

Caution

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

⚠️ Outside diff range comments (2)

tests/test_phase12_5.py (2)

578-606: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert the real filename fields here, not just the slugged IDs.

This test says it protects the alias-layout filename behavior, but it only checks @id. Add name/contentUrl assertions so a regression that rewrites the referenced filename still fails here.

Suggested fix

-        ids = {entry["@id"] for entry in crois["distribution"]}
-        assert ids == {"train_jsonl", "dev_jsonl"}, (
+        expected = {
+            "train_jsonl": "train.jsonl",
+            "dev_jsonl": "dev.jsonl",
+        }
+        ids = {entry["@id"] for entry in crois["distribution"]}
+        assert ids == set(expected), (
             "alias layout must keep the real filename (slugged for JSON-LD), not the canonical split label"
         )
+        for entry in crois["distribution"]:
+            assert entry["name"] == expected[entry["@id"]]
+            assert entry["contentUrl"] == expected[entry["@id"]]

🤖 Prompt for AI Agents

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

In `@tests/test_phase12_5.py` around lines 578 - 606, The test
test_alias_layout_keeps_real_filename currently only asserts the slugged `@id`
values; update it to also assert that each distribution entry's "name" and
"contentUrl" reference the real filenames (e.g., "train.jsonl" and "dev.jsonl")
rather than canonical split labels. After loading crois = json.loads(...
)["croissant"], iterate or map over crois["distribution"] entries and assert
their "name" equals the original file basename and that "contentUrl" ends with
or contains that basename (and not the canonical label), so regressions that
rewrite referenced filenames will fail; keep checks scoped to the distribution
entries where ids were previously validated.

---

272-284: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Patch the Presidio class sentinel too.

_HAS_PRESIDIO=False alone does not fully model the missing-extra path. If _require_presidio() starts keying off _PresidioAnalyzer instead of the boolean, this test could still pass without exercising the failure mode you want to lock down.

Suggested fix

-        with patch("forgelm.data_audit._optional._HAS_PRESIDIO", False):
+        with patch("forgelm.data_audit._optional._HAS_PRESIDIO", False), \
+             patch("forgelm.data_audit._optional._PresidioAnalyzer", None):
             with pytest.raises(ImportError) as exc_info:
                 _require_presidio()

🤖 Prompt for AI Agents

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

In `@tests/test_phase12_5.py` around lines 272 - 284, Update the test
test_presidio_missing_extra_raises_with_install_hint so it simulates the missing
Presidio extra by patching both forgelm.data_audit._optional._HAS_PRESIDIO to
False and the Presidio sentinel (for example _PresidioAnalyzer) to a
falsy/missing state; specifically, in the context where you currently patch
_optional._HAS_PRESIDIO, also patch the symbol _PresidioAnalyzer (imported in
forgelm.data_audit) to None or a sentinel indicating absence before calling
_require_presidio(), so the failure path in _require_presidio() is exercised
regardless of whether it checks the boolean or the class symbol.

🧹 Nitpick comments (1)

tools/generate_sbom.py (1)

149-160: ⚡ Quick win

json.dump/sys.stdout.write are unguarded — BrokenPipeError bypasses the exit-code contract.

BrokenPipeError (a subclass of OSError, not RuntimeError) is raised by json.dump or sys.stdout.write("\n") whenever a downstream consumer closes the pipe early — e.g., python generate_sbom.py | jq . or python generate_sbom.py | head. This is identical to the bug CPython fixed in json.tool (bpo-39828). The exception is not caught by either except ValueError or except RuntimeError, so it escapes main() entirely and produces a noisy traceback instead of a clean, contract-compliant exit code.

Additionally, Line 160 uses the magic literal 0 instead of the _EXIT_SUCCESS constant defined on Line 144, breaking the established naming pattern.

🛠️ Proposed fix

 def main() -> int:
     try:
         sbom = build_sbom()
     except ValueError as exc:
         print(f"error: invalid input — {exc}", file=sys.stderr)
         return _EXIT_CONFIG_ERROR
     except RuntimeError as exc:
         print(f"error: {exc}", file=sys.stderr)
         return _EXIT_RUNTIME_ERROR
-    json.dump(sbom, sys.stdout, indent=2, sort_keys=False)
-    sys.stdout.write("\n")
-    return 0
+    try:
+        json.dump(sbom, sys.stdout, indent=2, sort_keys=False)
+        sys.stdout.write("\n")
+    except OSError as exc:
+        # Covers BrokenPipeError (piped to head/jq that closes early) and other
+        # I/O failures on stdout.
+        print(f"error: failed to write SBOM: {exc}", file=sys.stderr)
+        return _EXIT_RUNTIME_ERROR
+    return _EXIT_SUCCESS

As per coding guidelines, "Error handling must follow docs/standards/error-handling.md standards including exit codes (0/1/2/3/4) as public contracts for CI/CD pipelines."

🤖 Prompt for AI Agents

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

In `@tools/generate_sbom.py` around lines 149 - 160, The main() function currently
calls json.dump(sbom, sys.stdout, ...) and sys.stdout.write("\n") without
catching BrokenPipeError (an OSError subclass), so a downstream closed pipe will
raise and bypass the defined exit-code contract; update main() to wrap the
json.dump and final write in a try/except that catches BrokenPipeError and other
OSError exceptions and returns the appropriate exit code (use existing
_EXIT_RUNTIME_ERROR for runtime I/O failures), and replace the literal 0 return
with the defined _EXIT_SUCCESS constant; references: main(), build_sbom(),
json.dump, sys.stdout.write, _EXIT_SUCCESS, and _EXIT_RUNTIME_ERROR.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/analysis/code_reviews/closure-plan-202604300906.md`:
- Line 5: The file docs/analysis/code_reviews/closure-plan-202604300906.md
contains newly added Turkish text (e.g., the line around "Last revised" and
other sections); translate those Turkish passages into clear, idiomatic English
to comply with the repository policy for internal docs. Update the edited
content at the shown area and the other flagged ranges (81-110, 952-967,
1933-1934) so all Turkish phrases are replaced with English equivalents,
preserving original meaning, formatting, and any metadata (dates/version notes)
and ensuring the document adheres to docs/standards/documentation.md's
English-only requirement.
- Line 1331: The markdown link referencing `ghost-features-analysis-20260502.md`
uses a broken relative path
`../analysis/code_reviews/ghost-features-analysis-20260502.md`; update that link
to the same-directory form used elsewhere in this file (e.g.,
`ghost-features-analysis-20260502.md`) so the reference resolves correctly from
docs/analysis/code_reviews/closure-plan-202604300906.md; locate the link text
"**Kaynak:**
[`ghost-features-analysis-20260502.md`](../analysis/code_reviews/ghost-features-analysis-20260502.md)"
and replace the target with the local relative path used elsewhere in this
document.

In `@docs/analysis/code_reviews/ghost-features-analysis-20260502.md`:
- Around line 21-23: Update the summary total of ghost entries at the top of the
document so it matches the detailed counts in Category A and the later summary
(change "13 ghost entries" to the correct total used elsewhere, e.g., "15 ghost
entries"); verify and synchronize the same corrected total wherever the old
number appears (lines showing counts around the initial summary, the 25-42
range, and the standalone value at line 82) so the header, Category A, and
summary tables all agree.
- Around line 349-352: Add language identifiers to the fenced code blocks
flagged by markdownlint (MD040) in
docs/analysis/code_reviews/ghost-features-analysis-20260502.md: update the
blocks containing "--benchmark-only   Run benchmark..." and the blocks starting
with "forgelm chat CHECKPOINT...", "forgelm ingest INPUT_DIR --output
PATH.jsonl...", the note block starting with "> **Note:** This command...", and
the block beginning "GH-001 (doctor):" by changing the opening fences to include
the appropriate language tags (e.g., ```text or ```md) so markdownlint stops
reporting MD040 for those snippets.

In `@forgelm/cli/subcommands/_approve.py`:
- Around line 179-185: The YAML loader may return a non-dict (list/scalar) as
manifest, causing manifest.get(...) to raise; modify the parsing after safe_load
to verify manifest is a dict (e.g., if not isinstance(manifest, dict):
logger.warning("Unexpected manifest root type %s at %s",
type(manifest).__name__, manifest_path); return {}) before calling
manifest.get("final_metrics", {}), so metrics = manifest.get(...) and the return
logic for metrics remains unchanged.

In `@forgelm/cli/subcommands/_ingest.py`:
- Around line 21-65: The CLI currently catches all ImportError from
ingest_path(), masking real bugs; change this to only translate a dedicated
optional-dependency error: add a custom exception (e.g.,
OptionalDependencyError) in the ingestion module and have ingest_path (and any
lazy optional imports inside it) raise that when an optional extra is missing,
then replace the broad "except ImportError as exc:" in _ingest.py with "except
OptionalDependencyError as exc:" so only intended missing-extra failures map to
EXIT_TRAINING_ERROR while other ImportError instances propagate and keep their
tracebacks.

---

Outside diff comments:
In `@tests/test_phase12_5.py`:
- Around line 578-606: The test test_alias_layout_keeps_real_filename currently
only asserts the slugged `@id` values; update it to also assert that each
distribution entry's "name" and "contentUrl" reference the real filenames (e.g.,
"train.jsonl" and "dev.jsonl") rather than canonical split labels. After loading
crois = json.loads(... )["croissant"], iterate or map over crois["distribution"]
entries and assert their "name" equals the original file basename and that
"contentUrl" ends with or contains that basename (and not the canonical label),
so regressions that rewrite referenced filenames will fail; keep checks scoped
to the distribution entries where ids were previously validated.
- Around line 272-284: Update the test
test_presidio_missing_extra_raises_with_install_hint so it simulates the missing
Presidio extra by patching both forgelm.data_audit._optional._HAS_PRESIDIO to
False and the Presidio sentinel (for example _PresidioAnalyzer) to a
falsy/missing state; specifically, in the context where you currently patch
_optional._HAS_PRESIDIO, also patch the symbol _PresidioAnalyzer (imported in
forgelm.data_audit) to None or a sentinel indicating absence before calling
_require_presidio(), so the failure path in _require_presidio() is exercised
regardless of whether it checks the boolean or the class symbol.

---

Nitpick comments:
In `@tools/generate_sbom.py`:
- Around line 149-160: The main() function currently calls json.dump(sbom,
sys.stdout, ...) and sys.stdout.write("\n") without catching BrokenPipeError (an
OSError subclass), so a downstream closed pipe will raise and bypass the defined
exit-code contract; update main() to wrap the json.dump and final write in a
try/except that catches BrokenPipeError and other OSError exceptions and returns
the appropriate exit code (use existing _EXIT_RUNTIME_ERROR for runtime I/O
failures), and replace the literal 0 return with the defined _EXIT_SUCCESS
constant; references: main(), build_sbom(), json.dump, sys.stdout.write,
_EXIT_SUCCESS, and _EXIT_RUNTIME_ERROR.

🪄 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: 1764f04a-7a1a-4f7e-a0b0-14e96e2abee7

📥 Commits

Reviewing files that changed from the base of the PR and between 536be49 and 78bab20.

📒 Files selected for processing (19)

• docs/analysis/code_reviews/closure-plan-202604300906.md
• docs/analysis/code_reviews/ghost-features-analysis-20260502.md
• forgelm/cli/__init__.py
• forgelm/cli/_dispatch.py
• forgelm/cli/_training.py
• forgelm/cli/subcommands/_approve.py
• forgelm/cli/subcommands/_ingest.py
• forgelm/cli/subcommands/_quickstart.py
• forgelm/data_audit/_croissant.py
• forgelm/model.py
• forgelm/trainer.py
• tests/test_cli_helpers.py
• tests/test_cli_quickstart_wiring.py
• tests/test_faz27_narrow_exceptions.py
• tests/test_human_approval_gate.py
• tests/test_model_moe_naming.py
• tests/test_phase12_5.py
• tools/check_site_claims.py
• tools/generate_sbom.py

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

• tests/test_model_moe_naming.py
• tests/test_faz27_narrow_exceptions.py

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

• forgelm/model.py
• forgelm/cli/_training.py
• forgelm/data_audit/_croissant.py
• tools/check_site_claims.py
• forgelm/cli/_dispatch.py
• forgelm/trainer.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Keep docs/analysis/* content English-only per documentation standard.

These newly edited sections introduce/extend Turkish text in an internal analysis doc. Please translate the added content to English to match repository documentation policy.

As per coding guidelines, docs/**/*.md must follow docs/standards/documentation.md, which states: “Internal docs (standards/, marketing/, analysis/, qms/, design/) are English only.”

Also applies to: 81-110, 952-967, 1933-1934

🤖 Prompt for AI Agents

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

In `@docs/analysis/code_reviews/closure-plan-202604300906.md` at line 5, The file
docs/analysis/code_reviews/closure-plan-202604300906.md contains newly added
Turkish text (e.g., the line around "Last revised" and other sections);
translate those Turkish passages into clear, idiomatic English to comply with
the repository policy for internal docs. Update the edited content at the shown
area and the other flagged ranges (81-110, 952-967, 1933-1934) so all Turkish
phrases are replaced with English equivalents, preserving original meaning,
formatting, and any metadata (dates/version notes) and ensuring the document
adheres to docs/standards/documentation.md's English-only requirement.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate audit-derived staging_path stays inside output_dir.

At Line 264 and Line 380, staging_path comes from audit data and is used directly for filesystem mutation/checks. A tampered audit file can point outside output_dir and redirect approve/reject behavior to unrelated paths.

💡 Suggested fix

     staging_path = required_event.get("staging_path") or os.path.join(output_dir, f"final_model{_STAGING_SUFFIX}")
+    output_dir_abs = os.path.abspath(output_dir)
+    staging_path_abs = os.path.abspath(staging_path)
+    if os.path.commonpath([output_dir_abs, staging_path_abs]) != output_dir_abs:
+        _output_error_and_exit(
+            output_format,
+            f"Refusing to use staging_path outside output_dir: {staging_path!r}",
+            EXIT_CONFIG_ERROR,
+        )
@@
     staging_path = required_event.get("staging_path") or os.path.join(output_dir, f"final_model{_STAGING_SUFFIX}")
+    output_dir_abs = os.path.abspath(output_dir)
+    staging_path_abs = os.path.abspath(staging_path)
+    if os.path.commonpath([output_dir_abs, staging_path_abs]) != output_dir_abs:
+        _output_error_and_exit(
+            output_format,
+            f"Refusing to use staging_path outside output_dir: {staging_path!r}",
+            EXIT_CONFIG_ERROR,
+        )

Also applies to: 380-383

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle audit.log_event(...) write failures explicitly in both handlers.

At Line 322 and Line 399, audit.log_event(...) can raise OSError; currently that escapes and breaks structured CLI error emission (and in approve, after promotion has already happened).

💡 Suggested fix

-    audit.log_event(
-        _EVT_HUMAN_APPROVAL_GRANTED,
-        gate="final_model",
-        run_id=run_id,
-        approver=approver,
-        comment=args.comment or "",
-        promote_strategy=promote_strategy,
-    )
+    try:
+        audit.log_event(
+            _EVT_HUMAN_APPROVAL_GRANTED,
+            gate="final_model",
+            run_id=run_id,
+            approver=approver,
+            comment=args.comment or "",
+            promote_strategy=promote_strategy,
+        )
+    except OSError as exc:
+        _output_error_and_exit(
+            output_format,
+            f"Promotion completed but failed to append audit event for run {run_id!r}: {exc}",
+            EXIT_TRAINING_ERROR,
+        )
@@
-    audit.log_event(
-        _EVT_HUMAN_APPROVAL_REJECTED,
-        gate="final_model",
-        run_id=run_id,
-        approver=approver,
-        comment=args.comment or "",
-        staging_path=staging_path,
-    )
+    try:
+        audit.log_event(
+            _EVT_HUMAN_APPROVAL_REJECTED,
+            gate="final_model",
+            run_id=run_id,
+            approver=approver,
+            comment=args.comment or "",
+            staging_path=staging_path,
+        )
+    except OSError as exc:
+        _output_error_and_exit(
+            output_format,
+            f"Failed to append rejection audit event for run {run_id!r}: {exc}",
+            EXIT_TRAINING_ERROR,
+        )

As per coding guidelines, "Error handling must follow docs/standards/error-handling.md standards including exit codes (0/1/2/3/4) as public contracts for CI/CD pipelines".

Also applies to: 399-406

[coderabbitai]

Actionable comments posted: 7

Caution

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

⚠️ Outside diff range comments (1)

tests/test_phase12_5.py (1)

626-651: ⚠️ Potential issue | 🟠 Major

Use an OS-neutral absolute-path precondition.

absolute_input.startswith("/") is POSIX-only and will cause this test to fail on Windows before reaching the leakage assertion. When Path.resolve() returns a Windows path like C:\..., the precondition silently fails.

Suggested fix

-        assert absolute_input.startswith("/"), "test precondition: absolute path"
+        assert Path(absolute_input).is_absolute(), "test precondition: absolute path"

🤖 Prompt for AI Agents

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

In `@tests/test_phase12_5.py` around lines 626 - 651, The test's precondition uses
POSIX-only startswith("/") so it fails on Windows; change the check to use an
OS-neutral absolute-path test such as Path(absolute_input).is_absolute() (or
os.path.isabs(absolute_input)) inside test_url_does_not_leak_absolute_path so
the assertion validates the input is absolute on all platforms before proceeding
to the leakage assertions that inspect the crois["url"] value.

♻️ Duplicate comments (3)

forgelm/cli/subcommands/_approve.py (3)

333-340: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve the CLI error contract around audit writes.

Both audit.log_event(...) calls can raise OSError. In approve, that happens after promotion, so the command can fall out with a traceback while the model is already at final_path but no human_approval.granted event was recorded.

💡 Suggested fix

-    audit.log_event(
-        _EVT_HUMAN_APPROVAL_GRANTED,
-        gate="final_model",
-        run_id=run_id,
-        approver=approver,
-        comment=args.comment or "",
-        promote_strategy=promote_strategy,
-    )
+    try:
+        audit.log_event(
+            _EVT_HUMAN_APPROVAL_GRANTED,
+            gate="final_model",
+            run_id=run_id,
+            approver=approver,
+            comment=args.comment or "",
+            promote_strategy=promote_strategy,
+        )
+    except OSError as exc:
+        _output_error_and_exit(
+            output_format,
+            f"Promotion completed but failed to append audit event for run {run_id!r}: {exc}",
+            EXIT_TRAINING_ERROR,
+        )
@@
-    audit.log_event(
-        _EVT_HUMAN_APPROVAL_REJECTED,
-        gate="final_model",
-        run_id=run_id,
-        approver=approver,
-        comment=args.comment or "",
-        staging_path=staging_path,
-    )
+    try:
+        audit.log_event(
+            _EVT_HUMAN_APPROVAL_REJECTED,
+            gate="final_model",
+            run_id=run_id,
+            approver=approver,
+            comment=args.comment or "",
+            staging_path=staging_path,
+        )
+    except OSError as exc:
+        _output_error_and_exit(
+            output_format,
+            f"Failed to append rejection audit event for run {run_id!r}: {exc}",
+            EXIT_TRAINING_ERROR,
+        )

Also applies to: 410-417

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_approve.py` around lines 333 - 340, Wrap the calls
to audit.log_event in the approve command so OSError from audit writes cannot
raise a traceback and break the CLI contract; specifically, in _approve.py
around the audit.log_event call that emits _EVT_HUMAN_APPROVAL_GRANTED (and the
other audit.log_event that emits _EVT_HUMAN_APPROVAL_REQUESTED), catch OSError
and handle it gracefully (e.g., log a warning via the existing logger or
click.echo to stderr) and continue/return normally instead of letting the
exception propagate. Ensure the try/except only catches OSError so other errors
bubble appropriately and keep the rest of the promotion/flow unchanged.

---

265-324: ⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

The terminal-decision guard is still racy across concurrent processes.

Both flows still do find decision event -> mutate state without any cross-process lock. os.rename() only helps one approve beat another on the same path; it does not stop an approve and a reject from both passing the guard and writing conflicting terminal events for the same run_id. Hold a per-run interprocess lock from the decision check through the rename/audit write in both handlers.

Also applies to: 381-417

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_approve.py` around lines 265 - 324, The
decision-guard is racy: both approve and reject handlers call
_cli_facade._find_human_approval_decision_event then mutate state without
coordination, so concurrent processes can both pass and write conflicting
terminal events; fix by introducing a per-run interprocess lock (e.g., a context
manager like acquire_run_lock(run_id) or a file-lock using
output_dir/run_id.lock) and acquire it before calling
_cli_facade._find_human_approval_decision_event, hold it across the
promote/rename call (_cli_facade._atomic_rename_or_move) and the AuditLogger
write (AuditLogger(...).log or equivalent), and release it in a finally block;
apply the same locking pattern to the other handler block referenced (the flow
around lines 381-417) so both approve and reject use the same lock name/scoping
to prevent concurrent terminal decisions.

---

275-293: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate the audit-derived staging_path before touching the filesystem.

required_event["staging_path"] is trusted as-is here. A tampered audit log can point approve/reject at a path outside output_dir, and the current isdir() or islink() guard also accepts broken or non-directory symlinks as valid staging directories. Canonicalize the path against output_dir and require a real directory before proceeding.

Also applies to: 391-399

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_approve.py` around lines 275 - 293, The code
currently trusts required_event["staging_path"] and accepts symlinks/broken
symlinks; instead canonicalize and validate the path: resolve staging_path
against output_dir (use os.path.abspath/os.path.realpath joining with output_dir
when staging_path is relative), ensure the resolved final_path remains inside
output_dir (compare commonprefix or use os.path.commonpath), and require that
staging_path is a real directory (os.path.isdir True) — reject symlinks/broken
links and anything outside output_dir by calling _output_error_and_exit with the
existing error pattern; apply the same canonicalization/validation logic for the
other occurrence handling approval/rejection (the block referencing
staging_path/final_path around lines ~391-399).

🧹 Nitpick comments (1)

tests/test_phase12_5.py (1)

617-624: ⚡ Quick win

Make the alias-layout contentUrl assertion exact.

endswith(filename) still passes if the implementation regresses to /tmp/.../dev.jsonl or C:\...\dev.jsonl, which is the path leak this test is supposed to catch.

Suggested diff

-            assert entry["contentUrl"].endswith(filename), (
-                f"contentUrl {entry['contentUrl']!r} must point at the real file {filename!r}, "
-                "not the canonical split label or the slugged `@id`"
-            )
+            assert entry["contentUrl"] == filename, (
+                f"contentUrl {entry['contentUrl']!r} must equal the real file name {filename!r}"
+            )
+            assert "\\" not in entry["contentUrl"] and "/" not in entry["contentUrl"], (
+                "contentUrl must stay a bare filename, not a path"
+            )

🤖 Prompt for AI Agents

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

In `@tests/test_phase12_5.py` around lines 617 - 624, The test's fuzzy check using
endswith(filename) can still pass if a filesystem path leaks into contentUrl;
change the assertion to verify the URL's final path segment exactly equals the
filename (e.g. assert entry["contentUrl"].split("/")[-1] == filename) and also
assert there are no backslashes in entry["contentUrl"] to catch Windows path
leaks, keeping the existing "validation" check; reference the by_name loop and
entry["contentUrl"] when making the change.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/analysis/code_reviews/closure-plan-202604300906.md`:
- Around line 1923-1925: The summary line that reads "7 carry-over" is
inconsistent with the enumerated items; update that headline count to "8
carry-over" to match the listed IDs (C-1, C-2, C-3, C-4, C-5, C-6, C-9, C-10) in
the same sentence in the paragraph containing "2 madde explicit-skip ... ve 7
carry-over"; ensure the numeric value and the comma-separated list are
synchronized so the roll-up matches.

In `@docs/analysis/code_reviews/ghost-features-analysis-20260502.md`:
- Around line 93-106: Update the GH-001 header in the document so the file-count
matches the table: either change the headline text ("30 occurrences across 8
files") to reflect the full table total ("30 occurrences across 10 files") or
add a clarifying note that the headline counts unique source pages only
(excluding mirrored EN/TR files); apply the same approach consistently for
subsequent GH sections that list both source and mirrored files and ensure the
header and the tabulated entries (the table listing EN/TR files) remain
consistent with each other.

In `@forgelm/cli/__init__.py`:
- Around line 24-153: The file currently eagerly imports almost every CLI leaf
(e.g. symbols like main, _run_training_pipeline, _run_chat_cmd,
_run_quickstart_cmd, _run_approve_cmd, _resolve_resume_checkpoint, _get_version,
logger, etc.), which makes import forgelm.cli heavy; replace these top-level
from-imports with a lazy re-export: remove the direct imports and add a
module-level __getattr__ that maps exported names to their originating
submodule, performs on-demand import of that submodule, and returns the
requested attribute; also implement __dir__ (and update __all__) to expose the
same public names for tests and autocompletion. Use the existing exported symbol
names from this file as keys in the mapping so callers keep the old dotted paths
while avoiding import-time cost.

In `@forgelm/cli/subcommands/_ingest.py`:
- Around line 21-23: The try block imports OptionalDependencyError along with
ingest_path and summarize_result but then uses except OptionalDependencyError
which can raise UnboundLocalError if the import itself fails; fix by ensuring
OptionalDependencyError is bound before being referenced — either move "from
...ingestion import OptionalDependencyError" out of the try to a top-level
import, or import the module (e.g. "from ... import ingestion" or "import
...ingestion as ingestion") and change the exception handler to "except
ingestion.OptionalDependencyError" while keeping ingest_path/summarize_result
imports inside the try; update references to OptionalDependencyError accordingly
so the except in the block that wraps ingest_path and summarize_result reliably
matches the exception.

In `@forgelm/cli/subcommands/_quickstart.py`:
- Around line 104-133: The helper _run_quickstart_train_subprocess currently
calls sys.exit on OSError or non-zero child return codes, which prevents the
parent _run_quickstart_cmd from emitting a JSON failure envelope; change
_run_quickstart_train_subprocess to return a mapped exit code (one of the public
codes in _PUBLIC_EXIT_CODES or EXIT_TRAINING_ERROR) or raise a specific
exception containing that mapped code and error details instead of calling
sys.exit, and then update _run_quickstart_cmd to catch that return/exception and
emit the JSON {"success": false, ...} payload (including error details and
mapped exit code) before exiting with the same mapped code so CI gets a
parent-owned JSON failure envelope.

In `@forgelm/data_audit/_croissant.py`:
- Around line 157-169: The slug generation for field `@id` values can collide
because _slug() is many-to-one; change the comprehension so you first generate
unique per-column slugs (use _slug(column, fallback='field') as the base, track
counts in a dict and append a numeric suffix like -1, -2 for duplicates) and
then build the field dicts using f"{split_id}/{unique_slug}" (referencing _slug,
columns, split_id, file_id and the fields list construction) so each field gets
a distinct `@id` even when base slugs repeat.

In `@forgelm/ingestion.py`:
- Around line 246-249: The except blocks currently catching ImportError should
be changed to catch ModuleNotFoundError and validate exc.name before converting
to OptionalDependencyError: in the three places where you raise
OptionalDependencyError (the PDF ingestion block and the two other
import-protection blocks), replace "except ImportError as exc" with "except
ModuleNotFoundError as exc" and if exc.name matches the expected optional module
name raise OptionalDependencyError with the same message, otherwise re-raise the
original exception so genuine import errors (syntax/circular/corrupt installs)
are not swallowed; keep the OptionalDependencyError class and messages
unchanged.

---

Outside diff comments:
In `@tests/test_phase12_5.py`:
- Around line 626-651: The test's precondition uses POSIX-only startswith("/")
so it fails on Windows; change the check to use an OS-neutral absolute-path test
such as Path(absolute_input).is_absolute() (or os.path.isabs(absolute_input))
inside test_url_does_not_leak_absolute_path so the assertion validates the input
is absolute on all platforms before proceeding to the leakage assertions that
inspect the crois["url"] value.

---

Duplicate comments:
In `@forgelm/cli/subcommands/_approve.py`:
- Around line 333-340: Wrap the calls to audit.log_event in the approve command
so OSError from audit writes cannot raise a traceback and break the CLI
contract; specifically, in _approve.py around the audit.log_event call that
emits _EVT_HUMAN_APPROVAL_GRANTED (and the other audit.log_event that emits
_EVT_HUMAN_APPROVAL_REQUESTED), catch OSError and handle it gracefully (e.g.,
log a warning via the existing logger or click.echo to stderr) and
continue/return normally instead of letting the exception propagate. Ensure the
try/except only catches OSError so other errors bubble appropriately and keep
the rest of the promotion/flow unchanged.
- Around line 265-324: The decision-guard is racy: both approve and reject
handlers call _cli_facade._find_human_approval_decision_event then mutate state
without coordination, so concurrent processes can both pass and write
conflicting terminal events; fix by introducing a per-run interprocess lock
(e.g., a context manager like acquire_run_lock(run_id) or a file-lock using
output_dir/run_id.lock) and acquire it before calling
_cli_facade._find_human_approval_decision_event, hold it across the
promote/rename call (_cli_facade._atomic_rename_or_move) and the AuditLogger
write (AuditLogger(...).log or equivalent), and release it in a finally block;
apply the same locking pattern to the other handler block referenced (the flow
around lines 381-417) so both approve and reject use the same lock name/scoping
to prevent concurrent terminal decisions.
- Around line 275-293: The code currently trusts required_event["staging_path"]
and accepts symlinks/broken symlinks; instead canonicalize and validate the
path: resolve staging_path against output_dir (use
os.path.abspath/os.path.realpath joining with output_dir when staging_path is
relative), ensure the resolved final_path remains inside output_dir (compare
commonprefix or use os.path.commonpath), and require that staging_path is a real
directory (os.path.isdir True) — reject symlinks/broken links and anything
outside output_dir by calling _output_error_and_exit with the existing error
pattern; apply the same canonicalization/validation logic for the other
occurrence handling approval/rejection (the block referencing
staging_path/final_path around lines ~391-399).

---

Nitpick comments:
In `@tests/test_phase12_5.py`:
- Around line 617-624: The test's fuzzy check using endswith(filename) can still
pass if a filesystem path leaks into contentUrl; change the assertion to verify
the URL's final path segment exactly equals the filename (e.g. assert
entry["contentUrl"].split("/")[-1] == filename) and also assert there are no
backslashes in entry["contentUrl"] to catch Windows path leaks, keeping the
existing "validation" check; reference the by_name loop and entry["contentUrl"]
when making the change.

🪄 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: 127020c7-9f06-4818-8445-f7033ba15333

📥 Commits

Reviewing files that changed from the base of the PR and between 536be49 and 55ca1be.

📒 Files selected for processing (20)

• docs/analysis/code_reviews/closure-plan-202604300906.md
• docs/analysis/code_reviews/ghost-features-analysis-20260502.md
• forgelm/cli/__init__.py
• forgelm/cli/_dispatch.py
• forgelm/cli/_training.py
• forgelm/cli/subcommands/_approve.py
• forgelm/cli/subcommands/_ingest.py
• forgelm/cli/subcommands/_quickstart.py
• forgelm/data_audit/_croissant.py
• forgelm/ingestion.py
• forgelm/model.py
• forgelm/trainer.py
• tests/test_cli_helpers.py
• tests/test_cli_quickstart_wiring.py
• tests/test_faz27_narrow_exceptions.py
• tests/test_human_approval_gate.py
• tests/test_model_moe_naming.py
• tests/test_phase12_5.py
• tools/check_site_claims.py
• tools/generate_sbom.py

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

• tests/test_model_moe_naming.py

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

• forgelm/cli/_training.py
• forgelm/cli/_dispatch.py
• tests/test_human_approval_gate.py
• forgelm/trainer.py
• tests/test_faz27_narrow_exceptions.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix the carry-over total in the summary sentence.

This line says there are "7 carry-over" items, but the list that follows names eight IDs: C-1, C-2, C-3, C-4, C-5, C-6, C-9, and C-10. Please sync the headline number with the enumerated items so the roll-up stays trustworthy.

🤖 Prompt for AI Agents

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

In `@docs/analysis/code_reviews/closure-plan-202604300906.md` around lines 1923 -
1925, The summary line that reads "7 carry-over" is inconsistent with the
enumerated items; update that headline count to "8 carry-over" to match the
listed IDs (C-1, C-2, C-3, C-4, C-5, C-6, C-9, C-10) in the same sentence in the
paragraph containing "2 madde explicit-skip ... ve 7 carry-over"; ensure the
numeric value and the comma-separated list are synchronized so the roll-up
matches.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align the file-count headline with the table.

The GH-1 header says "30 occurrences across 8 files", but the table below enumerates 10 file entries once the EN/TR mirrors are included. Please either count the mirrored files in the headline or say explicitly that the count is for unique source pages only; the same mismatch seems to repeat in later GH sections too.

🤖 Prompt for AI Agents

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

In `@docs/analysis/code_reviews/ghost-features-analysis-20260502.md` around lines
93 - 106, Update the GH-001 header in the document so the file-count matches the
table: either change the headline text ("30 occurrences across 8 files") to
reflect the full table total ("30 occurrences across 10 files") or add a
clarifying note that the headline counts unique source pages only (excluding
mirrored EN/TR files); apply the same approach consistently for subsequent GH
sections that list both source and mirrored files and ensure the header and the
tabulated entries (the table listing EN/TR files) remain consistent with each
other.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Keep the facade lazy instead of importing every CLI leaf up front.

This preserves the old dotted paths, but it also makes import forgelm.cli eagerly import almost the entire CLI tree again. That reintroduces exactly the kind of load-bearing import graph the split was trying to remove, and it makes unrelated entry points vulnerable to any future optional-dependency import or circular-import added in one leaf module.

A __getattr__-based re-export table, or similarly lazy wrappers for the test-facing helpers, would keep the historical import surface without paying the import-time cost for every subcommand.

Based on learnings, treat heavy/optional dependencies as opt-in extras: use a lazy-import pattern instead of failing eagerly at module import time.

🧰 Tools

🪛 GitHub Check: Codacy Static Code Analysis

[warning] 24-24: forgelm/cli/init.py#L24
'._argparse_types._add_common_subparser_flags' imported but unused (F401)

---

[warning] 31-31: forgelm/cli/init.py#L31
'._config_load._apply_offline_flag' imported but unused (F401)

---

[warning] 37-37: forgelm/cli/init.py#L37
'._dispatch._dispatch_subcommand' imported but unused (F401)

---

[warning] 43-43: forgelm/cli/init.py#L43
'._dry_run._build_dry_run_result' imported but unused (F401)

---

[warning] 52-52: forgelm/cli/init.py#L52
'._exit_codes._PUBLIC_EXIT_CODES' imported but unused (F401)

---

[warning] 65-65: forgelm/cli/init.py#L65
'._logging._CLI_MODULE' imported but unused (F401)

---

[warning] 73-73: forgelm/cli/init.py#L73
'._no_train_modes._maybe_run_no_train_mode' imported but unused (F401)

---

[warning] 82-82: forgelm/cli/init.py#L82
'._parser._add_approve_subcommand' imported but unused (F401)

---

[warning] 96-96: forgelm/cli/init.py#L96
'._result._build_result_json_envelope' imported but unused (F401)

---

[warning] 108-108: forgelm/cli/init.py#L108
'._training._report_training_error' imported but unused (F401)

---

[warning] 117-117: forgelm/cli/init.py#L117
'.subcommands._approve._atomic_rename_or_move' imported but unused (F401)

---

[warning] 129-129: forgelm/cli/init.py#L129
'.subcommands._audit._run_audit_cmd' imported but unused (F401)

---

[warning] 141-141: forgelm/cli/init.py#L141
'.subcommands._quickstart._emit_quickstart_list' imported but unused (F401)

🤖 Prompt for AI Agents

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

In `@forgelm/cli/__init__.py` around lines 24 - 153, The file currently eagerly
imports almost every CLI leaf (e.g. symbols like main, _run_training_pipeline,
_run_chat_cmd, _run_quickstart_cmd, _run_approve_cmd,
_resolve_resume_checkpoint, _get_version, logger, etc.), which makes import
forgelm.cli heavy; replace these top-level from-imports with a lazy re-export:
remove the direct imports and add a module-level __getattr__ that maps exported
names to their originating submodule, performs on-demand import of that
submodule, and returns the requested attribute; also implement __dir__ (and
update __all__) to expose the same public names for tests and autocompletion.
Use the existing exported symbol names from this file as keys in the mapping so
callers keep the old dotted paths while avoiding import-time cost.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t use a try-local imported class in an except clause.

At Line 22, OptionalDependencyError is imported inside the same try that later uses except OptionalDependencyError (Line 56). If that import fails before binding, exception matching can crash with UnboundLocalError, hiding the original import traceback.

💡 Suggested fix

 def _run_ingest_cmd(args, output_format: str) -> None:
     """Phase 11 dispatch: raw documents → SFT-ready JSONL."""
+    from ... import ingestion as ingestion_mod
+
     # Phase 12.5: --all-mask is a shorthand that ORs into the two individual
     # masking flags. Resolve it here at the CLI boundary so ``ingest_path``
     # keeps its narrow API (only ``pii_mask`` / ``secrets_mask`` booleans).
@@
     try:
-        from ...ingestion import OptionalDependencyError, ingest_path, summarize_result
-
-        result = ingest_path(
+        result = ingestion_mod.ingest_path(
             args.input_path,
             output_path=args.output,
@@
-    except OptionalDependencyError as exc:
+    except ingestion_mod.OptionalDependencyError as exc:
@@
     else:
-        print(summarize_result(result))
+        print(ingestion_mod.summarize_result(result))

Also applies to: 56-68

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_ingest.py` around lines 21 - 23, The try block
imports OptionalDependencyError along with ingest_path and summarize_result but
then uses except OptionalDependencyError which can raise UnboundLocalError if
the import itself fails; fix by ensuring OptionalDependencyError is bound before
being referenced — either move "from ...ingestion import
OptionalDependencyError" out of the try to a top-level import, or import the
module (e.g. "from ... import ingestion" or "import ...ingestion as ingestion")
and change the exception handler to "except ingestion.OptionalDependencyError"
while keeping ingest_path/summarize_result imports inside the try; update
references to OptionalDependencyError accordingly so the except in the block
that wraps ingest_path and summarize_result reliably matches the exception.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Emit a parent-owned JSON error envelope on training failures too.

In --output-format json mode, this helper still calls sys.exit(...) directly on child launch failure or non-zero training exit. That means quickstart now prints a single JSON success payload on the happy path, but prints no JSON payload at all on the training-failure path. CI callers will still have an unparseable contract for failures.

Please route the training failure back to _run_quickstart_cmd so the parent can emit something like {"success": false, ...} before preserving the mapped exit code.

As per coding guidelines, error handling must follow docs/standards/error-handling.md standards including exit codes (0/1/2/3/4) as public contracts for CI/CD pipelines.

🧰 Tools

🪛 GitHub Check: Codacy Static Code Analysis

[failure] 126-126: forgelm/cli/subcommands/_quickstart.py#L126
Detected subprocess function 'run' without a static string.

🤖 Prompt for AI Agents

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

In `@forgelm/cli/subcommands/_quickstart.py` around lines 104 - 133, The helper
_run_quickstart_train_subprocess currently calls sys.exit on OSError or non-zero
child return codes, which prevents the parent _run_quickstart_cmd from emitting
a JSON failure envelope; change _run_quickstart_train_subprocess to return a
mapped exit code (one of the public codes in _PUBLIC_EXIT_CODES or
EXIT_TRAINING_ERROR) or raise a specific exception containing that mapped code
and error details instead of calling sys.exit, and then update
_run_quickstart_cmd to catch that return/exception and emit the JSON {"success":
false, ...} payload (including error details and mapped exit code) before
exiting with the same mapped code so CI gets a parent-owned JSON failure
envelope.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Disambiguate slug collisions before assigning field @ids.

_slug() is many-to-one, so distinct columns like user-id/user id/user.id — or multiple non-ASCII names that all fall back to field — can produce the same @id here. That makes separate fields indistinguishable in the emitted card.

Possible fix

         columns = info.get("columns") or []
-        fields = [
-            {
-                _JSONLD_TYPE_KEY: "cr:Field",
-                _JSONLD_ID_KEY: f"{split_id}/{_slug(column, fallback='field')}",
-                "name": column,
-                "dataType": "sc:Text",
-                "source": {
-                    "fileObject": {_JSONLD_ID_KEY: file_id},
-                    "extract": {"jsonPath": "$['" + column.replace("\\", "\\\\").replace("'", "\\'") + "']"},
-                },
-            }
-            for column in columns
-        ]
+        seen_field_slugs: Dict[str, int] = {}
+        fields = []
+        for column in columns:
+            base_slug = _slug(column, fallback="field")
+            seen_field_slugs[base_slug] = seen_field_slugs.get(base_slug, 0) + 1
+            suffix = "" if seen_field_slugs[base_slug] == 1 else f"_{seen_field_slugs[base_slug]}"
+            fields.append(
+                {
+                    _JSONLD_TYPE_KEY: "cr:Field",
+                    _JSONLD_ID_KEY: f"{split_id}/{base_slug}{suffix}",
+                    "name": column,
+                    "dataType": "sc:Text",
+                    "source": {
+                        "fileObject": {_JSONLD_ID_KEY: file_id},
+                        "extract": {"jsonPath": "$['" + column.replace("\\", "\\\\").replace("'", "\\'") + "']"},
+                    },
+                }
+            )

🤖 Prompt for AI Agents

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

In `@forgelm/data_audit/_croissant.py` around lines 157 - 169, The slug generation
for field `@id` values can collide because _slug() is many-to-one; change the
comprehension so you first generate unique per-column slugs (use _slug(column,
fallback='field') as the base, track counts in a dict and append a numeric
suffix like -1, -2 for duplicates) and then build the field dicts using
f"{split_id}/{unique_slug}" (referencing _slug, columns, split_id, file_id and
the fields list construction) so each field gets a distinct `@id` even when base
slugs repeat.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let me check the actual code at the specified lines
head -n 260 forgelm/ingestion.py | tail -n 30

Repository: cemililik/ForgeLM

Length of output: 1035

---

🏁 Script executed:

# Also check lines around 355-358 and 384-387
sed -n '345,395p' forgelm/ingestion.py

Repository: cemililik/ForgeLM

Length of output: 2076

---

🏁 Script executed:

# Check the OptionalDependencyError class definition
rg -n "class OptionalDependencyError" forgelm/

Repository: cemililik/ForgeLM

Length of output: 129

---

🏁 Script executed:

# Verify Python exception hierarchy - ModuleNotFoundError vs ImportError
python3 << 'EOF'
import sys

# Check exception hierarchy
print("ModuleNotFoundError MRO:")
print(ModuleNotFoundError.__mro__)

print("\nImportError MRO:")
print(ImportError.__mro__)

print("\nIs ModuleNotFoundError a subclass of ImportError?")
print(issubclass(ModuleNotFoundError, ImportError))

# Test what exception is actually raised for missing modules
print("\n--- Testing missing module exception ---")
try:
    import nonexistent_module_xyz
except Exception as e:
    print(f"Exception type: {type(e).__name__}")
    print(f"Exception.name: {getattr(e, 'name', 'N/A')}")
    print(f"Full exception: {e}")
EOF

Repository: cemililik/ForgeLM

Length of output: 541

---

🏁 Script executed:

# Check the exact error handling guideline mentioned
cat docs/standards/error-handling.md 2>/dev/null | head -n 100

Repository: cemililik/ForgeLM

Length of output: 3979

---

🏁 Script executed:

# Check for any existing test files that test this behavior
fd -e py forgelm/.*test | xargs grep -l "OptionalDependencyError\|_extract_pdf\|_extract_docx\|_extract_epub" 2>/dev/null

Repository: cemililik/ForgeLM

Length of output: 45

---

Catch ModuleNotFoundError specifically to avoid hiding real import errors.

At lines 246–249, 355–358, and 384–387, except ImportError is too broad: it also catches import failures within installed packages (syntax errors, circular imports, or corrupted installations) and incorrectly treats them as missing optional dependencies. This obscures the real error and misdirects the operator.

Use ModuleNotFoundError instead, and check the .name attribute to ensure it matches the expected module:

Suggested fix

 def _extract_pdf(path: Path, *, dedup_state: Optional[Dict[str, int]] = None) -> str:
     try:
         from pypdf import PdfReader
-    except ImportError as exc:  # pragma: no cover — covered by extras
+    except ModuleNotFoundError as exc:  # pragma: no cover — covered by extras
+        if exc.name != "pypdf":
+            raise
         raise OptionalDependencyError(
             "PDF ingestion requires the 'ingestion' extra. Install with: pip install 'forgelm[ingestion]'"
         ) from exc

 def _extract_docx(path: Path) -> str:
     try:
         from docx import Document
         from docx.table import Table
-    except ImportError as exc:  # pragma: no cover
+    except ModuleNotFoundError as exc:  # pragma: no cover
+        if exc.name != "docx":
+            raise
         raise OptionalDependencyError(
             "DOCX ingestion requires the 'ingestion' extra. Install with: pip install 'forgelm[ingestion]'"
         ) from exc

 def _extract_epub(path: Path) -> str:
     try:
         from bs4 import BeautifulSoup
         from ebooklib import ITEM_DOCUMENT, epub
-    except ImportError as exc:  # pragma: no cover
+    except ModuleNotFoundError as exc:  # pragma: no cover
+        if exc.name not in {"bs4", "ebooklib"}:
+            raise
         raise OptionalDependencyError(
             "EPUB ingestion requires the 'ingestion' extra. Install with: pip install 'forgelm[ingestion]'"
         ) from exc

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	except ImportError as exc: # pragma: no cover — covered by extras
	raise ImportError(
	raise OptionalDependencyError(
	"PDF ingestion requires the 'ingestion' extra. Install with: pip install 'forgelm[ingestion]'"
	) from exc
	except ModuleNotFoundError as exc: # pragma: no cover — covered by extras
	if exc.name != "pypdf":
	raise
	raise OptionalDependencyError(
	"PDF ingestion requires the 'ingestion' extra. Install with: pip install 'forgelm[ingestion]'"
	) from exc

🤖 Prompt for AI Agents

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

In `@forgelm/ingestion.py` around lines 246 - 249, The except blocks currently
catching ImportError should be changed to catch ModuleNotFoundError and validate
exc.name before converting to OptionalDependencyError: in the three places where
you raise OptionalDependencyError (the PDF ingestion block and the two other
import-protection blocks), replace "except ImportError as exc" with "except
ModuleNotFoundError as exc" and if exc.name matches the expected optional module
name raise OptionalDependencyError with the same message, otherwise re-raise the
original exception so genuine import errors (syntax/circular/corrupt installs)
are not swallowed; keep the OptionalDependencyError class and messages
unchanged.

[sonarqubecloud]

Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarQube Cloud