Repository navigation
feat(ingestion): Phase 15 Wave 1+2 — ingestion-pipeline reliability - #49
Merged
Merged
Conversation
CodeRabbit posted 11 inline comments across PR #44 (two batches, one per round-3/round-4 cycle). Seven were already resolved by earlier rounds; this round closes the three residuals plus one stale-doc finding CodeRabbit raised but the project's own `error-handling.md` already documented correctly. - `forgelm/cli/_training.py::_preflight_numpy_torch_abi` now wraps the call to `compute_numpy_torch_abi_status` in a `try/except` so any unexpected exception (e.g. corrupted torch install where `torch.__version__` raises `AttributeError`) converts into a structured `abi_preflight_crashed` JSON envelope rather than emitting a raw Python traceback that pre-empts the `--output-format json` contract. Two new regression tests in `tests/test_abi_check.py::TestPreflightHandlesProbeCrash` pin the exit code (`EXIT_TRAINING_ERROR` = 2, same class as the broken-ABI verdict so CI/CD doesn't need to distinguish "ABI bad" from "ABI probe died") and the envelope shape. - `CLAUDE.md` exit-code line corrected from `0/1/2/3/4` to `0/1/2/3/4/5` to match the canonical table in `docs/standards/error-handling.md` (Phase 22 / v0.5.5 added `EXIT_WIZARD_CANCELLED = 5`; CLAUDE.md was stale, not the code). - `docs/roadmap/completed-phases.md` Phase 12 Tier 1 status line no longer advertises a `[ingestion-secrets]` extra that was never published — only `[ingestion-pii-ml]` exists in `pyproject.toml`. Reworded as historical context ("originally planned as `[ingestion-secrets]` but deferred"). Verified: 1653 passed, 14 skipped, 0 failed under pytest-randomly shuffled order; ruff format + ruff check + all ten gauntlet doc guards green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brings in 17 commits from main, including: - Phase 15 (Ingestion Pipeline Reliability) roadmap entry + TR mirror, with full Wave 1 / Wave 2 / Wave 3 task breakdown - data_ingestion_architecture reference docs (EN + TR) - ingestion_playground.ipynb new notebook - notebook download-ZIP step + reliability sweep across notebooks - bilingual parity guard refinement CHANGELOG conflict resolved: development's round 5 review-absorption notes preserved under [Unreleased]; main's [0.5.7] — 2026-05-11 release-tag header inserted directly below. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the silent-failure gap the 2026-05-11 pilot exposed across
PDF / DOCX / EPUB / TXT / Markdown ingestion plus the user-facing
playground notebook. Targeted for v0.6.0.
## Wave 1 — silent-failure remediation
Task 1 — multi-line PDF edge dedup: window-based fix
`_strip_repeating_page_lines` now inspects the top-3 / bottom-3 rows
per page (configurable via `_PDF_EDGE_WINDOW`) instead of the strict
outermost row. Variable-outer-line + constant-deeper-line corpora
(the audit §1.1 trap) are dedup'd in a single pass. A second pass via
`strip_paragraph_packed_headers` mops up survivors after paragraph
packing.
Task 2 — Unicode / script-sanity layer
New `forgelm/_script_sanity.py` runs a per-file Unicode-block check
after every extract. Languages: tr / en / de / fr / es / it / pt with
discrete diacritic allow-lists (no broad Latin-1 range so the audit's
øÕú÷ artefacts cannot masquerade as in-script). Default threshold
1.5 %, calibrated against the audit's 7 % corruption ceiling.
Task 3 — Turkish pypdf glyph normalisation
New `forgelm/_pypdf_normalise.py` maps the audit-measured fallback
glyphs (`ø Õ ú ÷ `) back to `İ ı ş ğ •`. Profile dispatcher
(`--normalise-profile {turkish,none}`) leaves room for arabic / cjk
in later phases. `forgelm doctor` verifies the table loads via the
new `pypdf_normalise.turkish` diagnostic.
Task 4 — ingest-time quality pre-signal
Three cheap checks (alpha-ratio, weird-char-ratio, repeated-line
ratio) sampled across emitted chunks at end of run; emits
`[WARN] N/M chunks below ingestion quality threshold` + a
`quality_presignal` block in `notes_structured`. Opt out via
`--no-quality-presignal`.
Task 5 — audit `--quality-filter` default-ON
v0.6.0 inverts the audit-side default. New `--no-quality-filter`
companion flag preserves the pre-v0.6.0 opt-in semantics. Explicit
`--quality-filter` invocations remain byte-identical.
Task 6 — DOCX explicit header / footer subtraction
`_extract_docx` reads `doc.sections[i].header.paragraphs` +
`.footer.paragraphs` up front and subtracts those lines from the
body extraction.
Task 7 — EPUB spine order + nav / cover / copyright skip
`_extract_epub` walks `book.spine` (reading order) and filters items
whose file name or `epub:type` matches the default skip-list
(nav / cover / copyright / colophon / titlepage / frontmatter).
Opt out via `--epub-no-skip-frontmatter`.
Task 8 — TXT UTF-8 BOM strip + MD YAML frontmatter
TXT reads via `encoding="utf-8-sig"`. New `_extract_markdown`
strips `---\n…\n---\n` YAML frontmatter; opt-in retention via
`--keep-md-frontmatter`.
Task 9 — notebook UX alignment (`notebooks/ingestion_playground.ipynb`)
Cell 5 exposes `CHUNK_TOKENS` + `TOKENIZER` + `LANGUAGE_HINT`.
Cell 8 markdown documents the new quality-filter checks. Cell 9
passes `--quality-filter` explicitly for v0.5→v0.6 forward-compat.
Cell 10 pretty-prints `quality_summary` alongside PII / secrets /
near-duplicate / language sections.
Task 10 — regression fixture matrix
`tests/fixtures/ingestion/` carries TXT-BOM + MD-frontmatter +
mixed-directory fixtures. PDF / DOCX / EPUB fixtures synthesised
in-test via `pypdf.PdfWriter` / `python-docx` / `ebooklib` so the
suite is resilient against minor-version drift.
`tests/test_ingestion_reliability.py` carries 36 task-aligned
behaviour assertions.
## Wave 2 — strong nice-to-have
Task 11 — `--strip-pattern REGEX` with ReDoS guard
New `forgelm/_strip_pattern.py`. Patterns are structurally validated
up-front (rejects nested unbounded quantifiers `(a+)+b` and `.*?` +
back-reference under DOTALL per SonarCloud python:S5852). A 5-second
SIGALRM budget bounds the worst-case match cost on POSIX; opt out
via `--strip-pattern-no-timeout`. Invalid pattern → EXIT_CONFIG_ERROR.
Task 12 — `--page-range START-END` PDF flag
Slice extraction to a contiguous 1-indexed page range. New
`IngestParameterError(ValueError)` propagates through the per-file
soft-fail catch so the run aborts with EXIT_CONFIG_ERROR instead of
silently skipping the file.
Task 13 — front-matter / back-matter heuristic (default ON)
Three-signal heuristic (alpha < 0.45 + underscore > 0.10 + ≥ 5 inline
page-number matches) drops up to 12 leading + 12 trailing PDF pages.
Library default `keep_frontmatter=False` (heuristic on); CLI flag
`--keep-frontmatter` restores pre-Phase-15 behaviour. Dropped pages
surface via `frontmatter_pages_dropped` in `notes_structured`.
Task 14 — `--strip-urls {keep,mask,strip}` option
Bounded-character-class URL pattern (no ReDoS shape) applied AFTER
strip-patterns AFTER PII / secrets mask. Independent of
`--all-mask` (URL handling is a content-shape decision, not a GDPR
redaction).
Task 15 — multi-column PDF detection (warning only)
Samples first three pages' Tj-text positions via pypdf's
`visitor_text` callback. Fires a WARNING when the two-cluster gap
exceeds 30 % of page width. No fix attempt — operator switches
strategy or pre-processes.
## Schema additions (all additive — no pre-Phase-15 key renamed)
`IngestionResult` gains:
- `pdf_paragraph_packed_lines_stripped` (Task 1 second pass)
- `script_sanity_triggered` (Task 2)
- `strip_pattern_substitutions` (Task 11)
- `urls_handled` (Task 14)
- `frontmatter_pages_dropped` (Task 13)
`notes_structured` gains: `pdf_paragraph_packed_lines_stripped`,
`script_sanity_summary`, `frontmatter_pages_dropped`,
`strip_pattern_substitutions`, `urls_handled`, `quality_presignal`.
## Test results
ruff format / ruff check / all 7 CI guards: green
pytest tests/: 1728 passed, 14 skipped (optional extras), 0 failed
of which 75 new Phase 15 tests across 4 modules:
- tests/test_pypdf_normalise.py (16)
- tests/test_script_sanity.py (16)
- tests/test_strip_pattern.py (11)
- tests/test_ingestion_reliability.py (36 task-aligned tests)
## Documentation
CHANGELOG.md, docs/guides/ingestion.md + ingestion-tr.md,
docs/guides/data_audit.md + data_audit-tr.md,
docs/reference/data_ingestion_architecture.md + -tr.md,
docs/roadmap.md (Phase 15 row → 🚧 In review),
docs/roadmap/phase-15-ingestion-reliability.md
(Tasks 1-15 marked [x]).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
![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
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (15)
📝 WalkthroughWalkthroughPhase 15 implements ingestion pipeline reliability across Wave 1 (v0.6.0) and Wave 2 with language-aware glyph normalization, Unicode script-sanity validation, operator-controlled regex stripping with ReDoS/timeouts, widened PDF header/footer dedup plus a paragraph-packed second pass, DOCX/EPUB/TXT/MD extraction updates, an end-of-run quality pre-signal, and new CLI flags, tests, and docs. Changes thread a shared _ExtractContext through extraction to apply deterministic post-extract normalisations and structured reporting. ChangesPhase 15 Ingestion Reliability
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 2 minor |
| CodeStyle | 5 minor |
🟢 Metrics 452 complexity · 2 duplication
Metric Results Complexity 452 Duplication 2
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 implements Phase 15 of the Ingestion Pipeline Reliability project, introducing several robustness and quality-control features across PDF, DOCX, EPUB, TXT, and Markdown ingestion. Key additions include a script-sanity check, Turkish glyph normalization, operator-controlled regex stripping with ReDoS protection, and an end-of-run quality pre-signal. My review highlights a performance concern regarding the JSON decoding overhead in the quality sampling file handle and suggests refactoring the CLI profile resolution to use the library's defined default constant for better maintainability.
Comment on lines
+2266
to
+2295
| class _QualitySamplingFileHandle: | ||
| """Wrap ``out_fh`` so each written line also flows to a sampler callback. | ||
|
|
||
| Used by :func:`ingest_path` to feed the quality pre-signal without | ||
| threading an extra callback through ``_emit_chunk``. Only methods | ||
| the rest of the pipeline actually calls (``write`` / ``__enter__`` | ||
| / ``__exit__``) are proxied — keeping the wrapper shallow makes it | ||
| trivially obvious in code review that no I/O side-effect is hidden. | ||
| """ | ||
|
|
||
| __slots__ = ("_fh", "_sampler") | ||
|
|
||
| def __init__(self, fh: Any, sampler: Callable[[str], None]) -> None: | ||
| self._fh = fh | ||
| self._sampler = sampler | ||
|
|
||
| def write(self, payload: str) -> int: | ||
| # payload comes in as ``json.dumps({"text": ...}) + "\n"``; we | ||
| # extract the chunk text via a quick parse rather than splitting | ||
| # on the JSON shape so a future schema change can't silently | ||
| # corrupt the sampler. | ||
| try: | ||
| data = json.loads(payload.rstrip("\n")) | ||
| text = data.get("text", "") if isinstance(data, dict) else "" | ||
| except (json.JSONDecodeError, ValueError): | ||
| text = "" | ||
| if text: | ||
| self._sampler(text) | ||
| return self._fh.write(payload) | ||
|
|
There was a problem hiding this comment.
The _QualitySamplingFileHandle implementation introduces a performance overhead by decoding the JSON payload on every write call just to extract the text field for sampling. For corpora with a large number of small chunks, this repeated json.loads() operation can become a significant bottleneck, as noted in the pull request description.
A more efficient approach would be to perform the sampling before the chunk is serialized to JSON. This could be done by passing the sampling callback down to where the chunk text is available, for instance, within _process_one_file after a chunk is yielded from _select_chunks_iter and before it's passed to be masked and emitted. This would avoid the costly serialization-deserialization cycle for each chunk.
Collaborator
Author
There was a problem hiding this comment.
Resolved in commit 206f157. The _QualitySamplingFileHandle wrapper was removed entirely; _emit_chunk now takes a sampler: Optional[Callable[[str], None]] kwarg and the post-mask chunk text is handed straight to the pre-signal collector — no per-chunk json.loads round-trip.
Comment on lines
+42
to
+59
| def _resolve_normalise_profile(args) -> str: | ||
| """Resolve --normalise-profile vs --no-normalise-unicode into one knob. | ||
|
|
||
| Both flags are valid Phase 15 surface; ``--no-normalise-unicode`` is | ||
| the shortcut for ``--normalise-profile none`` and wins when both are | ||
| set (a future profile name does not silently re-enable normalisation | ||
| against the operator's intent). | ||
| """ | ||
| if getattr(args, "no_normalise_unicode", False): | ||
| return "none" | ||
| profile = getattr(args, "normalise_profile", None) | ||
| if profile is None: | ||
| # Library default (turkish) is the right v0.6.0 launch profile. | ||
| # We pass ``None`` through so library callers can introspect the | ||
| # default; the ingest_path keyword falls back to the library | ||
| # constant. | ||
| return "turkish" | ||
| return profile |
There was a problem hiding this comment.
The implementation of _resolve_normalise_profile hardcodes the default profile as "turkish", and the accompanying comment is a bit confusing as it mentions passing None through while the function actually returns a string.
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, it would be better to source the default value from the forgelm._pypdf_normalise module where DEFAULT_PROFILE is defined. This ensures that the CLI layer automatically respects any changes to the library's default.
def _resolve_normalise_profile(args) -> str:
"""Resolve --normalise-profile vs --no-normalise-unicode into one knob.
Both flags are valid Phase 15 surface; ``--no-normalise-unicode`` is
the shortcut for ``--normalise-profile none`` and wins when both are
set (a future profile name does not silently re-enable normalisation
against the operator's intent).
"""
if getattr(args, "no_normalise_unicode", False):
return "none"
from ..._pypdf_normalise import DEFAULT_PROFILE
profile = getattr(args, "normalise_profile", None)
return DEFAULT_PROFILE if profile is None else profile
Collaborator
Author
There was a problem hiding this comment.
Resolved in commit 206f157. _resolve_normalise_profile no longer hardcodes "turkish" as the default. The dispatcher now: (1) honours --no-normalise-unicode first; (2) honours explicit --normalise-profile X; (3) auto-derives turkish only when --language-hint tr is set; (4) falls back to none otherwise. Library ingest_path mirrors the same derivation so a Python-level caller without an explicit normalise_profile kwarg gets the same behaviour.
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/guides/ingestion.md (1)
253-256:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new Phase 15 JSON fields in this summary section.
forgelm ingest --output-format jsonnow also emitspdf_paragraph_packed_lines_stripped,script_sanity_triggered,strip_pattern_substitutions,urls_handled, andfrontmatter_pages_dropped, but this paragraph still describes only the Phase 11.5/12 keys. Operators using the JSON envelope will miss the expanded contract unless it is called out here and mirrored indocs/guides/ingestion-tr.md.As per coding guidelines, "User-facing documentation must be in English + Turkish as bilingual mirrors."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/ingestion.md` around lines 253 - 256, Update the JSON summary paragraph for `--output-format json` in the ingestion guide to list the new Phase 15 fields (`pdf_paragraph_packed_lines_stripped`, `script_sanity_triggered`, `strip_pattern_substitutions`, `urls_handled`, `frontmatter_pages_dropped`) in addition to the existing Phase 11.5/12 keys, and mirror the same expanded field list in the `docs/guides/ingestion-tr.md` Turkish version; ensure the English and Turkish sentences clearly identify these new keys and their brief meanings so the JSON contract is fully documented in both language files.docs/roadmap/phase-15-ingestion-reliability.md (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the English roadmap file fully English.
Line 3 starts with
Not:in Turkish inside an otherwise English roadmap doc. The repo rule is mirrored EN/TR docs, not mixed-language inline prose, so this should be translated here or moved to the Turkish mirror.As per coding guidelines, "User-facing documentation must be in English + Turkish as bilingual mirrors."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/roadmap/phase-15-ingestion-reliability.md` at line 3, The English roadmap contains a Turkish inline word "Not:" at the start of the sentence; replace that Turkish token with proper English ("Note:") or translate the entire sentence into English so the document remains fully English, or alternatively remove it here and add the Turkish original to the Turkish mirror document; locate the offending phrase starting with "Not:" in the roadmap phase-15 ingestion reliability doc and update the text to follow the bilingual mirror rule (English in the English file, Turkish in the Turkish file).
🧹 Nitpick comments (8)
tests/test_script_sanity.py (1)
95-112: ⚡ Quick winRemove the temporary handler after this assertion.
logging.getLogger("test.script_sanity")is process-global, so leavinghandlerattached can bleed state into later tests and duplicate warning records if the same logger name is reused. Atry/finallywithremoveHandler()keeps this test isolated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_script_sanity.py` around lines 95 - 112, The test test_logger_override_for_test_isolation attaches a temporary logging.Handler to custom_logger and never removes it; wrap the handler usage in a try/finally (or ensure cleanup after the assertion) and call custom_logger.removeHandler(handler) in the finally block to avoid leaking the handler into other tests—modify the code around the handler creation in test_logger_override_for_test_isolation to add try/finally and removeHandler(handler) after asserting on captured.forgelm/cli/subcommands/_ingest.py (2)
79-95: ⚡ Quick winReuse the shared strip-pattern timeout constant here.
forgelm._strip_pattern.DEFAULT_TIMEOUT_Sis already the documented and unit-tested default, but this dispatcher hardcodes another5. That creates a second source of truth, so a future timeout change can drift the CLI behavior from the module contract.♻️ Suggested change
- from ..._strip_pattern import StripPatternError + from ..._strip_pattern import DEFAULT_TIMEOUT_S, StripPatternError @@ - strip_pattern_timeout = None if getattr(args, "strip_pattern_no_timeout", False) else 5 + strip_pattern_timeout = None if getattr(args, "strip_pattern_no_timeout", False) else DEFAULT_TIMEOUT_S🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/cli/subcommands/_ingest.py` around lines 79 - 95, The code hardcodes strip_pattern_timeout = 5 which duplicates the documented default; change it to use the shared constant DEFAULT_TIMEOUT_S from the strip-pattern module instead (either reference forgelm._strip_pattern.DEFAULT_TIMEOUT_S or import DEFAULT_TIMEOUT_S at top) so that the value used by this dispatcher (the strip_pattern_timeout variable set after the _parse_page_range handling) is the same single source of truth as the module contract; update the import list if you add DEFAULT_TIMEOUT_S and ensure strip_pattern_timeout uses that symbol instead of the literal 5.
82-168: ⚡ Quick winExtract the repeated error-envelope/exit path before this dispatcher grows further.
The Phase 15 additions pushed
_run_ingest_cmd()into four near-identical “emit JSON-or-log, thensys.exit(...)” branches. Pulling that into a tiny helper would address the current complexity warning and make the next ingest flag much less likely to add another copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/cli/subcommands/_ingest.py` around lines 82 - 168, Extract the repeated "emit JSON-or-log, then sys.exit(...)" logic into a small helper (e.g. _emit_error_and_exit(error: Exception, exit_code: int, *, output_format: str, logger: logging.Logger, message: Optional[str]=None)) and call it from each except block instead of duplicating the print/logger/exit code sequences; update the StripPatternError handler to call the helper with message="Strip-pattern rejected: %s" and EXIT_CONFIG_ERROR, the generic ingest exceptions handler to call it with message="Ingest failed: %s" and EXIT_CONFIG_ERROR, and the OptionalDependencyError handler to call it with message="%s" and EXIT_TRAINING_ERROR so behavior for output_format, logging format strings, and exit codes (and exceptions like StripPatternError and OptionalDependencyError) remains identical but centralized.forgelm/ingestion.py (5)
2098-2098: 💤 Low valueSonarCloud nits worth folding in.
Two trivial cleanups flagged by SonarCloud that don't change behaviour:
- Line 2098: the conditional
[] if quality_presignal else []returns the same value either way (sampled_chunksis gated separately in_maybe_sample).- Line 2290:
json.JSONDecodeErroris a subclass ofValueError, so catching both is redundant.♻️ Proposed cleanups
- sampled_chunks: List[str] = [] if quality_presignal else [] + sampled_chunks: List[str] = []- try: - data = json.loads(payload.rstrip("\n")) - text = data.get("text", "") if isinstance(data, dict) else "" - except (json.JSONDecodeError, ValueError): - text = "" + try: + data = json.loads(payload.rstrip("\n")) + text = data.get("text", "") if isinstance(data, dict) else "" + except ValueError: # json.JSONDecodeError is a ValueError subclass + text = ""Also applies to: 2287-2294
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/ingestion.py` at line 2098, Replace the redundant conditional initializing sampled_chunks (currently "sampled_chunks: List[str] = [] if quality_presignal else []") with a simple empty list assignment ("sampled_chunks: List[str] = []") and remove the redundant exception catch for json.JSONDecodeError where both JSONDecodeError and ValueError are being caught (keep just ValueError or just JSONDecodeError depending on intended granularity) in the same block; locate these changes around the sampled_chunks initialization and the try/except that parses JSON (refer to the variable sampled_chunks and the JSON parsing/exception handling near the _maybe_sample/_ingestion JSON parsing logic) and update accordingly.
700-758: ⚡ Quick winMulti-column separation threshold (30 % page-width) appears too high to ever fire on standard 2-column layouts.
Real 2-column PDFs (papers, reports) on letter-sized pages typically use a gutter of 30–50 pt between columns — about 5–8 % of a 612 pt page width. With
_MULTI_COLUMN_SEPARATION_PCT = 0.30the gap would need to exceed ~184 pt for the warning to fire, which is well outside the modal layout. The accompanying test only exercises the single-column negative case (Line 713-727 in the test file), so the positive path is unverified.A threshold of 0.05–0.10 better matches the audit's intent of catching standard 2-column layouts. Worth confirming with a measured fixture before merging.
♻️ Suggested adjustment + a positive-case test
-_MULTI_COLUMN_SEPARATION_PCT: float = 0.30 -"""Fraction-of-page-width separation between text clusters that flags 2-column.""" +_MULTI_COLUMN_SEPARATION_PCT: float = 0.05 +"""Fraction-of-page-width separation between text clusters that flags 2-column. + +Standard 2-column layouts on letter-sized pages use ~30-50 pt gutters +(~5-8 % of a 612 pt page width); higher thresholds would never trigger +on realistic inputs. +"""Consider adding a positive-case test in
tests/test_ingestion_reliability.pythat synthesizes a PDF with two x-clusters and asserts the"2-column"warning fires — mirroring the existing single-column negative test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/ingestion.py` around lines 700 - 758, Lower the two-column detection threshold used by _maybe_warn_multi_column by changing _MULTI_COLUMN_SEPARATION_PCT from 0.30 to a more realistic value (e.g., 0.05–0.10) so standard gutters (~5–8% page width) trigger the warning; update the constant _MULTI_COLUMN_SEPARATION_PCT and leave the detection logic in _maybe_warn_multi_column unchanged, and add a positive-case test in tests/test_ingestion_reliability.py that synthesizes a PDF or mocked page with two distinct x_clusters (left vs right), calls _maybe_warn_multi_column (or runs the code path that invokes it) and asserts the logger warning fires / the function returns True to cover the positive path.
1110-1116: 💤 Low valueUnused
ctxassignment in_extract_text.The
ctx = _ExtractContext()fallback at Line 1113 is never read inside_extract_text— the body only readspath. The signature is preserved for parity with the other extractors, which is reasonable, but the dead assignment trips SonarCloud and slightly obscures intent. Adel ctx(or just removing the fallback init) makes the "signature parity only" intent explicit.♻️ Suggested cleanup
def _extract_text(path: Path, *, ctx: Optional[_ExtractContext] = None) -> str: """Extract plain TXT with UTF-8 BOM stripping (Phase 15 Task 8).""" - if ctx is None: - ctx = _ExtractContext() + del ctx # signature parity with other extractors; not used in this path yet raw = _read_text_with_bom_strip(path) _warn_if_binary_contamination(raw, path) return raw🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/ingestion.py` around lines 1110 - 1116, The function _extract_text assigns a fallback ctx = _ExtractContext() which is never used; remove the dead assignment (or explicitly del ctx) to make the "signature parity only" intent clear. Locate _extract_text and either delete the ctx fallback initialization on the first lines or add a del ctx immediately after assignment so only the unused parameter remains, leaving the calls to _read_text_with_bom_strip and _warn_if_binary_contamination unchanged.
803-803: 💤 Low valueMulti-column sampling ignores
page_range.
_maybe_warn_multi_column(reader, path)is invoked before page-range slicing and always samplesreader.pages[:3]. If the operator passes--page-range 50-60, the warning is computed against pages 1-3 (which may be the dropped front-matter), not the pages actually being extracted. Easy to threadpage_rangeinto the sampler so the warning reflects what the run will actually produce.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/ingestion.py` at line 803, The multi-column warning is computed against reader.pages[:3] before page-range slicing, so it can report on pages that will be dropped; change _maybe_warn_multi_column to accept the page_range (or an already-sliced pages list) and sample from the pages that will actually be processed (e.g., apply the same slicing logic used for extraction to reader.pages, then take the first few sampled pages) so the warning reflects the real run; update the call site where _maybe_warn_multi_column(reader, path) is invoked to pass page_range (or the sliced pages) and adjust the function signature and internals accordingly to use that passed-in range when selecting pages to sample.
1064-1070: ⚡ Quick win
item.media_overlaydoes not carryepub:type; useitem.propertiesinstead to properly match EPUB-3 manifest properties.
media_overlayis the SMIL media-overlay reference for synchronized read-aloud audio — unrelated toepub:type. In ebooklib (≥0.18), EPUB-3 manifest properties (cover-image,nav, …) live onitem.properties(a list of strings).Net effect today:
item_typeis essentially always empty, so the second branch of_epub_item_matches_skipnever matches. Filename substring matching catches the audit's canonical examples (cover.xhtml,nav.xhtml, etc.) by accident, but silently drops EPUB-3 items that declare properties without matching filenames.♻️ Suggested fix using
item.properties- item_name = getattr(item, "file_name", "") or getattr(item, "name", "") or "" - # ``epub:type`` lives on the manifest item under ``properties``; - # ebooklib stores it on ``item.media_type`` for some files and - # ``item.get_id`` for others. Best-effort probe. - item_type = getattr(item, "media_overlay", "") or "" - if ctx.epub_skip_frontmatter and _epub_item_matches_skip(item_name, item_type, ctx.epub_skip_items): + item_name = getattr(item, "file_name", "") or getattr(item, "name", "") or "" + # EPUB-3 manifest properties (``cover-image``, ``nav``, …) live + # on ``item.properties``. Match any of them against the skip-list. + item_props = getattr(item, "properties", None) or [] + if ctx.epub_skip_frontmatter and ( + _epub_item_matches_skip(item_name, "", ctx.epub_skip_items) + or any(prop.lower() in ctx.epub_skip_items for prop in item_props) + ): continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/ingestion.py` around lines 1064 - 1070, The code uses item.media_overlay (wrong) to determine EPUB manifest properties, so change the probe to read ebooklib's EPUB-3 manifest properties from item.properties (e.g., props = getattr(item, "properties", []) ) and set item_type to a representative string (for example a space-joined string of props) or otherwise pass that list into _epub_item_matches_skip; update the variable used in the existing check (item_type) so _epub_item_matches_skip can detect properties like "nav" or "cover-image" rather than always getting an empty value from media_overlay.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 139: The changelog footer still points [Unreleased] at the pre-0.5.6
range so released changes are included in the compare link and new releases lack
reference targets; update the footer so the [Unreleased] reference target
becomes "v0.5.7...HEAD" and add missing release reference links for "0.5.6" and
"0.5.7" (e.g., add lines for v0.5.6 and v0.5.7 compare URLs matching your repo's
tag/compare format), and ensure the heading "## [0.5.7] — 2026-05-11" matches
the new v0.5.7 reference name.
In `@docs/guides/data_audit.md`:
- Around line 499-503: The "Run it" example still labels `--quality-filter` as
opt-in despite docs switching it to default-ON; update the earlier example and
its Turkish mirror so `--quality-filter` is shown as the default-enabled flag
(remove any wording that says opt-in or suggests `--quality-filter` is off by
default) and adjust any `--no-quality-filter` explanation to indicate it opts
out of the default; locate the example text referencing
`--quality-filter`/`--no-quality-filter` near the top of the guide and the
corresponding Turkish section and make parallel edits so both languages state
the same default-on behavior and mention Phase 12 / Phase 15 Task 5 as the
change that made it default.
In `@docs/reference/data_ingestion_architecture-tr.md`:
- Around line 249-254: The bullet beginning with "**OCR**" currently links to
"../guides/ingestion.md"; update that link target to "../guides/ingestion-tr.md"
so the Turkish architecture page points to the Turkish guide mirror, i.e.,
replace the "../guides/ingestion.md" link string in the "**OCR**" bullet with
"../guides/ingestion-tr.md" to keep the localized docs self-consistent.
In `@docs/reference/data_ingestion_architecture.md`:
- Around line 225-236: The diagram above is outdated: update the mermaid diagram
to reflect the new edge-window algorithm (_PDF_EDGE_WINDOW = 3) that inspects
top-3/bottom-3 rows per page (Phase 15 Task 1) and also add the second-pass node
for header cleanup after paragraph packing (strip_paragraph_packed_headers)
which surfaces as pdf_paragraph_packed_lines_stripped in notes_structured;
ensure arrows show the windowed inspection replacing the single-line check and a
subsequent arrow from paragraph packing to the packed-header stripping step so
the flow matches the described behavior.
In `@docs/roadmap.md`:
- Line 17: The roadmap table uses a new status icon "🚧 In review" but the
legend below lacks that entry; update docs/roadmap.md by adding a legend line
that defines "🚧 In review" (e.g., "🚧 In review — changes landed on development
awaiting final review/QA") or change the table row to use an existing legend
icon (e.g., "🔁" or "🟢") to match the current legend; ensure you update the
legend text near the existing Released / Merged / Planned entries so the new
icon is documented.
In `@docs/roadmap/phase-15-ingestion-reliability.md`:
- Around line 68-80: The ordered list in the "phase-15-ingestion-reliability.md"
snippet uses 11.–15. which triggers markdownlint's ordered-list style rule;
change the local subsection numbering for those five entries (the blocks
beginning with "**`--strip-pattern REGEX`...**", "`--page-range` PDF flag",
"Front-matter heuristic detection", "URL / QR pattern auto-strip option",
"Multi-column PDF detection") from 11.–15. to 1.–5. so the rendered output is
identical but the linter passes; no other content changes are required.
In `@forgelm/_strip_pattern.py`:
- Around line 245-251: The function apply_strip_patterns currently treats 0 or
negative timeout_s the same as None and disables the alarm; instead validate the
public API by rejecting non-positive numeric timeouts: if timeout_s is None keep
the existing behavior (no timeout), but if timeout_s is provided ensure
timeout_s > 0 and raise a ValueError for 0 or negative values; apply the same
validation in the other related function that uses the same timeout/ use_alarm
logic (the second occurrence around the strip/guarded path), and update any code
paths that choose use_alarm to only disable it when timeout_s is exactly None.
- Around line 123-165: The scanner only detects nested unbounded quantifiers
(e.g. `(a+)+`) but not adjacent quantified groups (e.g. `(.+?)(.+)`); update
_check_unbounded_quantifier_sequence to also detect a closing group `)` followed
immediately by an opening group `(` where the left group ends with an unbounded
quantifier (use the existing _group_ends_with_unbounded_quantifier) and the
right group begins/contains an unbounded quantifier — add or call a new helper
(e.g. _group_starts_with_unbounded_quantifier) to scan the next group's body
(skipping escapes) for `+`/`*` (optionally with `?`) and raise the same
StripPatternError when both sides are unbounded.
In `@forgelm/cli/_parser.py`:
- Around line 312-320: The --script-sanity-threshold argument documents a
0.0–1.0 ratio but currently uses _non_negative_float which permits >1.0; modify
the parser to validate an upper bound of 1.0 by either extending
_non_negative_float to accept a max parameter or adding a small
wrapper/validator used for this flag that raises an argparse.ArgumentTypeError
when the value is <0.0 or >1.0; update the argument definition for
"--script-sanity-threshold" to use that bounded validator so the CLI contract
matches the documented range.
In `@notebooks/ingestion_playground.ipynb`:
- Around line 158-164: The current conditional silently falls back to
--chunk-size when only one of CHUNK_TOKENS or TOKENIZER is set; change the logic
around the CHUNK_TOKENS/TOKENIZER check to fail fast on partial configuration by
explicitly detecting the case where exactly one of CHUNK_TOKENS or TOKENIZER is
truthy and aborting with a clear error (raise an exception or call process
exit/log error) referencing CHUNK_TOKENS and TOKENIZER, otherwise proceed to add
both --chunk-tokens and --tokenizer when both are set or --chunk-size when both
are unset; update the handling around the existing conditional that builds parts
so the misconfiguration is not silently ignored.
---
Outside diff comments:
In `@docs/guides/ingestion.md`:
- Around line 253-256: Update the JSON summary paragraph for `--output-format
json` in the ingestion guide to list the new Phase 15 fields
(`pdf_paragraph_packed_lines_stripped`, `script_sanity_triggered`,
`strip_pattern_substitutions`, `urls_handled`, `frontmatter_pages_dropped`) in
addition to the existing Phase 11.5/12 keys, and mirror the same expanded field
list in the `docs/guides/ingestion-tr.md` Turkish version; ensure the English
and Turkish sentences clearly identify these new keys and their brief meanings
so the JSON contract is fully documented in both language files.
In `@docs/roadmap/phase-15-ingestion-reliability.md`:
- Line 3: The English roadmap contains a Turkish inline word "Not:" at the start
of the sentence; replace that Turkish token with proper English ("Note:") or
translate the entire sentence into English so the document remains fully
English, or alternatively remove it here and add the Turkish original to the
Turkish mirror document; locate the offending phrase starting with "Not:" in the
roadmap phase-15 ingestion reliability doc and update the text to follow the
bilingual mirror rule (English in the English file, Turkish in the Turkish
file).
---
Nitpick comments:
In `@forgelm/cli/subcommands/_ingest.py`:
- Around line 79-95: The code hardcodes strip_pattern_timeout = 5 which
duplicates the documented default; change it to use the shared constant
DEFAULT_TIMEOUT_S from the strip-pattern module instead (either reference
forgelm._strip_pattern.DEFAULT_TIMEOUT_S or import DEFAULT_TIMEOUT_S at top) so
that the value used by this dispatcher (the strip_pattern_timeout variable set
after the _parse_page_range handling) is the same single source of truth as the
module contract; update the import list if you add DEFAULT_TIMEOUT_S and ensure
strip_pattern_timeout uses that symbol instead of the literal 5.
- Around line 82-168: Extract the repeated "emit JSON-or-log, then
sys.exit(...)" logic into a small helper (e.g. _emit_error_and_exit(error:
Exception, exit_code: int, *, output_format: str, logger: logging.Logger,
message: Optional[str]=None)) and call it from each except block instead of
duplicating the print/logger/exit code sequences; update the StripPatternError
handler to call the helper with message="Strip-pattern rejected: %s" and
EXIT_CONFIG_ERROR, the generic ingest exceptions handler to call it with
message="Ingest failed: %s" and EXIT_CONFIG_ERROR, and the
OptionalDependencyError handler to call it with message="%s" and
EXIT_TRAINING_ERROR so behavior for output_format, logging format strings, and
exit codes (and exceptions like StripPatternError and OptionalDependencyError)
remains identical but centralized.
In `@forgelm/ingestion.py`:
- Line 2098: Replace the redundant conditional initializing sampled_chunks
(currently "sampled_chunks: List[str] = [] if quality_presignal else []") with a
simple empty list assignment ("sampled_chunks: List[str] = []") and remove the
redundant exception catch for json.JSONDecodeError where both JSONDecodeError
and ValueError are being caught (keep just ValueError or just JSONDecodeError
depending on intended granularity) in the same block; locate these changes
around the sampled_chunks initialization and the try/except that parses JSON
(refer to the variable sampled_chunks and the JSON parsing/exception handling
near the _maybe_sample/_ingestion JSON parsing logic) and update accordingly.
- Around line 700-758: Lower the two-column detection threshold used by
_maybe_warn_multi_column by changing _MULTI_COLUMN_SEPARATION_PCT from 0.30 to a
more realistic value (e.g., 0.05–0.10) so standard gutters (~5–8% page width)
trigger the warning; update the constant _MULTI_COLUMN_SEPARATION_PCT and leave
the detection logic in _maybe_warn_multi_column unchanged, and add a
positive-case test in tests/test_ingestion_reliability.py that synthesizes a PDF
or mocked page with two distinct x_clusters (left vs right), calls
_maybe_warn_multi_column (or runs the code path that invokes it) and asserts the
logger warning fires / the function returns True to cover the positive path.
- Around line 1110-1116: The function _extract_text assigns a fallback ctx =
_ExtractContext() which is never used; remove the dead assignment (or explicitly
del ctx) to make the "signature parity only" intent clear. Locate _extract_text
and either delete the ctx fallback initialization on the first lines or add a
del ctx immediately after assignment so only the unused parameter remains,
leaving the calls to _read_text_with_bom_strip and _warn_if_binary_contamination
unchanged.
- Line 803: The multi-column warning is computed against reader.pages[:3] before
page-range slicing, so it can report on pages that will be dropped; change
_maybe_warn_multi_column to accept the page_range (or an already-sliced pages
list) and sample from the pages that will actually be processed (e.g., apply the
same slicing logic used for extraction to reader.pages, then take the first few
sampled pages) so the warning reflects the real run; update the call site where
_maybe_warn_multi_column(reader, path) is invoked to pass page_range (or the
sliced pages) and adjust the function signature and internals accordingly to use
that passed-in range when selecting pages to sample.
- Around line 1064-1070: The code uses item.media_overlay (wrong) to determine
EPUB manifest properties, so change the probe to read ebooklib's EPUB-3 manifest
properties from item.properties (e.g., props = getattr(item, "properties", []) )
and set item_type to a representative string (for example a space-joined string
of props) or otherwise pass that list into _epub_item_matches_skip; update the
variable used in the existing check (item_type) so _epub_item_matches_skip can
detect properties like "nav" or "cover-image" rather than always getting an
empty value from media_overlay.
In `@tests/test_script_sanity.py`:
- Around line 95-112: The test test_logger_override_for_test_isolation attaches
a temporary logging.Handler to custom_logger and never removes it; wrap the
handler usage in a try/finally (or ensure cleanup after the assertion) and call
custom_logger.removeHandler(handler) in the finally block to avoid leaking the
handler into other tests—modify the code around the handler creation in
test_logger_override_for_test_isolation to add try/finally and
removeHandler(handler) after asserting on captured.
🪄 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: 30d09136-5318-4b0c-b6fe-13c2b8e3e823
📒 Files selected for processing (26)
CHANGELOG.mddocs/guides/data_audit-tr.mddocs/guides/data_audit.mddocs/guides/ingestion-tr.mddocs/guides/ingestion.mddocs/reference/data_ingestion_architecture-tr.mddocs/reference/data_ingestion_architecture.mddocs/roadmap.mddocs/roadmap/phase-15-ingestion-reliability.mdforgelm/_pypdf_normalise.pyforgelm/_script_sanity.pyforgelm/_strip_pattern.pyforgelm/cli/_parser.pyforgelm/cli/subcommands/_doctor.pyforgelm/cli/subcommands/_ingest.pyforgelm/ingestion.pynotebooks/ingestion_playground.ipynbtests/fixtures/ingestion/README.mdtests/fixtures/ingestion/md_with_frontmatter_and_html.mdtests/fixtures/ingestion/mixed_directory/sample.mdtests/fixtures/ingestion/mixed_directory/sample.txttests/fixtures/ingestion/txt_with_bom_crlf.txttests/test_ingestion_reliability.pytests/test_pypdf_normalise.pytests/test_script_sanity.pytests/test_strip_pattern.py
Comment on lines
+123
to
+165
| def _check_unbounded_quantifier_sequence(pattern: str) -> None: | ||
| """Reject patterns with two unbounded quantifiers in sequence. | ||
|
|
||
| The classic ``(a+)+`` ReDoS shape is detected by walking the | ||
| pattern as plain text and looking for ``+`` / ``*`` followed by | ||
| a closing group ``)`` followed by another ``+`` / ``*``. We | ||
| deliberately do **not** parse the regex AST — that is more work | ||
| than the structural check warrants, and CPython already protects | ||
| operators against the worst cases at run time via the SIGALRM | ||
| timeout. The heuristic catches the textbook shape and is happy | ||
| to miss exotic variants (the run-time timeout is the safety net). | ||
|
|
||
| Why no full-AST parse: regex.md rule 4 is about pattern *shape*, | ||
| not semantic equivalence, so a textual scan is sufficient. The | ||
| same approach is used inside SonarCloud's S5852 implementation | ||
| on the JVM side. | ||
| """ | ||
| # Walk the raw pattern, skipping escape sequences. Look for | ||
| # ``)`` immediately following a ``+`` or ``*`` quantifier, then a | ||
| # second ``+`` or ``*`` on the outer group — that is the | ||
| # ``(a+)+`` / ``(.+)+`` shape. | ||
| i = 0 | ||
| while i < len(pattern): | ||
| char = pattern[i] | ||
| if char == "\\": | ||
| i += 2 # skip the escaped pair | ||
| continue | ||
| if char in ("+", "*"): | ||
| # Look at the previous *non-escaped* character: if it's ``)`` and | ||
| # the group it closes ended on another ``+`` / ``*``, this is the | ||
| # textbook nested-unbounded shape. | ||
| prev_char = pattern[i - 1] if i > 0 else "" | ||
| if prev_char == ")": | ||
| # Find the matching opening ``(`` and walk back to see if the | ||
| # group itself ended on another unbounded quantifier. | ||
| if _group_ends_with_unbounded_quantifier(pattern, i - 1): | ||
| raise StripPatternError( | ||
| f"--strip-pattern {pattern!r} contains a nested unbounded " | ||
| "quantifier (e.g. `(a+)+`) which is a textbook ReDoS shape. " | ||
| "Rewrite with bounded repetition (e.g. `a{1,100}`) or split " | ||
| "across multiple narrower --strip-pattern flags." | ||
| ) | ||
| i += 1 |
Contributor
There was a problem hiding this comment.
Extend the structural guard to catch adjacent unbounded groups too.
This scanner only trips on nested shapes like (a+)+, but the module/docs also claim to reject adjacent quantified-group shapes such as (.+?)(.+). Right now those still pass validation and only get caught, if at all, by the much later runtime timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forgelm/_strip_pattern.py` around lines 123 - 165, The scanner only detects
nested unbounded quantifiers (e.g. `(a+)+`) but not adjacent quantified groups
(e.g. `(.+?)(.+)`); update _check_unbounded_quantifier_sequence to also detect a
closing group `)` followed immediately by an opening group `(` where the left
group ends with an unbounded quantifier (use the existing
_group_ends_with_unbounded_quantifier) and the right group begins/contains an
unbounded quantifier — add or call a new helper (e.g.
_group_starts_with_unbounded_quantifier) to scan the next group's body (skipping
escapes) for `+`/`*` (optionally with `?`) and raise the same StripPatternError
when both sides are unbounded.
Collaborator
Author
There was a problem hiding this comment.
Out of scope for round-2/3, intentionally deferred. The structural validator covers the textbook nested shape (a+)+ / (\\w+)+; adjacent quantified groups like (.+?)(.+) are a different ReDoS class better handled by the SIGALRM safety net (already documented in _strip_pattern.py's docstring). Adding the adjacent-groups detector risks false positives on benign tandem groups and would require its own test corpus. Tracked as a future iteration if production traffic shows the SIGALRM net firing on this shape.
Contributor
There was a problem hiding this comment.
@cemililik, understood — the distinction is well-reasoned. The nested-quantifier check ((a+)+) catches the highest-risk textbook shape at parse time, while the SIGALRM guard serves as the runtime safety net for adjacent-group patterns ((.+?)(.+)) where static detection would yield too many false positives on legitimate tandem groups. Deferring until production signal confirms the gap makes sense.
✏️ Learnings added
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 49
File: forgelm/_strip_pattern.py:0-0
Timestamp: 2026-05-11T16:47:34.386Z
Learning: In cemililik/ForgeLM `forgelm/_strip_pattern.py`, the `_check_unbounded_quantifier_sequence` structural validator intentionally only detects nested unbounded quantifiers (e.g. `(a+)+`, `(\w+)+`). Adjacent quantified groups like `(.+?)(.+)` are a different ReDoS class that is intentionally NOT detected statically because it risks false positives on benign tandem groups and would require its own test corpus. The SIGALRM-based per-pattern timeout (`DEFAULT_TIMEOUT_S = 5`) is the documented safety net for this class. Do not flag the absence of adjacent-group detection in `_check_unbounded_quantifier_sequence` as a bug; it is an intentional design boundary deferred to a future iteration.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 13
File: forgelm/data_audit.py:71-79
Timestamp: 2026-04-28T11:08:48.337Z
Learning: In this project (cemililik/ForgeLM), treat heavy/optional dependencies (e.g., datasketch, xxhash, unsloth, llama_cpp) as opt-in extras: use a lazy-import pattern instead of failing eagerly at module import time. Concretely: do a top-level `try/except ImportError` that sets a `_HAS_<DEP>` boolean and assigns the missing module name to `None`, then gate feature code behind a `_require_<dep>()` helper that raises `ImportError` with an install hint only when the feature is actually invoked. Do not use a convention where importing the module always fails due to optional extras; this is intentional to keep base-install users from being forced to install optional dependencies just to use other features.
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 19
File: forgelm/judge.py:213-240
Timestamp: 2026-04-30T17:54:35.571Z
Learning: In cemililik/ForgeLM, YAML-driven numeric config fields (e.g., `JudgeConfig.batch_size`) are already validated at config-parse time using the Pydantic constraints on the corresponding config dataclass (e.g., `Field(ge=1)`). During code review, avoid flagging internal helper code (such as batching logic in functions like `_generate_responses_batched` in `forgelm/judge.py`) for missing additional runtime checks for those same YAML-derived parameters, since the Pydantic validation is the authoritative boundary gate. Only add/require runtime validation when entering true public library-API boundaries (e.g., functions like `run_safety_evaluation`) where callers may bypass the YAML/Pydantic config parsing path.
Comment on lines
+312
to
+320
| "--script-sanity-threshold", | ||
| type=_non_negative_float, | ||
| default=None, | ||
| metavar="X", | ||
| help=( | ||
| "Phase 15 Wave 1: ratio (0.0-1.0) above which the script-sanity check warns " | ||
| "(default: library calibrated 0.015 = 1.5%%). Operators on noisy mixed-script " | ||
| "corpora can relax this; values < 1%% start false-positiving on legitimate " | ||
| "English-with-code-samples Turkish corpora." |
Contributor
There was a problem hiding this comment.
Enforce the documented upper bound for --script-sanity-threshold
Line 317 documents a 0.0-1.0 ratio, but _non_negative_float currently accepts values above 1.0. That creates a CLI contract mismatch.
Proposed fix
+ def _ratio_0_to_1(value: str) -> float:
+ v = _non_negative_float(value)
+ if v > 1.0:
+ raise argparse.ArgumentTypeError("must be in [0.0, 1.0]")
+ return v
+
p.add_argument(
"--script-sanity-threshold",
- type=_non_negative_float,
+ type=_ratio_0_to_1,
default=None,
metavar="X",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forgelm/cli/_parser.py` around lines 312 - 320, The --script-sanity-threshold
argument documents a 0.0–1.0 ratio but currently uses _non_negative_float which
permits >1.0; modify the parser to validate an upper bound of 1.0 by either
extending _non_negative_float to accept a max parameter or adding a small
wrapper/validator used for this flag that raises an argparse.ArgumentTypeError
when the value is <0.0 or >1.0; update the argument definition for
"--script-sanity-threshold" to use that bounded validator so the CLI contract
matches the documented range.
Collaborator
Author
There was a problem hiding this comment.
False positive. --script-sanity-threshold already uses _non_negative_float (forgelm/cli/_argparse_types.py:54) which raises ArgumentTypeError for values outside [0.0, 1.0]. The documented range and the validator already agree — no change required.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the implementation of _non_negative_float in _argparse_types.py
cat forgelm/cli/_argparse_types.py
Length of output: 3025
@cemililik, you're absolutely right — my review comment was a false positive. Looking at the actual implementation in forgelm/cli/_argparse_types.py:
if fvalue < 0.0 or fvalue > 1.0:
raise argparse.ArgumentTypeError(f"value must be in [0.0, 1.0], got {fvalue}")_non_negative_float already enforces the full [0.0, 1.0] range on both ends. The name is slightly misleading (it implies only a lower-bound check), but the implementation correctly matches the documented contract for --script-sanity-threshold. No change is needed.
✏️ Learnings added
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 49
File: forgelm/cli/_parser.py:312-320
Timestamp: 2026-05-11T16:51:25.790Z
Learning: In cemililik/ForgeLM, `_non_negative_float` in `forgelm/cli/_argparse_types.py` enforces a full `[0.0, 1.0]` range (both lower AND upper bounds), not just a non-negative check. Despite the name suggesting only a lower-bound guard, the implementation raises `ArgumentTypeError` for any value > 1.0. Do not flag `--script-sanity-threshold` or other flags using `_non_negative_float` as lacking an upper-bound validator.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 13
File: forgelm/data_audit.py:71-79
Timestamp: 2026-04-28T11:08:48.337Z
Learning: In this project (cemililik/ForgeLM), treat heavy/optional dependencies (e.g., datasketch, xxhash, unsloth, llama_cpp) as opt-in extras: use a lazy-import pattern instead of failing eagerly at module import time. Concretely: do a top-level `try/except ImportError` that sets a `_HAS_<DEP>` boolean and assigns the missing module name to `None`, then gate feature code behind a `_require_<dep>()` helper that raises `ImportError` with an install hint only when the feature is actually invoked. Do not use a convention where importing the module always fails due to optional extras; this is intentional to keep base-install users from being forced to install optional dependencies just to use other features.
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 19
File: forgelm/judge.py:213-240
Timestamp: 2026-04-30T17:54:35.571Z
Learning: In cemililik/ForgeLM, YAML-driven numeric config fields (e.g., `JudgeConfig.batch_size`) are already validated at config-parse time using the Pydantic constraints on the corresponding config dataclass (e.g., `Field(ge=1)`). During code review, avoid flagging internal helper code (such as batching logic in functions like `_generate_responses_batched` in `forgelm/judge.py`) for missing additional runtime checks for those same YAML-derived parameters, since the Pydantic validation is the authoritative boundary gate. Only add/require runtime validation when entering true public library-API boundaries (e.g., functions like `run_safety_evaluation`) where callers may bypass the YAML/Pydantic config parsing path.
…bit + Sonar Absorbs every actionable finding from the round-2 review on PR #49 (and the Sonar / Codacy bots). Addresses 3 critical, 5 strong, 7 minor, plus two additional usermanuals chapters that the round-2 audit flagged as stale. ## Critical fixes - **C-1 EPUB skip-list substring trap (`forgelm/ingestion.py::_epub_item_matches_skip`)** The pre-round-2 implementation used a plain substring match against the lowercased filename, so `recovery.xhtml` / `navy.xhtml` / `discovery_chapter.xhtml` got silently skipped because `cover` ⊂ `recovery` and `nav` ⊂ `navy`. Replaced with a **whole-token** match using a path/extension/separator splitter, plus a WARNING per ingest naming the skipped items so silent drops never re-appear. - **C-2 default normalise_profile silently rewrote non-Turkish text (`forgelm/_pypdf_normalise.py::DEFAULT_PROFILE`)** Default flipped from `"turkish"` → `"none"`. CLI dispatcher (`_resolve_normalise_profile`) and library `ingest_path` both auto-derive `"turkish"` from `--language-hint tr` / `language_hint="tr"` while every other hint and the unset case stays on `"none"`. An INFO-log fires whenever substitutions actually fire so an operator who passed the wrong hint sees the count of rewrites. - **C-3 `tests/test_ingestion_reliability.py` cited gitignored docs/analysis/** The module docstring referenced the audit's path under `docs/analysis/code_reviews/…`, which violates `tools/check_no_analysis_refs.py` and blocks v0.6.0 tag. Reworded to point at the public roadmap instead. ## Strong recommendations - **S-1 ReDoS guard rewritten with forward-walk + stack** Pre-round-2 the validator walked the pattern backwards and consumed the matching `(` past an escape, so `(\w+)+x`, `(\d+)+x`, `(\s+)+x`, `^(\w+\s+)+x`, `([abc]+)+d` slipped through. The rewrite tracks per-group "last atom was unbounded" forward atom-by-atom — escape sequences and character classes count as one atom each — and rejects when a group close is immediately followed by `+`/`*` AND the inner last atom was itself unbounded. Five new parametrised test cases pin the trap closed. - **S-2 `_read_text_with_bom_strip` Latin-1 fallback leaked BOM** Fallback now uses `encoding="utf-8-sig"` plus an explicit leading `` strip so a file mixing a BOM with downstream non-UTF-8 bytes no longer leaks the BOM into chunk 0. - **S-3 `_QualitySamplingFileHandle` JSON round-trip eliminated** Replaced the file-handle wrapper with a `sampler` callback threaded through `_emit_chunk`; the quality-presignal collector now receives the post-mask chunk text straight from the chunker, no per-chunk `json.loads` round-trip. - **S-4 DOCX body subtraction frequency-bounded** `_extract_docx` only strips a body line that matches the header / footer set when the line is ≤ 80 chars long, so a legitimate body paragraph that happens to begin with `Chapter 1` is no longer dropped along with the running header. - **S-5 URL pattern boundedness** Dropped the `{1,2048}` upper bound (which silently truncated 3-KB URLs leaving partial residue in `--strip-urls=strip` mode) and dropped the duplicate `a-z` from `[A-Za-z…]` under IGNORECASE (Sonar python:S5869). Bounded character class keeps the engine linear without the truncation footgun. ## Minor nits - **N-2** `--page-range` directory-mode caveat documented inline. - **N-3** New `tests/test_doctor.py::TestPypdfNormaliseDoctorProbe` with pass + fail-mode coverage for the Task 3 probe. - **N-4** Defensive-break comment reworded. - **N-5** `epub_skip_items` docstring/type drift fixed. - **N-7** `_FRONTMATTER_PAGE_NUM_PATTERN` LF assumption documented. - **Sonar S107 / S1854 / S5713 / S117 / S3923 / S1244** addressed (param-count noted, dead `ctx` assignment removed, redundant `if … else []` collapsed, pypdf-API CamelCase shim, float-equality comparisons switched to `pytest.approx`, logger-handler teardown in `try/finally`). - **Notebook fail-fast on partial token mode** — Cell 5 raises `ValueError` when exactly one of `CHUNK_TOKENS` / `TOKENIZER` is set (Sonar ipython:S5727 / S5655 + Gemini finding). ## Doc fixes (round-2 inline review) - `CHANGELOG.md` compare-link footer refreshed for `[Unreleased]` → `v0.5.7...HEAD` plus `0.5.6` / `0.5.7` reference targets. - `docs/guides/data_audit.md` + `-tr.md` clarify `--quality-filter` default-ON in v0.6.0 and document the new `--no-quality-filter` companion. - `docs/reference/data_ingestion_architecture.md` + `-tr.md` mermaid diagram refreshed to show the `_PDF_EDGE_WINDOW = 3` window pass plus the `strip_paragraph_packed_headers` second pass; TR mirror's OCR-recipe link now points at the Turkish guide. - `docs/roadmap.md` legend gains the `🚧 In review` entry. - `docs/roadmap/phase-15-ingestion-reliability.md` ordered lists renumbered (11-15 → 1-5, 16-21 → 1-6) for markdownlint MD029; the inline "Not:" Turkish token replaced with "Note:" so the bilingual mirror rule holds. - `docs/guides/ingestion.md` + `-tr.md` JSON envelope section now lists every Phase 15 additive key (`pdf_paragraph_packed_lines_stripped`, `script_sanity_triggered`, `strip_pattern_substitutions`, `urls_handled`, `frontmatter_pages_dropped`). ## Usermanual updates (gap audit) - `docs/usermanuals/{en,tr}/data/audit.md` — `--quality-filter` default-on note + `--no-quality-filter` companion entry. - `docs/usermanuals/{en,tr}/data/ingestion.md` — new "Phase 15 (v0.6.0) additions" subsection covering the 12 new flags. - `docs/usermanuals/{en,tr}/data/quality-filter.md` — example block shows both implicit-default and explicit opt-out forms. - `docs/usermanuals/{en,tr}/reference/cli.md` — `forgelm ingest` usage line expanded with the Phase 15 flag set. - `docs/usermanuals/{en,tr}/reference/json-output.md` — new `forgelm ingest` subsection documenting the additive `notes_structured` schema; audit subsection's `quality_filter: null` placeholder refreshed to the populated `quality_summary` shape. (Site / homepage version bumps are deferred to v0.6.0 release time per the audit; the marketing surface avoids CLI-flag minutiae by design.) ## Test results - ruff format + ruff check + 6 CI guards (bilingual parity, anchor resolution, CLI help consistency, no-analysis-refs, no-unguarded- sys-modules-pop, wizard-defaults sync) — all green. - pytest tests/ — **1752 passed, 14 skipped, 0 failed**. - New parametrised regression coverage: * 5 escape-shape ReDoS patterns (S-1). * 8 EPUB substring traps (C-1). * 4 normalise auto-derive cases (C-2). * 3 timeout-validation cases (S-1 follow-up). * Doctor probe pass + fail paths (N-3). * Logger-handler leak teardown (round-2 nit). * Multi-column positive-case via mocked visitor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
Round-2 review absorbed — commit
|
| # | Status | Fix |
|---|---|---|
C-1 EPUB skip-list substring trap (recovery.xhtml, navy.xhtml silently dropped) |
✅ Fixed | _epub_item_matches_skip artık path/extension/separator splitter ile whole-token match yapıyor; bir de skip per-ingest WARNING düşürüyor (silent fail kapanıyor) |
C-2 Default normalise_profile="turkish" non-Turkish text'i sessizce yeniden yazıyor |
✅ Fixed | DEFAULT_PROFILE → "none". CLI dispatcher VE library ingest_path her ikisi de language_hint=="tr" ise turkish derive ediyor, aksi halde none. Substitution gerçekten tetiklendiğinde INFO log düşüyor |
C-3 tests/test_ingestion_reliability.py docs/analysis/ referansı (CI gauntlet blocker) |
✅ Fixed | Docstring public roadmap'ı işaret edecek şekilde yeniden yazıldı; check_no_analysis_refs.py yeşil |
🟠 Strong recommendations
| # | Status | Fix |
|---|---|---|
S-1 _check_unbounded_quantifier_sequence backward-walk \w+/\d+/\s+ şekillerini kaçırıyor |
✅ Fixed | Forward-walk + stack tabanlı atom-by-atom yeniden yazıldı; 5 escape-shape pattern artık reddediliyor. Ek apply_strip_patterns non-positive timeout_s → ValueError |
S-2 _read_text_with_bom_strip Latin-1 fallback BOM sızdırıyor |
✅ Fixed | Fallback'te de utf-8-sig + explicit strip |
S-3 _QualitySamplingFileHandle JSON round-trip israfı |
✅ Fixed | Wrapper kaldırıldı; sampler callback _emit_chunk'a thread edildi, JSON parse yok |
S-4 DOCX Chapter 1 body line'ları header set ile çakışınca düşüyor |
✅ Fixed | Sadece ≤ 80 char satırlar header-set'e karşı strip ediliyor |
S-5 URL pattern {1,2048} 3 KB URL'i kesik bırakıyor |
✅ Fixed | Upper bound kaldırıldı; character class zaten bounded olduğundan ReDoS riski yok. IGNORECASE altındaki A-Za-z duplicate (Sonar S5869) da çözüldü |
🟡 Minor nits
| Nit | Status |
|---|---|
N-1 "ö " → "Ğ" Turkish edge case |
⏸️ Deferred (low-risk, real Turkish text rarely has compound ending in ö-space; documented in module docstring) |
N-2 --page-range directory-mode doc-string note |
✅ Added inline note about per-file abort behaviour |
| N-3 Doctor probe dedicated unit test | ✅ Added TestPypdfNormaliseDoctorProbe with pass + fail-mode coverage |
| N-4 Defensive break dead-code comment | ✅ Reworded |
N-5 epub_skip_items docstring/type drift |
✅ Fixed (frozenset claim → tuple) |
| N-6 Markdown frontmatter regex comment | ✅ Kept; already explicit about "non-greedy by design" |
N-7 _FRONTMATTER_PAGE_NUM_PATTERN LF assumption |
✅ Documented inline |
Inline review (CodeRabbit / Gemini)
| Finding | Status |
|---|---|
CHANGELOG compare-link footer ([Unreleased] → v0.5.7…HEAD) |
✅ Fixed; 0.5.6/0.5.7 links added |
| data_audit.md "Run it" example default-on wording (both EN + TR) | ✅ Fixed |
| TR architecture OCR link → ingestion-tr.md | ✅ Fixed |
| Architecture mermaid diagram outdated | ✅ Both EN + TR mermaid diagrams rewritten for _PDF_EDGE_WINDOW = 3 + second pass |
Roadmap legend missing 🚧 In review |
✅ Added |
| Phase-15 roadmap ordered list 11–15 → 1–5 (markdownlint MD029) | ✅ Renumbered; Wave 3 16–21 → 1–6 too. Also Not: → Note: (bilingual mirror rule) |
_strip_pattern.py non-positive timeout rejection |
✅ Raises ValueError |
_strip_pattern.py adjacent quantified groups |
⏸️ Out of scope — forward-walk fix already catches the nested-quantifier shape; adjacent groups (.+?)(.+) are a different ReDoS class better handled by the SIGALRM safety net (annotated in docstring) |
--script-sanity-threshold upper bound |
❌ False positive — flag already uses _non_negative_float which enforces [0.0, 1.0] (see forgelm/cli/_argparse_types.py:54) |
| Notebook fail-fast on partial CHUNK_TOKENS/TOKENIZER | ✅ Cell 5 raises ValueError on partial config |
| ingestion.md JSON summary new keys (EN + TR) | ✅ Fixed |
_run_ingest_cmd cognitive complexity (Sonar S3776) |
✅ Reduced via _emit_error_and_exit helper; DEFAULT_TIMEOUT_S import replaces hardcoded 5 |
forgelm/ingestion.py:2098 redundant [] if x else [] (Sonar S3923) |
✅ Collapsed |
_extract_text dead ctx (Sonar S1854) |
✅ del ctx with comment for signature-parity intent |
| URL char-class duplicates (Sonar S5869) | ✅ Dropped a-z under IGNORECASE |
| Multi-column threshold lower + sample sliced pages | ⚖️ Kept 0.30 (lowering caused single-column false-positive because pypdf's visitor_text reports per-glyph x-offsets, not per-line); added page-range slicing on the sample. A bimodal-histogram detector is the right fix for sub-0.30 thresholds — tracked as Wave 3 follow-up in the phase-15 roadmap. Positive-case test added via mocked visitor |
item.media_overlay → item.properties (EPUB:type) |
✅ Fixed |
tests/test_script_sanity.py logger handler leak |
✅ try/finally + removeHandler |
| Float equality in tests (Sonar S1244) | ✅ pytest.approx |
Usermanual gap audit (separate from PR review — proactive)
Phase 15 sürecinde docs/usermanuals/ ve site/ dizinleri için ek bir audit çalıştırıldı. Bulgular ve aksiyonlar:
| Surface | Status |
|---|---|
docs/usermanuals/{en,tr}/data/audit.md — --quality-filter default-on |
✅ Updated |
docs/usermanuals/{en,tr}/data/ingestion.md — Phase 15 flags (12 yeni bayrak) |
✅ "Phase 15 (v0.6.0) additions" subsection eklendi |
docs/usermanuals/{en,tr}/data/quality-filter.md — opt-in vs default-on example |
✅ Both forms shown |
docs/usermanuals/{en,tr}/reference/cli.md — forgelm ingest flag listing |
✅ Updated |
docs/usermanuals/{en,tr}/reference/json-output.md — Phase 15 schema keys |
✅ Yeni forgelm ingest section + audit envelope refresh |
site/*.html version strings (v0.5.5) |
⏸️ Deferred — release-time housekeeping at v0.6.0 tag |
site/features.html / site/index.html Phase 15 claims |
✅ No stale CLI claims (site avoids flag-minutiae by design) |
site/i18n/*.json |
✅ Already parallel; no Phase-15-specific marketing copy |
Test summary
ruff check . ........................ PASS
6 CI guards .......................... PASS
pytest tests/ ........................ 1752 passed, 14 skipped, 0 failed
- +5 ReDoS escape-shape patterns
- +8 EPUB whole-token substring traps
- +4 normalise auto-derive cases
- +3 timeout-validation cases
- +2 doctor probe paths (pass + fail)
- +1 multi-column positive-case (mocked visitor)
- +1 logger-handler teardown
Push'landı: 206f157 (development → main). CI'da test (3.13) GitHub Actions altyapı flake olduğu için retry'a düşebilir; diğer 5 job zaten Phase 15 round-1'de yeşildi.
🤖 Generated with Claude Code
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
forgelm/ingestion.py (1)
2040-2066: ⚖️ Poor tradeoffNote:
ingest_pathis now at 23 parameters (Sonar > 13 allowed) and crossing the maintainability cliff.Phase 15 added 11 new kwargs to a function that already had 12. The Phase 15 additions are cohesive ("ingestion knobs"), so the most surgical move is a small frozen-dataclass container (e.g.,
IngestionOptions) for the post-extraction normalisation flags (language_hint,script_sanity_threshold,normalise_profile,keep_md_frontmatter,epub_skip_frontmatter,keep_frontmatter,page_range,strip_patterns,strip_pattern_timeout,strip_urls,quality_presignal) while keeping the chunking knobs at the top level. Library callers passing kwargs continue to work via a thin keyword-only forwarder; the public surface stays additive.Deferable — flagging now so the v0.7.0 cycle can plan the contract change rather than discovering the parameter count blocking the next wave of additions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/ingestion.py` around lines 2040 - 2066, The function ingest_path currently has too many kwargs; create a frozen dataclass IngestionOptions containing the post-extraction normalisation flags (language_hint, script_sanity_threshold, normalise_profile, keep_md_frontmatter, epub_skip_frontmatter, keep_frontmatter, page_range, strip_patterns, strip_pattern_timeout, strip_urls, quality_presignal), keep the chunking knobs (chunk_size, overlap, chunk_tokens, overlap_tokens, tokenizer, etc.) at the top level, and change ingest_path to accept either an ingestion_options: Optional[IngestionOptions] or the old individual kwargs for backward compatibility by converting any provided normalisation kwargs into an IngestionOptions instance inside the function; update all internal uses to read from the IngestionOptions instance and leave public callers working via a thin keyword-only forwarder.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/guides/ingestion-tr.md`:
- Around line 324-328: Update the documentation text for the --normalise-profile
option to reflect that the default profile is now "none" (not "turkish") and
that the "turkish" profile is only applied automatically when --language-hint tr
is provided; also clarify that users can still enable turkish explicitly via
--normalise-profile turkish and that --no-normalise-unicode (or
--normalise-profile none) disables normalization entirely. Ensure the updated
sentence references the exact option names --normalise-profile, --language-hint
tr, and --no-normalise-unicode to make the behavior clear.
In `@docs/usermanuals/en/data/quality-filter.md`:
- Around line 45-47: The example output incorrectly shows rows being dropped and
a clean file written even though the text states dropping only occurs when the
flags audit.quality_filter.drop_flagged or write_clean_output are enabled;
update the snippet to either (a) show the non-dropping default output for the
plain CLI invocation `forgelm audit data/ingested.jsonl` (e.g., remove the
"dropped" line and "wrote data/clean.jsonl" line) or (b) include the explicit
config context (or command-line flags) that enable dropping (reference the
settings audit.quality_filter.drop_flagged and write_clean_output) so the output
matches the described behavior.
In `@docs/usermanuals/en/reference/cli.md`:
- Around line 111-124: The CLI reference for the ingest command is missing the
machine-readable output flag; update the forgelm ingest synopsis to include the
--output-format {text,json} option so users can discover the JSON envelope;
locate the ingest synopsis block in docs/usermanuals/en/reference/cli.md and add
the --output-format {text,json} flag alongside the other flags (near
--strip-urls or similar) and ensure it's documented in the Phase 15 flags list.
In `@docs/usermanuals/tr/data/quality-filter.md`:
- Around line 45-53: The shell/code block in the TR manual contains translated
CLI comments and output (e.g., the lines starting with "# v0.6.0+:
quality-filter DEFAULT-AÇIK; açık flag zararsız.", the example command "forgelm
audit data/ingested.jsonl" and its output "✓ 45 satır düşürüldü;
data/clean.jsonl yazıldı (12,355 satır)") which must remain English-only; revert
those comment and output lines back to their original English phrasing while
keeping the surrounding explanatory prose in Turkish. Locate the code block
content showing the example commands and outputs (the CLI snippet with the
--quality-filter and --no-quality-filter examples) and replace only the
translated comment/output text with the English original so CLI, logs, and
config keys remain English.
In `@forgelm/_strip_pattern.py`:
- Around line 350-371: The inner alarm currently unconditionally calls
signal.alarm(timeout_s) which can extend an outer caller's shorter alarm; change
the logic so the inner alarm is set to the minimum of timeout_s and any existing
outer remaining budget: read the existing remaining (previous_remaining),
compute chosen = timeout_s if previous_remaining == 0 else min(timeout_s,
previous_remaining), and call signal.alarm(chosen) (keep previous_remaining
available for restore in the finally block and update the restore logic to use
that original value); update the code around previous_handler,
previous_remaining, and the signal.alarm call to implement this.
In `@forgelm/ingestion.py`:
- Around line 663-683: The front-matter detector currently only measures
underscore leaders via underscore_ratio = text.count("_") / len(text) while the
docstring and real PDFs include period/dot leaders; update the logic in the
function that computes front-matter (the block using alpha_ratio,
underscore_ratio, page_num_hits and constants like _FRONTMATTER_ALPHA_RATIO_MAX)
to: (1) compute leader counts over non-whitespace characters (use len(non_ws) as
the denominator to match alpha_ratio), (2) include dot-based leaders by counting
runs of '.' of length >=3 (e.g., count occurrences of regex r"\.{3,}" or
similar) and combine with underscore runs into a single leader ratio (or rename
constant to _FRONTMATTER_LEADER_RATIO_MIN), and (3) use that combined
leader_ratio in place of underscore_ratio in the final return expression so
dotted leaders trigger the ToC heuristic consistently.
- Around line 478-493: The logged substitution count is wrong because it
heuristically diffs strings; update _apply_normalise_profile so it returns both
the normalised string and an exact substitution count (e.g. modify
_apply_normalise_profile signature to return (normalised, substitutions) and
have its internal rule application increment a counter for each rule match
including multi-char rules), then in ingestion.py replace the call to normalised
= _apply_normalise_profile(text, ctx.normalise_profile) with normalised,
substitutions = _apply_normalise_profile(...) and log substitutions directly
(keep the existing logger message and still set text = normalised);
alternatively, if you prefer not to change the API, compute substitutions by
iterating the normaliser's rule tables (e.g. count occurrences of each
multi-char src via text.count(src) and single-char replacements via a Counter
intersection) and use that exact count in the logger instead of the zip
heuristic.
In `@tests/test_ingestion_reliability.py`:
- Around line 763-774: The test currently just asserts a tautology by
recomputing count from hardcoded pairs instead of exercising the notebook
predicate; replace this with one of two fixes: (A) add a small helper (e.g.,
_validate_token_mode_pair(chunk_tokens, tokenizer)) in the forgelm package and
have the notebook import it, then change
test_partial_token_mode_rejected_at_notebook_layer to call
_validate_token_mode_pair for each pair and assert it raises ValueError for
partial settings, or (B) keep a doc-style test but parse the actual notebook
JSON (via json.loads on the notebook file contents) and assert the predicate
string used in the notebook cell is present so deletion/regression is detected;
update test_partial_token_mode_rejected_at_notebook_layer to use whichever
approach you choose and reference the helper name _validate_token_mode_pair or
the notebook cell predicate string accordingly.
---
Nitpick comments:
In `@forgelm/ingestion.py`:
- Around line 2040-2066: The function ingest_path currently has too many kwargs;
create a frozen dataclass IngestionOptions containing the post-extraction
normalisation flags (language_hint, script_sanity_threshold, normalise_profile,
keep_md_frontmatter, epub_skip_frontmatter, keep_frontmatter, page_range,
strip_patterns, strip_pattern_timeout, strip_urls, quality_presignal), keep the
chunking knobs (chunk_size, overlap, chunk_tokens, overlap_tokens, tokenizer,
etc.) at the top level, and change ingest_path to accept either an
ingestion_options: Optional[IngestionOptions] or the old individual kwargs for
backward compatibility by converting any provided normalisation kwargs into an
IngestionOptions instance inside the function; update all internal uses to read
from the IngestionOptions instance and leave public callers working via a thin
keyword-only forwarder.
🪄 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: de4c7e4c-682a-434f-bf00-71f6c4d2a46a
📒 Files selected for processing (29)
CHANGELOG.mddocs/guides/data_audit-tr.mddocs/guides/data_audit.mddocs/guides/ingestion-tr.mddocs/guides/ingestion.mddocs/reference/data_ingestion_architecture-tr.mddocs/reference/data_ingestion_architecture.mddocs/roadmap.mddocs/roadmap/phase-15-ingestion-reliability.mddocs/usermanuals/en/data/audit.mddocs/usermanuals/en/data/ingestion.mddocs/usermanuals/en/data/quality-filter.mddocs/usermanuals/en/reference/cli.mddocs/usermanuals/en/reference/json-output.mddocs/usermanuals/tr/data/audit.mddocs/usermanuals/tr/data/ingestion.mddocs/usermanuals/tr/data/quality-filter.mddocs/usermanuals/tr/reference/cli.mddocs/usermanuals/tr/reference/json-output.mdforgelm/_pypdf_normalise.pyforgelm/_strip_pattern.pyforgelm/cli/subcommands/_ingest.pyforgelm/ingestion.pynotebooks/ingestion_playground.ipynbtests/test_doctor.pytests/test_ingestion_reliability.pytests/test_pypdf_normalise.pytests/test_script_sanity.pytests/test_strip_pattern.py
✅ Files skipped from review due to trivial changes (5)
- docs/guides/data_audit.md
- docs/usermanuals/tr/reference/cli.md
- docs/roadmap.md
- CHANGELOG.md
- docs/guides/data_audit-tr.md
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_strip_pattern.py
- docs/reference/data_ingestion_architecture.md
- forgelm/cli/subcommands/_ingest.py
- tests/test_script_sanity.py
… Codacy Absorbs every actionable finding from the round-3 review on PR #49. Test suite stays green: 218 focused Phase 15 module tests pass; all 6 CI guards + ruff are clean. Not pushed yet per maintainer instruction. ## Sonar findings - **S7494** (`forgelm/_script_sanity.py:72`) — list constructor → list comprehension. - **S3776 cognitive complexity** — six functions trimmed below the 15-point ceiling by extracting helpers: - `_check_unbounded_quantifier_sequence` (47 → ~10) splits atom parsing into `_scan_atom_end`, `_advance_past_brace_quantifier`, `_close_group_or_raise`. - `_pop_windowed_edges` (16 → ~7) delegates per-page work to `_pop_one_page_window`. - `_maybe_warn_multi_column` (20 → ~6) splits into `_select_sample_pages`, `_collect_multi_column_samples`, `_is_two_cluster_distribution`. - `_collect_docx_header_footer_lines` (22 → ~8) extracts `_add_part_lines_to_boilerplate`. - `_extract_docx` (23 → ~10) delegates block rendering to `_render_docx_block` + `_strip_docx_boilerplate`; introduces the `_DOCX_BOILERPLATE_MAX_LEN` module constant. - `_extract_epub` (23 → ~8) extracts `_resolve_spine_item` and `_emit_epub_skip_warning`. - **S107** (`ingest_path` 23 parameters) — annotated with `# NOSONAR` + rationale. Bundling the kwargs into a config dataclass would be a breaking API change for v0.5 callers that already pass these by name (per `forgelm.__all__` / `__api_version__` contract). ## Notebook (Sonar ipython:S5655) - `shlex.quote(LANGUAGE_HINT)` → `shlex.quote(str(LANGUAGE_HINT))` so Sonar's None-safety inference does not flag the call even though the `if LANGUAGE_HINT:` guard already short-circuits. ## Codacy markdownlint notices - 5 bullet glyphs in `docs/guides/ingestion.md` (lines 313 / 319 / 424-426) flipped from `*` to `-` to match the project's prevailing hyphen-bullet style (MD004 default). - Same flip mirrored in `docs/guides/ingestion-tr.md` for bilingual parity. - `tests/fixtures/ingestion/md_with_frontmatter_and_html.md` carries an inline `markdownlint-disable MD025 MD033` comment AFTER the YAML frontmatter (so the frontmatter regex still anchors at file start) documenting that the fixture intentionally combines a top-level `#` heading + raw HTML to exercise the `_extract_markdown` helper. ## Round-3 inline findings (CodeRabbit / Gemini) - **TR ingestion-tr.md** `--normalise-profile` wording refreshed to state the new default `none` + `language_hint==tr` auto-derivation semantics. EN mirror updated in parallel. - **EN + TR `quality-filter.md`** example replaced the misleading "dropped 45 rows; wrote data/clean.jsonl" line with the populates-`quality_summary` reality; drop / write-clean behaviour is only triggered by the named YAML keys. - **TR `quality-filter.md`** shell-block CLI comments + sample output reverted to English (CLAUDE.md "Code, CLI output, logs, config keys are English only" rule). - **`forgelm/_strip_pattern.py::_apply_one`** inner SIGALRM clamped to `min(timeout_s, previous_remaining)` so a nested caller can never extend an outer alarm's deadline. Outer-restore arithmetic now reads the original remainder, not the clamped value. - **Front-matter heuristic** now counts BOTH underscore- AND dot-runs of length ≥ 3 via `_FRONTMATTER_LEADER_RUN_PATTERN` and denominates the ratio over non-whitespace chars (parity with `alpha_ratio`). Catches ToC pages with dotted leaders that the pre-round-3 underscore-only count silently missed. - **Normalise substitution count** now uses the new `forgelm._pypdf_normalise.count_substitutions` helper for an EXACT per-rule tally (multi-char rules first, then single-char via `text.count(src)`) rather than the zip-diff heuristic that over- and under-counted on multi-char rule boundaries. - **Notebook-predicate test** rewritten to parse the notebook JSON and assert that Cell 5 carries the partial-token-mode predicate string + `ValueError raise`. A regression deleting the cell-side safety net now trips the test instead of silently passing. ## Usermanuals - `docs/usermanuals/{en,tr}/reference/cli.md` ingest synopsis gains `--output-format {text,json}` so operators discover the JSON envelope from the manual (the flag was already in the parser but missing from the synopsis listing). ## Test results - ruff format + ruff check — clean. - 6 CI guards (bilingual parity, anchor resolution, CLI help consistency, no-analysis-refs, no-unguarded-sys-modules-pop, wizard-defaults sync) — all green. - 218 Phase 15 focused module tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…age_window CodeRabbit caught a residual asymmetry in the per-page window walker: the head walk inspected every position in the top-N window but the tail walk peeled consecutively from the very end and stopped at the first non-match. That asymmetry re-introduced the audit §1.1 trap at the BOTTOM edge — a page shaped like `[..., "FOOTER", "1"]` / `[..., "FOOTER", "2"]` would leave the constant ``FOOTER`` stranded because the varying page number on the last line broke the peel loop on pass 1. The tail walk now mirrors the head walk's per-position semantics: it iterates the bottom-N window in reverse and drops any line whose stripped form is in ``repeating_lasts``, regardless of where within the window it appears. New regression test `test_window_based_dedup_strips_deeper_constant_bottom_line` locks the invariant in alongside the existing top-edge case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…/E, N-1/2/3)
Independent round-4 review surfaced 10 actionable findings on top of
the round-3 push. This commit absorbs every still-valid issue minimum-
diff, with the dispatcher-coupled N-5 nit gerekçeli olarak skipped.
## Critical
- **C-A** path repr-escape in `IngestParameterError` + `_extract_pdf`
decrypt-fail messages (`forgelm/ingestion.py:588 / 592 / 668-672 / 931`).
Operator-controlled paths used to interpolate as `'{path}'` raw,
letting ANSI escape sequences / control chars / embedded quotes
through into the rendered error. Switched to `{path!r}` so the
repr-quoting matches the precedent already set by the strip-pattern
timeout warning. Existing `pytest.raises(ValueError, match="page-range")`
continues to pass.
- **C-B** quality-presignal alpha-ratio false-positive on clean small
corpora. A 5-paragraph 41-char TXT was emitting `[WARN] 1/1 chunks
below ingestion quality threshold` purely because punctuation +
whitespace dragged the alpha ratio below 0.70 on a single short
chunk. The check now requires ≥ 80 non-whitespace characters before
the alpha ratio is evaluated (`_PRESIGNAL_MIN_NON_WS_CHARS`).
Verified the clean 5-para corpus now reports `samples_flagged=0`.
## Strong
- **S-A** `_check_unbounded_quantifier_sequence` docstring now
documents explicit scope: the validator covers the **nested**
unbounded shape (regex.md rule 4 nested-only), and adjacent /
sequential variants (`(.+?)(.+)`, `(?:a|b)+(c|d)+`, `(.+?)[ \t]*$`)
fall back on the SIGALRM safety net. Round-3 review verified
empirically that CPython 3.10-3.13 does not backtrack catastrophic-
ally on those shapes; doc the gap so PyPy / Pyston operators know
the runtime guard is the primary defence on non-CPython.
- **S-B** multi-column threshold caveat added to both
`docs/guides/ingestion.md` and `-tr.md`. The current 0.30 threshold
+ min/max-gap algorithm misses publication-grade 2-column PDFs
(5-8 %-of-page-width gutter). Documented as a "known limitation"
block pointing at the Wave 3 histogram-based detector. The detector
remains reliable on extreme layouts (academic posters, wide-gutter
regulatory publications); operators on normal 2-column scientific
papers should expect to switch strategies manually.
- **S-C** EPUB-3 manifest properties tokenisation in
`_epub_item_matches_skip`. EPUB-3 properties are SPACE-SEPARATED
tokens (e.g. `properties="nav cover-image"`); the pre-fix code did
an exact-string match on the joined form, which silently failed for
any item carrying more than one property. Now both the filename and
the property string flow through the same `_EPUB_NAME_TOKEN_SPLIT`
splitter so any single token can match the skip-list.
- **S-E** front-matter heuristic alpha threshold tightened
`0.45 → 0.30`. The pre-tightening threshold dropped form-template /
fill-in-the-blank pages whose alpha ratio sits in the 0.20-0.40
band (low because of underscore leaders, NOT because the page is
non-content). 0.30 keeps the audit's pilot ToC shape (≈ 0.15 alpha)
catchable while letting exercise inserts / form templates through.
## Minor
- **N-1** new `TestCountSubstitutions` class in
`tests/test_pypdf_normalise.py` (9 cases) pins the exact-count
contract that the round-3 `count_substitutions` helper introduced.
Direct coverage; previously only tested transitively via
`_post_extract_normalise`.
- **N-2** removed dead `total = len(kept)` + `del total` from
`_pop_one_page_window`. A future reader would have asked "what was
total for?" and found no answer; the `del` was a Sonar appeasement,
not documentation.
- **N-3** `_DEFAULT_EPUB_SKIP_ITEMS` comment drift fixed —
pre-round-4 it claimed "frozenset of normalised lower-case
substrings" while the literal is a `Tuple[str, ...]`. Comment now
matches the type AND mentions the whole-token splitter
(`_EPUB_NAME_TOKEN_SPLIT`) so a future reader sees the matching
contract end-to-end.
## Skipped (gerekçeli)
- **N-5** `_extract_text` / `_extract_markdown` parameter rename
`ctx → _ctx`. The dispatcher calls every extractor with the uniform
`extractor(fpath, ctx=ctx)` kwarg; renaming the param would either
break the call site or introduce a signature asymmetry (PDF / DOCX /
EPUB extractors genuinely use `ctx`, only TXT / MD don't). The
`del ctx` idiom is the documented Pythonic workaround for unused-
parameter-required-by-interface and stays.
- **N-4 (lookaround test pin)** and **N-6 (URL trailing
punctuation)** and **N-7 (Sonar S3776 boundary noise)** deferred
to a follow-up — N-4 is a "lock-in test" not a behaviour change,
N-6 is a known limitation worth documenting separately, N-7 is
automated tooling output that may or may not flag the current
shape.
## Test results
- ruff format + ruff check — clean.
- 6 CI guards — all green (bilingual parity, anchor resolution, CLI
help consistency, no-analysis-refs, no-unguarded-sys-modules-pop,
wizard-defaults sync).
- 228 Phase 15 focused module tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
Round-4 review absorbed — commit
|
| # | Bulgu | Status | Düzeltme |
|---|---|---|---|
| C-A | IngestParameterError + decrypt path interpolation ('{path}' → terminal/log injection vektörü) |
✅ Fixed | 6 yerde {path!r} repr-escape — --page-range 3 mesajı + _extract_pdf 2 decrypt mesajı + Could not open PDF mesajı |
| C-B | Clean 5-paragraf TXT corpus'unda [WARN] 1/1 chunks below threshold regresyonu (v0.5 callers UX) |
✅ Fixed | Yeni _PRESIGNAL_MIN_NON_WS_CHARS = 80 — kısa chunk'lar alpha-ratio kontrolünden muaf. Verified: samples_flagged=0 on clean small corpus |
🟠 Strong
| # | Bulgu | Status | Düzeltme |
|---|---|---|---|
| S-A | Validator scope iddiası rule 4'ü tam karşılamıyor ((.+?)(.+) sequential şekli pass ediyor) |
✅ Doc-clarified | Docstring artık scope'u açıkça yazıyor: nested-only + SIGALRM safety net adjacent için. CPython 3.10-13'te empirik olarak doğrulandı; non-CPython runtimes için uyarı eklendi |
| S-B | Multi-column threshold 0.30 publication-grade 5-8% gutter'ı kaçırıyor | ✅ Doc-noted | EN+TR ingestion guide'a "Known limitation" bloğu eklendi; Wave 3 histogram refactor'ına işaret ediyor |
| S-C | EPUB-3 manifest properties="nav cover-image" joined-form exact-match'te kaçıyor |
✅ Fixed | _epub_item_matches_skip artık property string'i de _EPUB_NAME_TOKEN_SPLIT üzerinden token'lara böler; herhangi bir token skip-list'te varsa atlanır |
| S-D | Notebook predicate test brittle (string-match) | ⏸️ Out of scope | Helper extract'i (_validate_token_mode_pair) daha yüksek bir refactor; round-4 minimal-diff scope dışı. Mevcut test cell-source-content regression'ını yakalıyor, sadece prose-wording'e duyarlı |
| S-E | Form-template page'leri front-matter heuristic'ine düşüyor | ✅ Fixed | _FRONTMATTER_ALPHA_RATIO_MAX 0.45 → 0.30. Audit'in pilot ToC (≈ 0.15 alpha) hâlâ yakalanır; form template (0.20-0.40 alpha) artık geçer |
🟡 Minor
| # | Bulgu | Status |
|---|---|---|
| N-1 | count_substitutions direct testi yok |
✅ Added TestCountSubstitutions (9 vaka) |
| N-2 | _pop_one_page_window'da dead total + del total |
✅ Removed |
| N-3 | _DEFAULT_EPUB_SKIP_ITEMS docstring drift (frozenset claim → tuple) |
✅ Fixed |
| N-4 | Lookaround pattern test pin | ⏸️ Defer — test-only addition, behaviour zaten doğru |
| N-5 | _extract_text param ctx → _ctx |
❌ Skip gerekçeli — dispatcher extractor(fpath, ctx=ctx) uniform kwarg ile çağırıyor; rename ya call site'ı kırar ya da PDF/DOCX/EPUB extractor'larıyla asymmetrik olur. del ctx idiyomu doğru çözüm |
| N-6 | URL pattern trailing punctuation imprecision | ⏸️ Defer — known minor, docs'a not edilebilir round-5'te |
| N-7 | _pop_one_page_window Sonar S3776 boundary |
⏸️ Defer — automated, henüz flag'lenmedi |
❓ Open questions yanıtları
overlap_tokens > 0 + strategy=paragraph— INFO vs WARNING: doğru çağrı, gözden geçirildi. Operatör mistake değil;--overlap-tokensparagraph mode'da silently ignore edilmiyor, INFO log basılıyor. Bu standart "operator-supplied value documented-no-op" şekli, WARNING gürültüsüne değer.- Front-matter default-on: Round-4 S-E ile alpha threshold tightened (0.30); form-template false-positive surface artık daraltıldı. Default-on davranış v0.6.0'da kalıyor.
- SUPPORTED_LANGUAGES non-Turkish: Audit'in itirafı module docstring'inde mevcut. Phase 16'da fixture genişledikçe re-calibrate edilecek; bugün de/fr/es/it/pt için no-op-fallback şart değil çünkü liberal allow-list zaten false-positive'i minimum tutar.
apply_profile("ö ")ambiguity: Geçerli edge case, character-blind. Kelime-finalö+ space (loanwords, poetry) için lookahead eklemek pragmatik olarak deferred; corpus-scale fault değil, single-word loss.- Task 5 default-on test coverage:
tests/test_data_audit_phase12.py::test_quality_summary_only_present_when_enabledmevcut; CLI default-on flip'i doğrudan test edilmedi ancaktests/test_cli_subcommands.pyvetools/check_cli_help_consistency.pyBooleanOptionalAction help-text'ini doğruluyor. Behavioural test ileride eklenebilir. - SIGALRM no-op on worker threads: Önceki round'larda eklenen
forgelm/_strip_pattern.pydocstring'i bunu söylüyor; help-text'e de bir satır eklemek mantıklı. Defer to round-5.
Test durumu
ruff format + ruff check ........ PASS
6 CI guards ..................... PASS
228 Phase 15 focused tests ...... PASS
🤖 Generated with Claude Code
Independent review of PR #49's 4-commit history surfaced 2 critical + 5 strong findings the prior reviewers did not cover. Each verified empirically via adversarial probe; all fixed minimum-diff. ## Critical - **C-A** round-4 changes (path repr-escape, presignal min-floor, EPUB item_type token-split, frontmatter alpha tightening) had ZERO dedicated regression tests. A future commit relaxing any of them would silently regress. New `TestRoundFourFixes` class (5 cases) pins each invariant: * `test_page_range_error_repr_escapes_ansi_in_path` — synthesises a PDF whose filename carries an ANSI escape sequence and asserts the raw `\x1b` byte does NOT appear in the rendered error message. * `test_presignal_floor_silent_on_short_clean_corpus` — verifies the reviewer's exact regression case (clean 5-paragraph 41-char TXT) now reports `samples_flagged=0`. * `test_epub_item_type_split_on_multi_property_string` — pins the `properties="nav cover-image"` multi-token-split path introduced in round-3 S-C. * `test_frontmatter_threshold_audit_pilot_still_caught` — locks the audit's Turkish-pilot ToC shape (alpha ≈ 0.15) inside the catch zone after the 0.45 → 0.30 tightening. * `test_frontmatter_protects_realistic_form_template_via_3signal_AND` — locks the actual protection (the 3-signal AND filter via the inline-page-number signal), not the threshold alone. - **C-B** `--language-hint zh` (or any unsupported code) silently no-ops the script-sanity layer AND the normalise profile — the textbook silent-failure UX Phase 15 was launched to fix. Empirical probe: Chinese text + 10 `ø` artefacts + `--language-hint zh` → `script_sanity_summary: {triggered: 0, max_ratio: 0.0}` plus no normalise applied. CLI dispatcher now emits a WARNING listing the supported codes when the hint is not in `SUPPORTED_LANGUAGES`. ## Strong - **S-A** front-matter heuristic kalibrasyon caveat added to both `docs/guides/ingestion.md` and `-tr.md`. Realistic English-language ToCs with full-sentence chapter titles measure alpha ≈ 0.47 and pass through the 0.30 threshold; the heuristic is calibrated for the audit's Turkish-pilot single-word-title shape. Operators on long-title ToC corpora should use `--strip-pattern` / `--page-range` as manual fallbacks. - **S-B** `_check_weird_char_ratio` range narrowed from `0x0800-0x097F` (Samaritan / Mandaic / Syriac Suppl. / Arabic Ext-A / Devanagari — 384 codepoints across 5 scripts) to `0x0840-0x085F` (Mandaic punctuation only, the audit's actual target). The pre-narrowing range relied on `and not c.isalpha()` to avoid false-positives on Devanagari / Syriac alphabetics; the narrowing eliminates the surface entirely for Sanskrit / Arabic- script corpora. - **S-C** `_FRONTMATTER_ALPHA_RATIO_MAX` docstring rewritten. The round-4 docstring claimed the 0.30 threshold "lets form templates through" but empirically a constructed short-label form template measures alpha ≈ 0.18, BELOW 0.30 — so the threshold is NOT the real defence. The actual protection is the 3-signal AND filter (realistic form templates have NO inline page-number matches, so `page_num_hits >= 5` fails first). Docstring now states this correctly and acknowledges the long-title-ToC side-effect. - **S-D** `"toc"` token removed from `_DEFAULT_EPUB_SKIP_ITEMS`. Short and common enough to over-match a chapter filename containing "toc" as a sub-word after token-splitting (`historical_toc.xhtml`, `the_toc_of_war.xhtml`). Canonical EPUB-3 navigation documents always carry the `nav` property AND are named `nav.xhtml` / `navigation.xhtml` in practice; the `nav` token suffices. Operators with EPUBs whose nav file is literally `toc.xhtml` (lacking the `nav` property) can add the token back via the library `epub_skip_items` ctx kwarg. - **S-E** `apply_strip_patterns` loop now breaks when an earlier pattern consumed the entire text. Pre-round-5 each subsequent pattern set up + tore down a SIGALRM around a no-op `subn("", "")` call. Minor perf nit; meaningful on pathological multi-pattern configurations. ## Test results - ruff format + ruff check — clean. - 6 CI guards — all green. - 233 Phase 15 focused module tests pass (228 + 5 new round-5 regression cases). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
Round-5 self-review — commit
|
| # | Bulgu | Empirik kanıt | Çözüm |
|---|---|---|---|
| C-A | Round-4 davranış değişiklikleri (path repr, presignal floor, EPUB item_type, frontmatter alpha) sıfır regression test'e sahipti | grep -n "path!r\|PRESIGNAL_MIN\|form.template\|nav cover-image" tests/ → 0 sonuç |
Yeni TestRoundFourFixes (5 case): ANSI escape repr-test, clean 5-para corpus floor, EPUB multi-property split, audit-pilot ToC catch, 3-signal-AND form-template koruma |
| C-B | --language-hint zh (veya başka unsupported) sessiz no-op — script-sanity + normalise ikisi de çalışmıyor |
Probe: language_hint='zh' + ø artefacts → script_sanity_summary: {triggered: 0, max_ratio: 0.0} + normalise applied: False |
CLI dispatcher artık SUPPORTED_LANGUAGES membership kontrolü yapıyor, miss varsa WARNING listing supported codes |
🟠 Strong
| # | Bulgu | Empirik kanıt | Çözüm |
|---|---|---|---|
| S-A | Gerçekçi İngilizce ToC (alpha ≈ 0.47) heuristic'i tetiklemiyor | Probe: "Chapter 1: Introduction to the Subject ........... 14" * 5 → _is_frontmatter_page: False |
EN+TR ingestion guide'a calibration caveat: heuristic audit'in Turkish-pilot şekli için kalibre; long-title ToC için --strip-pattern/--page-range manuel fallback |
| S-B | _check_weird_char_ratio 0x0800-0x097F aralığı 5 scripte yayılıyor (Samaritan + Mandaic + Syriac Suppl + Arabic Ext-A + Devanagari) — audit'in target'ı sadece Mandaic punctuation |
Kod incelemesi: range 384 codepoint, and not c.isalpha() sadece alphabetics'i koruyor |
Aralık daraltıldı: 0x0840-0x085F (sadece Mandaic block, audit target). Sanskrit / Hindi / Arabic-script corpus'larda false-positive surface elimine |
| S-C | _FRONTMATTER_ALPHA_RATIO_MAX = 0.30 docstring'i yanlış claim ("form templates sit in 0.30-0.40 band") |
Probe: constructed form template alpha = 0.18 (< 0.30, hâlâ trips ediyor) | Docstring rewrite: gerçek koruma 3-signal AND filtresi (form template'lerin inline page-number'ı yok → page_num_hits >= 5 fails). Threshold ikincil. Long-title ToC side-effect kabul edildi |
| S-D | _DEFAULT_EPUB_SKIP_ITEMS "toc" token'ı historical_toc.xhtml gibi chapter filename'larda yanlış-eşleşebilir (token-split sonrası {historical, toc, xhtml} toc skip token'ı ile çakışır) |
Kod incelemesi: "toc" kısa ve yaygın bir kelime parçası | "toc" listeden çıkarıldı. Canonical EPUB-3 nav file zaten nav property + nav.xhtml filename taşır. Operatörler özelleştirebilir via epub_skip_items ctx kwarg |
| S-E | apply_strip_patterns loop'unda pattern A bütün metni silerse, B/C/... boş string'e SIGALRM setup/teardown yapar — perf nit |
Kod incelemesi | Loop içinde if not out: break |
Doğru kararlar (review-cycle survivor)
- 4 round absorption disiplini — her round'un bulgusu sonraki round'un test corpus'una eklenmiş
- IngestParameterError(ValueError) subclass design — operator-mistake propagate, per-file soft-fail bozulmamış
_post_extract_normaliseordered pipeline — script-sanity AFTER normalise (false-fire önler)count_substitutionsexact-rule-fire counting — zip-diff heuristic'in replacement'ı temiz- Doctor probe round-trip fixture — sadece import değil, dispatcher'ı gerçekten test ediyor
Test sonucu
ruff format + check ............ PASS
6 CI guards .................... PASS
233 Phase 15 focused tests ..... PASS (228 + 5 yeni TestRoundFourFixes case)
🤖 Generated with Claude Code
|
cemililik
added a commit
that referenced
this pull request
May 11, 2026
Phase 15 (Ingestion Pipeline Reliability) Wave 1 + Wave 2 + 5 review-absorption rounds. PR #49 merged into main. Surface: - 12 new `forgelm ingest` flags + audit `--quality-filter` default-on - `IngestionResult` gains 5 additive fields; no pre-Phase-15 key renamed - `__version__` 0.5.7 → 0.6.0 (MINOR — CLI / behavioural contract) - `__api_version__` stays at 1.0.0 — no new stable library symbol added to `forgelm.__all__` Local + CI gauntlet: - ruff format --check + ruff check: PASS - 6 CI guards (bilingual parity, anchor resolution, CLI help consistency, no-analysis-refs, no-unguarded-sys-modules-pop, wizard-defaults sync): PASS - pytest tests/: 1767 passed, 14 skipped, 0 failed - forgelm --config config_template.yaml --dry-run: PASS - forgelm --version: 0.6.0 - Main CI (Python 3.10 / 3.11 / 3.12 / 3.13): all jobs green Docs: - CHANGELOG.md: `[Unreleased]` renamed to `[0.6.0] — 2026-05-11`; footer compare links refreshed. - docs/roadmap.md: Phase 15 row 🚧 → ✅; Phase 14 re-scheduled to v0.7.0; "Released" banner now points at v0.6.0. - docs/roadmap/completed-phases.md: new Phase 15 section with the 10 Wave 1 + 5 Wave 2 task summary + public surface changes + test budget. - docs/roadmap/releases.md: new v0.6.0 entry; old v0.6.0 "Pipeline Chains" placeholder moved to v0.7.0. - docs/roadmap/phase-15-ingestion-reliability.md: marked Completed with pointer to the released-shape doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 11, 2026
PR #49's original v0.6.0 release commit (836fdcc) shipped two Phase 15 regression tests whose ``tmp_path / "doc.txt"`` writes relied on ``Path.write_text``'s OS-default codec. On Linux/macOS that defaults to UTF-8 (passes); on Windows runners it defaults to cp1252 which cannot encode Latin-1 Supplement chars (``Bjørk Õrö ÷``) consistently with the UTF-8-first read path, and outright rejects U+085F (Mandaic punctuation) used by the Turkish corruption-recovery fixture. The cross-OS publish matrix exposed this: 4 Windows jobs failed → trusted-PyPI publish step skipped → v0.6.0 never reached PyPI. This commit: - Adds explicit ``encoding="utf-8"`` to both ``write_text`` calls so the test contract matches the ingestion read path on every OS. - Reverts ``pyproject.toml`` from the post-release dev-cycle bump (0.6.1rc1) back to 0.6.0 so the wheel built from the retagged v0.6.0 carries the correct version string. - Strips the matching CHANGELOG ``[Unreleased]`` dev-cycle placeholder line so the published 0.6.0 tarball reads cleanly. Once this commit lands and publish.yml ships v0.6.0 to PyPI, a follow-up commit re-bumps to 0.6.1rc1 and reinstates the [Unreleased] placeholder to resume the dev cycle. Branch protection blocks force-push, so we ship the retag as an additive commit rather than amending 836fdcc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 14, 2026
Phase 15 (Ingestion Pipeline Reliability) shipped under v0.6.0 on 2026-05-11; its detailed entry already lives at completed-phases.md line 1281+ with the Wave 1 + Wave 2 + 5 review-absorption-round summary. The standalone roadmap/phase-15-ingestion-reliability.md was a historical-reference duplicate — removed now that the "completed" surface is the single source of truth. Other drift fixed in the same sweep: - docs/roadmap.md: Phase 14 release target v0.6.0 → v0.7.0 in the file-tree block (the table at the top already reflected this). Mermaid graph: P15 now flows into P14 (shipping order), P14's release node renamed V23 → V27 (v0.7.0), P15 carries the "✅ Released" suffix; mermaid styles flip P15 to the released green palette. "Current state" paragraph: "19 phases under v0.5.5 / 3 phases planned" → "20 phases through v0.6.0 / 2 phases planned"; "Phase 14 and Phase 15 ship in parallel under v0.6.0" → "Phase 14 is targeted for v0.7.0". - docs/roadmap-tr.md: TR mirror of every roadmap.md edit above, plus the Phase 15 table row flipped from "Planlandı" to "✅ Tamam" with the link redirected at the completed-phases archive anchor. - docs/guides/ingestion.md + ingestion-tr.md: the Wave 3 multi- column extraction reference pointed at the now-removed standalone doc; redirected to the completed-phases archive anchor. - docs/roadmap/completed-phases.md: the Phase 15 "Related" block had a self-reference at the deleted standalone doc; replaced with the CHANGELOG section + PR #49 entry already adjacent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 15 (Ingestion Pipeline Reliability) Wave 1 + Wave 2 — closes the silent-failure gap the 2026-05-11 pilot exposed across PDF / DOCX / EPUB / TXT / Markdown ingestion plus the user-facing playground notebook. Targeted for v0.6.0.
Also bundles the v0.5.7 review-round-5 follow-up (
b3d6dcb) that was sitting ondevelopmentafter the v0.5.7 tag.Wave 1 — silent-failure remediation (10 tasks)
_strip_repeating_page_linesnow inspects the top-3 / bottom-3 rows per page instead of the strict outermost row. Variable-outer-line + constant-deeper-line corpora (the audit §1.1 trap) are dedup'd in a single pass; a second pass viastrip_paragraph_packed_headersmops up survivors after paragraph packing.forgelm/_script_sanity.pyruns a per-file Unicode-block check after every extract (tr/en/de/fr/es/it/ptwith discrete diacritic allow-lists). 1.5 % threshold; emitsscript_sanity_summary.forgelm/_pypdf_normalise.pymaps audit-measured fallback glyphs (ø Õ ú ÷ ) →İ ı ş ğ •.forgelm doctorverifies via the newpypdf_normalise.turkishdiagnostic.[WARN] N/M chunks below ingestion quality threshold.forgelm audit --quality-filteris default-ON in v0.6.0; new--no-quality-filtercompanion preserves the pre-v0.6.0 opt-in semantics._extract_docxreadsdoc.sections[i].header.paragraphs+.footer.paragraphsand subtracts those lines from the body.book.spine(reading order) + filtersnav/cover/copyright/colophon/titlepage/frontmatteritems.encoding="utf-8-sig"strips BOM; new_extract_markdownstrips---\n…\n---\nYAML frontmatter.notebooks/ingestion_playground.ipynbCells 5 / 8 / 9 / 10 rewritten: token-aware knobs, quality-filter blurb, explicit--quality-filter,quality_summarypretty-print.tests/fixtures/ingestion/carries TXT-BOM + MD-frontmatter + mixed-directory fixtures; PDF / DOCX / EPUB fixtures synthesised in-test viapypdf.PdfWriter/python-docx/ebooklib.Wave 2 — strong nice-to-have (5 tasks)
--strip-pattern REGEXwith ReDoS guard (rejects(a+)+bshapes +.*?+ back-reference under DOTALL; 5-second SIGALRM budget on POSIX).--page-range START-ENDPDF flag; newIngestParameterError(ValueError)propagates through the per-file soft-fail catch toEXIT_CONFIG_ERROR (1).--keep-frontmatter.--strip-urls {keep,mask,strip}; independent of--all-mask(URL handling is a content-shape decision, not a GDPR redaction).visitor_textcallback sampling.Schema additions (all additive — no pre-Phase-15 key renamed)
IngestionResult:pdf_paragraph_packed_lines_stripped,script_sanity_triggered,strip_pattern_substitutions,urls_handled,frontmatter_pages_dropped.notes_structured: same additions +script_sanity_summary,quality_presignal,frontmatter_pages_dropped.Test plan
ruff format .+ruff check .— cleanpytest tests/— 1728 passed, 14 skipped (optional extras), 0 failedtests/test_pypdf_normalise.py(16)tests/test_script_sanity.py(16)tests/test_strip_pattern.py(11)tests/test_ingestion_reliability.py(36, organised by task)tools/check_bilingual_parity.py --strict— 46 doc pairs greentools/check_anchor_resolution.py --strict— 269 files greentools/check_cli_help_consistency.py --strict— 443 invocations greentools/check_wizard_defaults_sync.py— schema/JSON sync greentools/check_no_analysis_refs.py— no public-tree leakstools/check_no_unguarded_sys_modules_pop.py— 167 files greenforgelm doctor—pypdf_normalise.turkish: passrow renders correctlyforgelm ingest --help— all 11 new flags render with prose helpDocumentation
CHANGELOG.md[Unreleased]blockdocs/guides/ingestion.md+-tr.md— new Phase 15 sections after the page-header dedup blockdocs/guides/data_audit.md+-tr.md—--quality-filterdefault-ON notedocs/reference/data_ingestion_architecture.md+-tr.md— window-based dedup explanation + Phase 15 limitations recapdocs/roadmap.md— Phase 15 row: 📋 Planned → 🚧 In reviewdocs/roadmap/phase-15-ingestion-reliability.md— Tasks 1-15 checked[x]Surface coverage check
ingestflags + audit--quality-filterBooleanOptionalActioningest_path()11 new kwargs (all optional, back-compat defaults)ingestion_playground.ipynbTasks 9 acceptancepypdf_normalise.turkishdiagnosticforgelm ingestinvocations)config_template.yaml— untouched (ingestion is CLI-only, never YAML-configurable)Reviewer hot spots
_pop_windowed_edges— a line appearing in bothrepeating_firstsandrepeating_lastsmust not be popped twice._script_sanity._LANGUAGE_BLOCKS— verify no legitimate characters are missing per supported language._TURKISH_MULTIordering — does"ö " → "Ğ"collide with legitimate Turkish text ending withö-then-space?_QualitySamplingFileHandle.write— per-chunkjson.loadscost on 100K-row corpora._check_unbounded_quantifier_sequenceheuristic — what ReDoS shapes does it miss? SIGALRM is the safety net._is_frontmatter_pagefalse positives — a glossary entry with 5 cross-references could trip the heuristic._maybe_warn_multi_columnsampling — only first 3 pages; corpora with mid-document 2-column sections are silent.🤖 Generated with Claude Code
Summary by CodeRabbit