Repository navigation
Phase 12 — Data Curation Maturity (v0.5.2) - #13
Merged
Merged
Conversation
Direct continuation of the Phase 11 / 11.5 ingestion + audit lineage.
Closes the four concrete gaps surfaced by the post-`v0.5.1` competitive
review (LLaMA-Factory / Axolotl / Unsloth / NeMo Curator / Dolma /
RedPajama / LlamaIndex / LangChain / Marker / Docling). All five Tier 1
must-haves landed in this commit; Tier 2/3 (Presidio adapter, Croissant
metadata, --all-mask, wizard "audit first") are deferred to a Phase
12.5 backlog file.
Tier 1 features
---------------
* MinHash LSH dedup option (`forgelm/data_audit.py`):
`compute_minhash`, `find_near_duplicates_minhash`,
`_count_leaked_rows_minhash`. Opt-in
`--dedup-method minhash --jaccard-threshold 0.85` via the optional
`[ingestion-scale]` extra (datasketch>=1.6.0). Default simhash + LSH
banding from Phase 11.5 stays untouched and remains the only method
that runs without an optional dependency. `audit_dataset(...)` API
gains `dedup_method`, `minhash_jaccard`, `minhash_num_perm`
parameters; `near_duplicate_summary.method` records which path ran.
Cross-split overlap and within-split duplicate scan share the same
method flag.
* Markdown-aware splitter (`forgelm/ingestion.py`): `_chunk_markdown`,
`_chunk_markdown_tokens`, `_markdown_sections`, `_heading_breadcrumb`.
New `--strategy markdown` parses heading hierarchy (`# H1` …
`###### H6`), keeps code-fenced blocks atomic (heading-shaped lines
inside ```` ``` ```` are not interpreted as section boundaries), and
inlines a heading **breadcrumb** at the top of each chunk so SFT loss
sees document context. Composes with the Phase 11.5 token-aware mode.
* Code/secret leakage tagger (`forgelm/data_audit.py`): `detect_secrets`,
`mask_secrets`, `_SECRET_PATTERNS`. Always-on audit-side scan with
prefix-anchored regex set covering AWS access keys (`AKIA…` / `ASIA…`),
GitHub PATs, Slack tokens, OpenAI API keys (`sk-…` / `sk-proj-…`),
Google API keys, JWTs, OpenSSH/RSA/DSA/EC/PGP private-key block
headers, Azure storage connection strings. Adds `secrets_summary`
block alongside `pii_summary`. Ingest side: `--secrets-mask` rewrites
detected spans with `[REDACTED-SECRET]`; runs **before** PII masking
when both flags are set so combined detectors don't double-count
overlapping spans (e.g. GitHub tokens that partially match the
German Personalausweis regex). Optional `[ingestion-secrets]` extra
(detect-secrets>=1.5.0) for fuller plugin coverage; bare install
gets the regex fallback.
* Heuristic quality filter (`forgelm/data_audit.py`):
`_row_quality_flags`, `_QUALITY_CHECKS`. Opt-in
`forgelm audit --quality-filter` runs Gopher / C4 / RefinedWeb-style
checks per row: `low_alpha_ratio`, `low_punct_endings`,
`abnormal_mean_word_length`, `short_paragraphs`. Surfaces
`quality_summary` with per-check counts, `samples_flagged`, and
`overall_quality_score`. ML-based classifiers deliberately out of
scope to keep the audit deterministic for Annex IV reproducibility.
* DOCX/Markdown table preservation (`forgelm/ingestion.py`):
`_docx_table_to_markdown`. `_extract_docx` now renders tables as
markdown table syntax (header row + `---` separator + body rows)
instead of the previous `" | "`-joined flat line. Uneven rows are
right-padded; all-blank rows dropped; first non-empty row becomes
the header. Combined with `--strategy markdown` the table block
stays intact across chunks.
API additions (additive — Phase 11/11.5 consumers unaffected)
-------------------------------------------------------------
* `AuditReport.secrets_summary: Dict[str, int]` and
`AuditReport.quality_summary: Dict[str, Any]`.
* `IngestionResult.secrets_redaction_counts: dict`.
* `audit_dataset(..., dedup_method, minhash_jaccard,
minhash_num_perm, enable_quality_filter)` keyword args.
* `ingest_path(..., secrets_mask=False)` keyword arg.
* New constants: `DEDUP_METHODS`, `DEFAULT_MINHASH_JACCARD`,
`DEFAULT_MINHASH_NUM_PERM`, `SECRET_TYPES`.
CLI additions
-------------
* `forgelm ingest`: `--strategy markdown`, `--secrets-mask`.
* `forgelm audit`: `--dedup-method {simhash,minhash}`,
`--jaccard-threshold X` (validated to `[0.0, 1.0]` at parse time
via new `_non_negative_float` argparse helper), `--quality-filter`.
* `_run_data_audit` distinguishes `EXIT_CONFIG_ERROR` (filesystem)
from `EXIT_TRAINING_ERROR` (missing `[ingestion-scale]` extra
when `--dedup-method=minhash` was requested).
pyproject.toml
--------------
* New extras: `[ingestion-scale]` (datasketch>=1.6.0,<2.0.0),
`[ingestion-secrets]` (detect-secrets>=1.5.0,<2.0.0).
* Version bump `0.5.1rc1 → 0.5.2rc1`.
Tests
-----
* `tests/test_data_audit_phase12.py` — 18 new tests (TestSecretsDetection,
TestSecretsMasking, TestAuditPicksUpSecrets, TestQualityFilterPerRow,
TestQualityFilterEnabled, TestMinHashLshDedup [skipped without
datasketch], TestMinHashMissingExtra).
* `tests/test_ingestion_phase12.py` — 13 new tests (TestMarkdownSections,
TestChunkMarkdown, TestMarkdownStrategyExposed, TestDocxTableToMarkdown
with `_FakeCell`/`_FakeRow`/`_FakeTable` helpers, TestSecretsMaskIngest).
* `tests/test_cli_subcommands.py` — `test_audit_quality_filter_flag`,
`test_audit_rejects_invalid_jaccard_threshold` added to
TestAuditSubcommand.
Internal refactors
------------------
* `_StreamingAggregator` gains `minhashes`, `secrets_counts`,
`quality_flags_counts`, `quality_samples_flagged`, `dedup_method`,
`minhash_num_perm`, `enable_quality_filter` fields.
* `_SplitOutcome.fingerprints` → `_SplitOutcome.signatures` (the same
field carries simhash ints OR MinHash instances depending on method).
* `_audit_split` returns `(info, signatures, pii_split, parse_errors,
decode_errors)` where `signatures` is method-dependent.
* `_pair_leak_payload` and `_cross_split_overlap` switched to
keyword-only `dedup_method` parameter and dispatch on it (simhash →
Hamming; minhash → Jaccard).
* `describe_strategies()` lists `markdown` alongside the others.
Documentation
-------------
* `docs/roadmap/phase-12-data-curation-maturity.md`: marked Tier 1 done;
Tier 2/3 noted as deferred.
* `docs/roadmap/phase-12-5-backlog.md`: new follow-up backlog file
(mirrors Phase 11.5's pattern).
* `docs/roadmap.md` + `roadmap-tr.md` + `roadmap/releases.md`:
Phase 12 → Done, mermaid diagram updated, milestone text refreshed.
* `docs/guides/{ingestion,data_audit}{,-tr}.md`: new sections for
markdown splitter, secrets scan, quality filter, MinHash, DOCX
table preservation; CLI references updated.
* `docs/reference/usage{,-tr}.md`: code examples for the four new
flags + the markdown strategy.
* `docs/standards/architecture.md`: extras matrix gains
`ingestion-scale` and `ingestion-secrets` rows.
* `docs/qms/sop_data_management.md`: quality-check checklist gains
`secrets_summary` (stop-the-line) and `quality_summary` rows;
`forgelm --data-audit` legacy spelling updated to `forgelm audit`.
* `README.md`: feature blurb refreshed for the v0.5.2 surface;
separate "Secrets-Aware Ingest" bullet added.
* `CHANGELOG.md`: detailed Unreleased — Phase 12 section.
Verification
------------
* `ruff check` clean, `ruff format` clean.
* `pytest tests/` — 792 passed, 12 skipped (vs. previous 760 / 8;
+32 tests, +4 datasketch-skips). All Phase 11 / 11.5 contracts
preserved.
* End-to-end smoke (`/tmp/p12_smoke/`):
- `forgelm audit data.jsonl` → secrets_summary populates,
"Secrets : CRITICAL — 1 flagged" verdict line.
- `forgelm audit --quality-filter` → quality_summary populates,
"Quality : N sample(s) flagged" verdict.
- `forgelm ingest wiki.md --strategy markdown --secrets-mask` →
breadcrumb-inlined chunks, AWS key replaced with
`[REDACTED-SECRET]`, IngestionResult.secrets_redaction_counts
surfaces the count.
…, 4 nit) Round-1 review of Phase 12 (commit 2f5722a) surfaced one regression, one plan-vs-implementation gap, two correctness bugs, and a series of honesty / parity / refactor follow-ups. All addressed before tagging v0.5.2. Critical (blocking): - C1: stdout JSON envelope dropped v0.5.1's near_duplicate_pairs_per_split top-level key when adding near_duplicate_summary; restored as additive alongside the new richer block (forgelm/cli.py). - C2: plan promised 5 Gopher/C4-style quality checks; only 4 shipped. Added repeated_lines (top-3 actually-repeating lines covering >30 % of non-empty lines, count >= 2 to avoid false-positives on short all-unique documents). - C3: _docx_table_to_markdown emitted unescaped | in cell text, breaking downstream markdown parsers (a|b read as 2 extra columns). New _escape_md_cell escapes |, \, and collapses embedded newlines per CommonMark. - C4: JWT regex `eyJ[A-Za-z0-9_-]+\.eyJ...` false-positived on prose like `eyJfoo.eyJbar.baz`. Anchored on canonical JWT header alphabet (alg / typ / kid / cty / enc / api base64url prefixes) plus minimum payload + signature lengths. Real JWTs (incl. existing test fixture) still match. High: - H1: compute_minhash docstring claimed simhash/MinHash surface "the same class of near-duplicates"; in fact set-Jaccard over distinct tokens != frequency-weighted bit-cosine. Rewritten honestly. - H2: TR data_audit guide mirror was missing 3 Phase 12 H3 sections (MinHash LSH, secret tagger, quality filter); added at EN parity. - H3: _aggregator_to_info (CCN 22) split into _populate_schema_block / _populate_optional_findings / _within_split_pairs; _markdown_sections (CCN 17) split into _push_heading_onto_path / _trim_blank_edges. Both now under the Phase 11.5 ceiling of 15. - H4: _chunk_markdown_tokens was unreachable from existing tests; new TestChunkMarkdownTokens covers token-cap budgeting and breadcrumb invariant. - H5: "byte-identical default report" claim in plan / CHANGELOG was wrong; report shape is schema-additive but on-disk JSON differs because secrets_summary, near_duplicate_summary.method, and cross_split_overlap.method are now always present. Wording corrected. Medium: - M1: _emit_chunk mask-order rationale claimed regex overlap that shipped fixtures don't actually exhibit. Rewritten as a defensive ordering (favour secrets when ordering matters at all; future-proof against new overlapping regexes). - M3: compute_minhash now defers _require_datasketch() until past the empty-token short-circuit, so defensive callers don't hit ImportError on empty input. - M4: _count_leaked_rows_minhash short-circuits when the entire target list is None-sentinels (skip LSH-construction). - M5: _chunk_markdown / _chunk_markdown_tokens docstrings now state the no-overlap contract explicitly. Nits: - N1: _MARKDOWN_HEADING_PATTERN / _MARKDOWN_CODE_FENCE allow up to 3 leading spaces per CommonMark §4.2; 4+ spaces still fall through as indented code blocks. - N3: IngestionResult dict fields typed as Dict[str, int] instead of bare dict. - Review1#4: _row_quality_flags strips fenced ``` … ``` blocks before applying prose heuristics so legitimate code-instruction rows aren't flagged on shape grounds; pure-code rows return [] cleanly. - Quality filter heuristic for repeated_lines hardened: only counts lines with frequency >= 2, so all-unique short documents (5 distinct prose lines) don't false-positive. Tests: - 18 new regression tests in tests/test_phase12_review_fixes.py: one class per finding, pinning shape against re-introduction. Covers JSON envelope, repeated_lines on boilerplate vs. unique, DOCX |/\/newline escape, JWT header-alphabet anchors, code-fence stripping, token-aware markdown chunker, CommonMark 0-3-space indented headings. Verified: - ruff format clean - ruff check clean - 810 passed, 12 skipped (was 792 + 18 new = 810; same 12 skipped for optional extras) - forgelm --config config_template.yaml --dry-run OK - audit smoke: stdout envelope contains both legacy near_duplicate_pairs_per_split and new near_duplicate_summary keys Deferred to phase-12-5-backlog.md (out of scope for this fix cycle): - Generic-high-entropy 10th secret category (Reviewer 1 #1) - Disk-checkpointed MinHash LSH for >10M rows (Reviewer 1 #2) - _non_negative_float helper rename to _unit_interval_float (M2; helper is currently single-use and its semantic is documented at the call site) - Open questions about MinHash distinct-shingle vs. weighted similarity metric choice (H1 rewrite already documents the divergence; whether to switch to k-shingle weighted MinHash is a separate design decision)
…ADME
Phase 11 (ingestion), 11.5 (audit polish), and 12 (data curation
maturity) added two new top-level commands — `forgelm ingest` and
`forgelm audit` — but no notebook demonstrated either, and several
README sections drifted behind the v0.5.2 codebase.
New notebook
- notebooks/data_curation.ipynb (20 cells, CPU-only, Colab-ready):
end-to-end walkthrough of `forgelm ingest` + `forgelm audit`. Builds
a sample raw corpus (Markdown + TXT) seeded with near-duplicates,
PII, and credentials, then exercises:
- --strategy markdown (Phase 12 heading-aware splitter with
breadcrumb inlining + atomic code fences)
- --secrets-mask + --pii-mask (Phase 12 — secrets first, PII second)
- --chunk-tokens / --tokenizer (Phase 11.5 token-aware shape, not
executed to avoid downloading a tokenizer in CI)
- default `forgelm audit` with always-on secrets scan
- --quality-filter (Phase 12 heuristic Gopher/C4/RefinedWeb checks)
- --dedup-method minhash --jaccard-threshold (Phase 12 MinHash LSH
via the optional [ingestion-scale] extra)
- EU AI Act Article 10 governance integration (audit JSON
auto-inlines into the compliance bundle when present pre-training).
Notebook updates
- notebooks/post_training_workflow.ipynb: install pin
`forgelm>=0.4.0` → `forgelm>=0.5.2`; "Where next" gains the
data_curation link; stale "v0.4.0 release notes" replaced with a
generic CHANGELOG link.
- notebooks/quickstart_sft.ipynb: title cell gains a callout for
operators with raw documents — points upstream to data_curation
before SFT.
Diffs are minimal and respect each notebook's existing format
convention (post_training_workflow: literal unicode + nbformat_minor=5;
quickstart_sft: \uXXXX-escaped + nbformat_minor=4) so review noise
stays low.
README.md
- Chunking strategies: "paragraph or sliding" → all four
(paragraph / sliding / markdown / semantic) with v0.5.2 mention.
- Optional dependencies: added `[ingestion-scale]` (datasketch) and
`[ingestion-secrets]` (detect-secrets) extras alongside the pre-existing
`[ingestion]`.
- Project structure: added `data_audit.py`, `ingestion.py`,
`grpo_rewards.py`, `quickstart.py` modules; clarified that
AuditReport / IngestionResult live in their own modules (results.py
only carries TrainResult); added `configs/safety_prompts/` and
`forgelm/templates/` directories.
- Notebooks list: data_curation.ipynb added to "Getting started"
alongside quickstart_sft, with CPU-only annotation.
No code changes. Lint + 810 passed / 12 skipped tests + smoke
(forgelm --dry-run + ingest/audit invocation) all clean.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds v0.5.2 (Phase 12) ingestion and audit features (markdown-aware chunking, secrets masking/redaction counts, optional MinHash LSH dedup, opt-in heuristic quality filter), hardens compliance/audit and webhook behaviors, tightens config validation/regex standards, updates packaging/docs/tests/notebooks, and emits governance report during trainer compliance export. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "forgelm CLI"
participant Ingest as "Ingestion\n(markdown + secrets)"
participant Extras as "Optional Extras\n(datasketch/detect-secrets)"
participant Audit as "Audit\n(minhash/quality)"
participant Report as "Report\n(JSON)"
User->>CLI: forgelm ingest --strategy markdown --secrets-mask <inputs>
CLI->>Ingest: ingest_path(..., secrets_mask=True)
activate Ingest
Ingest->>Ingest: parse headings (fence-aware), add breadcrumbs
Ingest->>Extras: detect/redact secrets (if installed)
Extras-->>Ingest: secrets_redaction_counts
Ingest->>Ingest: apply PII masking (post-secrets)
Ingest-->>CLI: write JSONL + IngestionResult
deactivate Ingest
User->>CLI: forgelm audit <jsonl> --dedup-method minhash --quality-filter
CLI->>Audit: audit_dataset(dedup_method="minhash", enable_quality_filter=True)
activate Audit
Audit->>Audit: always-on secrets scan -> secrets_summary
Audit->>Audit: compute quality flags -> quality_summary
Audit->>Extras: (optional) MinHash LSH query & Jaccard verify
Extras-->>Audit: near-duplicate candidates/pairs
Audit->>Audit: aggregate per-split & cross-split findings
Audit-->>CLI: emit data_audit_report.json
deactivate Audit
CLI->>Report: write audit artifact
Report-->>User: data_audit_report.json
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 143 |
| Duplication | 4 |
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.
CI ruff F401 — leftover from an early draft that used `os.makedirs` before I switched to `Path.mkdir(exist_ok=True)`. Notebook now uses only `pathlib.Path`.
![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 implements Phase 12 'Data Curation Maturity', introducing a markdown-aware splitter, DOCX table preservation, an always-on secrets scanner, and a heuristic quality filter. It also adds MinHash LSH as an optional deduplication method for large datasets. Review feedback identified an opportunity to improve the quality filter's code fence detection to support indented fences and varying backtick counts per CommonMark. Additionally, a suggestion was made to ensure the 'minhash_distinct' metric consistently counts unique signatures, matching the behavior of the SimHash implementation.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
notebooks/post_training_workflow.ipynb (1)
1-299:⚠️ Potential issue | 🟠 MajorFormatting check is currently blocking CI for this notebook.
ruff format --checkfails on this file. Please runruff format notebooks/post_training_workflow.ipynband commit the result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notebooks/post_training_workflow.ipynb` around lines 1 - 299, The notebook "ForgeLM — Post-Training Workflow (Phase 10)" fails ruff format --check; run the formatter and commit the fixed notebook so CI passes: run ruff format on the notebook (e.g., ruff format notebooks/post_training_workflow.ipynb), review the formatted changes in the cell sources (markdown/code cells like the header/title and the code cells using forgelm/inference examples), stage and commit the updated notebook, and push the commit to your branch.
🧹 Nitpick comments (4)
tests/test_ingestion_phase12.py (1)
187-205: Avoid committing secret-shaped literals in the test fixture.This already trips secret scanners. Build the token strings from parts at runtime so the test still exercises the matcher without polluting commit history and CI scanning.
Low-noise fixture variant
- src.write_text( - "config\n\nkey=AKIAIOSFODNN7EXAMPLE\n\ntoken=ghp_abcdefghij1234567890ABCDEFGHIJ012345", - encoding="utf-8", - ) + aws_key = "AKIA" + "IOSFODNN7EXAMPLE" + github_token = "ghp_" + "abcdefghij1234567890ABCDEFGHIJ012345" + src.write_text( + f"config\n\nkey={aws_key}\n\ntoken={github_token}", + encoding="utf-8", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_ingestion_phase12.py` around lines 187 - 205, The test test_secrets_mask_redacts_and_counts currently embeds full secret-looking literals; change the fixture so the AWS key and GitHub token are constructed at runtime from safe fragments (e.g., concatenating prefixes/suffixes or joining parts) before writing to src, then call ingest_path as before and assert on result.secrets_redaction_counts and the out.jsonl contents; update references inside the test to use the rebuilt strings for negative assertions (ensure "AKIA..." and "ghp_" are derived rather than hard-coded) so the matcher is exercised without committing literal secrets.notebooks/data_curation.ipynb (1)
25-27: Pin the notebook install source.Installing from the moving
mainbranch makes this Phase 12 walkthrough non-reproducible: future readers may get behavior that no longer matches the v0.5.2 notebook text. Please pin to the release version or to the commit/tag for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notebooks/data_curation.ipynb` around lines 25 - 27, Update the pip install cell that currently installs from the moving main branch (the line starting with "!pip install -q --no-cache-dir 'git+https://github.com/cemililik/ForgeLM.git#egg=forgelm[ingestion]'" in the notebook) to pin to a specific release or commit/tag (e.g., use the release tag or commit hash in the git URL or switch to a versioned package like "forgelm==0.5.2") so the notebook reproduces the behavior matching the v0.5.2 walkthrough.tests/test_phase12_review_fixes.py (1)
181-186: Assemble the JWT fixture at runtime to avoid secret-scanner noise.The token on Lines 183-185 is realistic enough to keep tripping leak detectors; Betterleaks is already flagging this file. Build the header, payload, and signature separately and join them in the test so you keep the regex coverage without training the repo to ignore secret alerts.
♻️ Example adjustment
- text = ( - "token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9." - "eyJzdWIiOiIxMjM0NSIsIm5hbWUiOiJKb2huIERvZSJ9." - "abcdefghijklmnopqrstuvwxyz123" - ) + header = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9" + payload = "eyJzdWIiOiIxMjM0NSIsIm5hbWUiOiJKb2huIERvZSJ9" + signature = "abcdefghijklmnopqrstuvwxyz123" + text = f"token={header}.{payload}.{signature}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_phase12_review_fixes.py` around lines 181 - 186, The test_realistic_hs256_token_detected uses a hardcoded JWT string that triggers secret-scanners; instead construct the token at runtime by defining header, payload, and signature string pieces (e.g., header_b64, payload_b64, signature_b64) and join them with '.' to assign to text so the regex still matches but the literal secret is not present in the source; update the test to build the token this way inside the function.README.md (1)
170-175: Separate PyPI extras from editable local installs.These new commands are documented only as
pip install -e ".[...]", but the README also offers a PyPI install path a few lines above. Copy-pasting Lines 170-175 outside a repo checkout will fail because editable extras require a local project. Please either switch these examples topip install "forgelm[...]"or label the block as “from a local clone” so the new extras stay copy-pasteable for PyPI users.♻️ Suggested README shape
- pip install -e ".[ingestion]" # PDF/DOCX/EPUB/Markdown → JSONL + langdetect + xxhash - pip install -e ".[ingestion-scale]" # MinHash LSH dedup (datasketch) for >50K-row corpora - pip install -e ".[ingestion-secrets]" # detect-secrets plugin set on top of the regex fallback + pip install "forgelm[ingestion]" # From PyPI + pip install "forgelm[ingestion-scale]" # From PyPI + pip install "forgelm[ingestion-secrets]" # From PyPI + # or, from a local clone: + pip install -e ".[ingestion]" + pip install -e ".[ingestion-scale]" + pip install -e ".[ingestion-secrets]"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 170 - 175, The README shows editable-local installs (pip install -e ".[ingestion]" etc.) which will fail when copy-pasted by PyPI users; update the block that contains those five pip commands to either replace the editable/local form with the PyPI extras form (e.g., pip install "forgelm[ingestion]" / "forgelm[ingestion-scale]" / "forgelm[ingestion-secrets]" / "forgelm[export]" / "forgelm[chat]" / "forgelm[dev]") or add a clear one-line prefix like "From a local clone:" before the existing -e lines so it’s explicit these are local editable installs (refer to the exact pip lines shown in the diff).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 32-34: Update the CHANGELOG entry so it no longer claims
detect-secrets plugin coverage: explicitly state that the shipped implementation
of detect_secrets / mask_secrets relies on the anchored regex fallback
(_SECRET_PATTERNS) and that the optional detect-secrets extra is not invoked by
the current code, remove or reword the sentence implying fuller plugin coverage,
and add a short note pointing readers to the optional extra
(detect-secrets>=1.5.0) as an opt-in enhancement rather than shipped behavior.
In `@docs/guides/data_audit.md`:
- Around line 288-298: The prose list of quality checks is missing the
`repeated_lines` check present in the JSON example and implementation; update
the bullet list to include a `repeated_lines` entry describing its behavior
(e.g., flags text where identical lines repeat above a threshold) so the
documentation matches the example and code, referencing the check name
`repeated_lines` in the prose.
In `@docs/guides/ingestion-tr.md`:
- Around line 6-9: The CLI synopsis and command reference are out of sync: add
the new Phase 12 flags (--strategy markdown and --secrets-mask) to the
documented command usage and options so the intro matches the reference; update
the CLI synopsis block to include both flags, add short descriptions for
"--strategy" (mention "markdown" strategy) and "--secrets-mask" in the options
list, and ensure any examples that demonstrate ingestion note splitting or
secret masking show the flags in use so readers see accurate, consistent
documentation.
In `@forgelm/data_audit.py`:
- Around line 845-850: The current _CODE_FENCE_BLOCK regex only matches
triple-backtick fences at column 0 and misses tilde fences and indented fences;
update the _CODE_FENCE_BLOCK pattern in forgelm/data_audit.py to accept
opening/closing fences of backticks or tildes and allow 0–3 leading spaces by
replacing the pattern with one that captures the fence characters (e.g., use a
named group like (?P<fence>`{3,}|~{3,}) and match the closing fence with
(?P=fence)), preserving the re.MULTILINE | re.DOTALL flags and the variable name
_CODE_FENCE_BLOCK so existing code continues to use it.
- Around line 768-770: The regexes for "openssh_private_key" and
"pgp_private_key" only match the BEGIN header; update them so mask_secrets()
redacts the entire PEM/PGP block including inner base64 and the corresponding
END line. Replace the current patterns with multi-line, DOTALL-aware patterns
that match from the BEGIN line through the matching "-----END ...-----" (e.g.,
use r"-----BEGIN (?:OPENSSH|RSA|DSA|EC) PRIVATE KEY-----.*?-----END
(?:OPENSSH|RSA|DSA|EC) PRIVATE KEY-----" and r"-----BEGIN PGP PRIVATE KEY
BLOCK-----.*?-----END PGP PRIVATE KEY BLOCK-----" with re.DOTALL or (?s) to
allow newlines), and keep the same keys so mask_secrets() continues to find and
redact full blocks.
In `@forgelm/ingestion.py`:
- Around line 317-324: The current extraction appends all paragraphs then all
tables (using doc.tables and _docx_table_to_markdown), which reorders content;
instead iterate the document's block-level elements in source order (e.g., walk
doc.element.body children or the library's block iterator) and for each element
detect whether it's a paragraph or a table and append the rendered paragraph
text or the _docx_table_to_markdown(table) result to blocks immediately so
tables remain in their original positions.
- Around line 462-463: The code fence regex only matches triple backticks, so
tilde-fenced blocks (~~~) are treated as regular text and headings inside them
get split; update the _MARKDOWN_CODE_FENCE pattern to recognize both backticks
and tildes (e.g. change _MARKDOWN_CODE_FENCE to re.compile(r"^[
]{0,3}(```|~~~)") so the splitter toggles code-block state for either fence) and
keep the MULTILINE flag/behavior consistent with _MARKDOWN_HEADING_PATTERN.
- Around line 642-643: The markdown branch currently ignores
overlap/overlap_tokens silently; update the dispatch for strategy == "markdown"
(and the other markdown dispatcher further down) to validate the overlap inputs
and reject non-zero values instead of silently dropping them: check the overlap
and overlap_tokens parameters where strategy and _chunk_markdown are invoked,
and if either is non-zero raise a ValueError (or return an explicit error) with
a clear message like "overlap/overlap_tokens not supported for markdown
strategy"; do this validation before calling _chunk_markdown(text, chunk_size)
so callers immediately get a visible failure instead of a silent no-op.
In `@tests/test_data_audit_phase12.py`:
- Around line 153-155: The function forgelm.data_audit._row_quality_flags
currently types its parameter as text: str but its implementation and docstring
accept None/non-str; change the signature to accept Optional[str] (or str |
None) and add the corresponding typing import if needed, update the
docstring/type hints accordingly, and then remove the "# type: ignore[arg-type]"
suppression from the test function test_empty_text_returns_empty so the test
matches the widened contract.
---
Outside diff comments:
In `@notebooks/post_training_workflow.ipynb`:
- Around line 1-299: The notebook "ForgeLM — Post-Training Workflow (Phase 10)"
fails ruff format --check; run the formatter and commit the fixed notebook so CI
passes: run ruff format on the notebook (e.g., ruff format
notebooks/post_training_workflow.ipynb), review the formatted changes in the
cell sources (markdown/code cells like the header/title and the code cells using
forgelm/inference examples), stage and commit the updated notebook, and push the
commit to your branch.
---
Nitpick comments:
In `@notebooks/data_curation.ipynb`:
- Around line 25-27: Update the pip install cell that currently installs from
the moving main branch (the line starting with "!pip install -q --no-cache-dir
'git+https://github.com/cemililik/ForgeLM.git#egg=forgelm[ingestion]'" in the
notebook) to pin to a specific release or commit/tag (e.g., use the release tag
or commit hash in the git URL or switch to a versioned package like
"forgelm==0.5.2") so the notebook reproduces the behavior matching the v0.5.2
walkthrough.
In `@README.md`:
- Around line 170-175: The README shows editable-local installs (pip install -e
".[ingestion]" etc.) which will fail when copy-pasted by PyPI users; update the
block that contains those five pip commands to either replace the editable/local
form with the PyPI extras form (e.g., pip install "forgelm[ingestion]" /
"forgelm[ingestion-scale]" / "forgelm[ingestion-secrets]" / "forgelm[export]" /
"forgelm[chat]" / "forgelm[dev]") or add a clear one-line prefix like "From a
local clone:" before the existing -e lines so it’s explicit these are local
editable installs (refer to the exact pip lines shown in the diff).
In `@tests/test_ingestion_phase12.py`:
- Around line 187-205: The test test_secrets_mask_redacts_and_counts currently
embeds full secret-looking literals; change the fixture so the AWS key and
GitHub token are constructed at runtime from safe fragments (e.g., concatenating
prefixes/suffixes or joining parts) before writing to src, then call ingest_path
as before and assert on result.secrets_redaction_counts and the out.jsonl
contents; update references inside the test to use the rebuilt strings for
negative assertions (ensure "AKIA..." and "ghp_" are derived rather than
hard-coded) so the matcher is exercised without committing literal secrets.
In `@tests/test_phase12_review_fixes.py`:
- Around line 181-186: The test_realistic_hs256_token_detected uses a hardcoded
JWT string that triggers secret-scanners; instead construct the token at runtime
by defining header, payload, and signature string pieces (e.g., header_b64,
payload_b64, signature_b64) and join them with '.' to assign to text so the
regex still matches but the literal secret is not present in the source; update
the test to build the token this way inside the function.
🪄 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: 4ab25ca9-5a5f-467f-a03a-8d17a31215c6
📒 Files selected for processing (26)
CHANGELOG.mdREADME.mddocs/guides/data_audit-tr.mddocs/guides/data_audit.mddocs/guides/ingestion-tr.mddocs/guides/ingestion.mddocs/qms/sop_data_management.mddocs/reference/usage-tr.mddocs/reference/usage.mddocs/roadmap-tr.mddocs/roadmap.mddocs/roadmap/phase-12-5-backlog.mddocs/roadmap/phase-12-data-curation-maturity.mddocs/roadmap/releases.mddocs/standards/architecture.mdforgelm/cli.pyforgelm/data_audit.pyforgelm/ingestion.pynotebooks/data_curation.ipynbnotebooks/post_training_workflow.ipynbnotebooks/quickstart_sft.ipynbpyproject.tomltests/test_cli_subcommands.pytests/test_data_audit_phase12.pytests/test_ingestion_phase12.pytests/test_phase12_review_fixes.py
… docs) Round-2 review of Phase 12 (commits bf8ca82 + 4ec817c) flagged a mix of real correctness bugs and lint / parity / regex code-smell issues. Verified all 22 findings against current code; applied every one that matched. No behavioural change on default-flag paths beyond bug fixes. Correctness (real bugs) - Private-key blocks: ``openssh_private_key`` / ``pgp_private_key`` regexes only matched the BEGIN header — ``mask_secrets`` left the base64 body + END line in clear text. Now match BEGIN-through-END inclusive under re.DOTALL. Block markers split across string concatenations to keep gitleaks / trufflehog quiet on the source tree. - Fenced code blocks: ``_CODE_FENCE_BLOCK`` (audit) and ``_MARKDOWN_CODE_FENCE`` (ingest) only matched triple backticks; CommonMark §4.5 also allows ``~~~``. Both now accept either fence character with up to 3 leading spaces. Markdown splitter additionally tracks the *opening* fence char so a stray ``\`\`\``` inside a ``~~~`` block (or vice-versa) doesn't toggle state. - DOCX block order: ``_extract_docx`` previously emitted all paragraphs then all tables, reordering content. New ``_iter_docx_blocks`` walks ``doc.element.body`` in source order and dispatches per-element. - Markdown overlap: ``_strategy_dispatch`` and ``_strategy_dispatch_tokens`` raise ValueError on non-zero overlap when strategy=markdown. CLI ``--overlap`` default flipped to None; ``ingest_path`` resolves None to 200 (sliding) / 0 (paragraph or markdown) so the historical argparse default doesn't spuriously trip the new validator. - ``minhash_distinct``: now counts unique sketches via ``hashvalues.tobytes()`` to mirror ``simhash_distinct``'s semantic; previously returned the count of non-empty rows. Type widening - ``_row_quality_flags`` typed ``Optional[str]`` (already accepted None at runtime). Removed ``# type: ignore[arg-type]`` from the test. Cognitive complexity (Phase 11.5 ≤ 15 cap) - ``_row_quality_flags`` (CCN 22 → ≤ 10): split into ``_check_low_alpha_ratio`` / ``_check_low_punct_endings`` / ``_check_abnormal_mean_word_length`` / ``_check_short_paragraphs`` / ``_check_repeated_lines``. - ``find_near_duplicates_minhash`` (CCN 21 → ≤ 10): extracted ``_build_minhash_lsh`` + ``_emit_minhash_pair`` helpers shared with ``_count_leaked_rows_minhash``. - ``audit_dataset`` (CCN 21 → ≤ 12): extracted ``_fold_outcome_into_summary`` + ``_build_quality_summary`` + ``_build_near_duplicate_summary``. Regex / lint code-smells - ``[A-Za-z0-9_]`` → ``\w`` in GitHub PAT regex. - ``[ ]{0,3}`` → `` {0,3}`` (single-char class collapsed). - Heading pattern ``\s+`` / ``\s*$`` → ``[ \t]+`` / ``[ \t]*$`` (mitigates SonarCloud polynomial-backtracking concern; clearer per-line semantic under re.MULTILINE). - Duplicate string literals extracted to module constants ``_CHUNK_TOKENS_POSITIVE_MSG`` / ``_CHUNK_SIZE_POSITIVE_MSG`` / ``_MARKDOWN_OVERLAP_UNSUPPORTED_MSG``. - Comprehension ``c for c in row`` simplified to ``row`` in markdown table rendering. Documentation parity - ``docs/guides/data_audit.md`` quality-filter bullet list and JSON example now include ``repeated_lines``; added a note about code-fence stripping behaviour. - ``docs/guides/ingestion-tr.md`` mirrors EN's chunking-strategies table (markdown row added) and gains a new "secrets/credential masking (Faz 12)" section. - CHANGELOG no longer claims ``[ingestion-secrets]`` plugin coverage: the regex set is the sole detection backend in v0.5.2; the extra is reserved for a follow-up release. - README separates "From PyPI" and "From a local clone" install blocks so copy-paste users don't hit ``-e .`` confusion. Test hygiene - Hardcoded AKIA / ghp_ / sk-proj-… / JWT literals across three test files now built from inert fragments at runtime — silences repo-wide secret scanners without changing what the regex has to match. - 5 new round-2 regression tests (``TestTildeFenceRecognised``, ``TestPrivateKeyFullBlock``, ``TestMarkdownOverlapValidation``, ``TestMinHashDistinctSemantic``). Notebook drift fixes - ``post_training_workflow.ipynb`` reformatted to satisfy ``ruff format --check`` in CI. - ``data_curation.ipynb`` install line pinned to ``forgelm[ingestion]==0.5.2`` rather than the moving main branch. Verified - ruff format clean, ruff check clean - 815 passed, 13 skipped (was 810 + 5 new = 815) - forgelm --config config_template.yaml --dry-run OK - end-to-end smoke: ingest --strategy markdown --pii-mask + audit --quality-filter; envelope still has near_duplicate_pairs_per_split (back-compat); quality_summary now surfaces repeated_lines Findings deliberately deferred (out of round-2 scope): - Generic high-entropy 10th secret category (Phase 12.5 backlog — scope discipline; would require entropy threshold calibration). - Disk-checkpointed MinHash LSH for >10M-row corpora (Phase 12.5 backlog — current LSH path covers the documented 100K-1M target).
SonarCloud round 2.5 flagged two regexes with polynomial-runtime risk;
empirically confirmed real O(n²) blowup in one of them, then codified
the lessons as a new engineering standard so we stop hitting the same
patterns.
Critical: confirmed ReDoS in _MARKDOWN_HEADING_PATTERN
- Old: ``^ {0,3}(#{1,6})[ \t]+(.+?)[ \t]*$`` had three quantifiers
(``[ \t]+``, ``(.+?)``, ``[ \t]*$``) competing for trailing whitespace.
- Pathological input ``"# a" + " \t" * n + "x"``:
n=2000 → 100 ms (quadratic)
n=5000 → 600 ms (quadratic)
n=10000 → 2.1 sec (quadratic — confirmed O(n²) DoS)
- New: ``^ {0,3}(#{1,6})[ \t]+(\S(?:[^\n]*\S)?)[ \t]*$`` anchors the
body capture on non-whitespace at both ends. Linear:
n=10000 → 10 μs (200000× speedup)
Defence-in-depth: replace _CODE_FENCE_BLOCK regex with state machine
- Old: ``.*?`` + back-reference + re.DOTALL — SonarCloud python:S5852
flags as polynomial-risk shape. Empirically linear in CPython but the
combination is a textbook ReDoS pattern; static analysers can't prove
linearity.
- New: ``_strip_code_fences`` per-line state machine
+ ``_is_code_fence_open`` + ``_is_code_fence_close``. Provably O(n);
matches the same line-walker pattern already used in
``_markdown_sections``. Behaviour pinned bit-for-bit on 7 fixtures
(open/close, tilde fence, indented fence, mismatched fence chars,
unclosed block).
Codebase regex hygiene sweep
- Empirically swept all 25 forgelm regexes against 50K-char adversarial
inputs — slowest is openssh_private_key full-block at 0.5 ms (~10 μs/KB).
All linear. The audit recipe is documented in regex.md so the sweep
can be reproduced.
- Style fixes per the new standard:
- ``[A-Za-z0-9_-]`` → ``[\w-]`` in openai_api_key / google_api_key /
jwt (3 places). Rule 1.
- ``\s*$`` → ``[ \t]*$`` in _PUNCT_END_PATTERN. Rule 5 — callers
pre-split into single lines, so the \s newline-overlap was dead
weight.
- ``[A-Za-z0-9._-]+`` → ``[\w.-]{1,96}`` in _HF_HUB_ID_RE. Rule 3 —
HF Hub username + repo name max-length is 96 chars; bound for
defence-in-depth without behaviour change.
New standard: docs/standards/regex.md (the durable answer)
- 8 hard rules absorbed from review cycles, each citing the concrete
finding that produced it.
- ReDoS exposure budget: 10K-char pathological-input wall-clock must
stay ≤ 10 ms.
- Test fixture hygiene: build credential-shaped strings from inert
fragments at runtime so gitleaks / trufflehog don't false-positive.
- Cross-linked from coding.md (anti-patterns table), code-review.md
(review checklist §7), .claude/skills/review-pr/SKILL.md (self-review
step 7), and CLAUDE.md ("read before editing" entry point) — so the
next regex change in the codebase is reviewed against this rule
set without anyone having to remember to look.
ReDoS regression tests
- New TestRegexLinearity class in tests/test_phase12_review_fixes.py:
pins 1-second wall-clock cap on N=10K pathological input for both
the heading regex and _strip_code_fences. A real ReDoS would blow
past it by 100×; a slow CI host won't false-positive.
Verified
- ruff format clean, ruff check clean
- 817 passed, 13 skipped (was 815 + 2 new ReDoS tests)
- forgelm --config config_template.yaml --dry-run OK
- 50K-char adversarial sweep over all 25 regexes: max 0.5ms (linear)
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
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 (1)
.claude/skills/review-pr/SKILL.md (1)
20-23:⚠️ Potential issue | 🟡 MinorRename this to a seven-question review.
Question 7 was added below, so the heading now undercounts the checklist and makes the skill self-contradictory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/review-pr/SKILL.md around lines 20 - 23, Update the heading text "The six-question review" to accurately reflect the checklist now containing seven items by changing it to "The seven-question review" (or an equivalent phrase) so the heading matches the new content; locate and edit the heading string in .claude/skills/review-pr/SKILL.md where "The six-question review" appears.
♻️ Duplicate comments (2)
docs/guides/ingestion-tr.md (1)
207-217:⚠️ Potential issue | 🟡 MinorCLI synopsis is still missing the Phase 12 flags.
This block still documents only
sliding|paragraphand omits--secrets-mask, while the rest of the guide already teaches both. That leaves the Turkish guide internally inconsistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/ingestion-tr.md` around lines 207 - 217, The CLI synopsis for the "forgelm ingest" example is missing the Phase 12 flag --secrets-mask; update the documented options in the forgelm ingest usage block (the line beginning "forgelm ingest INPUT_PATH \") to include --secrets-mask alongside the other flags (e.g., add [--secrets-mask] in the options list and ensure it's mentioned with the existing --strategy {sliding,paragraph} entry) so the Turkish guide matches the rest of the documentation.forgelm/data_audit.py (1)
777-799:⚠️ Potential issue | 🟠 MajorThese secrets regexes accept non-ASCII Unicode characters.
github_token,openai_api_key,google_api_key, andjwtuse\wand[\w-], which match Unicode word characters by default in Python 3. This broadens the patterns beyond the actual token specifications (GitHub tokens, OpenAI keys, Google keys, and JWTs are all ASCII). For a low-false-positive secrets scanner, this creates false positives on Unicode-containing text. Usere.ASCIIto restrict to ASCII word characters.Possible fix
- "github_token": re.compile(r"\b(?:ghp|gho|ghu|ghs|ghr|github_pat)_\w{20,}"), + "github_token": re.compile(r"\b(?:ghp|gho|ghu|ghs|ghr|github_pat)_\w{20,}", re.ASCII), @@ - "openai_api_key": re.compile(r"\bsk-(?:proj-)?[\w-]{20,}\b"), + "openai_api_key": re.compile(r"\bsk-(?:proj-)?[\w-]{20,}\b", re.ASCII), @@ - "google_api_key": re.compile(r"\bAIza[\w-]{35}\b"), + "google_api_key": re.compile(r"\bAIza[\w-]{35}\b", re.ASCII), @@ - "jwt": re.compile( + "jwt": re.compile( r"\beyJ(?:hbGc|0eXA|raWQ|jdHk|lbmM|hcGk)[\w-]{10,}" r"\.eyJ[\w-]{10,}" - r"\.[\w-]{15,}\b" + r"\.[\w-]{15,}\b", + re.ASCII, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/data_audit.py` around lines 777 - 799, The secret regexes in _SECRET_PATTERNS (notably "github_token", "openai_api_key", "google_api_key", and "jwt") use \w/[\w-] which are Unicode-aware by default; update those re.compile calls to restrict to ASCII by passing the re.ASCII flag (e.g., re.compile(pattern, flags=re.ASCII)) so \w only matches ASCII word chars. Leave patterns that already use explicit ASCII character classes (like "aws_access_key" and "slack_token") unchanged and ensure the change is applied to the exact compiled patterns referenced by those keys in _SECRET_PATTERNS.
🤖 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/guides/data_audit.md`:
- Around line 265-266: The docs currently imply installing `[ingestion-secrets]`
layers detect-secrets behavior, but `forgelm.data_audit.detect_secrets()` still
uses only the regex fallback; update the sentence to mark this as
forward-looking guidance (e.g., "Optional (future/forward-compatibility):
install `[ingestion-secrets]` to enable layering when supported") and add a
brief clarifying note that today installing `[ingestion-secrets]` does not
change audit behavior because `forgelm.data_audit.detect_secrets()` currently
relies solely on the regex fallback.
In `@docs/standards/regex.md`:
- Around line 11-23: Rule 1 incorrectly states `\w` and `[A-Za-z0-9_]` are
equivalent; update the docs/standards/regex.md Rule 1 paragraph to explain that
in Python `\w` matches Unicode word characters while `[A-Za-z0-9_]` is
ASCII-only, show the practical differences (e.g., non-Latin characters match
`\w` but not the ASCII class), recommend using the `re.ASCII` flag when you
intend ASCII semantics or explicitly use `[A-Za-z0-9_]`, and keep the existing
guidance about using `[A-Za-z0-9]` if you need to exclude underscore (also
update the example `github_token` note to mention choosing `\w` vs ASCII class
intentionally).
In `@forgelm/data_audit.py`:
- Around line 896-979: The state machine only tracks fence character and allows
a shorter closing fence (e.g., opening with four backticks closed by three),
violating CommonMark §4.5; update _is_code_fence_open to return both fence char
and its run length (or add a separate helper to compute run length), change
open_fence in _strip_code_fences to store that length (e.g., tuple (fence_char,
length)), and modify _is_code_fence_close to accept/compare the closing run
length against the stored opening length so a close is valid only when its run
length >= opening run length; keep all other behavior (indent handling,
buffering) intact and only adjust how open_fence is set/checked in
_strip_code_fences and the signature/logic of _is_code_fence_close.
In `@forgelm/ingestion.py`:
- Around line 511-512: The regex and state for Markdown fenced code blocks only
track fence character and assume exactly 3 markers; update _MARKDOWN_CODE_FENCE
to capture the full fence (e.g., use a named group like (?P<fence>`{3,}|~{3,})
and (?P<rest>[^\n]*) ) and replace the open_fence_char state with open_fence:
Optional[Tuple[str,int]] to store (char, length); then in the loop use
fence_match.group("fence") to get fence_char and fence_len, use
group("rest").strip() to ensure no info string when closing, set open_fence when
opening, and only close when fence_char matches and fence_len >= open_fence[1],
and update the checks that used open_fence_char to use open_fence instead (also
apply the same changes in the other similar block around the 555–565 region).
In `@notebooks/data_curation.ipynb`:
- Around line 97-110: The cell currently writes a triple-quoted literal
containing credential-shaped values directly to disk (the write_text call that
writes the deploy_runbook.txt string); change it to construct demo tokens at
runtime from inert fragments (e.g., join small non-sensitive substrings or use
placeholders) so the notebook still contains token-shaped text for detection but
no contiguous secret-like literals; update the code that builds the file content
(the string passed to write_text) to assemble AWS_ACCESS_KEY_ID and GitHub PAT
from fragments or placeholders and ensure any phone/email remains non-sensitive
(e.g., replace with example.com or redact) before calling write_text.
- Around line 345-349: Replace the direct installation of datasketch in the
notebook cell with the project extra to match the package's optional-dependency
handling: instead of installing "datasketch>=1.6.0,<2.0.0" directly, run pip
install 'forgelm[ingestion-scale]' so the _require_datasketch() import hint in
forgelm/data_audit.py matches the notebook and ensures consistent dependency
management for MinHash dedup.
In `@README.md`:
- Line 32: The README currently lists "semantic" as a supported chunking
strategy even though the implementation in forgelm.ingestion raises
NotImplementedError and forgelm.cli hides "semantic" from the --strategy
choices; update the README line so "semantic" is marked as planned/reserved
(e.g., "semantic (planned/reserved — not yet implemented)") or remove it from
the shipped strategies list and add a short note pointing to the planned work,
ensuring the README text no longer implies it is available at runtime.
In `@tests/test_phase12_review_fixes.py`:
- Around line 467-486: The test currently creates a directory via
Path(__file__).parent / "_minhash_distinct_tmp", which mutates the repo and can
fail in parallel/read-only runs; change the test to accept pytest's tmp_path
fixture and create the temp directory there (e.g., path = tmp_path /
"minhash_distinct_tmp"), write the jsonl file to that path and call
audit_dataset with str(jsonl) as before (identifiers: path, jsonl,
audit_dataset, train_info), and remove the manual cleanup loop and rmdir since
pytest's tmp_path is managed automatically.
---
Outside diff comments:
In @.claude/skills/review-pr/SKILL.md:
- Around line 20-23: Update the heading text "The six-question review" to
accurately reflect the checklist now containing seven items by changing it to
"The seven-question review" (or an equivalent phrase) so the heading matches the
new content; locate and edit the heading string in
.claude/skills/review-pr/SKILL.md where "The six-question review" appears.
---
Duplicate comments:
In `@docs/guides/ingestion-tr.md`:
- Around line 207-217: The CLI synopsis for the "forgelm ingest" example is
missing the Phase 12 flag --secrets-mask; update the documented options in the
forgelm ingest usage block (the line beginning "forgelm ingest INPUT_PATH \") to
include --secrets-mask alongside the other flags (e.g., add [--secrets-mask] in
the options list and ensure it's mentioned with the existing --strategy
{sliding,paragraph} entry) so the Turkish guide matches the rest of the
documentation.
In `@forgelm/data_audit.py`:
- Around line 777-799: The secret regexes in _SECRET_PATTERNS (notably
"github_token", "openai_api_key", "google_api_key", and "jwt") use \w/[\w-]
which are Unicode-aware by default; update those re.compile calls to restrict to
ASCII by passing the re.ASCII flag (e.g., re.compile(pattern, flags=re.ASCII))
so \w only matches ASCII word chars. Leave patterns that already use explicit
ASCII character classes (like "aws_access_key" and "slack_token") unchanged and
ensure the change is applied to the exact compiled patterns referenced by those
keys in _SECRET_PATTERNS.
🪄 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: 624631e6-03fa-4e25-a6aa-10fbd6bb1e65
📒 Files selected for processing (19)
.claude/skills/review-pr/SKILL.mdCHANGELOG.mdCLAUDE.mdREADME.mddocs/guides/data_audit.mddocs/guides/ingestion-tr.mddocs/standards/README.mddocs/standards/code-review.mddocs/standards/coding.mddocs/standards/regex.mdforgelm/cli.pyforgelm/data_audit.pyforgelm/ingestion.pyforgelm/wizard.pynotebooks/data_curation.ipynbnotebooks/post_training_workflow.ipynbtests/test_data_audit_phase12.pytests/test_ingestion_phase12.pytests/test_phase12_review_fixes.py
✅ Files skipped from review due to trivial changes (4)
- docs/standards/README.md
- CLAUDE.md
- docs/standards/code-review.md
- forgelm/wizard.py
🚧 Files skipped from review as they are similar to previous changes (2)
- notebooks/post_training_workflow.ipynb
- tests/test_data_audit_phase12.py
…ngth
Round-3 review caught two real correctness bugs plus a handful of
doc/fixture parity issues. Verified each finding against current code
before applying.
Correctness — Unicode-aware \w in secret regexes
- ``[A-Za-z0-9_-]`` → ``[\w-]`` from round 2 broadened
github_token / openai_api_key / google_api_key / jwt to match
Unicode word chars (``\w`` is Unicode-aware in Python by default).
Real credentials are ASCII-only; the broadened class would silently
let non-ASCII chars leak into the match universe.
- Fix: added ``flags=re.ASCII`` to all four patterns. Patterns that
already use explicit ASCII char classes (aws_access_key,
slack_token, the explicit [A-Z0-9] ones) unchanged.
Correctness — CommonMark §4.5 fence-length rule
- _strip_code_fences (data_audit.py) and _markdown_sections
(ingestion.py) tracked only the fence character, so a 4-backtick
opener was prematurely closed by a 3-backtick line. Also missed
the "no info string on close" rule.
- Fix:
- _is_code_fence_open returns (char, run_length).
- _is_code_fence_close accepts the minimum run-length and rejects
shorter closes.
- _MARKDOWN_CODE_FENCE regex captures (?P<fence>) and (?P<rest>) so
the splitter can enforce both the length rule AND the "no info
string on close" rule together.
- Result: all three CommonMark §4.5 close-side rules now hold —
matching char, run length ≥ open, no info string.
Standard correction
- regex.md Rule 1 used to claim ``[A-Za-z0-9_]`` and ``\w`` are
equivalent. They are not. Rewrote with side-by-side example showing
the Unicode/ASCII divergence and a decision table:
- ASCII-only inputs → \w with re.ASCII (or explicit class)
- Natural-language inputs → bare \w (Unicode-aware)
- Mixed → be explicit
- Updated example regex in the rule to show ``flags=re.ASCII``.
Doc/fixture parity
- data_audit.md: ``[ingestion-secrets]`` extra reframed as
forward-compatibility — the current code does not invoke
detect-secrets. Operators can pin the extra in requirements files
but it doesn't change audit behaviour today.
- README: ``semantic`` chunking strategy clearly marked as planned
(raises NotImplementedError, hidden from CLI choices) so the
README no longer implies it's available at runtime.
- ingestion-tr.md: CLI synopsis options block now lists Phase 12
flags ``--strategy markdown`` and ``--secrets-mask``.
- review-pr SKILL.md: heading "The six-question review" → "The
seven-question review" — matches the regex-check question added
in the previous commit.
- data_curation.ipynb:
- Fragment ``AKIA…`` / ``ghp_…`` literals in deploy_runbook.txt
fixture so gitleaks / trufflehog scans of the repo don't flag
the notebook source. Same convention as tests/FAKE_AWS_KEY.
- MinHash install: ``datasketch>=1.6.0`` → ``forgelm[ingestion-scale]
==0.5.2`` to match the install hint baked into
``_require_datasketch``.
Test hygiene
- TestMinHashDistinctSemantic now uses pytest's ``tmp_path`` fixture
instead of a fixed directory under ``tests/``. Was breaking parallel
pytest runs and mutating the repo.
3 new regression tests (TestFenceRunLengthRule)
- 4-backtick block not closed by 3-backtick inner line.
- _strip_code_fences respects the length rule.
- Close fences with info strings are treated as content.
Verified
- ruff format clean, ruff check clean.
- 820 passed, 13 skipped (was 817 + 3 new fence-length tests).
- forgelm --config config_template.yaml --dry-run OK.
SonarCloud python:S5852 flagged forgelm/ingestion.py L515 —
``^ {0,3}(?P<fence>`{3,}|~{3,})(?P<rest>[^\n]*)$`` had two unbounded
greedy quantifiers in sequence over overlapping character classes
(``{3,}`` and ``[^\n]*``; the rest-class includes fence chars).
Textbook polynomial-runtime shape per docs/standards/regex.md rule 4.
Empirically linear in CPython (50K-char pure-backtick run = 16 μs),
but the static analyser can't prove that — and we already use
non-regex line walkers for every other markdown parser in the
codebase (``_markdown_sections`` body, ``_strip_code_fences``,
``_is_code_fence_open``, ``_is_code_fence_close``).
Fix
- Drop ``_MARKDOWN_CODE_FENCE`` regex.
- New ``_parse_md_fence(line)`` helper: non-regex parser returning
``(fence_char, run_length, rest_after_run)`` or ``None``.
Provably O(n) per line — single forward scan, no backtracking surface.
- ``_markdown_sections`` switched to the helper. The named-group output
shape is preserved, so the splitter logic that enforces CommonMark
§4.5 (matching char + run length ≥ open + no info string on close)
is unchanged.
Verified
- ruff format clean, ruff check clean.
- 822 passed, 13 skipped (was 820 + 2 new ReDoS regression tests).
- forgelm --config config_template.yaml --dry-run OK.
- 100K-char pure-backtick benchmark: ~10 μs (vs the standard's 100 ms cap).
Tests added
- TestRegexLinearity::test_parse_md_fence_linear_on_long_runs — ≤ 100 ms
cap on N=100_000 pure-backtick input.
- TestRegexLinearity::test_parse_md_fence_behaviour — pinned outputs
for opener with info string, 4-char fence (CommonMark §4.5 length
rule), 2-space indent (allowed), 4-space indent (rejected, indented
code block), sub-3-char run (rejected), mismatched chars after run.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
README.md (1)
31-31: Use proper noun capitalization for “Markdown” in prose.Consider changing “markdown table syntax” to “Markdown table syntax” for consistency with the rest of the README wording.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 31, Update the README text to use proper noun capitalization for "Markdown" in prose; specifically change the phrase "markdown table syntax" to "Markdown table syntax" in the line describing the ingest command and the new features (the diff containing `forgelm ingest ./policies/ --recursive --output data/policies.jsonl` and the note about `--strategy markdown`) so the term is consistently capitalized throughout the README.tests/test_phase12_review_fixes.py (1)
477-532: Stabilize timing-based tests with median sampling to reduce CI flakiness.Single-shot wall-clock assertions can intermittently fail on loaded runners; using a small median sample keeps the ReDoS guard while reducing noise.
♻️ Suggested refactor pattern
class TestRegexLinearity: + `@staticmethod` + def _median_elapsed_ms(fn, repeats=5): + import statistics + import time + + samples = [] + for _ in range(repeats): + t0 = time.perf_counter() + fn() + samples.append((time.perf_counter() - t0) * 1000) + return statistics.median(samples) + def test_heading_pattern_linear_on_pathological_whitespace(self): import time from forgelm.ingestion import _MARKDOWN_HEADING_PATTERN for n in (1_000, 5_000, 10_000): payload = "# a" + " \t" * n + "x" - t0 = time.perf_counter() - _MARKDOWN_HEADING_PATTERN.match(payload) - elapsed_ms = (time.perf_counter() - t0) * 1000 + elapsed_ms = self._median_elapsed_ms( + lambda: _MARKDOWN_HEADING_PATTERN.match(payload) + ) assert elapsed_ms < 1000Apply the same pattern to the other two timing tests in this class.
Based on learnings: "Tests must follow docs/standards/testing.md."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_phase12_review_fixes.py` around lines 477 - 532, Replace the single-shot timing assertions in the three tests with a small median-sampling loop: run the measurement N times (e.g., 5), collect elapsed_ms for each run of the target call (for test_heading_pattern_linear_on_pathological_whitespace call _MARKDOWN_HEADING_PATTERN.match(payload); for test_strip_code_fences_linear_on_unclosed_blocks call _strip_code_fences(payload); for test_parse_md_fence_linear_on_long_runs call _parse_md_fence(payload)), compute the median elapsed_ms, and assert the median is below the existing threshold (1000ms or 100ms as originally used); keep payloads, thresholds, and imports but replace the single measurement t0/t1 assert with the median-sample approach to reduce CI flakiness.
🤖 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/guides/data_audit.md`:
- Around line 247-252: The docs text understates secret detection for private
keys; update the wording around the coverage list to state that _SECRET_PATTERNS
matches full BEGIN…END private-key envelopes (not just headers) and that
mask_secrets() redacts entire key blocks (OpenSSH/PGP/RSA/DSA/EC), ensuring
examples and the phrase "block headers" are replaced with language describing
full-block matching and redaction.
In `@docs/guides/ingestion-tr.md`:
- Around line 6-9: Update the “Sınırlamalar” section to reflect the new behavior
introduced in Faz 12 (v0.5.2): state that DOCX tables can be emitted as Markdown
when using the markdown-aware splitter (flag --strategy markdown) instead of
describing PDF/DOCX tables as always flattened row-major; remove or
conditionally qualify the blanket “flattened row-major” statement and add a note
about when table flattening still applies (e.g., older versions or other
strategies), and ensure the revised wording follows
docs/standards/documentation.md and docs/standards/localization.md guidelines
and references the new DOCX table behavior and the --secrets-mask mention where
relevant.
In `@docs/standards/regex.md`:
- Around line 188-199: Update the Pre-merge checklist bullet that currently
reads "No `[A-Za-z0-9_]` (use `\w`)" to allow intentional ASCII-only classes:
change it to something like "No unintentional `[A-Za-z0-9_]` (use `\w`);
explicit ASCII-only classes permitted for deliberate ASCII-only grammars when
paired with re.ASCII or documented justification." Also add a short
parenthetical or link referencing Rule 1 and the `re.ASCII` / explicit-class
exception so reviewers can cross-check the allowed pattern and won’t reject the
documented safe usage.
In `@forgelm/data_audit.py`:
- Around line 71-79: The current silent import fallback in data_audit.py sets
_MinHash/_MinHashLSH/_HAS_DATASKETCH to None on ImportError; instead change this
to raise ImportError immediately at module import with a helpful install hint
(e.g., mentioning the datasketch extra or package name), and remove the fallback
assignments; keep the existing _require_datasketch() helper if lazy checks are
still used elsewhere, but ensure the top-level import of datasketch in
data_audit.py fails fast by raising the ImportError (reference symbols:
_MinHash, _MinHashLSH, _HAS_DATASKETCH, and _require_datasketch) so it matches
the eager-import pattern used in model.py/inference.py/export.py.
- Around line 2037-2041: When dedup_method == "minhash" in audit_dataset() you
must validate the new MinHash knobs before calling _build_minhash_lsh(): ensure
minhash_jaccard is a float in [0.0, 1.0] and minhash_num_perm is an integer > 0;
if not, raise a clear ValueError (e.g., "minhash_jaccard must be in [0.0,1.0];
got ..." and "minhash_num_perm must be a positive integer; got ..."). Add these
checks alongside the existing dedup_method check (the block that calls
_require_datasketch()) so programmatic callers get a descriptive API-boundary
error rather than a cryptic datasketch failure.
In `@forgelm/ingestion.py`:
- Around line 575-643: The _markdown_sections function is too complex—extract
the fence-state logic and the section flush/render logic into small helpers to
reduce complexity: move the per-line fence detection and state transitions into
a helper (e.g. _handle_fence_transition(open_fence, line, _parse_md_fence)) that
returns the new open_fence and whether the line should be appended as content,
and move the section boundary flushing and final render steps into helpers (e.g.
_flush_section(sections, current_path, current_lines, seen_heading) and
_render_sections(sections, _trim_blank_edges)) that encapsulate appending and
trimming logic; keep using _MARKDOWN_HEADING_PATTERN, _push_heading_onto_path
and _trim_blank_edges but replace the inline fence/flush code in
_markdown_sections with calls to these helpers so the function’s cyclomatic
complexity falls below the threshold.
- Around line 519-552: The parser currently allows backtick-fence openers even
when the info string contains backticks; update _parse_md_fence to reject such
lines by checking after computing fence_char and run_end: if fence_char == "`"
and the rest string (line[run_end:]) contains any backtick characters, return
None so the opener is not accepted; preserve the existing behavior for tilde
fences and other validations of run_len and leading indentation (use the
existing fence_char, run_end and run_len variables in this logic).
---
Nitpick comments:
In `@README.md`:
- Line 31: Update the README text to use proper noun capitalization for
"Markdown" in prose; specifically change the phrase "markdown table syntax" to
"Markdown table syntax" in the line describing the ingest command and the new
features (the diff containing `forgelm ingest ./policies/ --recursive --output
data/policies.jsonl` and the note about `--strategy markdown`) so the term is
consistently capitalized throughout the README.
In `@tests/test_phase12_review_fixes.py`:
- Around line 477-532: Replace the single-shot timing assertions in the three
tests with a small median-sampling loop: run the measurement N times (e.g., 5),
collect elapsed_ms for each run of the target call (for
test_heading_pattern_linear_on_pathological_whitespace call
_MARKDOWN_HEADING_PATTERN.match(payload); for
test_strip_code_fences_linear_on_unclosed_blocks call
_strip_code_fences(payload); for test_parse_md_fence_linear_on_long_runs call
_parse_md_fence(payload)), compute the median elapsed_ms, and assert the median
is below the existing threshold (1000ms or 100ms as originally used); keep
payloads, thresholds, and imports but replace the single measurement t0/t1
assert with the median-sample approach to reduce CI flakiness.
🪄 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: a2b007c4-edf6-49c4-b72b-bef24ce75f6c
📒 Files selected for processing (10)
.claude/skills/review-pr/SKILL.mdCHANGELOG.mdREADME.mddocs/guides/data_audit.mddocs/guides/ingestion-tr.mddocs/standards/regex.mdforgelm/data_audit.pyforgelm/ingestion.pynotebooks/data_curation.ipynbtests/test_phase12_review_fixes.py
✅ Files skipped from review due to trivial changes (1)
- notebooks/data_curation.ipynb
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/skills/review-pr/SKILL.md
…hygiene)
Doc accuracy:
- data_audit{,.tr}.md: state full BEGIN…END envelope matching for private
keys; mask_secrets() redacts the entire block, not the header line.
- ingestion{,-tr}.md: qualify the "tables flattened" limitation — DOCX
tables emit Markdown table syntax under --strategy markdown (Phase 12);
flatten only applies to other strategies / older versions / PDFs.
- README.md: capitalize "Markdown" as a proper noun in the ingest blurb.
- regex.md: pre-merge bullet now allows intentional ASCII-only character
classes per Rule 1 (\w + re.ASCII or explicit class are both RIGHT).
Parser correctness:
- _parse_md_fence: reject backtick fence openers whose info string
contains a backtick (CommonMark §4.5 — the run would be ambiguous with
the close). Tilde fences unaffected.
- _markdown_sections: extract _advance_fence_state, _flush_section, and
_render_sections so the dispatch loop reads top-down; behaviour pinned
by existing tests.
API boundary:
- audit_dataset: validate minhash_jaccard ∈ [0.0, 1.0] and
minhash_num_perm > 0 before _require_datasketch(), so programmatic
callers get a clear ValueError instead of a cryptic datasketch error.
Test stability:
- ReDoS-linearity tests now take the median of 5 samples instead of a
single perf_counter delta — reduces shared-CI flakiness without
weakening the regression bound.
Rejected (with justification): the suggestion to make datasketch a
fail-fast top-level import contradicts the optional-extras design —
datasketch lives in the separate ingestion-scale extra (not the base
ingestion extra), so failing at module import would force every default
simhash audit user to install it. The current _HAS_DATASKETCH +
_require_datasketch() pattern mirrors xxhash and is the correct
convention; model.py / inference.py / export.py also use lazy try/except
imports inside the consuming function, not eager top-level imports.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
docs/guides/ingestion-tr.md (1)
276-281:⚠️ Potential issue | 🟡 MinorTR DOCX-table note diverges from the implementation.
forgelm/ingestion.pyconverts DOCX tables to Markdown during extraction, regardless of the later chunking strategy. This text currently says non-markdown strategies flatten them to row-major plain text, which does not match the shipped pipeline.Based on learnings, "User-facing docs must be bilingual (English and Turkish) as mirror documents".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/ingestion-tr.md` around lines 276 - 281, The Turkish docs incorrectly state that DOCX tables are only converted to Markdown when using "--strategy markdown"; update docs/guides/ingestion-tr.md to match the implementation in forgelm/ingestion.py by saying DOCX tables are converted to Markdown during extraction regardless of later chunking strategy (and that PDF tables remain flattened), and make the guide bilingual by adding the equivalent English text alongside the Turkish so the doc is a mirrored EN/TR version.
🧹 Nitpick comments (1)
docs/standards/regex.md (1)
152-153: Absolute≤10msbudget is likely non-portable in docsThis hard cutoff is environment-dependent and can cause flaky pass/fail interpretation across machines/CI runners. Prefer a scaling-based rule (e.g., near-linear growth across 1k/5k/10k inputs, median-of-N runs) as the normative gate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/standards/regex.md` around lines 152 - 153, Replace the hard absolute "≤10ms" rule with a portable scaling-based guideline: update the text to require empirical verification of near-linear growth across multiple input sizes (e.g., 1k, 5k, 10k characters) using the median of N runs to reduce noise, and state acceptable growth characteristics rather than a fixed millisecond cutoff; reference the existing examples/use-cases in forgelm/data_audit.py::_strip_code_fences and forgelm/ingestion.py::_markdown_sections as places where non-regex parsers are used when regexes fail the scaling test.
🤖 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/guides/data_audit-tr.md`:
- Around line 266-273: The paragraph incorrectly claims `[ingestion-secrets]`
layers a `detect-secrets` plugin set; update the text to state that
forgelm.data_audit.detect_secrets() currently relies only on the built-in regex
set (it does not invoke a `detect-secrets` plugin layer) and mark
`[ingestion-secrets]` as optional configuration that does not change audit
behavior today; also ensure this guide is presented as a bilingual mirror
(English + Turkish) so both language versions state the same limitation.
- Around line 298-299: Update the `repeated_lines` bullet to match the
implemented logic: clarify that the check first filters for lines that actually
repeat (count >= 2) and then applies the "top-3 / 30%" threshold, instead of
implying it looks at the top 3 distinct non-empty lines; edit the Turkish
sentence to state this exact behavior and add the corresponding English sentence
so the document is bilingual (mirror EN and TR) around the `repeated_lines`
bullet.
- Around line 31-39: Update the Turkish CLI guide to mirror Phase 12 flags: add
descriptions and examples for the new flags `--dedup-method`,
`--jaccard-threshold`, and `--quality-filter` in the "CLI referansı" section and
ensure the example snippets (e.g., the lines that show `forgelm audit ...
--dedup-method minhash --jaccard-threshold 0.85` and `--quality-filter`) and
their Turkish translations match the English docs; specifically, add entries
explaining allowed values for `--dedup-method` (e.g., minhash, simhash), numeric
range/meaning for `--jaccard-threshold`, and the purpose/behavior of
`--quality-filter`, and update any legacy top-level-flag wording to the Phase 12
flag flow so the TR guide is internally consistent with the examples.
In `@docs/guides/ingestion.md`:
- Around line 293-298: The DOCX table claim is inaccurate: update the guide text
to reflect that _extract_docx() converts DOCX tables into Markdown table syntax
before any chunking strategy runs (so Markdown rendering is not limited to
`--strategy markdown`), and clarify that differences between
`paragraph`/`sliding`/`markdown` relate to whether table rows are preserved
across chunk boundaries rather than whether tables are flattened to row-major
text; also explicitly state that PDF tables remain flattened in all cases. Use
the function name _extract_docx() and mention chunk-boundary preservation to
align the docs with implementation.
- Around line 120-123: The docs currently claim an optional
"forgelm[ingestion-secrets]" extra and detect-secrets plugin layering is active
for ingest_path(), but the code only imports mask_secrets from
forgelm.data_audit and provides no detect-secrets integration or INFO-log
fallback; update the text to remove the present-tense claim and either (a)
reword to a future/optional enhancement note pointing to the ingest_path() and
mask_secrets symbols and saying integration is planned, or (b) drop the mention
entirely until you implement detect-secrets support and the INFO-log fallback in
the ingestion path (ensure any README claim references the actual
forgelm.data_audit.mask_secrets implementation).
In `@docs/standards/regex.md`:
- Around line 141-142: The regex example using re.compile(r"^[^E]*ERROR:") is
semantically wrong because the ^[^E]* prefix prevents matches when the line
begins with "E" characters (e.g., "EEE ERROR: boom"); update the example to use
the correct pattern depending on intent — use re.compile(r"^ERROR:") if you want
to match lines that start with "ERROR:" or re.compile(r"ERROR:") if you want to
match "ERROR:" anywhere in the string — replace the incorrect pattern in the
example accordingly (look for the re.compile(...) entry and the literal
"^[^E]*ERROR:" to modify).
In `@forgelm/ingestion.py`:
- Around line 737-743: The docstring for _chunk_markdown_tokens no longer
matches runtime behavior: update its description to remove the claim that
"--overlap-tokens" is silently ignored and instead state that non-zero overlap
is rejected (a ValueError is raised) when using the markdown/token-aware
strategy; reference the same contract as enforced by the overlap validation in
the dispatcher/ingestion logic so docs/tests and _chunk_markdown_tokens() align.
---
Duplicate comments:
In `@docs/guides/ingestion-tr.md`:
- Around line 276-281: The Turkish docs incorrectly state that DOCX tables are
only converted to Markdown when using "--strategy markdown"; update
docs/guides/ingestion-tr.md to match the implementation in forgelm/ingestion.py
by saying DOCX tables are converted to Markdown during extraction regardless of
later chunking strategy (and that PDF tables remain flattened), and make the
guide bilingual by adding the equivalent English text alongside the Turkish so
the doc is a mirrored EN/TR version.
---
Nitpick comments:
In `@docs/standards/regex.md`:
- Around line 152-153: Replace the hard absolute "≤10ms" rule with a portable
scaling-based guideline: update the text to require empirical verification of
near-linear growth across multiple input sizes (e.g., 1k, 5k, 10k characters)
using the median of N runs to reduce noise, and state acceptable growth
characteristics rather than a fixed millisecond cutoff; reference the existing
examples/use-cases in forgelm/data_audit.py::_strip_code_fences and
forgelm/ingestion.py::_markdown_sections as places where non-regex parsers are
used when regexes fail the scaling test.
🪄 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: 2144b0b0-21ed-429b-aa39-2a46133edbca
📒 Files selected for processing (9)
README.mddocs/guides/data_audit-tr.mddocs/guides/data_audit.mddocs/guides/ingestion-tr.mddocs/guides/ingestion.mddocs/standards/regex.mdforgelm/data_audit.pyforgelm/ingestion.pytests/test_phase12_review_fixes.py
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
…docstring) Documentation accuracy: - data_audit-tr.md: sync TR mirror with EN — `[ingestion-secrets]` is reserved for a follow-up release, not active plugin layering; `repeated_lines` requires count ≥ 2 before top-3/30% (mirrors EN). - data_audit-tr.md: CLI referansı now lists --dedup-method, --jaccard-threshold, --quality-filter (was missing the Phase 12 flags vs the EN guide). - ingestion.md / ingestion-tr.md: DOCX-table claim was wrong on both. _extract_docx() converts tables to Markdown at extraction time, before any chunking strategy runs — strategy choice only affects whether table rows stay together across chunk boundaries (markdown treats tables atomically; paragraph/sliding may slice). PDF tables remain flattened in all cases. - ingestion.md: drop the false "detect-secrets plugin layering + INFO log fallback" claim. ingest_path() delegates to forgelm.data_audit.mask_secrets which uses regex only; [ingestion-secrets] is reserved for a follow-up release. Standards corrections: - regex.md Rule 8: replace the broken `^[^E]*ERROR:` example. `[^E]*` matches zero chars when a line starts with 'E', so "EEE ERROR: boom" fails to match even though ERROR: is present. New examples spell intent: `^ERROR:` (starts-with), `ERROR:` (anywhere), text.startswith() for fixed prefix. - regex.md ReDoS budget: replace the absolute "≤ 10 ms" wall-clock rule with a scaling-based + median-of-N guideline (run at 1K/5K/10K, take the median, look for near-linear growth). Pins the actual practice in tests/test_phase12_review_fixes.py and stops the standard from contradicting the test thresholds (100 ms / 1 s safety floors). Code: - _chunk_markdown_tokens docstring: removed the "silently ignored" claim. Non-zero overlap_tokens is rejected with ValueError by _strategy_dispatch_tokens before this body is reached (_MARKDOWN_OVERLAP_UNSUPPORTED_MSG). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
✅ Actions performedReview triggered.
|
…ositive renames
Verified each Codacy finding against the actual code; applied 5 real
fixes and 2 cosmetic renames for false-positive heuristics.
i18n.js
- L16 ("hardcoded password"): false positive — STORAGE_KEY is a
localStorage key for the language preference. Renamed to
LANG_PREF_NAME to escape the *_KEY identifier heuristic.
- L22 (var hoisting): hoisted ``stored`` / ``nav`` / ``prefix``
declarations to the top of detectLanguage().
main.js
- L45 ("hardcoded password"): false positive — ``KEY`` is the theme
localStorage entry name. Renamed to THEME_PREF_NAME for the same
reason as i18n.js.
- L85, L91 (innerHTML XSS / unencoded HTML): removed the
``innerHTML`` round-trip on the copy-button label restore. Now
captures children via ``cloneNode(true)`` and re-appends them
after the timeout, so no string is ever written back to
innerHTML. Even though the original orig came from the same
button, the round-trip-via-string was exactly the pattern
flagged.
- L157, L158 (object-injection sink): captured ``steps[i]`` into a
``step`` local once per iteration so subsequent property reads
are not bracket-indexed accesses. The counter ``i`` was
always bounded (0..steps.length-1) so this is purely cosmetic
for the linter, but it makes safety obvious without a comment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Verified each finding against the current code; applied fixes for all eight that were real (Quality Gate's duplication metric should also drop after the data_audit refactor extracts the shared loop). forgelm/ingestion.py — split _chunk_markdown_tokens into three functions: _build_markdown_section_blocks (render breadcrumb + body), _count_section_tokens (batch tokenizer call with per-block fallback), and the main chunker that just packs the greedy buffer. Cognitive complexity drops from 16 → ~8. forgelm/data_audit.py — extracted _count_leaks_against_index from _count_leaked_rows_minhash_bidirectional. The two near-identical query loops (a→lsh_b, b→lsh_a) are now one helper called twice. Complexity drops from 24 → ~5 and the duplication that drove the 3.6% Quality Gate failure goes away. tests/test_phase12_review_fixes.py — rephrased the trailing "# alg=HS256, typ=JWT" inline comment as a leading prose line. Sonar's "commented out code" detector misread the key=value inside the inline comment. site/contact.html — replaced <div role="status"> with the semantic <output> element (with for="contact-form" attribute). Sonar accessibility rule: <output> beats role="status" for screen-reader compatibility. site/index.html — replaced <div role="group" aria-label="Language"> with <fieldset> + sr-only <legend>. Same Sonar rule; <fieldset>/<legend> is the recommended group-of-controls pattern. Added margin: 0 / min-width: 0 to .lang-toggle to neutralise the browser's default <fieldset> styling. site/css/style.css — three contrast fixes for WCAG AA compliance on the light theme: - ::selection alpha bumped .35 → .85 so white text on orange passes contrast even when the page background bleeds through. - .form-status.success / .error get [data-theme="light"] overrides to #15803d (green-700) / #b91c1c (red-700) — the default #4ade80 / #f87171 only pass on the dark theme. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forgelm/wizard.py (1)
212-233: 🛠️ Refactor suggestion | 🟠 MajorEmit structured append-only audit events for save/fallback decisions.
The new logs at
Line 212,Line 215,Line 229, andLine 232improve visibility, but these branches are still only plain logger output. Please also publish structured events to the project’s append-only audit stream for: primary success, primary failure, fallback success, fallback failure.As per coding guidelines, "Emit structured events to append-only audit log for every decision gate; never edit or delete entries."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/wizard.py` around lines 212 - 233, Add structured append-only audit events at each save/fallback decision point: right after the primary success logger.info("Wizard config saved to %s", requested_filename) emit a persistent audit event (e.g., emit_audit_event or append_audit_entry) with type "wizard_save_primary_success" and payload {requested_filename, timestamp, user/context}; in the primary except block after logger.error("Could not save wizard config to %s: %s", requested_filename, e) emit "wizard_save_primary_failure" with {requested_filename, error: str(e), timestamp, user/context}; after the fallback success logger.info("Wizard config saved to fallback location %s", fallback) emit "wizard_save_fallback_success" with {fallback, original_requested: requested_filename, timestamp, user/context}; and in the fallback except block after logger.error("Fallback wizard config save also failed (%s): %s", fallback, e) emit "wizard_save_fallback_failure" with {fallback, original_requested: requested_filename, error: str(e), timestamp, user/context}; ensure the chosen audit function writes append-only records and does not alter existing logs.
♻️ Duplicate comments (1)
CONTRIBUTING.md (1)
81-81:⚠️ Potential issue | 🟡 MinorResolve test file count discrepancy.
This line shows "26+ test files" but
docs/reference/architecture.mdline 61 states "47 test files". Ensure both files reflect the same accurate count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 81, The two docs disagree on the test count: CONTRIBUTING.md contains "26+ test files" while docs/reference/architecture.md has "47 test files"; run a reliable count of the repository's test files (e.g., by pattern used in CI or the test directory naming), then update both CONTRIBUTING.md and docs/reference/architecture.md so they contain the same, accurate number string (replace "26+ test files" and "47 test files" with the verified count) and ensure the wording format matches across both files.
🧹 Nitpick comments (10)
docs/roadmap.md (1)
27-27: Clarify milestone trigger wording.“Starts after Phase 12 lands” is ambiguous now that Phase 12 is already merged to
development. Consider anchoring to a concrete release event (e.g., “starts afterv0.5.2tag/PyPI publish”).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/roadmap.md` at line 27, Update the milestone trigger wording in the "Next: `v0.5.3` — Multi-Stage Pipeline Chains (Phase 14)" line to be unambiguous: replace "Starts after Phase 12 lands" with a concrete release event such as "starts after the `v0.5.2` tag is published (PyPI release)" or "starts after `v0.5.2` is released to PyPI", ensuring the line in docs/roadmap.md referencing v0.5.3 clearly anchors the start to a specific release event rather than the merged Phase 12 state.notebooks/quickstart_sft.ipynb (1)
205-205: Consider a phase-neutral section title for long-term clarity.Line 205 says “Phase 10” inside a Phase 12 release context; a neutral title like “Next Steps — Post-Training Toolchain” may avoid timeline confusion in docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notebooks/quickstart_sft.ipynb` at line 205, The header string "## Next Steps — Phase 10 Post-Training Toolchain" should be made phase-neutral to avoid confusion; locate the notebook cell containing that exact string and update it to "## Next Steps — Post-Training Toolchain" (remove "Phase 10") so the title reads generically and fits the Phase 12 release context.site/js/main.js (2)
85-92: Consider usingtextContentinstead ofinnerHTMLfor safer restoration.While the current code is safe (the button contains only static "copy" text), using
textContentfor restoration would be more defensive and avoids the innerHTML pattern flagged by static analysis.🛡️ Proposed safer alternative
navigator.clipboard.writeText(text).then(function () { - var orig = btn.innerHTML; + var origText = btn.textContent; btn.classList.add('copied'); var lang = document.documentElement.lang || 'en'; btn.textContent = lang === 'tr' ? 'kopyalandı' : 'copied'; setTimeout(function () { btn.classList.remove('copied'); - btn.innerHTML = orig; + btn.textContent = origText; }, 1600); }).catch(function () { /* clipboard blocked */ });This works because the copy buttons contain only text content ("copy").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/js/main.js` around lines 85 - 92, The code saves and restores the button label using innerHTML which can trigger static-analysis warnings; switch to using textContent for safety: when capturing the original label change orig = btn.innerHTML to orig = btn.textContent and when restoring change btn.innerHTML = orig to btn.textContent = orig (references: the btn variable and the save/restore block in the copy-button handler).
56-63: Theme initialization doesn't respect system preference.The code comment mentions "system-aware default" but the implementation always defaults to dark mode when no stored preference exists, ignoring
prefers-color-scheme. Consider honoring the system preference for better UX.🌙 Proposed system preference support
var stored; try { stored = localStorage.getItem(KEY); } catch (_) {} if (stored === 'light' || stored === 'dark') { applyTheme(stored); } else { - // System preference. Default = dark (forge identity). - applyTheme('dark'); + // Respect system preference, default to dark (forge identity). + var prefersDark = !window.matchMedia || window.matchMedia('(prefers-color-scheme: dark)').matches; + applyTheme(prefersDark ? 'dark' : 'light'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/js/main.js` around lines 56 - 63, When no stored preference is found the code always calls applyTheme('dark'); change this to honor system preference by using window.matchMedia('(prefers-color-scheme: dark)').matches to pick 'dark' or 'light' instead of defaulting to dark; keep the existing localStorage read (KEY, stored) and try/catch, and call applyTheme(stored) when stored is 'light' or 'dark', otherwise call applyTheme(systemPref ? 'dark' : 'light') where systemPref is the matchMedia result.site/contact.html (2)
84-85: Remove or update the placeholder comment.The comment references a different form ID (
mbdqglqj) than what's actually used (xnjlejbb). This creates confusion for future maintainers.📝 Proposed fix
-<!-- Replace mbdqglqj with your real Formspree form ID after creating it at formspree.io --> +<!-- Formspree form — see site/README.md for configuration --> <form id="contact-form" action="https://formspree.io/f/xnjlejbb" method="POST" novalidate>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/contact.html` around lines 84 - 85, The placeholder comment above the contact form is stale and references a different Formspree ID; update or remove it so it matches the actual form ID used by the form element with id="contact-form" (action="https://formspree.io/f/xnjlejbb"). Edit the comment to either remove the outdated "mbdqglqj" reference and state the correct guidance (e.g., "replace xnjlejbb with your Formspree form ID") or delete the comment entirely to avoid confusion; ensure the form tag (id="contact-form", action attribute) remains unchanged.
132-132: Consider using<output>for form submission feedback.The static analysis suggests using
<output>instead ofrole="status"for better semantic accessibility. The<output>element is specifically designed for form calculation/submission results.♿ Proposed semantic improvement
-<div id="form-status" class="form-status" role="status" aria-live="polite"></div> +<output id="form-status" class="form-status" aria-live="polite"></output>Note: You may need minor CSS adjustments since
<output>is inline by default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/contact.html` at line 132, Replace the div with an <output> element for the form feedback (keep id="form-status" and class="form-status"), remove role="status" (output is semantic for form results) and retain aria-live="polite" if you want screen readers to announce updates; adjust CSS because <output> is inline by default and add a name or for attribute if needed for form association within functions handling submission (look for the element with id/form-status in the submission code to ensure any JS that queries it still works).site/css/style.css (1)
1327-1335: The.sr-onlyutility uses deprecatedclipproperty.The
clip: rect(0,0,0,0)property is deprecated. While it has good browser support, the modern replacement isclip-path.📐 Proposed modern alternative
.sr-only { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; - clip: rect(0,0,0,0); + clip-path: inset(50%); white-space: nowrap; border: 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/css/style.css` around lines 1327 - 1335, Update the .sr-only utility to use the modern clip-path API: replace the deprecated clip declaration by adding a clip-path (e.g., clip-path: inset(0 0 0 0) or inset(50%)) and keep the existing clip line only as a fallback for older browsers; ensure the rest of the rules (position, width, height, padding, margin, overflow, white-space, border) remain unchanged so the element remains visually hidden but accessible.forgelm/ingestion.py (1)
737-782: Consider extracting a helper to reduce cognitive complexity.SonarCloud flags this function at 16 > 15 allowed complexity. The fallback
try/exceptblock (lines 763-768) and the greedy-packing loop could be extracted into small helpers to drop below the threshold.♻️ Proposed refactor: extract token-counting helper
+def _batch_token_counts(tokenizer: Any, blocks: List[str]) -> List[int]: + """Return token counts for each block; falls back to per-item encode.""" + try: + batch_enc = tokenizer(blocks, add_special_tokens=False) + return [len(ids) for ids in batch_enc["input_ids"]] + except Exception: + return [len(tokenizer.encode(b, add_special_tokens=False)) for b in blocks] + def _chunk_markdown_tokens(text: str, max_tokens: int, tokenizer: Any) -> Iterable[str]: ... section_blocks: List[str] = [] for path, body in sections: breadcrumb = _heading_breadcrumb(path, path[-1] if path else "") section_blocks.append(breadcrumb + body if breadcrumb else body) - try: - batch_enc = tokenizer(section_blocks, add_special_tokens=False) - section_token_counts = [len(ids) for ids in batch_enc["input_ids"]] - except Exception: - section_token_counts = [len(tokenizer.encode(block, add_special_tokens=False)) for block in section_blocks] + section_token_counts = _batch_token_counts(tokenizer, section_blocks)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/ingestion.py` around lines 737 - 782, The function _chunk_markdown_tokens is too complex—extract two small helpers: (1) a token counting helper (e.g., _count_tokens_for_blocks(blocks, tokenizer)) that contains the try/except batch-tokenization fallback logic that currently computes section_token_counts, and (2) a greedy-packing helper (e.g., _pack_sections_into_chunks(section_blocks, section_token_counts, sep_tokens, max_tokens)) that contains the loop zipping section_blocks with section_token_counts and yields/join logic currently constructing chunks; call these from _chunk_markdown_tokens and keep use of symbols _markdown_sections, _heading_breadcrumb, tokenizer, section_blocks, sep_tokens, section_token_counts so behavior is identical..github/workflows/ci.yml (1)
149-172: Consider addingdocs/guides/pairs to the bilingual H2 parity check.The current check covers only
docs/reference/documents. Phase 12 adds/updates several guide pairs (ingestion.md/ingestion-tr.md,data_audit.md/data_audit-tr.md) that would benefit from the same structure consistency enforcement.♻️ Proposed addition
pairs = [ ('docs/reference/configuration.md', 'docs/reference/configuration-tr.md'), ('docs/reference/usage.md', 'docs/reference/usage-tr.md'), ('docs/reference/distributed_training.md','docs/reference/distributed_training-tr.md'), ('docs/reference/data_preparation.md', 'docs/reference/data_preparation-tr.md'), ('docs/reference/architecture.md', 'docs/reference/architecture-tr.md'), + ('docs/guides/ingestion.md', 'docs/guides/ingestion-tr.md'), + ('docs/guides/data_audit.md', 'docs/guides/data_audit-tr.md'), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 149 - 172, The workflow step "Bilingual doc H2 parity check" currently enumerates only docs/reference/* pairs in the pairs list; update the pairs list variable in that Python block (the pairs assigned near the top of the step) to also include the guide pairs (e.g., add ('docs/guides/ingestion.md','docs/guides/ingestion-tr.md') and ('docs/guides/data_audit.md','docs/guides/data_audit-tr.md')) so the same H2 parity check loop (the for en, tr in pairs block and its en_count/tr_count comparisons) will validate the new guides as well.forgelm/compliance.py (1)
434-435: Narrow the exception catch to match realistic failure modes from datasets library.The current
except Exceptionis overly broad and masks programming errors. While this is non-fatal enrichment (allowed under guidelines), the codebase elsewhere favors specific exception handling. The suggested list is incomplete; based ondatasets.load_dataset_builderbehavior, catch:ImportError,FileNotFoundError,ValueError,AttributeError, anddatasets.exceptions.DatasetNotFoundError:♻️ Improved exception handling
try: from datasets import load_dataset_builder builder = load_dataset_builder(dataset_path) if builder.info.version: fingerprint["version"] = str(builder.info.version) if builder.info.description: fingerprint["description"] = builder.info.description[:200] if builder.info.download_size: fingerprint["download_size_bytes"] = builder.info.download_size - except Exception as e: + except (ImportError, FileNotFoundError, ValueError, AttributeError) as e: logger.debug("HF Hub metadata fetch skipped for '%s': %s", dataset_path, e)If using DatasetNotFoundError (from
datasets.exceptions), add that to the tuple for better precision on missing/inaccessible datasets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/compliance.py` around lines 434 - 435, Replace the overly broad "except Exception as e" that wraps the HF Hub metadata fetch (the logger.debug call referencing dataset_path) with a narrowed tuple catch: except (ImportError, FileNotFoundError, ValueError, AttributeError, datasets.exceptions.DatasetNotFoundError) as e, and ensure the datasets.exceptions.DatasetNotFoundError symbol is available (import datasets or reference the exception via datasets.exceptions) so only realistic dataset-loading failures are swallowed and logged.
🤖 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/reference/architecture.md`:
- Line 61: Update the test files count text in both documentation files to
reflect the current repository state: change the string "800+ unit tests across
47 test files" in architecture.md to "800+ unit tests across 50 test files" and
update the similar occurrence in CONTRIBUTING.md that currently says "26+ test
files" to "50 test files" (or "50+ test files" if you want to preserve the plus
style); search for those exact phrases to locate the lines to edit.
In `@docs/reference/compliance_summary.md`:
- Around line 88-93: The relative links in docs/reference/compliance_summary.md
point from docs/reference/ and thus resolve incorrectly; update each target to
the correct path from that directory: change forgelm/* links
(forgelm/trainer.py, forgelm/safety.py, forgelm/compliance.py) to
../../forgelm/<file>, change config_template.yaml to ../../config_template.yaml,
change tests/* links (tests/test_compliance.py, tests/test_trainer.py) to
../../tests/<file>, and change the safety guide link
docs/guides/safety_compliance.md to ../guides/safety_compliance.md so all links
resolve from docs/reference/.
In `@docs/reference/configuration-tr.md`:
- Around line 202-206: The FSDP options table in
docs/reference/configuration-tr.md is incomplete — add the missing FSDP knobs
exported by forgelm/config.py (fsdp_backward_prefetch and fsdp_state_dict_type)
with their types, defaults and allowed values in Turkish following existing
table style and localization standards; reference the existing rows (strategy,
deepspeed_config, fsdp_strategy, fsdp_auto_wrap, fsdp_offload) to match wording,
include correct boolean/string types and default values taken from
forgelm/config.py, and ensure phrasing and grammar adhere to
docs/standards/documentation.md and docs/standards/localization.md.
- Around line 235-240: The webhook docs table omits the new WebhookConfig fields
allow_private_destinations and tls_ca_bundle; update the table to document these
two fields (include Turkish labels), specifying types and defaults (e.g.,
allow_private_destinations: bool, default false; tls_ca_bundle: string|null or
path/PEM content, default null), and add brief descriptions in Turkish about
opting into private destination requests and providing a custom CA bundle;
follow docs/standards/documentation.md and docs/standards/localization.md for
phrasing, formatting, and translation conventions and ensure the field names
match the WebhookConfig symbols exactly (allow_private_destinations,
tls_ca_bundle).
In `@docs/reference/usage-tr.md`:
- Around line 167-170: Add a language identifier to the fenced log block so
markdownlint MD040 stops flagging it: change the opening fence from ``` to
```text for the block containing the log lines that start with "2026-03-24
10:30:00 [INFO] forgelm.trainer: Starting training..." and the warning about
"eval_steps (200) is larger than dataset (50 samples)." to explicitly mark it as
plain text.
In `@docs/roadmap.md`:
- Line 29: The planned-phase count is inconsistent: the text lists "2 phases
(13, 14) planned" while Phase 12.5 is also marked planned elsewhere; either
update the parenthetical to "3 phases (12.5, 13, 14) planned" and adjust the
version notes (`v0.5.3`, `v0.6.0-pro`) accordingly, or explicitly mark "Phase
12.5" as backlog/not counted and change the sentence to clarify that only Phases
13 and 14 are counted as planned; locate references to Phase 12.5, the sentence
"2 phases (13, 14) planned," and the version labels to make the consistent
change.
In `@forgelm/data_audit.py`:
- Around line 403-427: The numpy fast path (_compute_simhash_numpy) silently
truncates hashes >64 bits; update the dispatch in compute_simhash so it only
selects _compute_simhash_numpy when bits <= 64 (in addition to the existing
numpy availability checks), and ensure calls with bits > 64 fall back to the
non-numpy implementation (e.g., the pure-Python simhash path that uses
_token_digest for wider digests). Modify the dispatcher condition referencing
compute_simhash and _compute_simhash_numpy to include a bits <= 64 guard so
wider digests are not converted to np.uint64.
In `@forgelm/export.py`:
- Line 90: The check using env_override.endswith(".py") is case-sensitive and
will reject filenames like CONVERTER.PY; update the validation in the
export-related logic that inspects env_override to perform a case-insensitive
suffix check (e.g., normalize with .lower() or use str.casefold()) so that
filenames ending with any case variation of ".py" are accepted; ensure you only
change the condition around the env_override.endsWith check so other behavior
remains unchanged.
- Around line 90-95: The new ValueError raised when env_override doesn't end
with ".py" can escape the export_model() error handling and crash callers;
update export_model() (the try/except around import and file handling) to also
catch ValueError (in addition to ImportError/FileNotFoundError) and return an
ExportResult(success=False, error=...) instead of letting the exception
propagate, or alternatively change the env_override validation to raise a
FileNotFoundError/ImportError-like exception that export_model() already
handles; reference the env_override validation block and the
export_model()/ExportResult return path when making the fix.
In `@forgelm/trainer.py`:
- Around line 1236-1249: The current try/except only emits
compliance.governance_failed on OSError; change the exception handling around
generate_data_governance_report/json.dump (the block that sets governance_ok,
writes gov_path, calls self.audit.log_event("compliance.governance_exported"))
to also catch all other exceptions (e.g., add an except Exception as e branch or
broaden the existing except) and call
self.audit.log_event("compliance.governance_failed", reason=str(e)) and
logger.warning(...) so that any failure (TypeError, ValueError, etc.) is
recorded in the append-only audit trail while preserving the best-effort
behavior (do not re-raise).
In `@forgelm/webhook.py`:
- Around line 34-42: The DNS rebinding TOCTOU can be fixed by resolving and
returning the concrete IP(s) from socket.getaddrinfo inside
_is_private_destination (or a new helper) and using that resolved IP when
calling requests.post instead of re-resolving the hostname; modify
_is_private_destination/socket.getaddrinfo usage to return the selected public
IP literal and then call requests.post with that IP literal as the URL and set
the original hostname in the Host header (for HTTPS register
requests-toolbelt.HostHeaderSSLAdapter on the Session to preserve certificate
validation). Ensure requests.post still uses allow_redirects=False and include
the HostHeaderSSLAdapter setup on the Session before making the request.
In `@forgelm/wizard.py`:
- Around line 47-51: The regex _HF_HUB_ID_RE uses \w which is Unicode-aware and
thus allows non-ASCII characters; update the pattern to enforce ASCII-only
tokens by either replacing \w with an explicit ASCII class like [A-Za-z0-9_]
everywhere it appears in the pattern or by compiling with the re.ASCII flag
(e.g., re.compile(r"^[A-Za-z0-9_.-]{1,96}/[A-Za-z0-9_.-]{1,96}$") or
re.compile(r"^[\w.-]{1,96}/[\w.-]{1,96}$", flags=re.ASCII)) so that HF Hub IDs
are validated as ASCII-only in _HF_HUB_ID_RE.
In `@notebooks/quickstart_sft.ipynb`:
- Line 29: Remove the explicit bitsandbytes package from the notebook install
command: replace the pip line that currently reads "!pip install -q
--no-cache-dir \"forgelm[qlora]\" bitsandbytes" with a single-package install
forgelm[qlora] only, and search for any other occurrences of "bitsandbytes" in
the notebook to avoid reintroducing the platform-incompatible install; ensure
the new command is "!pip install -q --no-cache-dir \"forgelm[qlora]\"" so that
bitsandbytes is not installed unconditionally.
In `@site/contact.html`:
- Around line 102-108: The select options for the topic dropdown (element id
"topic") are hardcoded in English; update the <option> elements to use i18n
markers consistent with the rest of the form (e.g., replace the static texts
with translated keys or i18n-inline spans) and then extend the i18n.js
language-switch handler to update option text when language changes (ensure you
target the "topic" select and map each value:
"general","bug","feature","security","commercial" to the correct translated
strings).
In `@site/css/style.css`:
- Around line 1136-1147: The success/error form status text colors
(.form-status.success and .form-status.error) are too light to guarantee WCAG AA
contrast on their semi-transparent backgrounds; update these rules to use
darker/more saturated variants (e.g. your design tokens like --color-green-600 /
--color-red-600 or darker hexes) so the text contrast meets accessibility
guidelines, keeping the existing background/border treatments intact.
---
Outside diff comments:
In `@forgelm/wizard.py`:
- Around line 212-233: Add structured append-only audit events at each
save/fallback decision point: right after the primary success
logger.info("Wizard config saved to %s", requested_filename) emit a persistent
audit event (e.g., emit_audit_event or append_audit_entry) with type
"wizard_save_primary_success" and payload {requested_filename, timestamp,
user/context}; in the primary except block after logger.error("Could not save
wizard config to %s: %s", requested_filename, e) emit
"wizard_save_primary_failure" with {requested_filename, error: str(e),
timestamp, user/context}; after the fallback success logger.info("Wizard config
saved to fallback location %s", fallback) emit "wizard_save_fallback_success"
with {fallback, original_requested: requested_filename, timestamp,
user/context}; and in the fallback except block after logger.error("Fallback
wizard config save also failed (%s): %s", fallback, e) emit
"wizard_save_fallback_failure" with {fallback, original_requested:
requested_filename, error: str(e), timestamp, user/context}; ensure the chosen
audit function writes append-only records and does not alter existing logs.
---
Duplicate comments:
In `@CONTRIBUTING.md`:
- Line 81: The two docs disagree on the test count: CONTRIBUTING.md contains
"26+ test files" while docs/reference/architecture.md has "47 test files"; run a
reliable count of the repository's test files (e.g., by pattern used in CI or
the test directory naming), then update both CONTRIBUTING.md and
docs/reference/architecture.md so they contain the same, accurate number string
(replace "26+ test files" and "47 test files" with the verified count) and
ensure the wording format matches across both files.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 149-172: The workflow step "Bilingual doc H2 parity check"
currently enumerates only docs/reference/* pairs in the pairs list; update the
pairs list variable in that Python block (the pairs assigned near the top of the
step) to also include the guide pairs (e.g., add
('docs/guides/ingestion.md','docs/guides/ingestion-tr.md') and
('docs/guides/data_audit.md','docs/guides/data_audit-tr.md')) so the same H2
parity check loop (the for en, tr in pairs block and its en_count/tr_count
comparisons) will validate the new guides as well.
In `@docs/roadmap.md`:
- Line 27: Update the milestone trigger wording in the "Next: `v0.5.3` —
Multi-Stage Pipeline Chains (Phase 14)" line to be unambiguous: replace "Starts
after Phase 12 lands" with a concrete release event such as "starts after the
`v0.5.2` tag is published (PyPI release)" or "starts after `v0.5.2` is released
to PyPI", ensuring the line in docs/roadmap.md referencing v0.5.3 clearly
anchors the start to a specific release event rather than the merged Phase 12
state.
In `@forgelm/compliance.py`:
- Around line 434-435: Replace the overly broad "except Exception as e" that
wraps the HF Hub metadata fetch (the logger.debug call referencing dataset_path)
with a narrowed tuple catch: except (ImportError, FileNotFoundError, ValueError,
AttributeError, datasets.exceptions.DatasetNotFoundError) as e, and ensure the
datasets.exceptions.DatasetNotFoundError symbol is available (import datasets or
reference the exception via datasets.exceptions) so only realistic
dataset-loading failures are swallowed and logged.
In `@forgelm/ingestion.py`:
- Around line 737-782: The function _chunk_markdown_tokens is too
complex—extract two small helpers: (1) a token counting helper (e.g.,
_count_tokens_for_blocks(blocks, tokenizer)) that contains the try/except
batch-tokenization fallback logic that currently computes section_token_counts,
and (2) a greedy-packing helper (e.g.,
_pack_sections_into_chunks(section_blocks, section_token_counts, sep_tokens,
max_tokens)) that contains the loop zipping section_blocks with
section_token_counts and yields/join logic currently constructing chunks; call
these from _chunk_markdown_tokens and keep use of symbols _markdown_sections,
_heading_breadcrumb, tokenizer, section_blocks, sep_tokens, section_token_counts
so behavior is identical.
In `@notebooks/quickstart_sft.ipynb`:
- Line 205: The header string "## Next Steps — Phase 10 Post-Training Toolchain"
should be made phase-neutral to avoid confusion; locate the notebook cell
containing that exact string and update it to "## Next Steps — Post-Training
Toolchain" (remove "Phase 10") so the title reads generically and fits the Phase
12 release context.
In `@site/contact.html`:
- Around line 84-85: The placeholder comment above the contact form is stale and
references a different Formspree ID; update or remove it so it matches the
actual form ID used by the form element with id="contact-form"
(action="https://formspree.io/f/xnjlejbb"). Edit the comment to either remove
the outdated "mbdqglqj" reference and state the correct guidance (e.g., "replace
xnjlejbb with your Formspree form ID") or delete the comment entirely to avoid
confusion; ensure the form tag (id="contact-form", action attribute) remains
unchanged.
- Line 132: Replace the div with an <output> element for the form feedback (keep
id="form-status" and class="form-status"), remove role="status" (output is
semantic for form results) and retain aria-live="polite" if you want screen
readers to announce updates; adjust CSS because <output> is inline by default
and add a name or for attribute if needed for form association within functions
handling submission (look for the element with id/form-status in the submission
code to ensure any JS that queries it still works).
In `@site/css/style.css`:
- Around line 1327-1335: Update the .sr-only utility to use the modern clip-path
API: replace the deprecated clip declaration by adding a clip-path (e.g.,
clip-path: inset(0 0 0 0) or inset(50%)) and keep the existing clip line only as
a fallback for older browsers; ensure the rest of the rules (position, width,
height, padding, margin, overflow, white-space, border) remain unchanged so the
element remains visually hidden but accessible.
In `@site/js/main.js`:
- Around line 85-92: The code saves and restores the button label using
innerHTML which can trigger static-analysis warnings; switch to using
textContent for safety: when capturing the original label change orig =
btn.innerHTML to orig = btn.textContent and when restoring change btn.innerHTML
= orig to btn.textContent = orig (references: the btn variable and the
save/restore block in the copy-button handler).
- Around line 56-63: When no stored preference is found the code always calls
applyTheme('dark'); change this to honor system preference by using
window.matchMedia('(prefers-color-scheme: dark)').matches to pick 'dark' or
'light' instead of defaulting to dark; keep the existing localStorage read (KEY,
stored) and try/catch, and call applyTheme(stored) when stored is 'light' or
'dark', otherwise call applyTheme(systemPref ? 'dark' : 'light') where
systemPref is the matchMedia result.
🪄 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: f932153e-a0e9-41cf-b34b-5aed2c8287da
📒 Files selected for processing (49)
.github/workflows/ci.ymlCONTRIBUTING.mdREADME.mddocs/guides/enterprise_deployment.mddocs/guides/ingestion-tr.mddocs/guides/ingestion.mddocs/guides/safety_compliance.mddocs/product_strategy-tr.mddocs/product_strategy.mddocs/reference/architecture-tr.mddocs/reference/architecture.mddocs/reference/compliance_summary.mddocs/reference/configuration-tr.mddocs/reference/distributed_training-tr.mddocs/reference/usage-tr.mddocs/reference/usage.mddocs/roadmap.mddocs/roadmap/completed-phases.mdforgelm/cli.pyforgelm/compliance.pyforgelm/data.pyforgelm/data_audit.pyforgelm/export.pyforgelm/ingestion.pyforgelm/judge.pyforgelm/safety.pyforgelm/trainer.pyforgelm/utils.pyforgelm/webhook.pyforgelm/wizard.pynotebooks/quickstart_sft.ipynbsite/README.mdsite/compliance.htmlsite/contact.htmlsite/css/style.csssite/features.htmlsite/index.htmlsite/js/i18n.jssite/js/main.jssite/privacy.htmlsite/quickstart.htmlsite/terms.htmltests/test_alignment.pytests/test_cli_subcommands.pytests/test_compliance.pytests/test_data_edge_cases.pytests/test_distributed.pytests/test_eu_ai_act.pytests/test_phase7.py
✅ Files skipped from review due to trivial changes (11)
- docs/guides/enterprise_deployment.md
- tests/test_alignment.py
- tests/test_data_edge_cases.py
- docs/guides/safety_compliance.md
- forgelm/judge.py
- site/quickstart.html
- site/compliance.html
- forgelm/utils.py
- site/features.html
- site/terms.html
- docs/reference/architecture-tr.md
🚧 Files skipped from review as they are similar to previous changes (3)
- forgelm/data.py
- docs/reference/usage.md
- README.md
Comment on lines
+34
to
+42
| try: | ||
| # Pre-resolve so a hostname that points at a private IP still trips. | ||
| addrinfo = socket.getaddrinfo(host, None) | ||
| except (socket.gaierror, UnicodeError): | ||
| # DNS failure → not a private destination by our definition. Let | ||
| # `requests` produce its natural ConnectionError downstream so the | ||
| # operator gets the real "could not resolve host" message instead | ||
| # of an SSRF-shaped refusal that hides the typo. | ||
| return False |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python requests library connect to specific IP with Host header override
💡 Result:
To connect to a specific IP address using the Python requests library while overriding the Host header (e.g., to access a virtual host directly via IP), construct the URL with the IP and provide a custom 'Host' header. For HTTP (no SSL issues): import requests response = requests.get('http://192.0.2.1/path', headers={'Host': 'example.com'}) For HTTPS, the simple approach fails due to SNI and certificate verification using the IP from the URL instead of the Host header, causing hostname mismatch errors. Use the requests-toolbelt library's HostHeaderSSLAdapter to fix this by setting SNI and certificate verification based on the Host header. Install: pip install requests-toolbelt import requests from requests_toolbelt.adapters import host_header_ssl session = requests.Session session.mount('https://', host_header_ssl.HostHeaderSSLAdapter) response = session.get('https://192.0.2.1/path', headers={'Host': 'example.com'}) This works with certificate verification enabled. Requests official docs confirm custom headers like Host are supported via the headers dict. The Host header override is standards-compliant per RFC 7230, allowing differentiation of virtual hosts on the same IP.
Citations:
- 1: https://docs.python-requests.org/en/stable/user/quickstart/
- 2: https://toolbelt.readthedocs.org/en/latest/adapters.html
- 3: https://stackoverflow.com/questions/68974641/pass-ip-address-in-python-requests-instead-of-url-link
- 4: https://stackoverflow.com/questions/29995133/python-requests-use-navigate-site-by-servers-ip
- 5: https://www.tomechangosubanana.com/2017/forcing-python-requests-to-connect-to-a-specific-ip-address/
- 6: https://docs.python-requests.org/en/latest/api/?highlight=headers
- 7: https://datatracker.ietf.org/doc/html/rfc7230
🏁 Script executed:
# First, find and read the webhook.py file
find . -name "webhook.py" -type f | head -5Repository: cemililik/ForgeLM
Length of output: 82
🏁 Script executed:
# Read the file to understand the full function
cat -n forgelm/webhook.py | head -70Repository: cemililik/ForgeLM
Length of output: 3050
🏁 Script executed:
# Search for calls to _is_private_destination to understand how it's used
rg "_is_private_destination" --context 5Repository: cemililik/ForgeLM
Length of output: 1456
🏁 Script executed:
cat -n forgelm/webhook.py | sed -n '60,150p'Repository: cemililik/ForgeLM
Length of output: 4526
🏁 Script executed:
# Also search for where requests.post is called
rg "requests\." forgelm/webhook.py --context 3Repository: cemililik/ForgeLM
Length of output: 1213
🏁 Script executed:
cat -n forgelm/webhook.py | sed -n '143,155p'Repository: cemililik/ForgeLM
Length of output: 753
🏁 Script executed:
cat -n forgelm/webhook.py | sed -n '143,160p'Repository: cemililik/ForgeLM
Length of output: 1091
DNS rebinding creates a TOCTOU window between validation and connection.
The hostname is resolved at line 36 to check for private IPs, but requests.post() at line 143 resolves DNS again internally when making the connection. An attacker-controlled DNS server could return a public IP on the first lookup (passing the check) and a private IP on the second lookup (when requests connects).
This is mitigated by allow_redirects=False (which prevents redirect-based rebinding) and the short timing window, but the direct hostname rebinding vector remains. For defense-in-depth, consider extracting the resolved IP from _is_private_destination() and passing it directly to requests.post() as an IP literal with a Host header override (requires requests-toolbelt.HostHeaderSSLAdapter for HTTPS certificate validation).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forgelm/webhook.py` around lines 34 - 42, The DNS rebinding TOCTOU can be
fixed by resolving and returning the concrete IP(s) from socket.getaddrinfo
inside _is_private_destination (or a new helper) and using that resolved IP when
calling requests.post instead of re-resolving the hostname; modify
_is_private_destination/socket.getaddrinfo usage to return the selected public
IP literal and then call requests.post with that IP literal as the URL and set
the original hostname in the Host header (for HTTPS register
requests-toolbelt.HostHeaderSSLAdapter on the Session to preserve certificate
validation). Ensure requests.post still uses allow_redirects=False and include
the HostHeaderSSLAdapter setup on the Session before making the request.
Collaborator
Author
There was a problem hiding this comment.
Verified, but declining the full HostHeaderSSLAdapter rewrite for this PR. The TOCTOU window between _is_private_destination()'s pre-resolve and the actual requests.post is a real concern, but pinning the request to the IP literal would require:
- Adding
requests-toolbeltas a hard dep (currently only needed for this one webhook path). - Rebuilding the URL with the IP, threading the original hostname through the
Hostheader, registeringHostHeaderSSLAdapteron a per-callSession— substantial complexity for a code path that already has a primary mitigation in place. - Reasoning through TLS SNI + cert validation interactions across our supported
requestsversions.
The mitigation that does exist is allow_private_destinations: false (the default in WebhookConfig). Operators that explicitly opt in to private destinations are accepting the risk by definition. The remaining attack surface is "public DNS that flips to private between resolve and connect" — narrow, requires attacker control of the DNS for the configured webhook, and is bounded by the now-allow_redirects=False request not chaining further.
I'd rather track this as a follow-up (e.g., a v0.5.3 hardening item) than land a partially-tested HostHeaderSSLAdapter wiring in a PR that's otherwise about Phase 12 data curation. Open to revisiting if there's a real-world report — just want the dep + SSL plumbing on its own commit so it can be reverted cleanly if the SNI/cert behaviour turns out to differ across requests releases.
Contributor
There was a problem hiding this comment.
@cemililik, that's a well-reasoned call. The risk profile (attacker-controlled DNS + public-to-private flip between resolve and connect) is narrow, allow_private_destinations: false is a strong primary control, and allow_redirects=False meaningfully bounds the blast radius. Deferring the HostHeaderSSLAdapter wiring to its own commit/PR makes sense — it deserves isolated review for the SNI/cert validation behaviour.
Would you like me to open a GitHub issue to track this as a v0.5.3 hardening item? I can include the context from this discussion (the TOCTOU window, the proposed fix direction, and the dependencies/complexity tradeoffs) as a backlink.
🧠 Learnings used
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 13
File: forgelm/data_audit.py:71-79
Timestamp: 2026-04-28T11:08:48.337Z
Learning: In this project (cemililik/ForgeLM), treat heavy/optional dependencies (e.g., datasketch, xxhash, unsloth, llama_cpp) as opt-in extras: use a lazy-import pattern instead of failing eagerly at module import time. Concretely: do a top-level `try/except ImportError` that sets a `_HAS_<DEP>` boolean and assigns the missing module name to `None`, then gate feature code behind a `_require_<dep>()` helper that raises `ImportError` with an install hint only when the feature is actually invoked. Do not use a convention where importing the module always fails due to optional extras; this is intentional to keep base-install users from being forced to install optional dependencies just to use other features.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Sure! Let me create that issue now.
🏁 Script executed:
gh issue create \
--repo cemililik/ForgeLM \
--title "[Security / v0.5.3] Harden webhook SSRF guard against DNS rebinding (TOCTOU between pre-resolve and requests.post)" \
--body "## Context
Raised in PR `#13` (review comment: https://github.com/cemililik/ForgeLM/pull/13#discussion_r3156762511) by `@coderabbitai`.
Requested by: `@cemililik`
## Problem
\`_is_private_destination()\` in \`forgelm/webhook.py\` pre-resolves the webhook hostname via \`socket.getaddrinfo\` to block private/loopback destinations. However, \`requests.post()\` performs its own DNS resolution when making the actual connection, creating a TOCTOU window: an attacker-controlled DNS server could return a public IP on the first lookup (passing the check) and a private IP on the second lookup (when \`requests\` connects), bypassing the SSRF guard.
## Existing mitigations (already in place)
- \`allow_private_destinations: false\` is the default in \`WebhookConfig\` — operators must explicitly opt in to private destinations.
- \`allow_redirects=False\` prevents redirect-chaining to a private destination after a valid initial response.
- The attack surface is narrow: requires attacker control of the DNS server for the configured webhook domain.
## Proposed fix direction
1. Resolve the hostname to a concrete public IP inside \`_is_private_destination()\` (or a new helper) and return that IP alongside the boolean result.
2. Reconstruct the webhook URL with the IP literal.
3. Pass the original hostname in a \`Host\` header.
4. For HTTPS, register \`requests_toolbelt.adapters.host_header_ssl.HostHeaderSSLAdapter\` on a per-call \`Session\` to preserve SNI and certificate validation against the original hostname.
5. Keep \`allow_redirects=False\`.
## Dependencies / complexity
- Adds \`requests-toolbelt\` as a dependency (new hard dep or optional extra — TBD).
- Requires careful SNI/cert validation testing across supported \`requests\` versions.
- Should land in its own isolated commit/PR so it can be reverted cleanly if behaviour differs across \`requests\` releases.
## Target
v0.5.3 hardening cycle. Deferred from v0.5.2 (Phase 12) to keep the HostHeaderSSLAdapter wiring reviewable in isolation.
" \
--assignee cemililik
Length of output: 47
@cemililik, done! The tracking issue is open at #14, assigned to you, with the full context: the TOCTOU window, existing mitigations, the proposed HostHeaderSSLAdapter fix direction, and the dependency/complexity tradeoffs. Ready to pick up whenever the v0.5.3 hardening cycle starts.
ᓚᘏᗢ
🧠 Learnings used
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 13
File: forgelm/data_audit.py:71-79
Timestamp: 2026-04-28T11:08:48.337Z
Learning: In this project (cemililik/ForgeLM), treat heavy/optional dependencies (e.g., datasketch, xxhash, unsloth, llama_cpp) as opt-in extras: use a lazy-import pattern instead of failing eagerly at module import time. Concretely: do a top-level `try/except ImportError` that sets a `_HAS_<DEP>` boolean and assigns the missing module name to `None`, then gate feature code behind a `_require_<dep>()` helper that raises `ImportError` with an install hint only when the feature is actually invoked. Do not use a convention where importing the module always fails due to optional extras; this is intentional to keep base-install users from being forced to install optional dependencies just to use other features.
…tion Verified each new review finding against the current code; applied 18 real fixes and skipped 2 invalid (wizard audit events would conflate training-run scope; webhook DNS-rebinding TOCTOU full fix needs requests-toolbelt — left an inline note, mitigation already exists via allow_private_destinations opt-in). Source / config - forgelm/data_audit.py — compute_simhash now also gates the numpy fast path on bits <= 64. Wider digests fall back to the BLAKE2b pure-Python path that scales arbitrarily wide; the numpy path used np.uint64 and would silently truncate. - forgelm/export.py — env_override is now matched with .casefold() (CONVERTER.PY accepted on case-insensitive filesystems), and export_model() catches ValueError alongside ImportError / FileNotFoundError so a non-.py FORGELM_GGUF_CONVERTER produces an ExportResult instead of crashing the caller. - forgelm/trainer.py — broadened the governance-export catch from OSError to Exception (with explanatory comment) so TypeError / ValueError / AttributeError failures still appear as compliance.governance_failed in the audit chain instead of propagating up. - forgelm/compliance.py — narrowed `except Exception` for the HF Hub metadata fetch to the realistic failure modes (ImportError, FileNotFoundError, ValueError, AttributeError, ConnectionError, TimeoutError). - forgelm/wizard.py — added re.ASCII flag to _HF_HUB_ID_RE so the \w class means [A-Za-z0-9_]; HF Hub IDs are ASCII-only and the default Unicode-aware \w would let through characters the Hub itself rejects. Docs - CONTRIBUTING.md — test count refreshed from "26+ test files" to "47 test files" (verified via `find tests -name 'test_*.py'`). - docs/reference/compliance_summary.md — six relative links fixed (../../forgelm, ../../config_template.yaml, ../guides/, ../../tests). - docs/reference/configuration.md + configuration-tr.md — added fsdp_backward_prefetch and fsdp_state_dict_type rows (present in forgelm/config.py since the FSDP rollout but missing from the reference); added allow_private_destinations and tls_ca_bundle rows to the webhook table. - docs/reference/usage.md + usage-tr.md — log fence now declares ```text so markdownlint MD040 stops flagging it. - docs/roadmap.md — "2 phases (13, 14) planned" → "3 phases (12.5, 13, 14) planned"; "Starts after Phase 12 lands" → "Starts after the v0.5.2 PyPI tag is published" (concrete release event). - docs/guides/ingestion.md — promoted "Secrets masking (Phase 12)" from H3 to H2 so the EN/TR pair has matching H2 counts (11/11). Notebook - notebooks/quickstart_sft.ipynb — Step 1 install no longer pins bitsandbytes explicitly (it's already pulled in by forgelm[qlora] on supported platforms), and the next-steps header dropped the obsolete "Phase 10" prefix. CI - .github/workflows/ci.yml — extended bilingual H2 parity check to also cover docs/guides/ingestion + docs/guides/data_audit (5 → 7 pairs). Site - site/contact.html — topic <option> elements now carry data-i18n keys (contact.form.topic_*) so the dropdown switches with the rest of the page; stale "mbdqglqj" Formspree comment refreshed to reference the actual xnjlejbb form ID. - site/css/style.css — .sr-only adds clip-path: inset(50%) while retaining the deprecated `clip` rule as a legacy fallback. - site/js/main.js — initTheme now honours prefers-color-scheme on the first visit before falling back to the dark default. - site/js/* + site/css/* + site/js/translations.js — the broader language-dropdown rework + 6-language translations table is included in this commit because the IDE captured those edits as part of the same working tree state; no behavioural regressions spotted, all 822 tests + ruff still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
forgelm/compliance.py (1)
176-212:⚠️ Potential issue | 🔴 CriticalReload the chain head inside the locked write path.
self._prev_hashis loaded once in__init__, before any file lock is taken. TwoAuditLoggerinstances targeting the sameoutput_dircan both enterlog_event()with the same stale head;flockprevents interleaved bytes, but the later writer still appends a line whoseprev_hashignores the earlier append. That forks the chain and can also pin the wrong genesis manifest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/compliance.py` around lines 176 - 212, The log_event method can use a stale self._prev_hash loaded at init which allows two AuditLogger writers to fork the chain; to fix, after acquiring the file lock (inside the with open(self.log_path, "a") and after _flock_ex(f)), re-read the current chain head from the log file (seek to start/end, read last non-empty line, parse its hash) and set prev = that hash (update is_genesis based on that fresh prev) before constructing the entry and computing the HMAC; then append using f.write, call _write_genesis_manifest(new_hash) only when the freshly-determined genesis transition occurs, and update self._prev_hash = new_hash after the successful write. Ensure you still compute the HMAC over the entry without the _hmac tag and use _flock_un(f) in the finally block.docs/reference/usage-tr.md (1)
72-117:⚠️ Potential issue | 🟡 MinorThe Turkish reference still doesn't show the new Phase 12 flags.
Lines 72-117 update the version banners to mention
markdown,--secrets-mask,minhash, and--quality-filter, but the TR examples and prose below still stop at the older surface. Turkish readers will not see how to invoke the new options or what changed in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/usage-tr.md` around lines 72 - 117, The Turkish reference page still lacks examples and prose for the new Phase 12 flags/features (markdown ingestion, --secrets-mask, minhash, --quality-filter), so update docs/reference/usage-tr.md to add matching Turkish examples and short explanations next to the existing forgelm ingest and forgelm audit sections: show how to pass --secrets-mask and the markdown-aware ingestion (e.g., mention `--format markdown` or "markdown" support inline), add an audit example demonstrating minhash usage and the --quality-filter flag (e.g., `forgelm audit ... --minhash --quality-filter`), and add one-line Turkish descriptions of each new option (markdown, secrets masking, MinHash, quality filter) to mirror the version banner changes; ensure you modify the same blocks that contain the forgelm ingest and forgelm audit snippets so examples and prose are in Turkish and consistent with the updated version notes.
♻️ Duplicate comments (1)
forgelm/compliance.py (1)
82-110:⚠️ Potential issue | 🟠 MajorOversize final audit entries still re-root the chain on restart.
This still misses the case where the last JSONL entry itself is larger than 4096 bytes.
f.readline()consumes the remainder of that only entry,tailbecomes empty, and_load_last_hash()falls back to"genesis"instead of restoring the real head.🔧 Correctness-first fix
try: with open(self.log_path, "rb") as f: - f.seek(0, 2) - size = f.tell() - if size == 0: - return "genesis" - seek_start = max(0, size - 4096) - f.seek(seek_start) - # When the seek lands mid-line, the first newline-bounded chunk - # we'd see is a partial entry. ``splitlines()`` keeps it as - # element [0]; with a single oversize entry > 4096 B it would - # also become element [-1] and we'd hash a truncated body. - # Skip past the partial first line so every line we parse is - # whole. - if seek_start > 0: - f.readline() - tail = f.read() + last_line: Optional[bytes] = None + for raw in f: + if raw.strip(): + last_line = raw.rstrip(b"\r\n") except OSError as e: raise OSError( f"Audit log exists at {self.log_path!r} but could not be read: {e}. " "Refusing to silently re-root the hash chain." ) from e - try: - lines = [ln for ln in tail.decode("utf-8").splitlines() if ln.strip()] - except UnicodeDecodeError as e: - raise ValueError( - f"Audit log {self.log_path!r} contains non-UTF-8 data — likely corrupt: {e}. " - "Refusing to silently re-root the hash chain." - ) from e - if lines: - last_line = lines[-1].encode("utf-8") + if last_line is not None: + try: + last_line.decode("utf-8") + except UnicodeDecodeError as e: + raise ValueError( + f"Audit log {self.log_path!r} contains non-UTF-8 data — likely corrupt: {e}. " + "Refusing to silently re-root the hash chain." + ) from e return hashlib.sha256(last_line).hexdigest()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/compliance.py` around lines 82 - 110, The logic in _load_last_hash (reading tail from self.log_path, using seek_start and f.readline()) fails when the final JSONL entry itself exceeds 4096 bytes because tail becomes empty and you fall back to "genesis"; fix by detecting the empty-rows case and reading the remainder of the oversized last entry explicitly (e.g., after seeking to seek_start, call f.readline() or read from that seek_start to the file end and parse the last newline-terminated record) so you still recover the actual last JSONL line, decode/strip it and return its sha256; update the branch that handles empty lines (the lines == [] path) to attempt this additional read/parse before returning the genesis value.
🤖 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/reference/usage.md`:
- Line 195: The docs text for the always-on credentials scan is out of sync with
the JSON contract: update the wording around the `secrets_summary` to match the
detector/report surface by replacing “private-key headers” with wording that
reflects full private-key blocks and explicitly include `azure_storage_key` (and
any other secret types present in the JSON) so the prose matches the actual JSON
contract returned by the scanner.
In `@forgelm/compliance.py`:
- Around line 559-581: The template still interpolates several config-derived
fields raw, allowing CommonMark specials to break the Markdown; update all
places that inject user-config strings into the deployer instructions to pass
values through _sanitize_md (using the existing _COMMONMARK_SPECIALS logic) —
specifically sanitize system_name, config.model.name_or_path (or model.name),
final_path, misuse bullets/labels, and any metric names before template
substitution or f-string creation; ensure newline collapsing and .strip()
behavior from _sanitize_md is preserved so table cells, headings, code spans,
and pipe characters cannot be injected.
- Around line 57-62: The current logic in compliance.py builds a per-run HMAC
key even when FORGELM_AUDIT_SECRET is empty (secret = "", self._hmac_key =
hashlib.sha256(...)), which makes the HMAC forgeable because run_id is public;
update the code so it either (A) requires a non-empty secret before producing an
HMAC (raise/configure error or set self._hmac_key = None and stop emitting any
_hmac fields), or (B) treat the value as an unauthenticated checksum by
renaming/flagging it accordingly and not claiming tamper-evidence; locate uses
of self._hmac_key and any emission of "_hmac" (the initializer that sets
self._hmac_key and the code paths around lines 180-191) and implement the chosen
policy consistently (guard HMAC creation, skip/hide _hmac when secret missing,
or change label/behavior to checksum).
In `@forgelm/data_audit.py`:
- Around line 2151-2166: The overall_quality_score uses total_samples as the
denominator but _row_quality_flags only evaluates text rows, so change
_build_quality_summary to accept the actual number of evaluated rows (e.g.,
samples_evaluated or samples_checked) instead of total_samples, compute
overall_score = round(1.0 - (samples_flagged_total / samples_evaluated), 4) with
a safe fallback when samples_evaluated is zero, and update all call sites (the
code that invokes _build_quality_summary, which calls/relates to
_row_quality_flags) to pass the counted number of rows that were actually
checked rather than total_samples.
In `@forgelm/ingestion.py`:
- Around line 758-762: The code currently swallows all exceptions when calling
tokenizer(section_blocks...) which hides real bugs; change it to only fallback
on the specific batching-capability error and validate the batch result before
using it: call tokenizer(section_blocks, add_special_tokens=False) inside a try
that only catches the tokenizer batching failure (e.g., TypeError or a
tokenizer-specific exception), then verify batch_enc is a mapping containing
"input_ids" whose value is an iterable of sequences with length equal to
len(section_blocks) before returning [len(ids) for ids in
batch_enc["input_ids"]]; if validation fails, re-raise the error instead of
falling back, and only perform the per-block encode fallback using
tokenizer.encode(block, add_special_tokens=False) when the caught exception is
the expected unsupported-batch error.
In `@site/contact.html`:
- Around line 84-85: The contact form is using the demo Formspree endpoint
(action="https://formspree.io/f/xnjlejbb") and the inline comment still
instructs replacing xnjlejbb; update the <form id="contact-form"> action to the
actual Formspree form ID for this project (or switch to your own backend
endpoint) and remove or update the placeholder comment so submissions go to the
correct mailbox; verify the new form ID is valid in Formspree and that the form
still uses method="POST" and novalidate as intended.
In `@site/index.html`:
- Around line 16-19: Replace the placeholder origin "YOUR_DOMAIN" used in the
meta tags (og:image, og:url, og:locale) and the canonical link (link
rel="canonical") with the real deployment origin or remove the absolute URLs
until the real origin is available; update the content attributes for the
og:image and og:url meta tags and the href on the canonical link so they point
to the live site (e.g., https://your-real-domain/) or convert them to relative
paths if appropriate.
In `@site/js/i18n.js`:
- Around line 51-115: setLanguage only updates [data-i18n*] elements and buttons
under .lang-toggle-menu, so the new pages using paired .i18n-inline[data-lang]
spans, attributes like data-title-en/data-desc-en and global [data-set-lang]
buttons don't get toggled; extend setLanguage to (1) hide/show .i18n-inline
elements by matching their data-lang to the chosen lang, (2) swap metadata
attributes by reading elements with data-title-<lang>/data-desc-<lang> (or
generic data-title-* data-desc-*) and setting title/description accordingly, and
(3) bind click handlers for buttons matching [data-set-lang] (not just
.lang-toggle-menu button[data-set-lang]) so detectLanguage, setLanguage and the
existing menu-toggle logic work for the new controls. Ensure you reuse the
existing lookup/fallback logic for translations where applicable.
In `@site/js/main.js`:
- Around line 173-204: The submit handler attached via
form.addEventListener('submit', function (e) { ... }) currently sends FormData
unconditionally; before disabling the submit button and calling fetch(), call
form.checkValidity() and if it returns false call form.reportValidity() and
return to abort submission so native required/constraint checks run; move the
btn.disabled = true and the fetch() invocation after the validity gate so
invalid/empty forms are not posted.
In `@tests/test_phase12_review_fixes.py`:
- Around line 478-546: Replace the flaky absolute timing assertions in
test_heading_pattern_linear_on_pathological_whitespace,
test_strip_code_fences_linear_on_unclosed_blocks, and
test_parse_md_fence_linear_on_long_runs with a growth-ratio check (or gate the
tests behind an explicit performance marker). Concretely, compute median
elapsed_ms for each input size (the existing loop over n), then assert that
elapsed_ms_for_n / elapsed_ms_for_smallest <= (n / smallest_n) * TOLERANCE (pick
a small constant like 2–3) to verify linear scaling; alternatively annotate the
three test functions with a pytest marker (e.g., `@pytest.mark.performance`) and
skip them by default on shared CI. Use the existing function names
(_MARKDOWN_HEADING_PATTERN, _strip_code_fences, _parse_md_fence) to locate the
tests to modify.
---
Outside diff comments:
In `@docs/reference/usage-tr.md`:
- Around line 72-117: The Turkish reference page still lacks examples and prose
for the new Phase 12 flags/features (markdown ingestion, --secrets-mask,
minhash, --quality-filter), so update docs/reference/usage-tr.md to add matching
Turkish examples and short explanations next to the existing forgelm ingest and
forgelm audit sections: show how to pass --secrets-mask and the markdown-aware
ingestion (e.g., mention `--format markdown` or "markdown" support inline), add
an audit example demonstrating minhash usage and the --quality-filter flag
(e.g., `forgelm audit ... --minhash --quality-filter`), and add one-line Turkish
descriptions of each new option (markdown, secrets masking, MinHash, quality
filter) to mirror the version banner changes; ensure you modify the same blocks
that contain the forgelm ingest and forgelm audit snippets so examples and prose
are in Turkish and consistent with the updated version notes.
In `@forgelm/compliance.py`:
- Around line 176-212: The log_event method can use a stale self._prev_hash
loaded at init which allows two AuditLogger writers to fork the chain; to fix,
after acquiring the file lock (inside the with open(self.log_path, "a") and
after _flock_ex(f)), re-read the current chain head from the log file (seek to
start/end, read last non-empty line, parse its hash) and set prev = that hash
(update is_genesis based on that fresh prev) before constructing the entry and
computing the HMAC; then append using f.write, call
_write_genesis_manifest(new_hash) only when the freshly-determined genesis
transition occurs, and update self._prev_hash = new_hash after the successful
write. Ensure you still compute the HMAC over the entry without the _hmac tag
and use _flock_un(f) in the finally block.
---
Duplicate comments:
In `@forgelm/compliance.py`:
- Around line 82-110: The logic in _load_last_hash (reading tail from
self.log_path, using seek_start and f.readline()) fails when the final JSONL
entry itself exceeds 4096 bytes because tail becomes empty and you fall back to
"genesis"; fix by detecting the empty-rows case and reading the remainder of the
oversized last entry explicitly (e.g., after seeking to seek_start, call
f.readline() or read from that seek_start to the file end and parse the last
newline-terminated record) so you still recover the actual last JSONL line,
decode/strip it and return its sha256; update the branch that handles empty
lines (the lines == [] path) to attempt this additional read/parse before
returning the genesis value.
🪄 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: 2819f383-ff83-4552-8cc3-77b89b7e3328
📒 Files selected for processing (23)
.github/workflows/ci.ymlCONTRIBUTING.mddocs/guides/ingestion.mddocs/reference/compliance_summary.mddocs/reference/configuration-tr.mddocs/reference/configuration.mddocs/reference/usage-tr.mddocs/reference/usage.mddocs/roadmap.mdforgelm/compliance.pyforgelm/data_audit.pyforgelm/export.pyforgelm/ingestion.pyforgelm/trainer.pyforgelm/wizard.pynotebooks/quickstart_sft.ipynbsite/contact.htmlsite/css/style.csssite/index.htmlsite/js/i18n.jssite/js/main.jssite/js/translations.jstests/test_phase12_review_fixes.py
✅ Files skipped from review due to trivial changes (3)
- docs/reference/configuration.md
- docs/reference/compliance_summary.md
- forgelm/trainer.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- forgelm/wizard.py
- notebooks/quickstart_sft.ipynb
Comment on lines
+758
to
+762
| try: | ||
| batch_enc = tokenizer(section_blocks, add_special_tokens=False) | ||
| return [len(ids) for ids in batch_enc["input_ids"]] | ||
| except Exception: | ||
| return [len(tokenizer.encode(block, add_special_tokens=False)) for block in section_blocks] |
Contributor
There was a problem hiding this comment.
Don't hide arbitrary tokenizer failures behind the batch fallback.
The per-block fallback is fine when a tokenizer genuinely does not support batched input, but except Exception here will also swallow broken return shapes and real tokenizer bugs, then silently change behavior. Catch only the narrow capability failure you expect, or validate the batch result before falling back.
As per coding guidelines, **/*.py: No silent failures: do not use bare except:, except Exception: pass, or swallow exceptions except for explicitly non-fatal paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forgelm/ingestion.py` around lines 758 - 762, The code currently swallows all
exceptions when calling tokenizer(section_blocks...) which hides real bugs;
change it to only fallback on the specific batching-capability error and
validate the batch result before using it: call tokenizer(section_blocks,
add_special_tokens=False) inside a try that only catches the tokenizer batching
failure (e.g., TypeError or a tokenizer-specific exception), then verify
batch_enc is a mapping containing "input_ids" whose value is an iterable of
sequences with length equal to len(section_blocks) before returning [len(ids)
for ids in batch_enc["input_ids"]]; if validation fails, re-raise the error
instead of falling back, and only perform the per-block encode fallback using
tokenizer.encode(block, add_special_tokens=False) when the caught exception is
the expected unsupported-batch error.
Comment on lines
+84
to
+85
| <!-- Replace xnjlejbb with your own Formspree form ID after creating one at formspree.io. --> | ||
| <form id="contact-form" action="https://formspree.io/f/xnjlejbb" method="POST" novalidate> |
Contributor
There was a problem hiding this comment.
Don't ship the demo Formspree endpoint.
https://formspree.io/f/xnjlejbb is still the placeholder ID referenced by the inline comment. If this lands unchanged, contact submissions will go to the wrong mailbox or fail entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site/contact.html` around lines 84 - 85, The contact form is using the demo
Formspree endpoint (action="https://formspree.io/f/xnjlejbb") and the inline
comment still instructs replacing xnjlejbb; update the <form id="contact-form">
action to the actual Formspree form ID for this project (or switch to your own
backend endpoint) and remove or update the placeholder comment so submissions go
to the correct mailbox; verify the new form ID is valid in Formspree and that
the form still uses method="POST" and novalidate as intended.
Comment on lines
+16
to
+19
| <meta property="og:image" content="https://YOUR_DOMAIN/assets/og.png"> | ||
| <meta property="og:url" content="https://YOUR_DOMAIN/"> | ||
| <meta property="og:locale" content="en_US"> | ||
| <link rel="canonical" href="https://YOUR_DOMAIN/"> |
Contributor
There was a problem hiding this comment.
Replace the placeholder site origin before publishing.
These tags still point at YOUR_DOMAIN, so canonicalization and social previews will resolve to a non-existent host if this page ships as-is. Use the real deployment origin everywhere, or omit the absolute metadata until it exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site/index.html` around lines 16 - 19, Replace the placeholder origin
"YOUR_DOMAIN" used in the meta tags (og:image, og:url, og:locale) and the
canonical link (link rel="canonical") with the real deployment origin or remove
the absolute URLs until the real origin is available; update the content
attributes for the og:image and og:url meta tags and the href on the canonical
link so they point to the live site (e.g., https://your-real-domain/) or convert
them to relative paths if appropriate.
Comment on lines
+51
to
+115
| function setLanguage(lang) { | ||
| if (SUPPORTED.indexOf(lang) === -1) lang = DEFAULT; | ||
|
|
||
| try { localStorage.setItem(LANG_PREF_NAME, lang); } catch (_) {} | ||
|
|
||
| var all = getTranslations(); | ||
| var table = all[lang] || all[DEFAULT] || {}; | ||
| var fallback = all[DEFAULT] || {}; | ||
|
|
||
| document.documentElement.lang = lang; | ||
|
|
||
| // Plain-text replacements. | ||
| document.querySelectorAll('[data-i18n]').forEach(function (el) { | ||
| var k = el.getAttribute('data-i18n'); | ||
| var v = lookup(table, k); | ||
| if (v === undefined) v = lookup(fallback, k); | ||
| if (v === undefined) return; | ||
| el.textContent = v; | ||
| }); | ||
|
|
||
| // HTML replacements (for content with markup like <code>, <em>). | ||
| document.querySelectorAll('[data-i18n-html]').forEach(function (el) { | ||
| var k = el.getAttribute('data-i18n-html'); | ||
| var v = lookup(table, k); | ||
| if (v === undefined) v = lookup(fallback, k); | ||
| if (v === undefined) return; | ||
| el.innerHTML = v; | ||
| }); | ||
|
|
||
| // Attribute replacements: data-i18n-attr="content:meta.desc.home,placeholder:form.name". | ||
| document.querySelectorAll('[data-i18n-attr]').forEach(function (el) { | ||
| var spec = el.getAttribute('data-i18n-attr'); | ||
| if (!spec) return; | ||
| spec.split(',').forEach(function (pair) { | ||
| var idx = pair.indexOf(':'); | ||
| if (idx === -1) return; | ||
| var attr = pair.slice(0, idx).trim(); | ||
| var k = pair.slice(idx + 1).trim(); | ||
| var v = lookup(table, k); | ||
| if (v === undefined) v = lookup(fallback, k); | ||
| if (v === undefined) return; | ||
| el.setAttribute(attr, v); | ||
| }); | ||
| }); | ||
|
|
||
| // Mark the active language menu button. | ||
| document.querySelectorAll('.lang-toggle-menu button[data-set-lang]').forEach(function (btn) { | ||
| var on = btn.dataset.setLang === lang; | ||
| btn.classList.toggle('active', on); | ||
| btn.setAttribute('aria-pressed', on ? 'true' : 'false'); | ||
| }); | ||
|
|
||
| // Update the dropdown trigger label to the current language code. | ||
| var labelEl = document.querySelector('.lang-toggle-current'); | ||
| if (labelEl) labelEl.textContent = lang.toUpperCase(); | ||
| } | ||
|
|
||
| document.addEventListener('DOMContentLoaded', function () { | ||
| setLanguage(detectLanguage()); | ||
|
|
||
| document.querySelectorAll('.lang-toggle-menu button[data-set-lang]').forEach(function (btn) { | ||
| btn.addEventListener('click', function () { | ||
| setLanguage(btn.dataset.setLang); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
The new pages don't use the markup this switcher actually handles.
site/index.html and site/contact.html are built around paired .i18n-inline[data-lang] spans, data-title-en/tr / data-desc-en/tr, and direct [data-set-lang] buttons. This module only updates [data-i18n*] nodes and only binds buttons under .lang-toggle-menu, so both languages stay visible, metadata never switches, and the EN/TR controls on the new pages have no effect.
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 57-57: site/js/i18n.js#L57
Generic Object Injection Sink
[warning] 58-58: site/js/i18n.js#L58
Generic Object Injection Sink
[warning] 77-77: site/js/i18n.js#L77
Unencoded input 'v' used in HTML context
[warning] 77-77: site/js/i18n.js#L77
Unsafe assignment to innerHTML
[failure] 77-77: site/js/i18n.js#L77
User controlled data in a el.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
[failure] 77-77: site/js/i18n.js#L77
User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site/js/i18n.js` around lines 51 - 115, setLanguage only updates [data-i18n*]
elements and buttons under .lang-toggle-menu, so the new pages using paired
.i18n-inline[data-lang] spans, attributes like data-title-en/data-desc-en and
global [data-set-lang] buttons don't get toggled; extend setLanguage to (1)
hide/show .i18n-inline elements by matching their data-lang to the chosen lang,
(2) swap metadata attributes by reading elements with
data-title-<lang>/data-desc-<lang> (or generic data-title-* data-desc-*) and
setting title/description accordingly, and (3) bind click handlers for buttons
matching [data-set-lang] (not just .lang-toggle-menu button[data-set-lang]) so
detectLanguage, setLanguage and the existing menu-toggle logic work for the new
controls. Ensure you reuse the existing lookup/fallback logic for translations
where applicable.
Comment on lines
+173
to
+204
| form.addEventListener('submit', function (e) { | ||
| e.preventDefault(); | ||
|
|
||
| var status = document.getElementById('form-status'); | ||
| var btn = form.querySelector('button[type="submit"]'); | ||
| var subject = form.querySelector('[name="subject"]'); | ||
| var subjectField = form.querySelector('[name="_subject"]'); | ||
|
|
||
| if (subject && subjectField) { | ||
| subjectField.value = 'ForgeLM contact — ' + subject.value; | ||
| } | ||
|
|
||
| btn.disabled = true; | ||
|
|
||
| fetch(form.action, { | ||
| method: 'POST', | ||
| body: new FormData(form), | ||
| headers: { Accept: 'application/json' } | ||
| }).then(function (res) { | ||
| if (res.ok) { | ||
| status.className = 'form-status success'; | ||
| status.textContent = tr('contact.form.success'); | ||
| form.reset(); | ||
| } else { | ||
| throw new Error('formspree_failed'); | ||
| } | ||
| }).catch(function () { | ||
| status.className = 'form-status error'; | ||
| status.textContent = tr('contact.form.error'); | ||
| }).finally(function () { | ||
| btn.disabled = false; | ||
| }); |
Contributor
There was a problem hiding this comment.
Client-side submit bypasses the required-field checks.
This handler posts FormData immediately, and the current form markup disables native validation with novalidate. Empty or invalid submissions will still be sent unless you gate the fetch() call with checkValidity()/reportValidity().
Minimal fix
form.addEventListener('submit', function (e) {
e.preventDefault();
+
+ if (!form.checkValidity()) {
+ form.reportValidity();
+ return;
+ }
var status = document.getElementById('form-status');
var btn = form.querySelector('button[type="submit"]');
var subject = form.querySelector('[name="subject"]');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site/js/main.js` around lines 173 - 204, The submit handler attached via
form.addEventListener('submit', function (e) { ... }) currently sends FormData
unconditionally; before disabling the submit button and calling fetch(), call
form.checkValidity() and if it returns false call form.reportValidity() and
return to abort submission so native required/constraint checks run; move the
btn.disabled = true and the fetch() invocation after the validity gate so
invalid/empty forms are not posted.
… by another agent)
Verified each finding against the current code; applied fixes for the
seven non-site bugs and skipped the three site/* ones because the user
flagged that another agent is rebuilding that directory.
forgelm/compliance.py
- _hmac_key is now Optional[bytes]. When FORGELM_AUDIT_SECRET is empty
we set it to None and stop emitting the _hmac field altogether — a
key derived solely from the public run_id is forgeable, and we should
not claim tamper-evidence we cannot deliver.
- Extracted _read_chain_head(fh) as a pure helper that operates on an
already-open binary handle, then re-read the chain head from disk
*under the flock* in log_event() so two writers sharing the same log
cannot both append against a stale prev_hash and silently fork the
chain.
- _read_chain_head now also handles the oversized-final-entry case: if
skipping the partial first line empties the tail, re-read from the
4 KiB seek_start without skipping so a single >4 KiB entry still
recovers the actual last line instead of falling back to "genesis".
- generate_deployer_instructions now passes system_name,
config.model.name_or_path, _describe_adapter_method output,
final_path, foreseeable_misuse bullets, and metric names through
_sanitize_md before template substitution. CommonMark specials in
any of those would otherwise break out of table cells / headings /
bullets in the generated Article 13 document.
forgelm/data_audit.py
- _StreamingAggregator gains quality_samples_evaluated; _record_text_metrics
bumps it for each row that actually goes through _row_quality_flags
(null/empty/non-dict rows are skipped by the existing guard).
- _build_quality_summary now divides by samples_evaluated_total instead
of total_samples so overall_quality_score reflects the rows the
filter could actually evaluate; null/non-dict rows no longer depress
the score. _fold_outcome_into_summary now returns
(flagged, evaluated) and the audit_dataset orchestrator threads both
totals through.
forgelm/ingestion.py
- _count_section_tokens now narrows the batching-fallback catch to
TypeError / ValueError (the documented unsupported-batch signals)
instead of bare Exception, validates that the returned BatchEncoding
has an input_ids list of len(section_blocks), and re-raises an
explicit error for any other shape. Real bugs are no longer masked
behind the slow per-block fallback.
docs/reference/usage.md / usage-tr.md
- secrets_summary prose updated to reflect the actual JSON keys
emitted by detect_secrets (aws_access_key, github_pat, slack_token,
openai_key, google_api_key, jwt, private_key, azure_storage_key —
with a note that private_key matches full BEGIN…END blocks, not
just headers).
- usage-tr.md gains Phase 12 examples that already lived in
usage.md: --strategy markdown, --secrets-mask, --secrets-mask +
--pii-mask combo, --dedup-method minhash, --quality-filter, plus
prose mentioning the always-on secrets/credentials scan and the
opt-in heuristic quality filter.
tests/
- test_phase12_review_fixes.py: replaced the absolute-millisecond
ReDoS regression assertions (1000ms / 100ms thresholds) with a
growth-ratio check (timings_n / timings_smallest <= 3 ×
size_ratio). Linear scaling is what those tests actually
guarantee; the previous shape was flaky on shared CI under load.
- test_eu_ai_act.py + test_integration_smoke.py: substring
assertions for metric names / config-derived strings now compare
against content.replace("\\", "") because _sanitize_md backslash-
escapes CommonMark specials.
Skipped (per user instruction — another agent is rebuilding the site):
- site/contact.html Formspree action-id placeholder comment
- site/index.html YOUR_DOMAIN canonical / og:* placeholders
- site/js/i18n.js setLanguage extension (also stale finding —
legacy i18n-inline / data-title-en markup no longer present)
- site/js/main.js form-validity gate
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/usage.md (1)
132-195:⚠️ Potential issue | 🟠 MajorTurkish mirror is out of sync — missing entire sections beyond v0.5.2 features.
The v0.5.2 additions (markdown strategy, secrets-mask, MinHash dedup, quality filter) are present in
docs/reference/usage-tr.md. However, the Turkish file (272 lines) is significantly shorter than the English version (340 lines) and lacks critical sections: VRAM Fit Check, Post-Training Chat/Export/Deploy, and some training output structure details.Invoke the sync-bilingual-docs skill to fully synchronize EN ↔ TR mirrors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/usage.md` around lines 132 - 195, Turkish mirror docs/reference/usage-tr.md is missing sections added in the English docs (v0.5.2): add the VRAM Fit Check, Post-Training Chat/Export/Deploy, and training output structure details and ensure feature parity for the v0.5.2 additions shown in docs/reference/usage.md (e.g., "markdown-aware splitter" / --strategy markdown, --secrets-mask, MinHash LSH dedup / --dedup-method minhash, and --quality-filter); run the sync-bilingual-docs process to synchronize EN ↔ TR and verify that headings like "Document Ingestion (v0.5.0+; token-aware in v0.5.1; markdown + secrets-mask in v0.5.2)" and "Dataset Audit (v0.5.0+; ... v0.5.2)" along with example CLI flags (forgelm ingest, forgelm audit, --secrets-mask, --pii-mask, --chunk-tokens, --tokenizer, --dedup-method, --jaccard-threshold, --quality-filter) are present and translated consistently in docs/reference/usage-tr.md.
♻️ Duplicate comments (2)
docs/reference/usage-tr.md (1)
136-136:⚠️ Potential issue | 🟡 MinorSecrets-family list appears incomplete vs stated Phase-12 scope.
Line 136 currently lists 8 credential families in prose, while the Phase-12 objective states detection for 9 families. Please align this list (or reference canonical
SECRET_TYPES) to avoid user-facing mismatch.Suggested doc-safe fix
-Denetim şunları yakalar: ... her zaman açık **credentials/secrets taraması** (`secrets_summary` — AWS / GitHub / Slack / OpenAI / Google / JWT / tam private-key blokları / Azure storage), ... +Denetim şunları yakalar: ... her zaman açık **credentials/secrets taraması** (`secrets_summary`; desteklenen tipler kod tabanındaki `SECRET_TYPES` ile birebir), ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/usage-tr.md` at line 136, The docs list of credential families is missing one of the nine Phase-12 secret types; update the prose at the sentence describing secrets (the "credentials/secrets taraması" / "secrets_summary" line) to either (a) enumerate all nine families to match the Phase-12 scope or (b) replace the inline list with a canonical reference to the SECRET_TYPES constant so it always matches code; locate the string in docs/reference/usage-tr.md that mentions "AWS / GitHub / Slack / OpenAI / Google / JWT / tam private-key blokları / Azure storage" and add the missing family or switch to referencing SECRET_TYPES, keeping wording and translation consistent with surrounding Turkish text.forgelm/compliance.py (1)
86-117:⚠️ Potential issue | 🟠 Major
_read_chain_head()can still hash a truncated last entry.If the final JSONL record is larger than the 4 KiB tail window, the fallback here re-reads from
seek_startand hashes the tail fragment, not the full last line. This helper also accepts an unterminated trailing fragment as a valid head. After restart,prev_hashcan diverge from the real persisted entry and fork the audit chain.🛠️ Correctness-first fallback
- if seek_start > 0: - fh.seek(seek_start) - tail = fh.read() - try: - lines = [ln for ln in tail.decode("utf-8").splitlines() if ln.strip()] - except UnicodeDecodeError as e: - raise ValueError( - f"Audit log {self.log_path!r} contains non-UTF-8 data — likely corrupt: {e}. " - "Refusing to silently re-root the hash chain." - ) from e - if lines: - return hashlib.sha256(lines[-1].encode("utf-8")).hexdigest() + if seek_start > 0: + fh.seek(0) + last_line: Optional[bytes] = None + for raw in fh: + if raw.strip(): + last_line = raw.rstrip(b"\r\n") + if last_line is not None: + return hashlib.sha256(last_line).hexdigest()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/compliance.py` around lines 86 - 117, _read_chain_head currently can hash a truncated final JSONL record when the last entry is larger than the 4 KiB tail window; fix it by ensuring you only hash a complete last line: after the fallback fh.seek(seek_start) + tail = fh.read(), verify the tail contains a terminating newline (i.e. the final record is complete) before accepting lines[-1]; if the tail does not end with a newline, locate the true start of the final line by seeking backward from the file end to the previous newline (or read forward from that newline to EOF) and parse that full last line, or if you cannot recover a full last-line, raise an error rather than hashing a partial record; update the logic in _read_chain_head (use fh.seek and fh.read or fh.readline to obtain the complete final record) and keep the existing UnicodeDecodeError handling and the "genesis" fallback intact.
🧹 Nitpick comments (2)
docs/reference/usage-tr.md (1)
123-124: Add explicit install command where[ingestion-scale]is first required.Line 123 introduces a hard prerequisite, but there’s no adjacent command to install it. Adding one inline will prevent avoidable runtime confusion.
Suggested doc tweak
# v0.5.2: >50K satırlık korpuslar için MinHash LSH dedup (gerekli extra: `[ingestion-scale]`) +# Kurulum: pip install -e ".[ingestion-scale]" forgelm audit data/large_corpus.jsonl --dedup-method minhash --jaccard-threshold 0.85🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/usage-tr.md` around lines 123 - 124, The doc introduces the extra requirement "[ingestion-scale]" but doesn't show how to install it; update the line around the v0.5.2 MinHash example to add an explicit install command immediately where "[ingestion-scale]" is first mentioned (e.g., add a one-line instruction like "pip install forgelm[ingestion-scale]" or the project's package manager equivalent) so users can install the extra before running the example using the forgelm audit command with --dedup-method minhash and --jaccard-threshold 0.85.tests/test_phase12_review_fixes.py (1)
206-212: Prefer behavior checks over inspecting the private regex text.The true/false JWT fixtures already pin the contract. Asserting literal substrings inside
_SECRET_PATTERNS["jwt"].patternover-constrains the implementation and will fail harmless regex hardening/refactors even when detection behavior stays correct.Based on learnings, "Follow docs/standards/regex.md for any re.compile or regex changes to avoid ReDoS exposure and fixture fragmentation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_phase12_review_fixes.py` around lines 206 - 212, The test test_jwt_pattern_anchored_on_known_headers currently inspects the literal regex text (_SECRET_PATTERNS["jwt"].pattern) which over-constrains implementation; change it to a behavior-based test: remove the anchor substring assertions and instead use the existing JWT true/false fixtures to assert that the compiled pattern (or the detector that uses _SECRET_PATTERNS["jwt"]) matches all positive JWT fixtures and rejects all negative JWT fixtures, ensuring detection behavior remains correct while allowing regex refactors/hardening.
🤖 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/compliance.py`:
- Around line 56-69: The code currently reads FORGELM_AUDIT_SECRET from the
environment to decide whether to set self._hmac_key (raw_secret -> _hmac_key),
which makes emission depend on ambient env; change this so the secret is
supplied by the validated configuration object passed into the class (e.g. a
config parameter to the constructor) instead of os.getenv: remove use of
os.getenv("FORGELM_AUDIT_SECRET")/raw_secret, add a config.secret (or
config.audit_secret) input, and set self._hmac_key =
hashlib.sha256(config.audit_secret.encode() + self.run_id.encode()).digest()
when that config field is non-empty, otherwise None; keep the same semantics for
run_id and digest but ensure all callers create/validate the secret in the YAML
config rather than relying on environment variables.
In `@forgelm/ingestion.py`:
- Around line 1224-1229: When overlap is None and strategy == "sliding", clamp
the implicit DEFAULT_SLIDING_OVERLAP to the actual chunk size used so it won't
violate _chunk_sliding() bounds: compute the effective chunk size (the value the
dispatcher will use from chunk_size/defaults), then set overlap =
min(DEFAULT_SLIDING_OVERLAP, max(0, effective_chunk_size // 2)); otherwise keep
overlap = 0 for non-sliding. Update the branch that currently assigns overlap to
reference the effective chunk size and perform the min() clamp so sliding
overlap never exceeds half the chunk.
---
Outside diff comments:
In `@docs/reference/usage.md`:
- Around line 132-195: Turkish mirror docs/reference/usage-tr.md is missing
sections added in the English docs (v0.5.2): add the VRAM Fit Check,
Post-Training Chat/Export/Deploy, and training output structure details and
ensure feature parity for the v0.5.2 additions shown in docs/reference/usage.md
(e.g., "markdown-aware splitter" / --strategy markdown, --secrets-mask, MinHash
LSH dedup / --dedup-method minhash, and --quality-filter); run the
sync-bilingual-docs process to synchronize EN ↔ TR and verify that headings like
"Document Ingestion (v0.5.0+; token-aware in v0.5.1; markdown + secrets-mask in
v0.5.2)" and "Dataset Audit (v0.5.0+; ... v0.5.2)" along with example CLI flags
(forgelm ingest, forgelm audit, --secrets-mask, --pii-mask, --chunk-tokens,
--tokenizer, --dedup-method, --jaccard-threshold, --quality-filter) are present
and translated consistently in docs/reference/usage-tr.md.
---
Duplicate comments:
In `@docs/reference/usage-tr.md`:
- Line 136: The docs list of credential families is missing one of the nine
Phase-12 secret types; update the prose at the sentence describing secrets (the
"credentials/secrets taraması" / "secrets_summary" line) to either (a) enumerate
all nine families to match the Phase-12 scope or (b) replace the inline list
with a canonical reference to the SECRET_TYPES constant so it always matches
code; locate the string in docs/reference/usage-tr.md that mentions "AWS /
GitHub / Slack / OpenAI / Google / JWT / tam private-key blokları / Azure
storage" and add the missing family or switch to referencing SECRET_TYPES,
keeping wording and translation consistent with surrounding Turkish text.
In `@forgelm/compliance.py`:
- Around line 86-117: _read_chain_head currently can hash a truncated final
JSONL record when the last entry is larger than the 4 KiB tail window; fix it by
ensuring you only hash a complete last line: after the fallback
fh.seek(seek_start) + tail = fh.read(), verify the tail contains a terminating
newline (i.e. the final record is complete) before accepting lines[-1]; if the
tail does not end with a newline, locate the true start of the final line by
seeking backward from the file end to the previous newline (or read forward from
that newline to EOF) and parse that full last line, or if you cannot recover a
full last-line, raise an error rather than hashing a partial record; update the
logic in _read_chain_head (use fh.seek and fh.read or fh.readline to obtain the
complete final record) and keep the existing UnicodeDecodeError handling and the
"genesis" fallback intact.
---
Nitpick comments:
In `@docs/reference/usage-tr.md`:
- Around line 123-124: The doc introduces the extra requirement
"[ingestion-scale]" but doesn't show how to install it; update the line around
the v0.5.2 MinHash example to add an explicit install command immediately where
"[ingestion-scale]" is first mentioned (e.g., add a one-line instruction like
"pip install forgelm[ingestion-scale]" or the project's package manager
equivalent) so users can install the extra before running the example using the
forgelm audit command with --dedup-method minhash and --jaccard-threshold 0.85.
In `@tests/test_phase12_review_fixes.py`:
- Around line 206-212: The test test_jwt_pattern_anchored_on_known_headers
currently inspects the literal regex text (_SECRET_PATTERNS["jwt"].pattern)
which over-constrains implementation; change it to a behavior-based test: remove
the anchor substring assertions and instead use the existing JWT true/false
fixtures to assert that the compiled pattern (or the detector that uses
_SECRET_PATTERNS["jwt"]) matches all positive JWT fixtures and rejects all
negative JWT fixtures, ensuring detection behavior remains correct while
allowing regex refactors/hardening.
🪄 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: dddad6b0-d6e2-4aff-957e-fe900e08f969
📒 Files selected for processing (8)
docs/reference/usage-tr.mddocs/reference/usage.mdforgelm/compliance.pyforgelm/data_audit.pyforgelm/ingestion.pytests/test_eu_ai_act.pytests/test_integration_smoke.pytests/test_phase12_review_fixes.py
✅ Files skipped from review due to trivial changes (1)
- forgelm/data_audit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_eu_ai_act.py
Comment on lines
56
to
+69
| self.operator = os.getenv("FORGELM_OPERATOR", os.getenv("USER", "unknown")) | ||
| # Per-run HMAC key: SHA-256(operator_secret || run_id). The secret is | ||
| # required for tamper-evident HMACs because ``run_id`` is part of the | ||
| # public log header — without a non-empty secret an attacker who can | ||
| # rewrite the file knows the key and could re-sign forged entries. | ||
| # When the secret is missing we therefore disable HMAC emission | ||
| # entirely (the SHA-256 hash chain is still written; only the | ||
| # per-line authenticator drops out) so we never claim | ||
| # tamper-evidence we cannot deliver. | ||
| raw_secret = os.getenv("FORGELM_AUDIT_SECRET", "") | ||
| if raw_secret: | ||
| self._hmac_key: Optional[bytes] = hashlib.sha256(raw_secret.encode() + self.run_id.encode()).digest() | ||
| else: | ||
| self._hmac_key = None |
Contributor
There was a problem hiding this comment.
Don't let _hmac support depend on ambient environment state.
Whether _hmac is emitted now depends only on FORGELM_AUDIT_SECRET, so the same validated config can produce different audit artifacts and trust guarantees depending on the shell the run starts in. Please drive this from validated config and only resolve the secret from a config-declared source.
Based on learnings, "Behaviour must be determined by validated YAML configuration, not environment variables or hardcoded feature flags".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forgelm/compliance.py` around lines 56 - 69, The code currently reads
FORGELM_AUDIT_SECRET from the environment to decide whether to set
self._hmac_key (raw_secret -> _hmac_key), which makes emission depend on ambient
env; change this so the secret is supplied by the validated configuration object
passed into the class (e.g. a config parameter to the constructor) instead of
os.getenv: remove use of os.getenv("FORGELM_AUDIT_SECRET")/raw_secret, add a
config.secret (or config.audit_secret) input, and set self._hmac_key =
hashlib.sha256(config.audit_secret.encode() + self.run_id.encode()).digest()
when that config field is non-empty, otherwise None; keep the same semantics for
run_id and digest but ensure all callers create/validate the secret in the YAML
config rather than relying on environment variables.
…config)
Verified each finding against the current code; applied fixes for the
six real ones and rejected the proposal to move FORGELM_AUDIT_SECRET
into validated YAML config — that contradicts docs/standards/architecture.md
("only for secrets like HF_TOKEN, OPENAI_API_KEY") and CLAUDE.md
("env-var sniffing is allowed for secrets"). Putting a secret into a
version-controlled YAML file is a worse pattern than the env-var
convention the project deliberately settled on for HF_TOKEN /
OPENAI_API_KEY / etc.
forgelm/ingestion.py
- ingest_path() now clamps the implicit DEFAULT_SLIDING_OVERLAP to
effective_chunk_size // 2 when --overlap is not passed and the
strategy is "sliding". A small --chunk-size (e.g. 300) used to trip
_chunk_sliding's "overlap > chunk_size // 2" guard with the default
200 — surfacing as a confusing error for a knob the user did not set.
forgelm/compliance.py
- _read_chain_head now refuses to derive a chain head from a tail that
does not end with "\n". log_event always writes ``entry_json + "\n"``,
so an unterminated final line means a crash mid-write or external
truncation; hashing the partial body would silently anchor the chain
to a corrupted record. Raising forces the operator to inspect /
repair before resuming.
- Folded the duplicated UnicodeDecodeError handling into a small
``_decode_lines`` helper used by both the primary and oversize-entry
paths.
docs/reference/usage.md / usage-tr.md
- secrets_summary prose now lists all nine families per
``forgelm.data_audit.SECRET_TYPES`` — replacing my earlier (wrong)
mix of `github_pat` / `openai_key` / `private_key` keys with the
actual constants ``aws_access_key``, ``github_token``,
``slack_token``, ``openai_api_key``, ``google_api_key``, ``jwt``,
``openssh_private_key``, ``pgp_private_key``, ``azure_storage_key``
(BEGIN…END for the two private-key families).
- usage-tr.md gains the two H3 sections that were missing from the
EN parity check: ``### VRAM Fit Check`` and
``### Eğitim Sonrası: Chat, Export, Deploy``.
- ``[ingestion-scale]`` MinHash example in TR now ships an explicit
``pip install forgelm[ingestion-scale]`` reminder one line above
the ``forgelm audit`` invocation.
tests/test_phase12_review_fixes.py
- ``test_jwt_pattern_anchored_on_known_headers`` was inspecting the
literal regex source (``_SECRET_PATTERNS["jwt"].pattern``) and
failing on regex refactors that changed encoding without changing
matching semantics. Replaced with a behaviour-based fixture loop
that asserts ``detect_secrets`` flags a JWT carrying each canonical
header anchor (``alg`` / ``typ`` / ``kid`` / ``cty`` / ``enc`` /
``api``). Future broadening that drops one anchor still fails;
cosmetic regex changes that preserve coverage no longer do.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Apr 28, 2026
…d-on-main state PR #13 (Phase 12 — v0.5.2) is merged on `main`. This sweep aligns the narrative-doc state with that fact and consolidates the post-merge review-cycle fixes under a single follow-up section. roadmap.md / roadmap-tr.md - Phase 11.5 + 12 status descriptions: "merged to development" → "merged to main (PR #11/#12 / #13)" with the actual merge date. - "Latest release on PyPI" line now correctly says v0.4.5 (PyPI state) instead of v0.5.0. Adds explicit clarification that v0.5.0 / v0.5.1 / v0.5.2 are merged to main but not yet tagged + published. - Phase-count matches v0.5.3 plan: "3 phases (12.5, 13, 14) planned". - TR mirror status icons updated to "🟡 Birleşti" matching the EN "🟡 Merged" — were previously prematurely marked "✅ Tamam". - Mermaid diagram in roadmap-tr.md gains the missing P125 node and bumps merged-but-not-published phases to a lighter green. - New cross-link to issue #14 (webhook SSRF DNS-rebinding TOCTOU) surfaces in both the v0.5.2 row and the v0.5.3 "Next" line as the tracked v0.5.3 hardening follow-up. roadmap/phase-14-pipeline-chains.md / roadmap/releases.md - "starts after Phase 12 lands" → concrete release event (`v0.5.2` PyPI tag is published). - v0.5.1 + v0.5.2 release-page status both moved off "Merged on development" to "Merged on main … remaining release-engineering steps" with the issue #14 hardening reference on v0.5.2. README.md - Always-on secrets-scan list updated: was 7 families with the wrong "private-key headers" wording; now lists all nine families per `forgelm.data_audit.SECRET_TYPES` (full BEGIN…END for OpenSSH / PGP). CHANGELOG.md - New "Fixed — post-PR-#13 review-cycle batches (rounds 8-12)" block in [Unreleased] consolidating the post-merge inline-comment batches: audit log hardening, deployer-instructions Markdown injection, quality-filter denominator, numpy fast-path bits guard, sliding-overlap clamp, batch-tokenizer narrow-except, webhook redirect/secret-fallback safety, trainer governance failure visibility, compliance-manifest exception narrowing, strict messages-format validation, wizard ASCII regex flag, GGUF converter case-insensitive validation, markdown-chunker complexity refactor, bidirectional MinHash extraction, streaming length digest, and the doc/test-count drift sweep. Pointers to issue #14 for the remaining DNS-rebinding follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 28, 2026
…d-on-main state PR #13 (Phase 12 — v0.5.2) is merged on `main`. This sweep aligns the narrative-doc state with that fact and consolidates the post-merge review-cycle fixes under a single follow-up section. roadmap.md / roadmap-tr.md - Phase 11.5 + 12 status descriptions: "merged to development" → "merged to main (PR #11/#12 / #13)" with the actual merge date. - "Latest release on PyPI" line now correctly says v0.4.5 (PyPI state) instead of v0.5.0. Adds explicit clarification that v0.5.0 / v0.5.1 / v0.5.2 are merged to main but not yet tagged + published. - Phase-count matches v0.5.3 plan: "3 phases (12.5, 13, 14) planned". - TR mirror status icons updated to "🟡 Birleşti" matching the EN "🟡 Merged" — were previously prematurely marked "✅ Tamam". - Mermaid diagram in roadmap-tr.md gains the missing P125 node and bumps merged-but-not-published phases to a lighter green. - New cross-link to issue #14 (webhook SSRF DNS-rebinding TOCTOU) surfaces in both the v0.5.2 row and the v0.5.3 "Next" line as the tracked v0.5.3 hardening follow-up. roadmap/phase-14-pipeline-chains.md / roadmap/releases.md - "starts after Phase 12 lands" → concrete release event (`v0.5.2` PyPI tag is published). - v0.5.1 + v0.5.2 release-page status both moved off "Merged on development" to "Merged on main … remaining release-engineering steps" with the issue #14 hardening reference on v0.5.2. README.md - Always-on secrets-scan list updated: was 7 families with the wrong "private-key headers" wording; now lists all nine families per `forgelm.data_audit.SECRET_TYPES` (full BEGIN…END for OpenSSH / PGP). CHANGELOG.md - New "Fixed — post-PR-#13 review-cycle batches (rounds 8-12)" block in [Unreleased] consolidating the post-merge inline-comment batches: audit log hardening, deployer-instructions Markdown injection, quality-filter denominator, numpy fast-path bits guard, sliding-overlap clamp, batch-tokenizer narrow-except, webhook redirect/secret-fallback safety, trainer governance failure visibility, compliance-manifest exception narrowing, strict messages-format validation, wizard ASCII regex flag, GGUF converter case-insensitive validation, markdown-chunker complexity refactor, bidirectional MinHash extraction, streaming length digest, and the doc/test-count drift sweep. Pointers to issue #14 for the remaining DNS-rebinding follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
16 tasks
cemililik
added a commit
that referenced
this pull request
Apr 29, 2026
…prehensive v0.5.0 Originally planned as four sequential PyPI tags (v0.5.0 / v0.5.1 / v0.5.2 / v0.5.3). The four phases form one coherent surface (ingest → polish → mature → polish) that's hard to use in parts; consolidating into a single comprehensive v0.5.0 release keeps the user-facing release notes focused while git history retains the four phases as separate commit batches. pyproject.toml - version: 0.5.2rc1 → 0.5.0 CHANGELOG.md - Collapse [Unreleased] + [0.5.2] + [0.5.1] + [0.5.0] (summary stub) into a single [0.5.0] — 2026-04-29 entry. - Theme paragraph + consolidation note explaining the merger. - Internal phase boundaries preserved as section markers (### Phase 12.5 / Phase 12 / Phase 11.5 / Phase 11) so reviewers can map back to PR history (#11, #12, #13, current). - [Unreleased] is now empty (next minor picks up Phase 14). README.md - "Document Ingestion & Data Audit (v0.5.0 — polished in v0.5.1, matured in v0.5.2)" → "(v0.5.0 — Phases 11 + 11.5 + 12 + 12.5 consolidated)". - Drop "v0.5.2 added X" / "v0.5.1 added Y" / "Phase 12.5 polish (v0.5.3)" history phrasing — features now read as part of v0.5.0. docs/roadmap.md + roadmap-tr.md - Status table: four separate "🟡 Merged" rows for Phases 11 / 11.5 / 12 / 12.5 collapsed into one combined row pointing at releases.md#v050. Phase 14 row reslotted from v0.5.3 to v0.5.1. - "Latest PyPI: v0.4.5" wording trimmed (the v0.5.0/.1/.2 sentence about three pending tags is gone). - "Next: v0.5.3" → "Next: v0.5.1" (Phase 14). - Phase count: 16 complete + 3 planned → 17 complete + 2 planned (12.5 moves into the consolidated release; 13 + 14 remain planned). - Mermaid diagram: P11 / P115 / P12 / P125 all point at the same V2[v0.5.0 pending] node; styling bumped to merged-but-not-tagged light-green. docs/roadmap/releases.md - v0.5.0 / v0.5.1 / v0.5.2 sections collapsed into a single comprehensive v0.5.0 entry organised by phase (### Phase 11 / 11.5 / 12 / 12.5). - v0.5.3 (Phase 14) → v0.5.1 (Phase 14). docs/roadmap/phase-{11-5,12-5,12-data-curation,14-pipeline}.md - Consolidation note at the top of each historical phase plan ("originally targeted vX.Y.Z, ships as part of consolidated v0.5.0; see releases.md#v050"). Body kept intact as the historical planning trail. - phase-14: Target release v0.5.3 → v0.5.1; "starts after v0.5.2 PyPI tag" → "starts after v0.5.0 PyPI tag". - phase-12-5-backlog rows: "ships with v0.5.3" → "ships with v0.5.0". docs/guides/{ingestion,data_audit}{,-tr}.md + docs/reference/usage{,-tr}.md - Drop the "v0.5.2 added" / "v0.5.1 promoted" history phrasing inside feature descriptions. Features now read as v0.5.0 capabilities; phase labels (Phase 11 / 11.5 / 12 / 12.5) are kept as breadcrumbs for reviewers. - Section headings like "Document Ingestion (v0.5.0+; token-aware in v0.5.1; markdown + secrets-mask in v0.5.2)" → "(v0.5.0)". - Inline comments in CLI examples drop "v0.5.2:" / "v0.5.3 (Phase 12.5):" prefixes since the features all ship in the same release. notebooks/data_curation.ipynb - Install pin: 0.5.2 → 0.5.0 (cells 1 + 16). - "Phase 12.5 audit add-ons (v0.5.3)" markdown → "Other v0.5.0 audit add-ons". - Section 2 header drops the "Phase 12.5 shorthand (v0.5.3)" framing for --all-mask; the flag is just part of the v0.5.0 release. forgelm/cli.py - One internal comment tweaked from "v0.5.1 consumers" to "any pre-Phase-12 JSON consumer" (the legacy envelope key still has a real reason to exist; the framing just stops referencing a release that won't exist as a separate PyPI tag). - Phase labels in user-visible help text (Phase 11.5 / 12 / 12.5) kept intact — they're descriptive history, not version pins. Verification - ruff clean (78 files) - 840 tests pass (13 skipped) - 7/7 bilingual H2 parity (configuration / usage / distributed / data_preparation / architecture / ingestion / data_audit) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 29, 2026
Merged
This was referenced May 2, 2026
This was referenced May 5, 2026
cemililik
added a commit
that referenced
this pull request
May 6, 2026
…gent review findings - Wave 5 PR #34 round 3 absorption: full sweep of CodeRabbit's 6 OOD + 20 Minor inline review comments PLUS 24 multi-agent review findings (code-correctness / privacy-security / standards / test-rigor docs under docs/analysis/code_reviews/wave5-*.md). HIGH/Major absorptions: - 4 broken `forgelm.cli.subcommands.*` symbol citations across purge/approvals reference docs (en+tr): _run_row_erasure → _run_purge_row_id, _handle_pending → _run_approvals_list_pending, _handle_show → _run_approvals_show (F-W5-PRIV-02). - configuration.md (en+tr): drop `kserve | triton` ghost choices and the entire `## deployment:` ghost YAML block (no `DeploymentConfig` in ForgeConfig; extra="forbid" rejects it at load) — replace with a v0.6.0+ Phase 14 callout (F-W5-S1, F-W5-S2). - ingestion.md (en+tr): drop conflicting "instructions format default" claim; document raw-chunk JSONL with summary-only --output-format {text,json} (CR-OOD ingestion). - closure-plan PR #33 status: line 131 said "PR #33 pending" but line 119 said merged with `01e40ba` (CR-OOD closure-plan). - leakage.md (en+tr): remove `audit.leakage_check.remove_cross_split_overlap` knob reference — knob doesn't exist in code; replace with the documented jq-based removal flow (CR-OOD leakage). - TR + EN experiment-tracking: drop `$ ` prompt prefix in `text`-tagged preview blocks for `forgelm compare-runs` (CR-OOD experiment-tracking). - roadmap mermaid (en+tr): add Phase 12.6 (Closure Cycle) → v0.5.5 node + edge so visual flow matches table (CR-OOD roadmap). Minor absorptions: - phase-12-6-closure-cycle.md L123: stale "(currently 0.5.1rc1)" parenthetical → already-bumped during Wave 5 (F-W5-S4 / CR Minor). - gdpr-erasure.md (en+tr): rename duplicate `### What gets recorded` / `### Neyi kaydeder` H3 to disambiguate Article 17 vs 15 sections (CR Minor #2 + #20). - verify-gguf.md (en+tr) L147: revise misleading "still match SHA-256" hash explanation; clarify magic-header layer doesn't catch truncation independent of sidecar (CR Minor #3). - verify_audit link mismatch in 4 reference docs (en+tr × verify_gguf + verify_annex_iv): label text now matches href (CR Minor #4 + #8). - air_gap_deployment-tr.md L19: add `text` language tag to ASCII diagram fence (CR Minor #6). - site/index.html L374: drop internal "Phase 19" milestone leak; tag reads `embed` (user-facing label) (CR Minor #7). - air_gap_deployment.md L193: fix dangling sentence fragment in HF_TOKEN gated-models tip (CR Minor #9). - performance.md (en+tr) L184: drop ghost `forgelm audit --output-format parquet`; replace with pandas/pyarrow conversion step (CR Minor #10). - phase-11-5-backlog.md L10-12: link text → package paths matching href (forgelm/data_audit.py → forgelm/data_audit/, etc.) (CR Minor #11). - testing.md L140: stale `23/23` → `39/39` bilingual parity count (CR Minor #12). - testing.md L198: `python3` prefix consistency in checklist (CR Minor #13). - supply-chain.md (en+tr): bandit cadence aligned to standards doc (Nightly 03:00 UTC + release tag, not "every PR + main push") (CR Minor #14 + #15). - translations.js L1927: "Three values" → "Two values" (only forgelm-lang + forgelm-theme listed) (CR Minor #16). - approvals_subcommand.md (en+tr) L26: staging path → match L84 JSON sample (`final_model.staging.<run_id>` canonical, run-id-less form honoured as fallback) (CR Minor #17 + #19). - translations.js L1445/L1462/L1504/L1521 (en+tr): `<code>reject</code>` → `<code>forgelm reject</code>` for fully-qualified copy/paste parity (CR Minor #18). - _meta.yaml header: add formal DE/FR/ES/ZH deferred-policy banner per closure-plan Task L (F-W5-S6). - release.md L20 + L359: stale "0.3.1rc1 current" + "no __version__" claims; corrected to 0.5.5 + reflect that __version__ + __api_version__ ARE exported per Library API standard (F-W5-S3). - purge_subcommand.md (en+tr): add WARNING-on-fallback language for --output-dir asymmetry vs reverse-pii (F-W5-PRIV-05). - verify-audit.md (en+tr): replace overstated "escrow" wording with KMS/HSM-based separation (F-W5-PRIV-06). Tool tightening + new tests: - check_cli_help_consistency.py: tighten _FORWARD_REF_MARKERS to whole-phrase forms (drop bare "planned"/"future"/"roadmap" which matched "planned attacks" / "future work" / "roadmap-driven" in unrelated prose); add Turkish equivalents for bilingual docs (F-W5-01). - test_check_cli_help_consistency.py: 3 new regression tests: bare-substring negative (F-W5-01), >±3-line forward-ref window negative (F-W5-TR-01a), and Wrong:-block scope-to-one-block resumption (F-W5-TR-01b). Suite 17 → 20 tests, all green. User-manual orphan fix: - _meta.yaml: register `reference/json-output` page (existed in filesystem since Wave 2 but never declared in nav config — was invisible on the live site). Now declared = on-disk = 65 each. Skipped (with reason): - CR Minor #5 (library_api.md `forgelm.__version__` example): INVALID — verified `python3 -c "import forgelm; print(forgelm.__version__)"` prints 0.5.5 cleanly; both __version__ and __api_version__ ARE exported per Library API standard. CodeRabbit's web-search must have hit pre-Wave-5 PyPI. Doc is correct; code matches. - F-W5-S5 (parity tool autodiscover usermanuals): registry shape refactor (~50 new pairs) is scope creep for round 3; defer to v0.6. - F-W5-S8 (compliance_summary-tr.md): explicitly deferred to v0.6 by the finding itself. - F-W5-S9, F-W5-TR-11/12 (NIT cite/style): non-load-bearing. - F-W5-02/03/04 (cognitive complexity, unterminated fence, resume-after-Wrong unit test): refactor scope; covered by F-W5-TR-01 block-scope tests for the resume case. - F-W5-TR-02 (cleanup commit baseline regression test): the live CI --strict gate IS the guard; doubling up the integration-test layer is defensible but not blocking. - F-W5-TR-03/05/06/07/08/09/10 (extras): non-blocking coverage gaps. Self-review gauntlet (all green): ruff format/check ✅ bilingual_parity --strict 39/39 ✅ anchor_resolution --strict 264/264 ✅ cli_help_consistency --strict 425/425 ✅ (reduced from 449 due to intentional invocation removals: ghost YAML block, parquet ghost, 4 broken symbol-cite paths, etc.) site_claims --strict (artefacts/templates/GPU/version) ✅ field_descriptions --strict ✅ pytest 1433 passed / 14 skipped (+5 new tests vs round 2) ✅ forgelm --config config_template.yaml --dry-run ✅ build_usermanuals.py: en/tr 65/65 OK, JS bundles regenerate ✅ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks done
This was referenced May 9, 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
v0.5.2.|escape, and a JWT regex narrowing.data_curation.ipynbcovers the full ingest+audit flow end-to-end; existing notebooks pinned toforgelm>=0.5.2; README updated with new chunking strategies, new optional extras, and project-structure modules added during Phases 11/11.5/12.3 commits, 26 files changed, +2981 / -203 lines.
Commits
2f5722a feat(phase-12): Data Curation Maturity (v0.5.2)— feature implementation, 31 new tests4ec817c fix(phase-12): apply 17 review findings (4 critical, 5 high, 4 medium, 4 nit)— review-cycle correctness + honesty fixes, 18 regression testsbf8ca82 docs(notebooks+readme): cover Phase 11/11.5/12 in notebooks; align README—data_curation.ipynb(20 cells) + README sweepTier 1 features (all
[x])forgelm/data_audit.py--dedup-method minhash; needs[ingestion-scale]extra (datasketch); default simhash path untouchedforgelm/ingestion.py--strategy markdown; heading breadcrumb inlined; code fences atomicforgelm/data_audit.py+forgelm/ingestion.py--secrets-maskfor ingestforgelm/data_audit.py--quality-filter; 5 Gopher/C4/RefinedWeb checks (now includingrepeated_linesafter review)forgelm/ingestion.py|and\\escaped for CommonMark safetyTier 2 / 3 (Presidio adapter, Croissant metadata,
--all-mask, wizard "audit first") deferred tophase-12-5-backlog.md.Critical fixes from round-1 review (post-
2f5722a)near_duplicate_pairs_per_splittop-level key when addingnear_duplicate_summary— would have brokenjq '.near_duplicate_pairs_per_split.train'consumersrepeated_linescheck (count ≥ 2 floor — short all-unique docs don't false-positive)_docx_table_to_markdownleft|in cell text, breaking column count for any cell containing|_escape_md_cellescapes|and\\per CommonMark; collapses embedded newlineseyJfoo.eyJbar.bazalg/typ/kid/cty/enc/apibase64url prefixes) + minimum payload + signature lengths"byte-identical default report" claim corrected to "schema-additive" — older parsers keep working, but on-disk JSON is no longer byte-identical because
secrets_summaryand method-tag fields are now always present.Backwards compatibility
pii_summary/near_duplicate_pairs_per_splitkeep working unchanged (additive only).forgelm ingestandforgelm auditruns are schema-additive — new fields only appear when their feature is enabled, except always-onsecrets_summary(always present, empty when clean).ImportError+ install-hint guards.API additions
audit_dataset(...)keyword args:dedup_method,minhash_jaccard,minhash_num_perm,enable_quality_filteringest_path(...)keyword arg:secrets_mask: boolAuditReportfields:secrets_summary: Dict[str, int],quality_summary: Dict[str, Any]IngestionResultfield:secrets_redaction_counts: Dict[str, int]DEDUP_METHODS,DEFAULT_MINHASH_JACCARD,DEFAULT_MINHASH_NUM_PERM,SECRET_TYPESCLI additions
forgelm ingest:--strategy markdown,--secrets-maskforgelm audit:--dedup-method {simhash,minhash},--jaccard-threshold X,--quality-filterTests
810 passed, 12 skipped (Phase 12 added 49 new tests on top of v0.5.1's 761):
tests/test_data_audit_phase12.py(18) — secrets, quality, MinHashtests/test_ingestion_phase12.py(13) — markdown sections, DOCX tables, secrets-masktests/test_cli_subcommands.py(+2) — quality flag, jaccard threshold validationtests/test_phase12_review_fixes.py(18) — regression pins for the 17 review fixes (JSON envelope, repeated_lines, DOCX escape, JWT narrowing, code-fence stripping, token-aware markdown chunker, CommonMark indented headings)Documentation
docs/roadmap/phase-12-5-backlog.md(4 deferred items),notebooks/data_curation.ipynb(20 cells, CPU-only)docs/guides/{ingestion,data_audit}.md+ TR mirrors,docs/reference/usage.md+ TR,docs/qms/sop_data_management.md,docs/standards/architecture.mdextras matrix,docs/roadmap.md+ TR,docs/roadmap/{releases,phase-12-data-curation-maturity}.mdTest plan
pip install -e .[ingestion-scale]thenforgelm audit data.jsonl --dedup-method minhashsmokeforgelm ingest ./raw_docs/ --recursive --strategy markdown --secrets-masksmoke against a corpus with seeded credentialsforgelm audit data.jsonl --output-format jsonenvelope still containsnear_duplicate_pairs_per_split(back-compat)data_curation.ipynbruns end-to-end in Colab on a CPU runtimeforgelm --config config_template.yaml --dry-runexits 0 (smoke verified locally)Summary by CodeRabbit
New Features
Bug Fixes / Hardening
Documentation
Tests
Chores