Repository navigation
feat(phase-11.5): Ingestion / audit polish (v0.5.1) - #12
Merged
Merged
Conversation
Operational follow-up to v0.5.0's ingestion + audit surface — the 12 items
carved out of Phase 11's review backlog, all landed in one focused pass.
No new training capabilities; behaviour at default flags is identical, the
gains are large-corpus throughput, ergonomics, and a cleaner CLI shape.
Highlights:
* `forgelm audit PATH` is now a first-class subcommand with `--verbose`,
`--near-dup-threshold`, and its own `--output` default. The legacy
`forgelm --data-audit FLAG` is preserved as a deprecation alias and
logs a one-line notice; existing pipelines need zero changes.
* Near-duplicate detection swapped from O(n²) brute force to LSH banding
(`bands = threshold + 1` by pigeonhole). Recall stays exact at the
default threshold; brute-force fallback retained when bands shrink
below 4 bits. Both within-split and cross-split helpers benefit.
* `_read_jsonl_split` is now a generator and `_audit_split` consumes it
row-by-row via a `_StreamingAggregator` — RAM use no longer scales
with split size.
* `--chunk-tokens N --tokenizer MODEL` on `forgelm ingest` sizes chunks
against an HF AutoTokenizer instead of raw character counts.
`trust_remote_code=False` hard-pinned for safety.
* PDF page header/footer dedup (`_strip_repeating_page_lines`) strips
lines that recur on >=70% of pages — kills audit near-duplicate noise
on long policy / book PDFs.
* `pii_severity` block alongside `pii_summary`: `total`, `by_tier`,
`by_type`, `worst_tier`. Tier table follows PCI-DSS / GDPR Art. 9
weighting. Notes line leads with the worst tier.
* `summarize_report(verbose=False)` (default) folds zero-finding splits
into a single tail line. `forgelm audit --verbose` reverses it.
* `IngestionResult.notes_structured` carries machine-readable
`{key: value}` for CI/CD consumers; `extra_notes` keeps the prose.
JSON output exposes both.
* Wizard's BYOD + full-flow dataset prompts now offer to run
`forgelm ingest` inline when the typed path is a directory of raw
documents — closes the onboarding loop end-to-end.
* Optional `xxhash.xxh3_64` digest + module-scope token-level
`lru_cache(maxsize=10_000)` accelerate the simhash hot path; BLAKE2b
remains as the bare-install fallback.
* `data_audit_report.json` is written via tempfile + os.replace so a
crashed audit can never leave a half-written report on disk.
newline="\n" pinned for byte-exact reproducibility.
Module deltas:
* `forgelm/data_audit.py`: PII_SEVERITY table, `_token_digest` (cached
+ xxhash optional), LSH-banded `find_near_duplicates`,
`_count_leaked_rows` rebuilt on the same banded index, `_read_jsonl_split`
promoted to a generator, `_audit_split(split_name, path, ...)` rewritten
as a streaming aggregator, `_build_pii_severity`, `_atomic_write_json`,
`summarize_report(verbose=...)`, `_split_has_findings`,
`_render_split_block`, `_render_pii_severity`.
* `forgelm/ingestion.py`: `_strip_repeating_page_lines`, `_extract_pdf`
threads dedup state, `_chunk_sliding_tokens`, `_chunk_paragraph_tokens`,
`_strategy_dispatch_tokens`, `_load_tokenizer`,
`IngestionResult.notes_structured` + `pdf_header_footer_lines_stripped`,
`_process_one_file` extended for token-aware dispatch.
* `forgelm/cli.py`: `_add_audit_subcommand`, `_run_audit_cmd`, dispatcher
hooks "audit", `--data-audit` reflows through `_run_data_audit` with
`invoked_via_legacy_flag=True`.
* `forgelm/wizard.py`: `_offer_ingest_for_directory` +
`_prompt_dataset_path_with_ingest_offer`; BYOD path and Step 5 dataset
prompt both use them.
* `pyproject.toml`: version bumped to 0.5.1rc1; `xxhash` added to the
`[ingestion]` extra (optional — fallback path keeps bare installs OK).
Tests:
* `tests/test_data_audit.py`: TestLshBandedNearDuplicates,
TestPiiSeverity, TestSummarizeVerbosePolicy, TestAtomicWrite,
TestStreamingReader, TestTokenCachePerformance.
* `tests/test_ingestion.py`: TestPdfHeaderFooterDedup,
TestStructuredIngestionNotes, TestTokenAwareCli; TestSummary updated
for the new "Next: forgelm audit ..." hint.
* `tests/test_cli_subcommands.py`: TestAuditSubcommand (subcommand happy
path, JSON envelope, legacy `--data-audit` alias parity).
* `tests/test_wizard_byod.py`: refreshed for the ingest-first wording
(empty-directory rejection + decline-the-offer path).
Result: 754 passed, 8 skipped (extras-skip). Lint clean.
Docs:
* docs/roadmap.md + roadmap-tr.md: Phase 11.5 marked done; v0.5.1 set as
the merged-pending-publish milestone; v0.5.2 reserved for Phase 14.
* docs/roadmap/phase-11-5-backlog.md: rewritten as a "what landed" record
with module pointers per item.
* docs/roadmap/releases.md: v0.5.1 ticked across 11 features; v0.5.2
reslotted as Pipeline Chains.
* docs/guides/{ingestion,data_audit}{,-tr}.md: token-aware mode, PDF
header/footer dedup, audit subcommand, PII severity, verbose policy
documented; legacy --data-audit retained as deprecation alias.
* docs/reference/usage{,-tr}.md: CLI examples updated for the new
subcommand + token-aware ingestion.
* README.md: feature blurb refreshed; quick-start hint promoted to
`forgelm audit`.
* CHANGELOG.md: detailed Unreleased entry covering all 12 items + tests
+ behavioural deltas.
![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 (2)
📝 WalkthroughWalkthroughPhase 11.5 (v0.5.1) ships ingestion and audit polish: adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLIAudit as "forgelm audit"
participant AuditLogic as AuditRunner
participant Reader as _read_jsonl_split
participant Simhash as compute_simhash
participant Detector as find_near_duplicates
participant PIIGrader as _build_pii_severity
participant FileOps as TempFile/os.replace
participant Output as data_audit_report.json
User->>CLIAudit: run audit <path> --output <dir> --near-dup-threshold N
CLIAudit->>AuditLogic: start audit(path, threshold, verbose)
AuditLogic->>Reader: iterate rows (streaming generator)
Reader-->>AuditLogic: yield row/meta
AuditLogic->>Simhash: compute_simhash(tokens) (uses _token_digest cache / xxhash if available)
Simhash-->>AuditLogic: fingerprint
AuditLogic->>Detector: find_near_duplicates(fingerprints, threshold, bits=64)
Detector-->>AuditLogic: candidate pairs + distances
AuditLogic->>PIIGrader: detect PII & compute pii_severity
PIIGrader-->>AuditLogic: pii_severity + worst_tier
AuditLogic->>FileOps: write report to temp file
FileOps->>FileOps: os.replace(temp -> data_audit_report.json)
FileOps->>Output: final report persisted
Output-->>User: summary printed (summarize_report, verbose controls folding)
sequenceDiagram
participant User
participant CLIIngest as "forgelm ingest"
participant IngestPath as ingest_path()
participant Tokenizer as _load_tokenizer
participant PDFDedup as _strip_repeating_page_lines
participant Chunker as token-aware chunker
participant Aggregator as _emit_chunk/_StreamingAggregator
participant Output as IngestionResult (notes_structured)
User->>CLIIngest: run ingest <path> --chunk-tokens N --tokenizer name
CLIIngest->>IngestPath: call ingest_path(..., chunk_tokens, tokenizer)
IngestPath->>Tokenizer: load tokenizer (validate present)
IngestPath->>PDFDedup: detect & strip repeating page header/footer lines
PDFDedup-->>IngestPath: lines_stripped count
IngestPath->>Chunker: produce token-based chunks (respect overlap_tokens)
Chunker-->>Aggregator: emit chunks
Aggregator-->>IngestPath: accumulate metrics
IngestPath->>Output: populate notes_structured and pdf_header_footer_lines_stripped
Output-->>User: JSONL written and returned for downstream (wizard/next steps)
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 | 168 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Phase 11.5, focusing on operational polish for the ingestion and audit features. Key additions include a first-class forgelm audit subcommand, LSH-banded near-duplicate detection for improved performance on large corpora, streaming JSONL reading to bound memory usage, and token-aware chunking for ingestion. The audit report now includes PII severity tiers and is written atomically to disk. Feedback suggests optimizing the LSH implementation to handle large clusters of identical fingerprints more efficiently by grouping them before processing. Additionally, the PDF header/footer stripping logic could be improved to iteratively remove multi-line headers and footers.
Comment on lines
+474
to
+488
| for indices in buckets.values(): | ||
| if len(indices) < 2: | ||
| continue | ||
| for left_pos in range(len(indices)): | ||
| i = indices[left_pos] | ||
| fp_i = fingerprints[i] | ||
| for right_pos in range(left_pos + 1, len(indices)): | ||
| j = indices[right_pos] | ||
| if (i, j) in seen: | ||
| continue | ||
| seen.add((i, j)) | ||
| distance = hamming_distance(fp_i, fingerprints[j]) | ||
| if distance <= threshold: | ||
| pairs.append((i, j, distance)) | ||
|
|
There was a problem hiding this comment.
The current LSH implementation remains vulnerable to indices list for a bucket can be large, leading to a nested loop that generates a massive number of pairs, and the seen set can grow excessively.
Since the auditor only requires the count of near-duplicate pairs (and not the full list), this can be optimized by grouping identical fingerprints first and then running LSH on the unique fingerprints. For exact duplicates, the pair count is
Comment on lines
+107
to
+128
| first_counts: Counter = Counter(lines[0] for lines in page_lines if lines) | ||
| last_counts: Counter = Counter(lines[-1] for lines in page_lines if lines) | ||
|
|
||
| cutoff = max(2, int(_PDF_REPEAT_THRESHOLD * len(pages))) | ||
| repeating_firsts = {ln for ln, n in first_counts.items() if n >= cutoff} | ||
| repeating_lasts = {ln for ln, n in last_counts.items() if n >= cutoff} | ||
|
|
||
| if not repeating_firsts and not repeating_lasts: | ||
| return pages, 0 | ||
|
|
||
| cleaned: List[str] = [] | ||
| stripped = 0 | ||
| for lines in page_lines: | ||
| kept = list(lines) | ||
| while kept and kept[0] in repeating_firsts: | ||
| kept.pop(0) | ||
| stripped += 1 | ||
| while kept and kept[-1] in repeating_lasts: | ||
| kept.pop() | ||
| stripped += 1 | ||
| if kept: | ||
| cleaned.append("\n".join(kept)) |
There was a problem hiding this comment.
The current implementation of header/footer stripping only considers lines that were originally the first or last non-empty lines of a page. If a document has a multi-line header (e.g., Line 1: Company Name, Line 2: Confidential), stripping Line 1 will make Line 2 the new first line, but it won't be stripped because it wasn't in the initial repeating_firsts set.
Consider refactoring this to iteratively strip common first/last lines until no more lines meet the threshold.
stripped = 0
while True:
stripped_this_pass = 0
first_counts = Counter(lines[0] for lines in page_lines if lines)
repeating_firsts = {ln for ln, n in first_counts.items() if n >= cutoff}
if repeating_firsts:
for lines in page_lines:
if lines and lines[0] in repeating_firsts:
lines.pop(0)
stripped_this_pass += 1
last_counts = Counter(lines[-1] for lines in page_lines if lines)
repeating_lasts = {ln for ln, n in last_counts.items() if n >= cutoff}
if repeating_lasts:
for lines in page_lines:
if lines and lines[-1] in repeating_lasts:
lines.pop(-1)
stripped_this_pass += 1
if stripped_this_pass == 0:
break
stripped += stripped_this_pass
if stripped == 0:
return pages, 0
cleaned = ["\n".join(lines) for lines in page_lines if lines]![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/guides/data_audit-tr.md (1)
135-140:⚠️ Potential issue | 🟠 MajorStale "v0.5.0 dışı" / "karesel" paragraphs contradict Phase 11.5 LSH banding shipping in this PR.
The doc's intro now advertises LSH-banded near-duplicate detection as part of v0.5.1, but this paragraph still tells operators near-dup is quadratic and that an LSH band index is out of scope for v0.5.0. Same stale wording reappears in the troubleshooting table at line 221.
Both should be rewritten to reflect the new ~O(n × k) banded path and the brute-force fallback for high-threshold edge cases (the actual Phase 11.5 behavior tested in
tests/test_data_audit.py::TestLshBandedNearDuplicates).📝 Suggested rewrite
-Satır sayısında karesel; ~50K satıra kadar olan veri setlerinde -audit-zamanı kullanım için uygun. Daha büyük külliyatlar için LSH band -indeksi gerekecek — `v0.5.0` kapsamı dışı. +Faz 11.5 itibarıyla yakın-tekrar tespiti LSH band indeksi +kullanır (eşik = `b - 1` band, fingerprint başına ~`O(k)`); aday +çiftler ardından Hamming mesafesiyle doğrulanır. Çok yüksek +eşiklerde (band genişliği < ~4 bit) kod otomatik olarak brute-force +fallback'e döner.And line 221:
-| Büyük veri setinde `near_duplicate_pairs` çok büyük | Simhash karesel on-binlerce satırda koştu | Önce örnekleyin; LSH index desteği takip edecek | +| Büyük veri setinde `near_duplicate_pairs` çok büyük | Eşik gerçekten benzer çift sayısının üstünde | `--near-dup-threshold`'u düşürün veya veri setini örnekleyin |As per coding guidelines: "Documentation claims in README and marketing must point to real code; every feature mentioned must be backed by implementation" — and the inverse holds: docs must not advertise limitations the code no longer has.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/data_audit-tr.md` around lines 135 - 140, The paragraph claiming "karesel" (quadratic) behavior and that LSH band indexing is "v0.5.0 kapsamı dışı" is stale; update the intro paragraph and the troubleshooting table entry (around the second occurrence) to describe the Phase 11.5 LSH-banded near-duplicate path: explain that the default path uses an O(n × k) LSH banded approach with a brute-force fallback for high-threshold edge cases, and reference the implemented behavior exercised by tests/test_data_audit.py::TestLshBandedNearDuplicates; ensure wording removes the "out of scope" claim and replaces "quadratic" with a note about when the brute-force fallback may apply.docs/guides/ingestion-tr.md (1)
155-155:⚠️ Potential issue | 🟡 MinorEski
forgelm --data-auditörneklerini de subcommand'a güncelleyin (TR mirror).Bu dosyada line 101
forgelm audit'e geçirilmiş ama line 155 (uçtan uca örnek) ve line 309 (PII içeren formlar) hâlâ legacyforgelm --data-auditformunu öneri olarak gösteriyor. Doc, subcommand'ı yeni varsayılan olarak konumlandırdığı için tutarlılık açısından bu iki örneği de yeni forma çevirin (legacy alias'tan ayrıca tek bir yerde bahsedilmesi yeterli).📝 Önerilen değişiklik
-forgelm --data-audit data/policies.jsonl --output ./audit/ +forgelm audit data/policies.jsonl --output ./audit/(line 309 için aynı düzenlemeyi
data/medical.jsonl'a uygulayın.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/ingestion-tr.md` at line 155, Replace the legacy flag-style examples that use "forgelm --data-audit ..." with the new subcommand form "forgelm audit ..." in the end-to-end example and the PII/forms example; specifically, change instances like "forgelm --data-audit data/policies.jsonl --output ./audit/" to "forgelm audit data/policies.jsonl --output ./audit/" and likewise update the example that references the medical/PII JSONL to use "forgelm audit <file> --output <dir>"; keep a single note elsewhere mentioning the legacy alias if desired.docs/guides/ingestion.md (1)
96-96:⚠️ Potential issue | 🟡 MinorUpdate remaining
forgelm --data-auditexamples to the subcommand for consistency.Lines 96 (PII section), 151 (end-to-end example), and 306 (PII forms / OCR section) still recommend the legacy
forgelm --data-audit ...form, even though the rest of the guide (line 9, intro) now points atforgelm audit. Since the legacy flag is a deprecation alias going forward, switch these examples too — keeping legacy mentioned only in the "compatibility" callout in usage.md/README is enough.The TR mirror (
docs/guides/ingestion-tr.md) has the same partial migration at lines 155 and 309.📝 Suggested fix
-`forgelm --data-audit` to verify. +`forgelm audit` to verify.-forgelm --data-audit data/policies.jsonl --output ./audit/ +forgelm audit data/policies.jsonl --output ./audit/-forgelm --data-audit data/medical.jsonl --output ./audit/ +forgelm audit data/medical.jsonl --output ./audit/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/ingestion.md` at line 96, Replace the remaining legacy CLI examples that use the deprecated flag form `forgelm --data-audit` with the new subcommand `forgelm audit` in the docs; specifically update the occurrences in docs/guides/ingestion.md (PII section, end-to-end example, PII forms/OCR example) and the TR mirror docs/guides/ingestion-tr.md, and ensure the old `--data-audit` form is only mentioned in the dedicated "compatibility" / usage/README callout rather than in examples.docs/guides/data_audit.md (3)
261-261:⚠️ Potential issue | 🟡 MinorStale troubleshooting row: LSH is no longer a follow-up.
This entry pre-dates Phase 11.5 — the diagnosis ("Simhash quadratic ran") and the fix ("LSH index support is a follow-up") contradict the LSH-banded near-duplicate detection just documented at lines 162-169 of this same file. Either drop the row or rewrite it for the post-LSH world (e.g., suggest tuning
--near-dup-thresholdif pair counts are still high).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/data_audit.md` at line 261, The table row about "`near_duplicate_pairs` enormous on a large dataset" is stale because LSH-banded near-duplicate detection is now implemented; update that row to reflect the post-LSH behavior by removing the "LSH index support is a follow-up" text and either drop the row or rewrite it to recommend post-LSH actions (e.g., mention tuning the `--near-dup-threshold` parameter, sampling strategies, or LSH band/threshold settings) and change the diagnosis from "Simhash quadratic ran" to guidance that explains large pair counts can still occur and how to mitigate them.
211-219:⚠️ Potential issue | 🟡 MinorRecommended-workflow snippet still uses the deprecated flag.
The rest of the guide (lines 3, 20-29, 226, 248-250) tells operators the new shape is
forgelm audit PATH, but the "Recommended workflow" example here still invokes the deprecation aliasforgelm --data-audit. New readers copy-pasting from this section will eat the deprecation notice on every CI run.📝 Proposed fix
# Audit first — surfaces issues before you commit to a long training run -forgelm --data-audit data/policies.jsonl --output ./checkpoints/policy-run/ +forgelm audit data/policies.jsonl --output ./checkpoints/policy-run/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/data_audit.md` around lines 211 - 219, The "Recommended workflow" snippet still uses the deprecated flag invocation "forgelm --data-audit"; update it to use the new subcommand form "forgelm audit PATH" (e.g., replace "forgelm --data-audit data/policies.jsonl --output ./checkpoints/policy-run/" with the equivalent "forgelm audit data/policies.jsonl --output ./checkpoints/policy-run/") so the example aligns with the rest of the doc and avoids CI deprecation noise; look for the snippet around the "Recommended workflow" text and update the command invocation accordingly.
1-282:⚠️ Potential issue | 🟠 MajorUpdate the Turkish mirror (
docs/guides/data_audit-tr.md) to include Phase 11.5 documentation in full.The Turkish version is missing two substantial Phase 11.5 sections present in the English guide:
- PII severity tiers block — the JSON example (lines ~100–120 in English) with tier structure, regulatory references (PCI-DSS, GDPR Art. 9, ENISA), and
worst_tierguidance for pipeline gatekeeping.- LSH banding explanation — the detailed technical section (lines ~180–200 in English) covering algorithm (
bands = threshold + 1), complexity (O(n²)→O(n × k)), fallback logic, and the pluggable simhash backend (xxhash.xxh3_64vs BLAKE2b withlru_cache).The Turkish file currently mentions Phase 11.5 in the introduction but does not document these key features. Sync the mirror to match the English version's complete feature set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/data_audit.md` around lines 1 - 282, The Turkish guide is missing the Phase 11.5 PII severity tiers and LSH banding sections; update the Turkish mirror to mirror the English content by adding the full "pii_severity" JSON block (including total, by_tier, by_type, worst_tier and regulatory references PCI‑DSS/GDPR Art.9/ENISA and the gatekeeping guidance), and add the LSH banding explanation describing "bands = threshold + 1", the O(n²)→O(n×k) complexity improvement, the brute-force fallback behavior of find_near_duplicates, and the simhash backend details (xxhash.xxh3_64 vs BLAKE2b and the lru_cache(maxsize=10_000) token memoisation) so the Turkish doc contains the same Phase 11.5 feature set and examples as the English guide.
🧹 Nitpick comments (5)
tests/test_data_audit.py (1)
520-642: LGTM — strong Phase 11.5 coverage; one optional brittleness note.The new test classes (LSH parity vs brute, severity tiers, atomic write hygiene, streaming-reader 3-tuple contract,
_token_digestcache observability) lock in the most error-prone surfaces from this PR.Optional:
TestSummarizeVerbosePolicy.test_clean_split_folded_in_default_modeasserts the literal"clean split"string in rendered output (line 586). If the renderer's copy ever changes, this test becomes a tripwire on wording rather than behavior. A regex likere.search(r"clean.*split", ...)or asserting the validation header is absent in default mode (and present in verbose) would be more robust. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_data_audit.py` around lines 520 - 642, The test TestSummarizeVerbosePolicy.test_clean_split_folded_in_default_mode is brittle because it asserts the exact substring "clean split" in rendered_default; update the assertion to check behavior rather than exact wording by either using a regex match (e.g., re.search(r"clean.*split", rendered_default)) or by asserting the validation split header is absent in rendered_default and present in rendered_verbose; modify the assertion lines that reference rendered_default/rendered_verbose in that test to use the more robust check against summarize_report's output.forgelm/wizard.py (2)
389-392: Default ingest output path is CWD-relative; consider anchoring it deterministically.
Path("data").resolve()resolves against the current working directory — if the wizard is launched from~/work/projects/foo/, the default lands at~/work/projects/foo/data/<dir>_ingested.jsonl, which is usually fine. But if the user justcdd into a system directory or scratch path, the default may surprise.Lower-priority polish: derive the default from the input directory itself (e.g.
resolved.parent / "data" / f"{resolved.name}_ingested.jsonl") so the path lives next to the source corpus regardless of CWD. The user can still override at the prompt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/wizard.py` around lines 389 - 392, The default output directory currently uses out_dir = Path("data").resolve() which anchors the JSONL file to the current working directory; change out_dir to be derived from the input path (e.g., out_dir = resolved.parent / "data") so default_out = out_dir / f"{resolved.name}_ingested.jsonl" lives next to the source corpus; ensure you still expand and resolve the user-provided out_path_raw via out_path = Path(out_path_raw).expanduser().resolve() and keep using the same _prompt call to let users override.
338-343: Drift risk:_INGEST_SUPPORTED_EXTENSIONSduplicatesforgelm.ingestion.SUPPORTED_EXTENSIONS.The comment correctly identifies the trade-off (avoid the extras-module import cost on the JSONL fast path), but if
forgelm/ingestion.pyever gains a new supported format the wizard will silently miss it —_directory_has_ingestible_fileswill return False and the user will hit the "doesn't contain any" message even though ingestion would have handled the file.A unit test assertion that the two sets agree prevents this drift:
♻️ Lock the two lists to drift via a test
# tests/test_wizard.py (new file or add to existing) def test_wizard_supported_extensions_match_ingestion(): from forgelm.ingestion import SUPPORTED_EXTENSIONS from forgelm.wizard import _INGEST_SUPPORTED_EXTENSIONS assert set(_INGEST_SUPPORTED_EXTENSIONS) == set(SUPPORTED_EXTENSIONS), ( "wizard's _INGEST_SUPPORTED_EXTENSIONS drifted from forgelm.ingestion.SUPPORTED_EXTENSIONS" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/wizard.py` around lines 338 - 343, The wizard hard-codes _INGEST_SUPPORTED_EXTENSIONS which can drift from forgelm.ingestion.SUPPORTED_EXTENSIONS; add a unit test (e.g., tests/test_wizard.py) that imports forgelm.ingestion.SUPPORTED_EXTENSIONS and forgelm.wizard._INGEST_SUPPORTED_EXTENSIONS and asserts their sets are equal so any future change in SUPPORTED_EXTENSIONS fails the test, ensuring _directory_has_ingestible_files behavior stays in sync with ingestion.forgelm/ingestion.py (1)
622-634:chunk_size != 2048heuristic silently swallows the warning when the user passed the default value.The warning is supposed to flag stale CLI invocations that mix
--chunk-sizewith--chunk-tokens, but it can only fire whenchunk_sizeis not equal to the in-file default of 2048. A user who explicitly passes--chunk-size 2048 --chunk-tokens N --tokenizer Mgets no warning, and any future change to thechunk_sizedefault would silently invert the behaviour for everyone.A sentinel default (or routing the explicit-vs-default determination through the CLI layer) makes the intent crash-proof.
♻️ Sketch — sentinel default
-def ingest_path( +_DEFAULT_CHUNK_SIZE = 2048 + +def ingest_path( input_path: str, *, output_path: str, - chunk_size: int = 2048, + chunk_size: Optional[int] = None, overlap: int = 200, ... ) -> IngestionResult: ... + effective_chunk_size = _DEFAULT_CHUNK_SIZE if chunk_size is None else chunk_size if chunk_tokens is not None: if not tokenizer: raise ValueError(...) - if chunk_size != 2048: + if chunk_size is not None: logger.warning( "Token-aware mode active (--chunk-tokens=%d): --chunk-size=%d is ignored.", chunk_tokens, - chunk_size, + chunk_size, )(Then thread
effective_chunk_sizethrough_process_one_file.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/ingestion.py` around lines 622 - 634, The current check uses chunk_size != 2048 to detect whether the user explicitly provided a chunk size, which wrongly treats an explicit "--chunk-size 2048" as the default; change the API so chunk_size is a sentinel (e.g., None or a special value) when not provided by the CLI and compute an effective_chunk_size later, then update this block (references: chunk_tokens, chunk_size, tokenizer_obj, _load_tokenizer and the caller like _process_one_file) to check the sentinel instead of 2048 and log the warning whenever the user explicitly supplied a chunk-size while also using --chunk-tokens; thread effective_chunk_size through the processing call so downstream code uses the resolved value.forgelm/data_audit.py (1)
1057-1067: Optional:dict.fromkeysfor the zero-initialised tier maps.SonarCloud flags both occurrences. Cosmetic, but the
fromkeysform is a touch shorter and signals "all values share the same initial value":♻️ Proposed cleanup
- if not pii_summary: - return { - "total": 0, - "by_tier": {tier: 0 for tier in PII_SEVERITY_ORDER}, - "by_type": {}, - "worst_tier": None, - } - - by_tier: Dict[str, int] = {tier: 0 for tier in PII_SEVERITY_ORDER} + if not pii_summary: + return { + "total": 0, + "by_tier": dict.fromkeys(PII_SEVERITY_ORDER, 0), + "by_type": {}, + "worst_tier": None, + } + + by_tier: Dict[str, int] = dict.fromkeys(PII_SEVERITY_ORDER, 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/data_audit.py` around lines 1057 - 1067, Replace the explicit comprehension used to zero-initialise tier maps with dict.fromkeys to be more concise and signal a shared initial value: update the inline return "by_tier": {tier: 0 for tier in PII_SEVERITY_ORDER} and the later by_tier = {tier: 0 for tier in PII_SEVERITY_ORDER} to use dict.fromkeys(PII_SEVERITY_ORDER, 0); leave by_type and other structures unchanged.
🤖 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/ingestion.md`:
- Around line 4-7: The intra-doc anchor `#phase-115-deltas` is missing; add a
new heading "## Phase 11.5 deltas" immediately after the intro paragraph that
briefly summarizes the two changes (token-aware chunking and PDF page-level
header/footer dedup) and includes links to the existing sections titled
"Token-aware chunking — `--chunk-tokens` (Phase 11.5)" and "PDF page-level
header/footer dedup (Phase 11.5)"; alternatively, if you prefer minimal change,
remove the `[Phase 11.5 deltas](`#phase-115-deltas`)` link from the intro.
---
Outside diff comments:
In `@docs/guides/data_audit-tr.md`:
- Around line 135-140: The paragraph claiming "karesel" (quadratic) behavior and
that LSH band indexing is "v0.5.0 kapsamı dışı" is stale; update the intro
paragraph and the troubleshooting table entry (around the second occurrence) to
describe the Phase 11.5 LSH-banded near-duplicate path: explain that the default
path uses an O(n × k) LSH banded approach with a brute-force fallback for
high-threshold edge cases, and reference the implemented behavior exercised by
tests/test_data_audit.py::TestLshBandedNearDuplicates; ensure wording removes
the "out of scope" claim and replaces "quadratic" with a note about when the
brute-force fallback may apply.
In `@docs/guides/data_audit.md`:
- Line 261: The table row about "`near_duplicate_pairs` enormous on a large
dataset" is stale because LSH-banded near-duplicate detection is now
implemented; update that row to reflect the post-LSH behavior by removing the
"LSH index support is a follow-up" text and either drop the row or rewrite it to
recommend post-LSH actions (e.g., mention tuning the `--near-dup-threshold`
parameter, sampling strategies, or LSH band/threshold settings) and change the
diagnosis from "Simhash quadratic ran" to guidance that explains large pair
counts can still occur and how to mitigate them.
- Around line 211-219: The "Recommended workflow" snippet still uses the
deprecated flag invocation "forgelm --data-audit"; update it to use the new
subcommand form "forgelm audit PATH" (e.g., replace "forgelm --data-audit
data/policies.jsonl --output ./checkpoints/policy-run/" with the equivalent
"forgelm audit data/policies.jsonl --output ./checkpoints/policy-run/") so the
example aligns with the rest of the doc and avoids CI deprecation noise; look
for the snippet around the "Recommended workflow" text and update the command
invocation accordingly.
- Around line 1-282: The Turkish guide is missing the Phase 11.5 PII severity
tiers and LSH banding sections; update the Turkish mirror to mirror the English
content by adding the full "pii_severity" JSON block (including total, by_tier,
by_type, worst_tier and regulatory references PCI‑DSS/GDPR Art.9/ENISA and the
gatekeeping guidance), and add the LSH banding explanation describing "bands =
threshold + 1", the O(n²)→O(n×k) complexity improvement, the brute-force
fallback behavior of find_near_duplicates, and the simhash backend details
(xxhash.xxh3_64 vs BLAKE2b and the lru_cache(maxsize=10_000) token memoisation)
so the Turkish doc contains the same Phase 11.5 feature set and examples as the
English guide.
In `@docs/guides/ingestion-tr.md`:
- Line 155: Replace the legacy flag-style examples that use "forgelm
--data-audit ..." with the new subcommand form "forgelm audit ..." in the
end-to-end example and the PII/forms example; specifically, change instances
like "forgelm --data-audit data/policies.jsonl --output ./audit/" to "forgelm
audit data/policies.jsonl --output ./audit/" and likewise update the example
that references the medical/PII JSONL to use "forgelm audit <file> --output
<dir>"; keep a single note elsewhere mentioning the legacy alias if desired.
In `@docs/guides/ingestion.md`:
- Line 96: Replace the remaining legacy CLI examples that use the deprecated
flag form `forgelm --data-audit` with the new subcommand `forgelm audit` in the
docs; specifically update the occurrences in docs/guides/ingestion.md (PII
section, end-to-end example, PII forms/OCR example) and the TR mirror
docs/guides/ingestion-tr.md, and ensure the old `--data-audit` form is only
mentioned in the dedicated "compatibility" / usage/README callout rather than in
examples.
---
Nitpick comments:
In `@forgelm/data_audit.py`:
- Around line 1057-1067: Replace the explicit comprehension used to
zero-initialise tier maps with dict.fromkeys to be more concise and signal a
shared initial value: update the inline return "by_tier": {tier: 0 for tier in
PII_SEVERITY_ORDER} and the later by_tier = {tier: 0 for tier in
PII_SEVERITY_ORDER} to use dict.fromkeys(PII_SEVERITY_ORDER, 0); leave by_type
and other structures unchanged.
In `@forgelm/ingestion.py`:
- Around line 622-634: The current check uses chunk_size != 2048 to detect
whether the user explicitly provided a chunk size, which wrongly treats an
explicit "--chunk-size 2048" as the default; change the API so chunk_size is a
sentinel (e.g., None or a special value) when not provided by the CLI and
compute an effective_chunk_size later, then update this block (references:
chunk_tokens, chunk_size, tokenizer_obj, _load_tokenizer and the caller like
_process_one_file) to check the sentinel instead of 2048 and log the warning
whenever the user explicitly supplied a chunk-size while also using
--chunk-tokens; thread effective_chunk_size through the processing call so
downstream code uses the resolved value.
In `@forgelm/wizard.py`:
- Around line 389-392: The default output directory currently uses out_dir =
Path("data").resolve() which anchors the JSONL file to the current working
directory; change out_dir to be derived from the input path (e.g., out_dir =
resolved.parent / "data") so default_out = out_dir /
f"{resolved.name}_ingested.jsonl" lives next to the source corpus; ensure you
still expand and resolve the user-provided out_path_raw via out_path =
Path(out_path_raw).expanduser().resolve() and keep using the same _prompt call
to let users override.
- Around line 338-343: The wizard hard-codes _INGEST_SUPPORTED_EXTENSIONS which
can drift from forgelm.ingestion.SUPPORTED_EXTENSIONS; add a unit test (e.g.,
tests/test_wizard.py) that imports forgelm.ingestion.SUPPORTED_EXTENSIONS and
forgelm.wizard._INGEST_SUPPORTED_EXTENSIONS and asserts their sets are equal so
any future change in SUPPORTED_EXTENSIONS fails the test, ensuring
_directory_has_ingestible_files behavior stays in sync with ingestion.
In `@tests/test_data_audit.py`:
- Around line 520-642: The test
TestSummarizeVerbosePolicy.test_clean_split_folded_in_default_mode is brittle
because it asserts the exact substring "clean split" in rendered_default; update
the assertion to check behavior rather than exact wording by either using a
regex match (e.g., re.search(r"clean.*split", rendered_default)) or by asserting
the validation split header is absent in rendered_default and present in
rendered_verbose; modify the assertion lines that reference
rendered_default/rendered_verbose in that test to use the more robust check
against summarize_report's output.
🪄 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: b07a2e81-7b58-475a-b0fb-73080666678e
📒 Files selected for processing (22)
CHANGELOG.mdREADME.mddocs/guides/data_audit-tr.mddocs/guides/data_audit.mddocs/guides/ingestion-tr.mddocs/guides/ingestion.mddocs/reference/usage-tr.mddocs/reference/usage.mddocs/roadmap-tr.mddocs/roadmap.mddocs/roadmap/phase-11-5-backlog.mddocs/roadmap/phase-11-data-ingestion.mddocs/roadmap/releases.mdforgelm/cli.pyforgelm/data_audit.pyforgelm/ingestion.pyforgelm/wizard.pypyproject.tomltests/test_cli_subcommands.pytests/test_data_audit.pytests/test_ingestion.pytests/test_wizard_byod.py
…/ 8 nits)
Verified each finding against the current code; applied 17 of the 20 raised,
declined 1 (LSH cluster-grouping optimisation — defer to follow-up; current
LSH already covers common cases and the win only kicks in on pathological
identical-fingerprint clusters), folded 2 into others.
Critical
--------
* xxhash perf claim toned to match measurement. Reviewer benched
1.05× end-to-end / 1.3× raw on a Python-wrapped path; my repeat
produced 1.06× / 1.31-1.40×. The "4-10×" figure in xxhash's
README is C-level pure-hash, not Python. Edited the inline comment
in `forgelm/data_audit.py:46-56`, the `_token_digest` docstring,
`docs/guides/data_audit.md`, and the CHANGELOG. Added
`tests/bench_simhash.py` so future reviewers can re-measure on
their own hardware (run manually; not in CI). Recorded the
measured numbers in `docs/roadmap/phase-11-5-backlog.md`.
High
----
* `_StreamingAggregator.keysets: List[frozenset]` → `keyset_counts:
Counter[frozenset]`. Stable schema = 1 entry instead of N. Updated
`_aggregator_to_info` to use `most_common(1)`. Tightened the
`_audit_split` docstring: memory is dominated by per-row
fingerprints (~30 MB / 1 M rows), not "bounded" — accurate now.
* `_chunk_paragraph_tokens` now budgets the `"\n\n"` separator
token cost. Compute `sep_tokens` once per call, add when a join
would happen. Prevents the small-but-real per-chunk overflow
the docstring already promised against.
* `forgelm/compliance.py:150` hint string now says
`forgelm audit <dataset>` instead of the deprecated
`forgelm --data-audit`. Updated the matching test in
`tests/test_compliance.py` to accept either form during the
deprecation window.
Medium
------
* Restored the round-3 archaeology in the non-dict row branch of
`_ingest_row`: the double-counting (`non_object_rows += 1`
AND `null_or_empty += 1`) is intentional — without the
`null_or_empty` bump, `null_or_empty_rate` would understate
corruption while length stats / fingerprints excluded the row.
Comment now says so, with a pointer to the test that pins the
contract.
* `_chunk_paragraph_tokens` paragraph-mode `overlap_tokens`
silent ignoring is now documented in the docstring AND surfaces
as a one-line `INFO` log in `ingest_path` so an operator who
mixes `--strategy paragraph --overlap-tokens N` sees what
happened.
* New direct unit tests:
- `TestCountLeakedRowsLshParity`: `_count_leaked_rows` LSH path
matches the brute-force formula; high-threshold fallback still
counts identical rows.
- `TestAtomicWriteFailure::test_failure_during_replace_leaves_no_tmp`:
monkey-patches `os.replace` to raise; verifies the `.tmp`
cleanup actually runs and the canonical file is not created.
- `TestPiiSeveritySnapshot`: pins the per-call snapshot contract
so a future "let me deep-freeze the table" change is loud.
* `_build_pii_severity` snapshots `PII_SEVERITY` at call time
(`severity_table = dict(PII_SEVERITY)`) so a test mutating the
module-level dict cannot corrupt audit output mid-run.
Nits
----
* `dict.fromkeys(PII_SEVERITY_ORDER, 0)` instead of dict
comprehensions in `_build_pii_severity`. Same shape, signals
shared initial value.
* `chunk_size` in `ingest_path` is now `Optional[int] = None`
with a public `DEFAULT_CHUNK_SIZE = 2048` sentinel. The "did the
operator pass it explicitly?" check no longer compares against
`2048` (which would falsely flag an explicit `--chunk-size 2048`
alongside `--chunk-tokens`). CLI default is now `None`, library
resolves to `DEFAULT_CHUNK_SIZE`.
* Wizard ingest-first hook anchors the default JSONL path to the
source corpus's parent (`resolved.parent / "data"`) instead of
`cwd / "data"`. Pairs with the operator's existing layout instead
of dumping next to wherever they happened to invoke `forgelm`.
* New `tests/test_wizard_phase11_5.py::test_wizard_extensions_match_ingestion`
fails loudly if `wizard._INGEST_SUPPORTED_EXTENSIONS` drifts from
`ingestion.SUPPORTED_EXTENSIONS`.
* `TestSummarizeVerbosePolicy` now asserts behaviour, not the
fold-line wording: "└─ validation" header absent in default mode,
present in verbose. Substring-tied to fold-line wording was
brittle.
* `TestSummary::test_summary_contains_next_step_hint` tightened:
must contain `forgelm audit`; explicitly forbids `forgelm
--data-audit` so a future regression to legacy wording is loud.
* PR-agent inline (PDF multi-line headers): `_strip_repeating_page_lines`
now iterates — pop a repeating first/last line, recompute the
count maps, repeat until no line crosses the threshold. New test
`test_multiline_header_stripped_iteratively` covers the
ACME / CONFIDENTIAL pattern that the single-pass version missed.
Doc fixes
---------
* `docs/guides/ingestion.md`: removed dangling `#phase-115-deltas`
anchor (the per-feature sections already document what landed);
replaced 3 `forgelm --data-audit` examples with `forgelm audit`.
* `docs/guides/ingestion-tr.md`: same `--data-audit` → `audit`
replacements (2 examples).
* `docs/guides/data_audit.md`: workflow snippet uses `forgelm
audit`; troubleshooting "near_duplicate_pairs enormous" row
rewritten — "LSH index support is a follow-up" is no longer
true; new wording explains genuine high near-dup is signal,
not algorithmic noise, and recommends `--near-dup-threshold`
tuning + PDF dedup; xxhash blurb tones the speedup claim and
cites the bench script.
* `docs/guides/data_audit-tr.md`: added the missing "PII şiddet
katmanları (Faz 11.5)" section mirroring the EN one (full
`pii_severity` JSON shape + tier table provenance + gatekeeping
guidance); rewrote "Near-duplicate tespiti" — replaced the
outdated "karesel; LSH band indeksi v0.5.0 kapsamı dışı"
paragraph with the implemented LSH banding semantics, brute
fallback note, xxhash backend description, and the lru_cache
story; troubleshooting row updated to match the EN one.
* `docs/roadmap/phase-14-pipeline-chains.md`: stale `v0.5.1`
release target → `v0.5.2` (Phase 11.5 took the v0.5.1 slot).
Result: 760 passed (+6 vs. previous 754), 8 skipped. Lint clean.
Smoke verified: `forgelm audit ...`, `forgelm --data-audit ...`
(legacy with deprecation log), `forgelm ingest --strategy
paragraph --overlap-tokens 50` (logs the ignore note),
`forgelm ingest --output-format json` (`notes_structured` +
`pdf_header_footer_lines_stripped` shapes intact).
Declined / deferred
-------------------
* PR-agent inline on `forgelm/data_audit.py:474-488` (LSH cluster
optimisation): real but only matters on pathological
identical-fingerprint clusters. Adds non-trivial complexity
(group → unique fingerprints → arithmetic combination) for an
edge case the LSH already handles fine on real corpora. Moved
to follow-up backlog rather than landing in this fix pass.
…ssions
5 functions exceeded the 15-cog cap; broken into single-concern helpers
without changing observable behaviour. 760 passed / 8 skipped, lint clean.
Refactors
---------
* `forgelm/data_audit.py:find_near_duplicates` (22 → ≤15) — extracted
`_bucket_pairs_within_threshold` for the per-bucket Hamming
verification pass; reused the existing `_build_band_index` helper
instead of inlining the bucket build. The orchestrator is now a
flat band → bucket → verify pipeline.
* `forgelm/data_audit.py:_count_leaked_rows` (28 → ≤15) — split into
`_count_leaked_brute` (the linear-scan fallback) and
`_source_row_leaks` (single-row LSH walk with early exit). Keeps
per-source-row `seen` set scoping intact.
* `forgelm/ingestion.py:_strip_repeating_page_lines` (17 → ≤15) —
pulled `_repeating_edge_lines` (count → set) and
`_pop_repeating_edges` (mutation pass) out of the iteration loop.
Multi-line header handling is unchanged (still iterative until no
edge crosses the threshold).
* `forgelm/ingestion.py:_extract_pdf` (16 → ≤15) — encryption /
decrypt-attempt logic moved to `_try_pdf_decrypt`; per-page
text extraction loop moved to `_read_pdf_pages`.
* `forgelm/ingestion.py:_process_one_file` (18 → ≤15) — three
concerns now have their own helpers:
- `_extract_text_for_ingest`: extractor dispatch + tolerant
skip-on-failure (ImportError still propagates).
- `_select_chunks_iter`: token-aware vs character chunker pick.
- `_emit_chunk`: mask + serialise + outcome counter update.
bench_simhash.py
----------------
* L78 (`int.from_bytes` return value not used): assigned to `_` for
both backends so the two timed loops stay symmetrical and the
intentional discard is explicit. Bench numbers unchanged
(1.32-1.34× raw, 1.10× e2e on this run).
* 7× SonarCloud `python:S2245` PRNG warning: added per-call-site
`# NOSONAR(python:S2245)` annotations on every `random.choices` /
`random.randint` / `random.seed` site, plus a module-docstring
paragraph documenting that this is microbenchmark-only test data
and never security-sensitive. The same comment also covers the
ruff/bandit S311 check (the ruff config already excludes the `S`
group, but the inline annotations document intent regardless).
…tspots Line-level ``# NOSONAR(python:S2245)`` annotations didn't suppress the hotspots; the rule continues to flag every ``random.choices`` / ``random.randint`` / ``random.seed`` call site even with the suppression comment attached. Switching to deterministic generators removes the issue at the source without falling back to ``secrets.SystemRandom`` (overkill for a microbenchmark) or littering the file with rule-specific opt-outs. Changes ------- * ``import random`` removed entirely. The module is ``random``-free. * ``_gen_short`` and ``_gen_long`` now build tokens via a tiny ``_det_token(idx, length)`` helper that walks ``string.ascii_lowercase`` with prime-step (7, 13) permutations — varied-looking output, no PRNG. * ``_gen_zipfian_english`` cycles deterministically through a 20-token stop-word list. The Zipfian effect comes from the small vocab being hashed against a 50K-row corpus, which is what the ``lru_cache`` is meant to absorb anyway. * ``_bench_compute_simhash`` token-count cycles deterministically through 50-150 via ``50 + (i * 17 % 101)`` and packs each text via a strided walk through the base vocab. * ``random.seed(42)`` in ``main()`` removed — there is nothing left to seed. Reproducibility is now structural (same code → same inputs) rather than seeded. Verification ------------ * ``ruff check`` clean (after auto-import-sort). * ``python tests/bench_simhash.py`` produces numbers in the same ballpark as the seeded version: 1.32-1.37× raw, ~1.0-1.1× e2e (run-to-run wall-clock variance dominates either way). * ``pytest tests/`` — 760 passed, 8 skipped (unchanged). * ``grep -E '^[^#]*random\.' tests/bench_simhash.py`` returns nothing — only docstring/comment mentions remain, and those are background context for the change.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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)
forgelm/cli.py (1)
1177-1205:⚠️ Potential issue | 🟠 MajorMap ingest I/O failures to the CLI contract.
This handler misses
PermissionError,IsADirectoryError,ENOSPC, and otherOSErrors from opening the output file or walking the input tree. Those currently fall out as tracebacks instead of returningEXIT_CONFIG_ERROR. As per coding guidelines,forgelm/cli.py: Exit codes are a public contract: 0/1/2/3/4 as per error-handling.md, especially for CLI.Suggested fix
- except (FileNotFoundError, ValueError) as exc: + except (OSError, ValueError) as exc: if output_format == "json": print(json.dumps({"success": False, "error": str(exc)})) else: logger.error("Ingest failed: %s", exc) sys.exit(EXIT_CONFIG_ERROR)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/cli.py` around lines 1177 - 1205, The current except block around the ingest_path call only catches FileNotFoundError and ValueError and misses OS-level I/O errors; update the first except to also catch PermissionError, IsADirectoryError and OSError (e.g., change "except (FileNotFoundError, ValueError) as exc:" to include these exceptions) so any file/open/walk errors are handled the same way (print JSON on json output_format or logger.error otherwise) and then sys.exit(EXIT_CONFIG_ERROR); keep the existing ImportError handler for EXIT_TRAINING_ERROR unchanged and reference the same symbols (ingest_path, output_format, logger, EXIT_CONFIG_ERROR, EXIT_TRAINING_ERROR) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guides/ingestion-tr.md`:
- Around line 21-26: Update the English mirror docs/guides/ingestion.md to match
the Turkish change by adding the optional `xxhash` backend to the listed
optional packages (`pypdf`, `python-docx`, `ebooklib`, `beautifulsoup4`,
`langdetect`) and note that `xxhash` is available from version 11.5; ensure the
wording about these being optional for plain text/Markdown and that import of
the module does not load extractors is preserved so both guides stay in sync.
In `@forgelm/cli.py`:
- Around line 345-350: The audit subparser's "--output" currently sets
default="./audit" and thus overwrites a top-level --output; change the audit
subparser's add_argument for "--output" to use default=argparse.SUPPRESS so the
subparser only provides the value when explicitly passed, and ensure the
existing fallback logic in _run_data_audit() is used to set "./audit" when no
audit-specific output is present (i.e., check for the presence of the attribute
on the parsed args and apply the "./audit" default inside _run_data_audit()).
- Around line 361-368: The CLI currently accepts any integer for
--near-dup-threshold; add validation so negative values are rejected at
parse-time. Replace the current type=int on the add_argument call for
"--near-dup-threshold" (the add_argument call in the parser setup in cli.py)
with a validator function (e.g., non_negative_int) that raises
argparse.ArgumentTypeError when value < 0, or alternatively perform an immediate
check after parsing and call parser.error("...") for negative values so the
process exits via the standard CLI config-error path (parser.error -> exit code
2).
In `@forgelm/data_audit.py`:
- Around line 1355-1378: _render_split_block currently omits showing
parse_errors, decode_errors, and non_object_rows even though
_split_has_findings() promotes splits with those problems; update
_render_split_block to detect these keys on the info dict and append
human-readable lines (e.g. "parse errors: X", "decode errors: X", "non-object
rows: X" or short details) similar to how languages_top3/pii_counts are rendered
so malformed JSONL splits are clearly flagged in the CLI output; keep the
existing block prefix/indenting and ordering so these error lines appear after
the sample_count line and before other metrics.
In `@forgelm/ingestion.py`:
- Around line 136-138: The cutoff calculation currently floors the threshold by
using int(_PDF_REPEAT_THRESHOLD * len(pages)), which can make a 70% rule behave
as <70%; change the computation to use math.ceil to enforce "≥70%" (e.g. cutoff
= max(2, math.ceil(_PDF_REPEAT_THRESHOLD * len(pages)))) and add the required
import for math if not present; update the reference in the dedup logic that
uses cutoff (variable cutoff and related use of
page_lines/_PDF_REPEAT_THRESHOLD) accordingly.
- Around line 177-188: The current _read_pdf_pages silently swallows
page.extract_text() failures; change it to stop hiding these errors by (a)
catching only the PDF/text-extraction-specific exception your PDF library throws
(or Exception if there is no narrower type) and logging a warning with path and
page index, and then (b) propagate a file-level failure (raise a
FileIngestionError or IOError) instead of returning a partial page list so the
caller can mark the entire file failed; update any caller of _read_pdf_pages to
handle the propagated exception or the new error type.
- Around line 167-174: The post-decrypt check currently uses reader.is_encrypted
which remains True in pypdf>=4.0 even after a successful decrypt; change the
logic to capture the return value of reader.decrypt(path) (the
PasswordType/boolean result) and gate the subsequent validation on that return
instead of is_encrypted: call result = reader.decrypt("") and if result
indicates failure (not PasswordType.SUCCESS / falsy) then raise the ValueError,
otherwise proceed; update the exception handling block around reader.decrypt
and, if needed, import or reference the PasswordType enum used by pypdf to
compare the return value (symbols: reader.decrypt, reader.is_encrypted,
FileNotDecryptedError).
In `@forgelm/wizard.py`:
- Around line 405-424: The try/except around calling ingest_path currently only
catches FileNotFoundError, ValueError and ImportError, so
PermissionError/OSError from writing JSONL or traversing the tree slip through;
update the ingest_path call's exception handling (the try block that invokes
ingest_path) to also catch PermissionError and broader OSError, and in that
handler print a concise, retryable message including the exception details
(e.g., "Ingest failed due to filesystem error: {exc} — check permissions or
output path") and return None so the wizard can re-prompt; keep the existing
ImportError handling separate.
In `@tests/test_wizard_phase11_5.py`:
- Around line 19-23: The test currently compares extensions via set equality
which hides ordering drift and duplicates; change the assertion in
tests/test_wizard_phase11_5.py to compare the sequences verbatim (i.e., assert
wizard._INGEST_SUPPORTED_EXTENSIONS == ingestion.SUPPORTED_EXTENSIONS) so
ordering and duplicates are detected — reference the symbols
wizard._INGEST_SUPPORTED_EXTENSIONS and ingestion.SUPPORTED_EXTENSIONS and keep
the existing failure message.
---
Outside diff comments:
In `@forgelm/cli.py`:
- Around line 1177-1205: The current except block around the ingest_path call
only catches FileNotFoundError and ValueError and misses OS-level I/O errors;
update the first except to also catch PermissionError, IsADirectoryError and
OSError (e.g., change "except (FileNotFoundError, ValueError) as exc:" to
include these exceptions) so any file/open/walk errors are handled the same way
(print JSON on json output_format or logger.error otherwise) and then
sys.exit(EXIT_CONFIG_ERROR); keep the existing ImportError handler for
EXIT_TRAINING_ERROR unchanged and reference the same symbols (ingest_path,
output_format, logger, EXIT_CONFIG_ERROR, EXIT_TRAINING_ERROR) when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dcbd19ba-f137-4005-b3b9-161612610196
📒 Files selected for processing (17)
CHANGELOG.mddocs/guides/data_audit-tr.mddocs/guides/data_audit.mddocs/guides/ingestion-tr.mddocs/guides/ingestion.mddocs/roadmap/phase-11-5-backlog.mddocs/roadmap/phase-14-pipeline-chains.mdforgelm/cli.pyforgelm/compliance.pyforgelm/data_audit.pyforgelm/ingestion.pyforgelm/wizard.pytests/bench_simhash.pytests/test_compliance.pytests/test_data_audit.pytests/test_ingestion.pytests/test_wizard_phase11_5.py
✅ Files skipped from review due to trivial changes (2)
- docs/roadmap/phase-14-pipeline-chains.md
- forgelm/compliance.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_data_audit.py
Comment on lines
+177
to
+188
| def _read_pdf_pages(reader: Any, path: Path) -> List[str]: | ||
| """Extract per-page text; tolerate page-level failures with a debug log.""" | ||
| pages: List[str] = [] | ||
| for page in reader.pages: | ||
| try: | ||
| text = page.extract_text() or "" | ||
| except Exception as exc: | ||
| logger.debug("PDF page extraction failed for %s: %s", path, exc) | ||
| text = "" | ||
| if text.strip(): | ||
| pages.append(text) | ||
| return pages |
Contributor
There was a problem hiding this comment.
Don't silently drop unreadable PDF pages.
A page-level extract_text() failure currently turns into a debug log plus an empty page, so the default ingest path can emit a partial document with no operator-visible signal. This should be surfaced as a warning/structured note or treated as a file failure.
As per coding guidelines "No silent failures: no bare except:, no except Exception: pass, no logging-and-swallowing for non-fatal paths, no || true in CI".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forgelm/ingestion.py` around lines 177 - 188, The current _read_pdf_pages
silently swallows page.extract_text() failures; change it to stop hiding these
errors by (a) catching only the PDF/text-extraction-specific exception your PDF
library throws (or Exception if there is no narrower type) and logging a warning
with path and page index, and then (b) propagate a file-level failure (raise a
FileIngestionError or IOError) instead of returning a partial page list so the
caller can mark the entire file failed; update any caller of _read_pdf_pages to
handle the propagated exception or the new error type.
Phase 12's slot was never planned — `docs/roadmap/phase-12-quickstart.md` was a leftover filename from early ordering when Quickstart held that slot. Renamed it to `phase-10-5-quickstart.md` (matches the current phase number) and used the freed Phase 12 slot for the natural continuation of the Phase 11 + 11.5 ingestion / audit lineage. What landed in this commit -------------------------- - `docs/roadmap/phase-12-data-curation-maturity.md` (new) — full 4-6 week plan with three Tiers (Must-have / Should-have / Could-have), five "Won't-do" lines, kill criteria, module-touchpoint table, and a week-by-week sequencing. - `docs/roadmap/phase-12-quickstart.md` → `phase-10-5-quickstart.md` rename; the file's "Filename history" note now records the rename rationale so the link in releases.md / roadmap.md / roadmap-tr.md stays self-explanatory. - Phase 14 (Multi-Stage Pipeline Chains) reslotted from `v0.5.2` to `v0.5.3`. Phase 12 takes `v0.5.2` so the ingestion lineage (11 → 11.5 → 12) finishes uninterrupted before the trainer orchestration surface gets reshaped. Phase 14's own delivery section + releases.md updated; rationale documented in both files. - `docs/roadmap.md` + `docs/roadmap-tr.md` status table, current / next milestone paragraphs, mermaid diagram, and documentation map all updated for the new sequence (P115 → P12 → P14 → P13). - `docs/roadmap/phase-11-5-backlog.md` "What's next" section now links forward to Phase 12 with the competitive-review provenance. - `docs/roadmap/releases.md` v0.5.2 entry rewritten for Phase 12 features; v0.5.3 entry added for Phase 14. - `CHANGELOG.md` automatically picks up the new Phase 12 link through the doc-rename pass; no behavioural delta in this commit. Phase 12 thesis (one paragraph) ------------------------------- Mature `forgelm ingest` + `forgelm audit` along three axes — scale (MinHash LSH dedup for >50K-row corpora via the optional `datasketch` dependency), security (code/secret leakage scanning + optional ML-based PII via Presidio), quality (markdown-aware splitter, Gopher/C4-style heuristic filter, DOCX/Markdown table preservation). Closes the four concrete gaps the post-`v0.5.1` competitive review surfaced against LLaMA-Factory / Axolotl / Unsloth / NeMo Curator / Dolma / RedPajama / LlamaIndex / LangChain / Marker / Docling. Layout-aware PDF parsing (Marker / Docling delegation) and embedding-based semantic dedup are deliberately deferred to Phase 13+ — both would add runtime ML dependencies that conflict with `docs/marketing/strategy/05-yapmayacaklarimiz.md` and Annex IV reproducibility guarantees. Verification ------------ - `forgelm --config config_template.yaml --dry-run` clean. - `pytest tests/` — 760 passed, 8 skipped (no code changes; doc-only commit). - `grep -rn phase-12-quickstart docs/ --include='*.md'` returns only the historical-filename note in phase-10-5-quickstart.md (intended). - Cross-references between roadmap.md, roadmap-tr.md, releases.md, phase-11-5-backlog.md, phase-14-pipeline-chains.md, and the new phase-12-data-curation-maturity.md all resolve.
…, docs)
All 10 review items verified against current code; every one applied.
Lint clean, 760 passed / 8 skipped, smoke verified for the three
behavioural changes (negative threshold rejection, --output SUPPRESS
fallback, deprecated --data-audit + top-level --output).
CLI (forgelm/cli.py)
--------------------
* `audit` subparser ``--output`` now uses ``argparse.SUPPRESS``. The
top-level ``--output`` (default=None) wins when the operator doesn't
pass it on the subcommand; ``_run_data_audit`` then falls back to
``./audit`` as before. Removes the silent-overwrite footgun where a
``forgelm --output ./tmp audit X`` invocation could be ignored by
the subparser default. ``_run_audit_cmd`` now reads via
``getattr(args, "output", None)``.
* ``--near-dup-threshold`` gains a `_non_negative_int` argparse type
helper that raises ``ArgumentTypeError`` on negative values; the
resulting "argument --near-dup-threshold: value must be ≥ 0" error
surfaces through argparse's standard exit path (code 2). Also
rejects non-numeric input cleanly.
* ``_run_ingest_cmd``'s except tuple now covers PermissionError,
IsADirectoryError, and OSError alongside FileNotFoundError /
ValueError. ImportError handler stays separate (EXIT_TRAINING_ERROR).
Comment documents that the OSError subclasses are listed explicitly
for reader visibility even though OSError catches them.
data_audit (forgelm/data_audit.py)
----------------------------------
* `_render_split_block` now surfaces JSONL hygiene counters
(``parse_errors``, ``decode_errors``, ``non_object_rows``) ahead of
body metrics. ``_split_has_findings`` already promoted splits with
these issues, but the renderer dropped them on the floor — operators
scrolling the multi-split summary now see the malformed-JSONL signal
at the top of the offending block instead of having to open the
on-disk JSON.
ingestion (forgelm/ingestion.py)
--------------------------------
* `_strip_repeating_page_lines` cutoff: ``int(_PDF_REPEAT_THRESHOLD *
N)`` → ``math.ceil(...)``. The 70 % rule was firing as ≤ 70 % on
page counts where the multiplication landed mid-integer (e.g., 5
pages × 0.7 = 3.5 floored to 3 = 60 %). math.ceil enforces the
documented "≥ 70 %" promise. Added ``import math``.
* `_try_pdf_decrypt` now decides on ``reader.decrypt("")``'s return
value (PasswordType IntEnum; NOT_DECRYPTED == 0 is falsy), not on
``reader.is_encrypted``. pypdf ≥ 4.0 keeps ``is_encrypted`` true
even after a successful decrypt, so the old check would falsely
reject correctly-decrypted owner-encrypted PDFs.
* `_read_pdf_pages` upgraded the swallowed page-extract error from
``logger.debug`` to ``logger.warning`` and now includes the page
index in the message. Decision: still tolerate (don't raise) page-
level failure — a 500-page PDF with one malformed page should
produce 499 good pages, not 0. The new warning makes the failure
visible in CI logs; rationale documented inline.
wizard (forgelm/wizard.py)
--------------------------
* `_offer_ingest_for_directory` now catches PermissionError,
IsADirectoryError, and OSError around the ingest_path call (in
addition to FileNotFoundError / ValueError / ImportError). Filesystem
errors (output path not writable, source tree locked, ENOSPC, broken
symlink during walk) were leaking through and crashing the wizard.
New handler prints a retry-friendly message and returns None so the
caller can re-prompt.
tests (tests/test_wizard_phase11_5.py)
--------------------------------------
* Drift assertion changed from ``set(...) == set(...)`` to verbatim
sequence equality (``wizard._INGEST_SUPPORTED_EXTENSIONS ==
ingestion.SUPPORTED_EXTENSIONS``). Now catches ordering drift and
accidental duplicates that set-comparison would silently accept.
docs (docs/guides/ingestion.md)
-------------------------------
* EN guide's "Install" paragraph now lists ``xxhash`` alongside the
other optional packages and notes it landed in v0.5.1. Restores
parity with the TR mirror that already had this; the wording about
the import not pulling extras until exercised is preserved.
Verification
------------
* ``ruff check`` clean (one auto-format on touched files).
* ``pytest tests/`` — 760 passed, 8 skipped (no test count change;
drift test now sequence-strict but tuple equality still holds).
* Smoke:
- ``forgelm audit --near-dup-threshold -5 path`` → argparse error,
exits 2.
- ``forgelm --data-audit path --output /tmp/x/`` (legacy) → writes
to /tmp/x/data_audit_report.json (top-level --output wins).
- ``forgelm audit path`` (no --output) → writes to ./audit/
(SUPPRESS-driven fallback through _run_data_audit).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
forgelm/cli.py (1)
313-333: Validate token-based numeric flags at parse-time as well.
--chunk-tokensand--overlap-tokenscurrently allow negatives and rely on downstreamingest_pathrejection. Parsing them with_non_negative_intwould fail earlier and stay consistent with--near-dup-threshold.Suggested refactor
p.add_argument( "--chunk-tokens", - type=int, + type=_non_negative_int, default=None, metavar="N", help=( @@ p.add_argument( "--overlap-tokens", - type=int, + type=_non_negative_int, default=0, metavar="N", help=(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/cli.py` around lines 313 - 333, The --chunk-tokens and --overlap-tokens argparse entries currently accept negative values and defer validation to ingest_path; update the parser to validate at parse-time by using the existing _non_negative_int type (the same validator used for --near-dup-threshold) for both p.add_argument calls for "--chunk-tokens" and "--overlap-tokens" so negatives are rejected early; keep the same defaults and metavar but ensure help text still notes that --overlap-tokens is ignored when --chunk-tokens is not set and adjust any references to ingest_path validation accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/roadmap/phase-12-data-curation-maturity.md`:
- Around line 62-66: The fenced code block starting with ``` in the
docs/roadmap/phase-12-data-curation-maturity.md should include a language tag to
satisfy markdownlint MD040; update that opening backtick fence (the ``` block
that contains the table) to include a language such as "text" (e.g., change ```
to ```text) so the table example is explicitly tagged.
---
Nitpick comments:
In `@forgelm/cli.py`:
- Around line 313-333: The --chunk-tokens and --overlap-tokens argparse entries
currently accept negative values and defer validation to ingest_path; update the
parser to validate at parse-time by using the existing _non_negative_int type
(the same validator used for --near-dup-threshold) for both p.add_argument calls
for "--chunk-tokens" and "--overlap-tokens" so negatives are rejected early;
keep the same defaults and metavar but ensure help text still notes that
--overlap-tokens is ignored when --chunk-tokens is not set and adjust any
references to ingest_path validation accordingly.
🪄 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: 8cde5d5d-197b-44ca-a087-fd337454e0a8
📒 Files selected for processing (14)
CHANGELOG.mddocs/guides/ingestion.mddocs/roadmap-tr.mddocs/roadmap.mddocs/roadmap/phase-10-5-quickstart.mddocs/roadmap/phase-11-5-backlog.mddocs/roadmap/phase-12-data-curation-maturity.mddocs/roadmap/phase-14-pipeline-chains.mddocs/roadmap/releases.mdforgelm/cli.pyforgelm/data_audit.pyforgelm/ingestion.pyforgelm/wizard.pytests/test_wizard_phase11_5.py
✅ Files skipped from review due to trivial changes (2)
- docs/roadmap/phase-10-5-quickstart.md
- docs/roadmap/phase-14-pipeline-chains.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_wizard_phase11_5.py
- forgelm/data_audit.py
…t validation) * `docs/roadmap/phase-12-data-curation-maturity.md`: the table-syntax example fence under task #5 ("DOCX / Markdown table preservation") was a bare ``` block — markdownlint MD040 flag. Added the ``text`` language tag so the markdown table illustration stays as plain text instead of being interpreted as a different language. * `forgelm/cli.py`: ``--chunk-tokens`` and ``--overlap-tokens`` now use the existing ``_non_negative_int`` argparse type (previously ``type=int`` accepted negatives and deferred the check to ``ingest_path``'s validation, which surfaced as a confusing EXIT_CONFIG_ERROR much later in the pipeline). Negatives now exit at parse-time with the same standard "argument --X: value must be ≥ 0" error already used by ``--near-dup-threshold``. Help text updated to document the new bound and to keep the existing paragraph-mode note about ``--overlap-tokens`` being ignored. Note: ``--chunk-tokens 0`` still passes argparse (non-negative is the cheap CLI check) and is rejected by ``ingest_path`` itself ("chunk_tokens must be positive") — keeping that validator there avoids two parallel definitions of "what counts as valid". Verification ------------ * ``forgelm ingest --chunk-tokens -10 ...`` → argparse error, exit 2. * ``forgelm ingest --overlap-tokens -5 ...`` → argparse error, exit 2. * ``ruff check`` clean. * ``pytest tests/`` — 760 passed, 8 skipped (unchanged).
|
6 tasks
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
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
6 tasks done
This was referenced May 8, 2026
cemililik
added a commit
that referenced
this pull request
Jun 15, 2026
…view) merge_pipeline_stage_config dumped stage model/lora/training/data/evaluation with exclude_none only; an override omitting staging_ttl_days re-materialised the default 7 as explicitly-set and falsely raised ConfigError against a canonical retention block. Match the root dump (exclude_unset=True). CodeRabbit #12. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5 tasks done
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
Phase 11.5 — operational polish on top of
v0.5.0's ingestion + audit surface. No new training capabilities; behaviour at default flags is identical. The wins are large-corpus throughput, ergonomics, and a cleaner CLI shape.All 12 follow-ups carved out of Phase 11's review backlog (phase-11-5-backlog.md) landed in one focused pass:
forgelm audit PATHis now a first-class subcommand (--verbose,--near-dup-threshold, dedicated--outputdefault). The legacyforgelm --data-audit FLAGis preserved as a deprecation alias and logs a one-line notice — existing CI pipelines need zero changes.O(n²)to ~O(n × k)using pigeonhole banding (bands = threshold + 1). Recall stays exact at the default Hamming threshold; brute-force fallback retained for edge thresholds where bands shrink below 4 bits. Same banded shape powers_count_leaked_rows._read_jsonl_split— the audit's JSONL reader is now a generator yielding(row, parse_err, decode_err);_audit_splitconsumes it row-by-row via a_StreamingAggregator. RAM stays bounded on multi-million-row splits.--chunk-tokens+--tokenizeronforgelm ingestsizes chunks against an HFAutoTokenizer.encodeinstead of raw character counts.trust_remote_code=Falsehard-pinned for safety.near_duplicate_pairsnoise on long policy / book PDFs.pii_severityblock (total,by_tier,by_type,worst_tier) alongside the flatpii_summary. Tier table follows PCI-DSS / GDPR Art. 9 weighting (credit_card/iban→ critical, national IDs → high,email→ medium,phone→ low).summarize_reporttruncation policy — defaultverbose=Falsefolds zero-finding splits into a single tail line;forgelm audit --verbosereverses it.IngestionResult.notes_structured: {key: value}for CI/CD consumers;extra_noteskeeps the prose. JSON output exposes both.forgelm ingestinline when the typed dataset path is a directory of raw documents.xxhash.xxh3_64digest path (added toforgelm[ingestion]) is several times faster than BLAKE2b on short keys; BLAKE2b stays as the bare-install fallback. Module-scopelru_cache(maxsize=10_000)memoises per-token digest (Zipfian token frequency makes this very effective).data_audit_report.jsonwritten viatempfile.NamedTemporaryFile+os.replaceso a crashed audit can never leave a half-written report on disk.newline="\n"pinned for byte-exact reproducibility.Module deltas
forgelm/data_audit.py:PII_SEVERITYtable,_token_digest(cached + xxhash optional), LSH-bandedfind_near_duplicates,_count_leaked_rowsrebuilt on the same banded index,_read_jsonl_splitpromoted to a generator,_audit_split(split_name, path, ...)rewritten as a streaming aggregator,_build_pii_severity,_atomic_write_json,summarize_report(verbose=...),_split_has_findings,_render_split_block,_render_pii_severity.forgelm/ingestion.py:_strip_repeating_page_lines,_extract_pdfthreads dedup state,_chunk_sliding_tokens,_chunk_paragraph_tokens,_strategy_dispatch_tokens,_load_tokenizer,IngestionResult.notes_structured+pdf_header_footer_lines_stripped,_process_one_fileextended for token-aware dispatch.forgelm/cli.py:_add_audit_subcommand,_run_audit_cmd, dispatcher hooksaudit;--data-auditreflows through_run_data_auditwithinvoked_via_legacy_flag=True.forgelm/wizard.py:_offer_ingest_for_directory+_prompt_dataset_path_with_ingest_offer; BYOD path and full-wizard Step 5 dataset prompt both use them.pyproject.toml: version bumped to0.5.1rc1;xxhashadded to the[ingestion]extra (optional — fallback path keeps bare installs OK).Behavioural / API deltas (additive unless noted)
AuditReportgains apii_severity: Dict[str, Any]field. JSON consumers reading onlypii_summarycontinue to work.find_near_duplicates(fingerprints, *, threshold, bits=64)accepts abitskeyword for adaptive banding._read_jsonl_splitis now a generator — old buffered tuple return is gone. Callers that were materialising rows can wrap withlist(...)._audit_split(split_name, path, ...)now takes a path instead of an in-memory list. Returns(info, fingerprints, pii_split, parse_errors, decode_errors)so OSError handling stays in the orchestrator.Documentation
Test plan
ruff format . && ruff check .— all checks passpytest tests/— 754 passed, 8 skipped (extras-skip for optional deps)tests/test_data_audit.py:TestLshBandedNearDuplicates(LSH parity vs. brute force + high-threshold fallback),TestPiiSeverity(critical-tier verdict + neutral case),TestSummarizeVerbosePolicy,TestAtomicWrite,TestStreamingReader,TestTokenCachePerformance.tests/test_ingestion.py:TestPdfHeaderFooterDedup(collapse + short-doc skip + no-repeats pass-through),TestStructuredIngestionNotes,TestTokenAwareCli.tests/test_cli_subcommands.py:TestAuditSubcommand(happy path, JSON envelope, legacy--data-auditalias parity).tests/test_wizard_byod.py: refreshed for ingest-first wording.forgelm --config config_template.yaml --dry-run→ DRY RUN COMPLETEforgelm audit <jsonl> --output <dir>→ writesdata_audit_report.json, summary showsPII severity / worst tier = criticalfor credit-card seedforgelm --data-audit <jsonl> --output <dir>(legacy) → identical on-disk product + deprecation notice in INFO logforgelm audit … --output-format json→ envelope containspii_severity+report_pathforgelm ingest <txt> --pii-mask --output-format json→notes_structuredpopulated,pdf_header_footer_lines_stripped: 0for non-PDFv0.5.1, build, publish) — handled separately after mergeNotes for reviewer
--data-auditflag and thepii_summaryfield stay around exactly as they were; new fields (pii_severity) and new subcommand are extras alongside them._read_jsonl_splitrewrite is the largest behavioural change in scope. It preserves the same per-line tolerance semantics (parse errors, decode errors, non-dict rows) but yields tuples instead of returning a list — see the migration notes in the CHANGELOG and theTestStreamingReadercases for exact contract.bands = threshold + 1) keeps recall exact at the defaultthreshold=3by pigeonhole. This is not an approximation; the brute-force fallback engages only whenbits // bands < 4(i.e. very wide thresholds where the index would degenerate anyway).--chunk-tokensdeliberately requires--tokenizer. We refuse to pick a default vocab because the resulting chunk count would silently differ per-model; surfacing the dependency keeps the operator in control.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
forgelm auditsubcommand (legacy--data-auditkept as deprecation alias); token-aware ingestion flags and an inline "ingest first" wizard flow; structured ingestion metadata and PDF header/footer dedup.Enhancements
Documentation & Tests