Repository navigation
v0.5.5 pre-release cleanup — audit P0 + agent-side reference fixes - #35
Merged
Merged
Conversation
…ge audit P0 + agent-side reference fixes
Pre-Faz-33 (v0.5.5 PyPI publish) cleanup driven by the post-merge
closure-plan verification audit (`/tmp/closure-plan-verification-report.md`,
9-agent parallel verification). Two parallel motivations folded into
one commit:
1. Audit-surfaced P0 fixes (load-bearing pre-PyPI tag).
2. Verification-agent-side reference cleanup (broken `docs/marketing/`
+ `docs/analysis/QKV-Core/` paths in 7 standards/roadmap files —
those source dirs are gitignored, so the inline links resolved to
404 in any public-rendered surface).
## P0 audit-surfaced fixes
### M1 — `forgelm/cli/subcommands/_safety_eval.py:128-129`
Add explicit `trust_remote_code=False` to the standalone-load
`AutoTokenizer.from_pretrained` + `AutoModelForCausalLM.from_pretrained`
call pair. Closes the only remaining defence-in-depth gap against
Faz 7 §13 acceptance — `forgelm safety-eval --model <hub-name>` could
load Hub models with `trust_remote_code=True` defaults silently.
Inline comment cites the contract + mirrors `trainer.py`'s pattern.
### M2 — Ghost template names in 4 user-manual files
Sweep `byod-domain-expert` → `domain-expert` and `math-reasoning` →
`grpo-math` (the canonical template handles per
`forgelm/templates/{domain-expert,grpo-math}/`). Files:
- `docs/usermanuals/en/getting-started/first-run.md`
- `docs/usermanuals/en/concepts/choosing-trainer.md`
- `docs/usermanuals/tr/getting-started/first-run.md`
- `docs/usermanuals/tr/concepts/choosing-trainer.md`
Note: `model-merging.md` and `yaml-templates.md` still mention
`./checkpoints/math-reasoning` — those are operator-supplied
checkpoint dir names (not template handles), legitimately allowed.
### M3 — `annex_iv.json` → `annex_iv_metadata.json` sweep
Canonical filename per `forgelm/compliance.py:1115`
(`os.path.join(output_dir, "annex_iv_metadata.json")`) and
`forgelm/trainer.py:1341` docstring. User-facing docs and
operator-facing log/tree examples updated to match. Files (en+tr):
- `compliance/annex-iv.md` (en+tr) — verifier description, mermaid
diagram, quickstart shell snippet, JSON output sample, "no
manifest hash" failure example
- `compliance/overview.md` (en+tr) — artefact-tree diagram,
Article 11 row, `compliance:` YAML flow text
- `compliance/model-card.md` (en+tr) — EU AI Act compliance bullet
- `getting-started/first-run.md` (en+tr) — log output ("Annex IV →
…"), output-tree diagram
- `operations/troubleshooting.md` (en+tr) — fix command in
"Annex IV missing required fields" entry
The verify-annex-iv help text still uses `<annex_iv.json>` as the
argparse placeholder — operators name their own copies, so the
generic placeholder shape is preserved.
### C-1 — `docs/reference/usage.md` + `usage-tr.md` GPU count drift
"18 GPU models" → "16 GPU models" (the live count per
`forgelm.trainer.ForgeTrainer._GPU_PRICING`, verified via
`python -c 'from forgelm.trainer import ForgeTrainer as T; print(len(T._GPU_PRICING))'`
→ `16`). Cite added so a future drift trips the eye. Closes the
trivial Wave-1-foundation carry-over registry C-1 still open at
HEAD.
## Agent-side reference cleanup (7 files, broken-link fix)
The closure-plan verification agents discovered that 7
public-facing standards / roadmap files inline-link to documents
under `docs/marketing/` and `docs/analysis/QKV-Core/` — both
gitignored per `.gitignore` (internal-strategy / research-artefact
trees). On any public site or fresh checkout the links resolve to
404. Each is replaced with a prose pointer to a public surface
(usually `CLAUDE.md` "What ForgeLM is not" / `master-review-opus`)
that captures the same intent without depending on internal-only
paths.
Files:
- `docs/standards/architecture.md` — "Things we do not own"
blockquote no longer links to gitignored marketing strategy doc.
- `docs/standards/code-review.md` — anti-patterns table preface
now points to `CLAUDE.md` instead of marketing strategy.
- `docs/standards/coding.md` — anti-patterns reference points to
`master-review-opus-202604300906.md` instead of QKV-Core analysis.
- `docs/standards/localization.md` — persona reasoning rewritten
without the marketing personas link.
- `docs/standards/release.md` — release sequence references
cleaned up.
- `docs/roadmap/phase-10-5-quickstart.md` — strategy decision
reference rewritten.
- `docs/roadmap/phase-13-pro-cli.md` — revenue model + pricing
reference rewritten.
No semantic change to the underlying guidance; only link targets
swap from gitignored to public.
## Self-review gauntlet (all green at HEAD)
| Check | Result |
|---|---|
| `ruff format --check .` + `ruff check .` | ✅ clean (167 files) |
| `check_bilingual_parity --strict` | ✅ 39/39 |
| `check_anchor_resolution --strict` | ✅ 264/264 |
| `check_cli_help_consistency --strict` | ✅ 425/425 |
| `check_site_claims --strict` | ✅ artefacts/templates/GPU/version |
| `check_field_descriptions --strict forgelm/config.py` | ✅ |
| `pytest --no-cov -q tests/` | ✅ 1436 passed / 14 skipped |
| `forgelm --config config_template.yaml --dry-run` | ✅ exit 0 |
| `build_usermanuals.py` | ✅ en/tr 65/65 |
## Audit deliverable
The full 9-agent verification report — covering all 38 phases of
`docs/analysis/code_reviews/closure-plan-202604300906.md`, plus
§11 dimension-exit gates and §15.5 carry-over registry — was
authored at `/tmp/closure-plan-verification-report.md` (local,
not committed pending owner review). Headline finding: closure
plan claim of "geride iş yok" is **substantively delivered** for
user-facing surface; residue is internal code-cohesion ceilings
(`_parser.py` 1135 vs documented 662; `_orchestrator.py` 504 vs
ceiling 400) plus a small set of late-shipped task bullets. This
commit absorbs the load-bearing P0 items (M1+M2+M3+C-1) before
Faz 33 PyPI tag.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Reviewer's GuidePre-release cleanup that (1) hardens the standalone safety-eval model loading path by explicitly disabling remote code execution and (2) aligns user-facing docs, manuals, and standards/roadmap references with the current canonical templates, artifact filenames, GPU registry, and only-public documentation surfaces. Sequence diagram for hardened safety eval model loadingsequenceDiagram
actor Operator
participant CLI_safety_eval as forgelm_safety_eval
participant AutoTokenizer as Transformers_AutoTokenizer
participant AutoModel as Transformers_AutoModelForCausalLM
participant HubModelRepo
Operator->>CLI_safety_eval: run safety_eval with model_path
CLI_safety_eval->>AutoTokenizer: from_pretrained(model_path, trust_remote_code=False)
AutoTokenizer->>HubModelRepo: fetch tokenizer weights and config
HubModelRepo-->>AutoTokenizer: tokenizer data (no remote code)
CLI_safety_eval->>AutoModel: from_pretrained(model_path, trust_remote_code=False)
AutoModel->>HubModelRepo: fetch model weights and config
HubModelRepo-->>AutoModel: model data (no remote code)
CLI_safety_eval-->>Operator: model and tokenizer ready for safety evaluation
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
trust_remote_code=Falsepolicy forfrom_pretrainedcalls in_safety_eval.pylooks good; consider centralizing this into a small helper or constant so future model-loading sites can’t accidentally drift from the enforcement contract. - Now that
annex_iv_metadata.jsonis the canonical filename across the manuals, it might be worth defining this name in a single code-level constant and referencing it in docs generation or templates to avoid future mismatches if the filename ever changes again.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `trust_remote_code=False` policy for `from_pretrained` calls in `_safety_eval.py` looks good; consider centralizing this into a small helper or constant so future model-loading sites can’t accidentally drift from the enforcement contract.
- Now that `annex_iv_metadata.json` is the canonical filename across the manuals, it might be worth defining this name in a single code-level constant and referencing it in docs generation or templates to avoid future mismatches if the filename ever changes again.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 2 minor |
🟢 Metrics 0 complexity · 0 duplication
Metric Results Complexity 0 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates documentation to reflect changes in GPU support counts, renames compliance artifacts (specifically annex_iv.json to annex_iv_metadata.json), and updates quickstart template names. It also replaces internal document links with descriptive text. In the code, security constraints were added to model loading in the safety evaluation subcommand. Feedback indicates that the model loading logic in the CLI should be refactored to use core primitives, ensuring architectural consistency and delegating business logic away from the CLI layer.
I am having trouble creating individual review comments. Click here to see my feedback.
forgelm/cli/subcommands/_safety_eval.py (126-137)
This implementation violates the Architecture Standard (Principle 1: Each module owns one concern) and the CLI shim rule (Business logic in any cli/ module is a bug). Model loading logic should be delegated to the project's core primitives.
By using forgelm.inference.load_model, you centralize the loading logic, ensuring consistency with other parts of the tool (like forgelm chat). This also automatically provides benefits like device_map="auto" and proper eval() mode setting, while still satisfying the trust_remote_code=False security requirement.
from forgelm.inference import load_model
# Defence-in-depth (Faz 7 §13 acceptance): every from_pretrained
# call in forgelm/ MUST pass trust_remote_code=False explicitly.
# Mirrors the contract forgelm/trainer.py already enforces.
return load_model(model_path, trust_remote_code=False)
References
- Principle 1: Each module owns one concern. CLI is a thin shim. Business logic in any cli/ module is a bug. (link)
…o forgelm.inference.load_model PR #35 round-1 absorption: gemini-code-assist surfaced an architecture nudge that the round-1 (`50d0edb`) direct `from_pretrained` calls in `forgelm/cli/subcommands/_safety_eval.py` violate `docs/standards/architecture.md` Principle 1 ("CLI is a thin shim — business logic in any cli/ module is a bug") even though they ship the correct `trust_remote_code=False` defence. The canonical primitive `forgelm.inference.load_model` already: - defaults `trust_remote_code=False` and accepts an explicit kwarg - mirrors the loader contract used by `forgelm chat`, `forgelm export`, and other surfaces - centralises `device_map="auto"` placement + adapter merge logic - becomes the single upgrade point when Phase 36+ wires `inference.load_gguf_model` for the GGUF safety-eval path Refactor `_load_model_for_safety` to delegate to `load_model(...)` with `trust_remote_code=False` explicitly passed (defence-in-depth gating per Faz 7 §13 — never trust the primitive's default; gate at every call site). The original M1 fix (round-1 commit `50d0edb`) is preserved semantically: `from_pretrained` calls still happen with `trust_remote_code=False`, just inside the inference primitive where they belong instead of duplicated in the CLI shim. Skipped (with reason): - **Sourcery suggestion #2** ("define `annex_iv_metadata.json` in a single code-level constant"): scope creep for round-1 absorption. The canonical filename has one writer site (`forgelm/compliance.py:1115`) and one verifier reader (`_verify_annex_iv.py`); extracting to a module-level constant is a sensible v0.6.x cleanup but is not load-bearing for v0.5.5. Round-1 docs already pinned the filename across user-facing surfaces (10 files en+tr) so a one-line drift in the future would surface via `check_site_claims --strict` (it already pins `annex_iv_metadata.json` per `tests/test_check_site_claims.py:62`). - **Sourcery suggestion #1** ("centralize `trust_remote_code=False` policy"): subsumed by this commit — `load_model`'s default-False + explicit kwarg pattern IS the centralised contract. Self-review gauntlet (all green at HEAD): ruff format/check ✅ clean cli_help_consistency --strict ✅ 425/425 pytest --no-cov -q tests/ ✅ 1436 passed / 14 skipped (no behavioural change — `_load_model_for_safety` still returns `(model, tokenizer)` tuple from the same load path) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
Round 1 absorption (commit
|
| Check | Result |
|---|---|
ruff format + ruff check |
✅ clean |
check_bilingual_parity --strict |
✅ 39/39 |
check_anchor_resolution --strict |
✅ 264/264 |
check_cli_help_consistency --strict |
✅ 425/425 |
check_site_claims --strict |
✅ |
check_field_descriptions --strict |
✅ |
pytest --no-cov -q |
✅ 1436 passed / 14 skipped (no behavioural change — _load_model_for_safety still returns (model, tokenizer) from the same load path) |
CI re-running on this push.
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Triggering a full review on commit [review] |
|
6 tasks done
This was referenced May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pre-Faz-33 (v0.5.5 PyPI publish) cleanup driven by the post-merge
9-agent closure-plan verification audit (
/tmp/closure-plan-verification-report.md,local-only research artefact). Two parallel motivations folded into
one commit (
50d0edb): audit-surfaced P0 fixes + verification-agent-sidebroken-link repair.
What's in this PR (22 files, +56/-49)
P0 audit-surfaced fixes
forgelm/cli/subcommands/_safety_eval.py:128-129Add explicit
trust_remote_code=Falseto the 2×from_pretrainedcalls in the standalone safety-eval load path. Closes the only
remaining defence-in-depth gap against Faz 7 §13 acceptance:
forgelm safety-eval --model <hub-name>could otherwise load Hubmodels with
trust_remote_code=Truedefaults silently.(en+tr).
byod-domain-expert→domain-expertandmath-reasoning→grpo-math(the canonical handles perforgelm/templates/). Closes Faz 1 acceptance grep that theaudit found still failing.
annex_iv.json→annex_iv_metadata.jsonsweep in 10 user-manual files. The canonical filename ForgeLM
writes is
annex_iv_metadata.json(percompliance.py:1115andtrainer.py:1341); the verifier's argparse placeholder<annex_iv.json>is preserved (operators name their own copies).docs/reference/usage.md+usage-tr.md"18 GPUmodels" → "16" (live count per
forgelm.trainer.ForgeTrainer._GPU_PRICING). Closes the trivialWave-1 carry-over registry C-1 still open at HEAD.
Agent-side reference cleanup (7 standards/roadmap files)
The closure-plan verification agents discovered 7 public-facing
docs/standards/*.mdanddocs/roadmap/*.mdfiles inline-linkingto docs under
docs/marketing/anddocs/analysis/QKV-Core/—both gitignored. On any public site or fresh checkout the links
404. Each link target swapped to a public surface
(
CLAUDE.md"What ForgeLM is not" ormaster-review-opus).No semantic change to the underlying guidance; only link targets.
Self-review gauntlet (all green at HEAD
50d0edb)ruff format --check .+ruff check .check_bilingual_parity --strictcheck_anchor_resolution --strictcheck_cli_help_consistency --strictcheck_site_claims --strictcheck_field_descriptions --strictpytest --no-cov -q tests/forgelm --config config_template.yaml --dry-runbuild_usermanuals.pyAudit context
The full 9-agent verification report — covering all 38 phases of
docs/analysis/code_reviews/closure-plan-202604300906.md, plus§11 dimension-exit gates and §15.5 carry-over registry — was
authored at
/tmp/closure-plan-verification-report.md(localresearch artefact, not committed pending owner review).
Headline finding: closure plan claim of "geride iş yok / stub
kalmadı / ghost feature kalmadı" is substantively delivered
for user-facing surface (CLI, exit codes, audit events,
GDPR/ISO-SOC2/Library API contracts, bilingual parity, CI guards).
Residue: internal code-cohesion ceiling drift
(
forgelm/cli/_parser.py1135 lines vs documented 662;forgelm/data_audit/_orchestrator.py504 lines vs ceiling 400),3 over-claimed Faz 28 task bullets (
--allow-custom-converter,pytest-xdist,_chunk_semantic), and a small NIT cluster.This PR absorbs the load-bearing P0 items before Faz 33 PyPI
tag. The HIGH ceiling violations + 3 over-claimed Faz 28 bullets
are recommended for v0.6.x formal re-classification in a
closure-plan v3 revision (NOT this PR).
Test plan
pytest tests/1436 passed / 14 skippedforgelm --config config_template.yaml --dry-runexit 0build_usermanuals.pyproduces en/tr 65/65_safety_eval.pychange verified viatests/test_verification_toolbelt.pysite/js/usermanuals/{en,tr}.js) regenerated by CI deploy workflow on merge — gitignored, not committed🤖 Generated with Claude Code
Summary by Sourcery
Tighten safety-eval model loading defaults and align documentation references with current templates, artifact filenames, and public-facing standards links.
Bug Fixes:
Enhancements:
Documentation:
Summary by CodeRabbit
Documentation
Bug Fixes