Repository navigation
feat(wave-3): Faz 24 + 28 + 38 consolidated integration - #31
Merged
Merged
Conversation
Three closure-plan phases on a single integration branch. Suite
1298 → 1333 (+35 net); all green; ruff clean; dry-run green.
PHASE 38 — `forgelm reverse-pii` (GDPR Article 15 right-of-access)
- forgelm/cli/subcommands/_reverse_pii.py (new):
* `forgelm reverse-pii --query VALUE [--type X] [--salt-source X]
JSONL_GLOB...` — walks JSONL corpora, reports every line where the
supplied identifier appears.
* Two scan modes: plaintext residual (mask-leak detection) +
hash-mask (SHA256(salt + identifier) via the same per-output-dir
salt forgelm purge uses for target_id hashing).
* Identifier types: email, phone, tr_id, us_ssn, iban, credit_card,
custom regex. Glob expansion (recursive ** honoured).
* Centre-truncated snippets (160 chars max) defend log spam.
- New audit event `data.access_request_query` with the identifier
*hashed* (Article 15 access requests must not themselves leak the
subject's data into the audit log). Catalogued bilingually.
- forgelm/cli/_parser.py + _dispatch.py + __init__.py: registrar +
dispatch row + facade re-exports.
- 18 regression tests in tests/test_reverse_pii.py.
PHASE 24 — Bilingual TR mirror sweep + parity CI guard
- tools/check_bilingual_parity.py (new): replaces the inline
H2-only check at ci.yml:197-222 with an extended H2 + H3 + H4
structural diff. AST-free; runs in the lint job; ``--strict``
mode for CI gating.
- 4 doc-pair drift fixes:
* ingestion-tr.md +2 H3 (Markdown-aware splitter, DOCX table
preservation — Phase 12 features).
* architecture-tr.md +4 H3 (results.py, benchmark.py, judge.py,
model_card.py — were missing).
* distributed_training-tr.md +3 H3 (Custom DeepSpeed Config,
"When to choose FSDP over DeepSpeed", LoRA + Distributed) +
Multi-Node ↔ Docker reorder.
* configuration-tr.md +1 H4 (model.multimodal) +
evaluation.benchmark/safety order swap.
- 4 user-manual H2 drift fixes: tr/training/sft.md ("Diskte ne elde
edersiniz"), tr/training/simpo.md ("Veri formatı"),
tr/compliance/overview.md ("Annex IV neyi içerir"),
tr/concepts/data-formats.md ("Verinizi doğrulama").
- alignment.md:230 "v0.5.1 (Phase 14)" → phase-number reference (no
version anchor — pipeline chains slated for v0.6.0+).
- 16 regression tests in tests/test_check_bilingual_parity.py
including a live-repo smoke that pins the canonical pair set
passes ``--strict``.
PHASE 28 — Curated cleanup (tier 1+2 absorbed, large items deferred)
Code:
- forgelm/config.py F-compliance-110 (**breaking**): high-risk +
safety.enabled=false now raises `ConfigError`. Was a warning;
EU AI Act Article 9 risk-management evidence cannot be derived
from a disabled safety eval.
- forgelm/config.py F-compliance-106: WebhookConfig.timeout default
raised 5s → 10s (Slack/Teams gateway latency spikes).
- forgelm/compliance.py F-compliance-111: missing
data_audit_report.json escalated INFO → WARNING (Article 10
compliance gap should be visible in operator log dashboards).
- forgelm/compliance.py M-204: added `_sanitize_md_list` helper +
migrated foreseeable_misuse bullet build.
- forgelm/deploy.py M-205: ollama Modelfile SYSTEM line escapes
newlines (\n / \r) so multi-line operator-supplied prompts don't
break the parser.
- forgelm/webhook.py C-54: dropped `_is_private_destination`
re-export from `__all__` (Phase 7 split moved the helper to
forgelm._http; no downstream importer found).
- forgelm/trainer.py C-57: GRPO reward token list carries explicit
GSM8K + MATH-tuned docstring caveat.
- tests/test_integration_smoke.py → tests/test_integration.py
(F-test-011): rename — file is integration test, not smoke.
Tests:
- tests/test_eu_ai_act.py: updated 5 tests for the F-compliance-110
behaviour change + added regression `test_high_risk_safety_disabled_raises_config_error`.
- tests/test_compliance.py: renamed `test_info_log_when_audit_missing`
→ `test_warning_log_when_audit_missing` (F-compliance-111).
- tests/test_data_edge_cases.py: webhook timeout default 5 → 10
(F-compliance-106).
- tests/test_integration.py: high-risk-without-safety test now
asserts ConfigError raise (was warn).
Tier 3 deferred to v0.6.0:
- F-code-108 (Pydantic _meta.yaml schema), F-business-006
(compliance-export populated bundle), F-business-008 (QMS README
rewrite), M-203 (FORGELM_GGUF_CONVERTER manifest), C-52
(_chunk_semantic implementation), F-test-009 (pytest-xdist),
F-test-002 (runtime_smoke deletion). Each is >1 PR scope.
DOCS
- CHANGELOG.md `[Unreleased]`: Wave 3 section with detailed sub-bullets.
- docs/guides/gdpr_erasure.md + -tr.md: new "Article 15
right-of-access" section pointing operators at reverse-pii.
- docs/reference/audit_event_catalog.md + -tr.md: new
`data.access_request_query` event row.
- docs/usermanuals/{en,tr}/reference/cli.md: reverse-pii row added.
- docs/usermanuals/{en,tr}/reference/json-output.md: reverse-pii
envelope schema + key table + exit-code mapping.
- docs/analysis/code_reviews/closure-plan-202604300906.md: 3 phases
marked Wave 3 ✅, kapanış özeti added.
- forgelm/cli/_parser.py epilog: reverse-pii row.
- .github/workflows/ci.yml: bilingual H2-only check replaced with
`tools/check_bilingual_parity.py --strict`.
VALIDATION
- ruff format + ruff check clean
- pytest 1333 passed / 14 skipped (was 1298 → +35 net)
- forgelm --config config_template.yaml --dry-run green
- forgelm reverse-pii --help round-trip via main CLI
- tools/check_bilingual_parity.py --strict: 8/8 doc pairs at parity
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ation Self-contained 4-agent review prompt at HEAD ``61d74fa``. 4 specialty agents: - Code-correctness — Phase 38 reverse-pii dispatcher defects, audit payload tracing, hash-mask round-trip with forgelm purge. - Standards-compliance — XPR-01..07 (audit vocab, exit codes, JSON envelope, regex / ReDoS, error handling, BC, bilingual parity). - Test-rigor — non-vacuous assertions, edge-case coverage, regression pinning of new contracts. - Privacy / security — reverse-pii cleartext-PII surface, audit-log leak paths, snippet truncation safety, custom-regex ReDoS exposure. Severity bar tuned: behaviour changes need migration paths; reverse-pii cleartext-leak paths are CRITICAL; F-compliance-110 (high-risk + safety-disabled raise) is the only intentional break. ``.gitignore`` allowlist extended to re-include the new prompt under the otherwise-internal ``docs/analysis/code_reviews/`` tree. 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, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 minor |
| ErrorProne | 2 high |
| CodeStyle | 28 minor |
🟢 Metrics 294 complexity · 49 duplication
Metric Results Complexity 294 Duplication 49
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the forgelm reverse-pii subcommand for GDPR Article 15 right-of-access and a new CI tool, check_bilingual_parity.py, to maintain structural consistency across English and Turkish documentation. It also includes several compliance-driven refinements, such as escalating high-risk safety warnings to hard errors and increasing webhook timeouts. Feedback identifies critical improvements for the PII scanner, specifically regarding UTF-8 decoding robustness and match-centered snippet truncation, alongside logic corrections for the parity tool's markdown parsing.
| Raises :class:`OSError` on read failure; the caller handles it. | ||
| """ | ||
| matches: List[Dict[str, Any]] = [] | ||
| with open(path, "r", encoding="utf-8") as fh: |
There was a problem hiding this comment.
Opening the file without specifying an error handling strategy for decoding will cause the tool to crash if it encounters invalid UTF-8 sequences. For a robust PII scanner, it's better to use errors="replace" to ensure the scan continues even if individual bytes are malformed.
Suggested change
| with open(path, "r", encoding="utf-8") as fh: | |
| with open(path, "r", encoding="utf-8", errors="replace") as fh: |
Comment on lines
+216
to
+223
| if not pattern.search(stripped): | ||
| continue | ||
| matches.append( | ||
| { | ||
| "file": path, | ||
| "line": line_no, | ||
| "snippet": _truncate_snippet(stripped), | ||
| } |
There was a problem hiding this comment.
The snippet truncation logic is flawed for a search tool. _truncate_snippet (called on line 222) takes the beginning and end of the line, which will likely elide the actual match in long JSONL lines. The snippet should instead be centered around the match location to allow the operator to verify the hit.
Suggested change
| if not pattern.search(stripped): | |
| continue | |
| matches.append( | |
| { | |
| "file": path, | |
| "line": line_no, | |
| "snippet": _truncate_snippet(stripped), | |
| } | |
| match_obj = pattern.search(stripped) | |
| if not match_obj: | |
| continue | |
| matches.append( | |
| { | |
| "file": path, | |
| "line": line_no, | |
| "snippet": _truncate_snippet(stripped, match_obj.start(), match_obj.end()), | |
| } | |
| ) |
Comment on lines
+228
to
+238
| def _truncate_snippet(line: str) -> str: | ||
| """Centre-truncate ``line`` to at most ``_SNIPPET_MAX_CHARS``. | ||
|
|
||
| For lines longer than the budget we keep both ends with a literal | ||
| ``…`` separator so the operator can see the start and tail context | ||
| around the match. Short lines pass through unchanged. | ||
| """ | ||
| if len(line) <= _SNIPPET_MAX_CHARS: | ||
| return line | ||
| half = (_SNIPPET_MAX_CHARS - 1) // 2 | ||
| return f"{line[:half]}…{line[-half:]}" |
There was a problem hiding this comment.
Update _truncate_snippet to center the snippet around the match window rather than just taking the head and tail of the line.
def _truncate_snippet(line: str, start: int, end: int) -> str:
"""Centre-truncate ``line`` around the match to at most ``_SNIPPET_MAX_CHARS``.
For lines longer than the budget we keep a window around the match
with literal ``…`` separators if we truncated either end.
"""
if len(line) <= _SNIPPET_MAX_CHARS:
return line
match_len = end - start
if match_len >= _SNIPPET_MAX_CHARS:
return "…" + line[start : start + _SNIPPET_MAX_CHARS - 2] + "…"
context_budget = _SNIPPET_MAX_CHARS - match_len
left_budget = context_budget // 2
right_budget = context_budget - left_budget
win_start = max(0, start - left_budget)
win_end = min(len(line), end + right_budget)
if win_start == 0:
win_end = min(len(line), win_end + (left_budget - start))
elif win_end == len(line):
win_start = max(0, win_start - (right_budget - (len(line) - end)))
prefix = "…" if win_start > 0 else ""
suffix = "…" if win_end < len(line) else ""
return f"{prefix}{line[win_start:win_end]}{suffix}"
Comment on lines
+54
to
+65
| _PAIRS: Tuple[Tuple[str, str], ...] = ( | ||
| # docs/guides/ | ||
| ("docs/guides/data_audit.md", "docs/guides/data_audit-tr.md"), | ||
| ("docs/guides/gdpr_erasure.md", "docs/guides/gdpr_erasure-tr.md"), | ||
| ("docs/guides/ingestion.md", "docs/guides/ingestion-tr.md"), | ||
| # docs/reference/ | ||
| ("docs/reference/architecture.md", "docs/reference/architecture-tr.md"), | ||
| ("docs/reference/configuration.md", "docs/reference/configuration-tr.md"), | ||
| ("docs/reference/data_preparation.md", "docs/reference/data_preparation-tr.md"), | ||
| ("docs/reference/distributed_training.md", "docs/reference/distributed_training-tr.md"), | ||
| ("docs/reference/usage.md", "docs/reference/usage-tr.md"), | ||
| ) |
There was a problem hiding this comment.
The _PAIRS registry is manually maintained and lacks a mechanism to ensure all bilingual mirrors in the repository are covered. A developer could add a new *-tr.md file and forget to register it, causing the CI gate to silently ignore it. Consider adding a check that discovers all *-tr.md files and verifies their presence in _PAIRS.
Comment on lines
+116
to
+122
| marker = fence_match.group(1)[:3] # collapse runs to first three chars | ||
| if not in_fence: | ||
| in_fence = True | ||
| fence_marker = marker | ||
| elif fence_marker is not None and marker == fence_marker: | ||
| in_fence = False | ||
| fence_marker = None |
There was a problem hiding this comment.
The code fence detection logic is too simplistic. By truncating the fence marker to 3 characters ([:3]), it fails to correctly handle nested fences of different lengths (e.g., a 3-backtick fence inside a 4-backtick fence). A closing fence of 3 backticks would incorrectly close a 4-backtick opening fence. Per CommonMark, a closing fence must be at least as long as the opening fence.
Suggested change
| marker = fence_match.group(1)[:3] # collapse runs to first three chars | |
| if not in_fence: | |
| in_fence = True | |
| fence_marker = marker | |
| elif fence_marker is not None and marker == fence_marker: | |
| in_fence = False | |
| fence_marker = None | |
| if fence_match: | |
| marker = fence_match.group(1) | |
| if not in_fence: | |
| in_fence = True | |
| fence_marker = marker | |
| elif marker.startswith(fence_marker): | |
| in_fence = False | |
| fence_marker = None | |
| continue |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/architecture-tr.md (1)
89-111:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winYeni H3 başlıklarının etrafına boş satır ekleyin (MD022).
### results.py,### benchmark.py,### judge.py,### model_card.pybaşlıklarında alt satır boşluğu eksik görünüyor; markdownlint uyumu için başlık sonrası bir boş satır bırakın.🤖 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/reference/architecture-tr.md` around lines 89 - 111, Add a blank line immediately after each H3 heading to satisfy MD022: insert a single empty line after the `### results.py`, `### benchmark.py`, `### safety.py` (if present), `### judge.py`, and `### model_card.py` headings so the heading is followed by a newline before the paragraph/content that describes it; update those heading blocks in the diff so each heading is separated from its subsequent paragraph by one blank line.
🤖 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/analysis/code_reviews/closure-plan-202604300906.md`:
- Around line 128-130: The "Kalan iş" list contradicts the "Wave 3 kapanış
özeti": update the "Kalan iş" section so its Phase entries match the summary by
removing or marking Faz 24, Faz 28 and Faz 38 as completed (or moving them out
of pending) and ensuring remaining items (Faz 22, 23, 26, 30, 33) remain
correctly listed; locate the "Wave 3 kapanış özeti (2026-05-05...)" and the
"Kalan iş (Wave 3+)" headings and synchronize the phase numbers and statuses
accordingly.
In `@docs/analysis/code_reviews/wave3-review-prompt.md`:
- Around line 18-19: Update the frozen SHA and diff stats in the prompt text:
replace the outdated HEAD SHA `61d74fa` with the current PR SHA `95e2bf8`, and
refresh the diff scope numbers (~32 files, ~685 insertions, ~84 deletions) to
reflect the current snapshot; also remove or change the instruction that tells
reviewers to "use this prompt verbatim" so it references the live SHA
dynamically (or instruct reviewers to confirm the HEAD SHA before use) to avoid
pointing reviewers at a stale snapshot.
In `@docs/reference/audit_event_catalog.md`:
- Around line 68-73: The Article column value for the row titled "Article 15 —
GDPR Right-of-Access (Phase 38 — `forgelm reverse-pii`)" (event
`data.access_request_query`) is ambiguous because it uses the bare number 15
which collides with the EU AI Act entry; update that Article cell to qualify the
statute (for example "GDPR Art. 15" or "GDPR — Art. 15") so it is distinct from
the EU AI Act entry while keeping the rest of the row intact.
In `@docs/usermanuals/tr/concepts/data-formats.md`:
- Line 145: Satırdaki "✓ PII tespit edilmedi (5 orta ciddiyet, rapora bakın)"
ifadesinde mantıksal çelişki var; "✓ PII tespit edilmedi" kısmını ya "✓ PII
tespit edildi" olarak değiştirin ya da parantez içindeki "(5 orta ciddiyet,
rapora bakın)" bilgisini PII dışı bir bulguya açıkça atıf yapacak şekilde
yeniden yazın; hedef metin olarak dosyadaki tam metin "✓ PII tespit edilmedi (5
orta ciddiyet, rapora bakın)" ifadesini bulun ve tutarlı bir ifade olacak
şekilde düzeltin.
In `@docs/usermanuals/tr/training/simpo.md`:
- Line 48: The long Markdown line containing "DPO ile aynı — `prompt`, `chosen`,
`rejected` taşıyan `preference` formatı. Bkz. [Veri
Formatları](`#/concepts/data-formats`)." should be split into two shorter lines to
improve readability and linting; break it just before the link so the first line
reads the description referencing DPO and the `prompt`/`chosen`/`rejected`
`preference` format, and place the "Bkz. [Veri
Formatları](`#/concepts/data-formats`)." on the next line.
In `@forgelm/cli/subcommands/_reverse_pii.py`:
- Around line 176-194: The code currently always calls _resolve_salt(output_dir)
and rejects when its resolved_source differs from the user-requested
salt_source, which prevents honoring an explicit --salt-source choice; change
the flow so the requested salt_source drives which salt retrieval path is used:
if salt_source == "per_dir" attempt to load the per-directory salt (read from
disk for output_dir) and error only if that per-dir salt is missing, if
salt_source == "env_var" read the env-backed secret and error if it's absent,
and if salt_source == "auto" keep existing behavior via
_resolve_salt(output_dir); keep using _hash_target_id(query, salt) and
_output_error_and_exit to report failures but do not reject when resolved_source
!= salt_source—instead explicitly try the retrieval corresponding to
salt_source.
- Around line 216-223: _scan_file is passing the full line to _truncate_snippet
which currently preserves line start/end and can drop the actual regex match for
long lines; change _truncate_snippet to accept the match span (tuple[int,int])
and use that span to center the returned snippet around the matched region
(ensuring the match is always included, adding leading/trailing ellipses when
trimming), then update calls in _scan_file (and the other similar call at lines
228-238) to pass match.span() (or match.start()/end()) along with the line so
the truncated snippet always contains the identifier that triggered the hit.
- Around line 275-311: The audit logger currently silently disables on init
errors and later audit.log_event calls can raise unhandled exceptions; change
behavior so that if the user explicitly passed --audit-dir (i.e. getattr(args,
"audit_dir", None) is not None) and _maybe_audit_logger(...) returns None or
raises, call _output_error_and_exit with a clear message and
EXIT_TRAINING_ERROR; additionally wrap each audit.log_event call (the ones that
log _EVT_ACCESS_REQUEST_QUERY) in a try/except Exception as exc and on any
exception call _output_error_and_exit with the exception details and
EXIT_TRAINING_ERROR so auditing failures follow the runtime-error contract
(apply the same fixes to the later block referenced around the
_EVT_ACCESS_REQUEST_QUERY calls at the other location).
- Around line 113-122: The custom-regex branch currently returns
re.compile(query) with no ReDoS protections; add a validation step (e.g.
validate_regex_safety(query)) called before re.compile when identifier_type ==
"custom" that enforces the project's regex rules (static checks from
docs/standards/regex.md such as rejecting multiple competing unbounded
quantifiers, backreference+lazy quantifier patterns, or disallowed \s under
re.MULTILINE) and/or runs a small empirical linearity check (feed pathological
inputs at ~1K/5K/10K lengths, measure median runtimes, and fail if growth is
superlinear) and call _output_error_and_exit with a clear message if validation
fails, then compile; also add a ReDoS regression test in
tests/test_reverse_pii.py that pins acceptable scaling behavior and asserts
unsafe patterns are rejected by validate_regex_safety.
---
Outside diff comments:
In `@docs/reference/architecture-tr.md`:
- Around line 89-111: Add a blank line immediately after each H3 heading to
satisfy MD022: insert a single empty line after the `### results.py`, `###
benchmark.py`, `### safety.py` (if present), `### judge.py`, and `###
model_card.py` headings so the heading is followed by a newline before the
paragraph/content that describes it; update those heading blocks in the diff so
each heading is separated from its subsequent paragraph by one blank line.
🪄 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: d6b51ae9-a731-4fc3-b09c-6f051404ca42
📒 Files selected for processing (38)
.github/workflows/ci.yml.gitignoreCHANGELOG.mddocs/analysis/code_reviews/closure-plan-202604300906.mddocs/analysis/code_reviews/wave3-review-prompt.mddocs/guides/alignment.mddocs/guides/gdpr_erasure-tr.mddocs/guides/gdpr_erasure.mddocs/guides/ingestion-tr.mddocs/reference/architecture-tr.mddocs/reference/audit_event_catalog-tr.mddocs/reference/audit_event_catalog.mddocs/reference/configuration-tr.mddocs/reference/distributed_training-tr.mddocs/usermanuals/en/reference/cli.mddocs/usermanuals/en/reference/json-output.mddocs/usermanuals/tr/compliance/overview.mddocs/usermanuals/tr/concepts/data-formats.mddocs/usermanuals/tr/reference/cli.mddocs/usermanuals/tr/reference/json-output.mddocs/usermanuals/tr/training/sft.mddocs/usermanuals/tr/training/simpo.mdforgelm/cli/__init__.pyforgelm/cli/_dispatch.pyforgelm/cli/_parser.pyforgelm/cli/subcommands/_reverse_pii.pyforgelm/compliance.pyforgelm/config.pyforgelm/deploy.pyforgelm/trainer.pyforgelm/webhook.pytests/test_check_bilingual_parity.pytests/test_compliance.pytests/test_data_edge_cases.pytests/test_eu_ai_act.pytests/test_integration.pytests/test_reverse_pii.pytools/check_bilingual_parity.py
👮 Files not reviewed due to content moderation or server errors (6)
- tests/test_eu_ai_act.py
- docs/guides/gdpr_erasure-tr.md
- CHANGELOG.md
- tests/test_check_bilingual_parity.py
- tools/check_bilingual_parity.py
- tests/test_reverse_pii.py
Comment on lines
+18
to
+19
| - **HEAD SHA at prompt freeze:** `61d74fa` | ||
| - **Diff scope vs `development`:** ~32 files, ~685 insertions, ~84 deletions. |
Contributor
There was a problem hiding this comment.
Refresh the frozen SHA and diff stats before telling reviewers to use this prompt verbatim.
The prompt anchors agents to 61d74fa, but the PR context here is already at 95e2bf8. That makes the “use this prompt verbatim” instruction stale and can send reviewers to the wrong snapshot/diff size.
Also applies to: 362-362
🤖 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/analysis/code_reviews/wave3-review-prompt.md` around lines 18 - 19,
Update the frozen SHA and diff stats in the prompt text: replace the outdated
HEAD SHA `61d74fa` with the current PR SHA `95e2bf8`, and refresh the diff scope
numbers (~32 files, ~685 insertions, ~84 deletions) to reflect the current
snapshot; also remove or change the instruction that tells reviewers to "use
this prompt verbatim" so it references the live SHA dynamically (or instruct
reviewers to confirm the HEAD SHA before use) to avoid pointing reviewers at a
stale snapshot.
…dings
Convergent CRITICAL/HIGH absorptions across the 4 reviewer reports
(code-correctness, privacy-security, standards, test-rigor) on the
Phase 38 reverse-pii surface:
* F-W3-01 / F-W3-PS-02 / F-W3S-01 — audit fail-closed.
ConfigError still skips with a WARNING (was DEBUG); every other
exception class refuses with EXIT_TRAINING_ERROR naming the audit
dir. Closes the silent-drop path that let Article 15 access
requests complete without a forensic-chain entry on read-only
audit dirs.
* F-W3-PS-01 — salted audit hash. _hash_for_audit now reuses the
same per-output-dir salt purge uses for target_id, defeating
wordlist attacks against the audit log AND making purge ↔
reverse-pii digests cross-tool correlatable for the same subject.
* F-W3-02 — default --type changed `custom` → `literal`. Stops
silently treating dots in e-mails as regex wildcards on the
no-flag path documented in the operator guide.
* F-W3-03 — snippet truncation centred on the match span. The
matched identifier is preserved within the 160-char window; long
lines no longer ellide the very thing the operator is trying to
verify.
* F-W3-04 / F-W3-PS-11 — UnicodeDecodeError caught on mid-scan; emits
a failure-flavoured `data.access_request_query` event and exits
EXIT_TRAINING_ERROR (was Python's uncontrolled exit 1 with no
audit record).
* F-W3-05 — docs no longer claim a "ForgeLM hash strategy" the
toolkit does not ship. Hash-mode framed as "external pipeline
using purge's salt"; gdpr_erasure.md (EN+TR) + dispatcher
docstring updated.
* F-W3-06 / F-W3-PS-06 / F-W3S-02 — audit-dir default
`output_dir → <output_dir>/audit/`. The Article 15 forensic
record no longer co-locates with the corpus the subject is
asking about.
* F-W3-PS-03 / F-W3-07 / F-W3S-03 — ReDoS guard for `--type custom`
via per-file SIGALRM budget (POSIX-only; 30s). A pathological
pattern surfaces as OSError → EXIT_TRAINING_ERROR + failure audit
event instead of pegging a CPU core indefinitely.
* F-W3-08 — regex validation runs before salt resolution. An
unparseable `--type custom` regex no longer creates the salt
file as a side effect of the config-error exit.
* F-W3-09 — `error_message` audit field bounded to 200 chars,
closing the future-leak risk where a refactor could surface raw
query data through `str(exc)`.
* F-W3-PS-04 — `--salt-source per_dir` with `FORGELM_AUDIT_SECRET`
set now refuses with a direction-aware diagnostic naming the
unset path.
* F-W3-PS-07 — `salt_source` field recorded in every audit event
(plaintext / per_dir / env_var); compliance reviewer sees how
two digests were salted.
* F-W3-12 — directory positional argument produces a targeted
diagnostic naming the glob form, not a generic empty-glob error.
* F-W3-XPR-PS-02 — text-mode envelope prints a privacy banner
warning operators not to redirect snippet output to a persistent
log.
Test-rigor HIGH absorptions:
* F-W3T-01 — failure-path audit event no-leak assertion (the same
json.dumps membership check as the success path).
* F-W3T-02 — `unacceptable + safety.disabled` raise pinned via
parametrize over both `_STRICT_RISK_TIERS` members.
* F-W3T-03 — setext heading exclusion regression in the parity
tool's heading extractor.
* F-W3T-05 — multi-byte UTF-8 (CJK + emoji) snippet truncation
test.
* F-W3T-07 — orphan-TR-mirror meta-test pinning the `_PAIRS`
registry as source of truth for guides + reference dirs (with
documented allowlist for safety_compliance-tr.md whose
EN→TR completion is a Wave 4 task).
* F-W3T-08 — docstring on the live-repo smoke test explaining the
deliberate redundancy with the CI lint job.
* F-W3T-09 — `WebhookConfig.test_defaults` pins `timeout == 10`
next to every other field default.
* F-W3T-10 — `--audit-dir` explicit + audit-init OSError fail-closed
regression.
* F-W3T-12 — overlapping-glob dedupe regression.
Standards low-cost:
* F-W3S-04 — webhook.py timeout literal-5 fallback now reads from
`WebhookConfig.model_fields["timeout"].default`; clamp message
reports the real default.
* F-W3S-06 — tools/check_bilingual_parity.py module docstring now
documents its 0/1 exit-code contract for CI gate authors.
* F-W3S-07 — `.claude/skills/add-test/SKILL.md` and
`add-trainer-feature/SKILL.md` updated to reference the post-rename
`tests/test_integration.py` (was `test_integration_smoke.py`).
* F-W3S-09 — `_validate_identifier_type` docstring notes argparse
enforces the choice set; the dispatcher guard is for direct
library callers.
Inline PR review comments:
* `closure-plan-202604300906.md:130` — "Kalan iş" listesi Wave 3
kapanış özeti ile senkronize: Faz 24 + 28 + 38 kaldırıldı,
pending sayısı 7 → 4 + 1 release.
* `audit_event_catalog{,-tr}.md` — `data.access_request_query`
Article column "15" → "GDPR Art. 15" / "GDPR Md. 15"
(disambiguates from EU AI Act Article 15); payload column
refreshed for new fields (salted query_hash, salt_source,
literal type).
* `data-formats.md` (EN + TR L145) — "✓ no PII detected (5 medium
severity, see report)" mantıksal çelişkisi → "⚠ PII detected: 5
medium severity (see report)". Both locales fixed; example
now self-consistent.
* `simpo.md` (TR L48) — uzun Markdown satırı iki satıra bölündü.
* `_reverse_pii.py` `--audit-dir` flow — explicit-flag awareness +
fail-closed handler, per the inline ask.
Skipped with reasons:
* `wave3-review-prompt.md:18-19` SHA refresh — review prompts are
frozen artefacts pointing at the snapshot the reviewers reviewed
(61d74fa). Refreshing it now would invalidate the connection
between the 4 review reports we are absorbing AND the snapshot
they reviewed. The next review wave will write its own prompt
pointing at a fresh SHA.
* `architecture-tr.md` MD022 blank-lines-after-H3 — the EN
counterpart uses the same no-blank-line pattern; no markdownlint
runs in CI; blank-line-only changes do not affect parity.
Adding blank lines to TR alone would diverge from EN
unnecessarily. If MD022 ever becomes a CI gate, fix both
mirrors in the same commit.
* F-W3-PS-05 (symlink follow), F-W3-PS-08 (DoS via crafted corpus
size cap), F-W3-PS-10 (grapheme cluster split), F-W3-PS-13/14
(NITs), F-W3-XPR-PS-01 (per-event field allowlist refactor),
F-W3T-04 (true purge → reverse-pii integration test),
F-W3T-13/14 (NITs), F-W3S-05 (INFO startup line),
F-W3S-08 (audit-catalog row format polish) — defer to Wave 4.
* F-W3-PS-09 / F-W3-PS-12 / F-W3T-06 — subsumed by the F-W3-PS-01
+ F-W3-04 + F-W3-PS-03 absorptions.
Suite 1333 → 1344 tests (+11 net). Lint clean. Dry-run green.
Bilingual parity passes against 9 doc pairs (was 8; added
audit_event_catalog).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* python:S1172 (MAJOR) — `_scan_files_with_redos_guard` `output_format` parameter is unused; dropped (and from the call site). No public-API break; the helper is private to the dispatcher module. * python:S2201 (MAJOR) — multi-byte UTF-8 truncation regression test was discarding the `bytes.decode` return value. Now asserts the round-trip equals the original snippet, which is the actual contract ("no broken surrogates from a slice landing mid-rune"). * python:S3776 (CRITICAL) — `ForgeConfig._warn_high_risk_compliance` cognitive complexity 20 vs 15 allowed. Refactored into three focused helpers (`_resolve_risk_label`, `_warn_unacceptable_practice`, `_enforce_safety_gate_for_strict_tier`) plus a thin orchestrator. No behaviour change: the F-compliance-110 ConfigError raise, the Article 5 prohibited-practices banner, and the auto_revert / track_categories warnings all fire identically; the existing parametrised tests in test_eu_ai_act.py + test_integration.py continue to pin the contract. * DOS hotspot (MEDIUM) — `tools/check_bilingual_parity.py:_HEADING_RE` used `^(#{1,6})\s+(.+?)\s*#*\s*$`, which chains a lazy `(.+?)` against a trailing `\s*#*\s*$` — polynomial backtracking on a pathological all-spaces-and-hashes line. Split heading recognition into a linear-time prefix match (`^(#{1,6}) +`) plus a deterministic trailing-hash trim (`\s+#+$`); no nested quantifiers, no chained lazy/greedy interplay. Live-tree `--strict` invocation still passes against all 9 doc pairs. Skipped: * publicly-writable-directories hotspot at `test_integration.py:285` flags `model_path="/tmp/model"` — a string literal passed as a structured-log payload, never written to disk. False positive on test fixture data; would suppress in Sonar UI rather than alter the test. Codacy 2.0 / v3 issue API requires auth; the 21 Codacy issues (2 high · 19 minor) overlap heavily with Sonar's smaller authoritative set. Re-running CI on this push will surface any remaining Codacy finding that Sonar did not catch. Suite: 1344 passed, 14 skipped (no regressions). Lint clean. Bilingual parity: 9/9 OK against the live tree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* python:S5852 (MEDIUM, dos) `tools/check_bilingual_parity.py:94` — the previous absorption split heading recognition into a prefix regex + a trailing-hash regex (`\s+#+$`). Sonar conservative analysis still flagged the trailing regex's adjacent quantifiers even though the two character classes (`\s` vs `#`) cannot overlap — no actual backtracking surface exists. To clear the hotspot rather than annotate it, replaced the trailing regex entirely with a deterministic O(n) Python walk (`_strip_atx_close_and_whitespace`); zero regex on the ATX-close-trim path. Behaviour identical: ATX-closing hashes are stripped only when preceded by whitespace, matching CommonMark §4.2. All 16 parity-tool regression tests still pass; live-tree `--strict` still 9/9 OK. * python:S5443 (LOW) `tests/test_integration.py:285` — the `model_path="/tmp/model"` string literal was a structured-log payload, never written to disk, so this was a Sonar false positive on test fixture data. Routed the path-shaped literal through pytest's per-test ``tmp_path`` fixture instead; cosmetic clean-up. Test still asserts the same audit-event chain shape. Suite: 1344 passed, 14 skipped (no regressions). Lint clean. Bilingual parity: 9/9 OK. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`tools/check_bilingual_parity.py` previously truncated fence markers to 3 characters (`fence_match.group(1)[:3]`), so a 4-backtick opener could be closed by a 3-backtick line — violating CommonMark §4.5 (closing fence must be at least as long as the opener) AND letting heading-shaped content inside a long fence sneak into the parity tool's heading counts. Now stores the full opener and requires the closer to: 1. Use the same marker character (backtick vs tilde — already partly enforced; now explicit), AND 2. Be at least as long as the opener (the new bit). Two regression tests pin both invariants: * `test_longer_opening_fence_not_closed_by_shorter_run` — 4-backtick fence with a 3-backtick "close" inside; headings inside stay as code, not as document structure. * `test_backtick_fence_not_closed_by_tilde_fence` — tilde line cannot close a backtick fence. Suite 1344 → 1346. Lint clean. Live-tree `--strict` 9/9 OK. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
Wave 3 absorption summary —
|
| Comment | File:Line | Status |
|---|---|---|
| Kalan iş list contradicts Wave 3 kapanış özeti | closure-plan-202604300906.md:130 | ✅ 5491572 — synced; pending count 7 → 4 |
| Frozen SHA refresh in review prompt | wave3-review-prompt.md:19 | ⏭️ Skipped — review prompts are frozen artefacts pointing at the snapshot the reviewers reviewed (61d74fa); refreshing would invalidate the connection between the 4 review reports we are absorbing AND the snapshot they reviewed |
| Article 15 column ambiguity (GDPR vs EU AI Act) | audit_event_catalog.md:73 + -tr.md |
✅ 5491572 — Article column → "GDPR Art. 15" / "GDPR Md. 15" |
| TR data-formats L145 logical contradiction | data-formats.md:145 | ✅ 5491572 — fixed in both EN and TR |
| TR simpo L48 long-line split | simpo.md:48 | ✅ 5491572 — split |
--type custom ReDoS guard |
_reverse_pii.py:164 | ✅ 5491572 — POSIX SIGALRM 30s per-file budget; pathological pattern surfaces as OSError → EXIT_TRAINING_ERROR + failure audit event (chose timeout over static AST validator per the comment's "Option 3" suggestion) |
Honor requested --salt-source instead of comparing |
_reverse_pii.py:194 | ✅ 5491572 — direction-aware diagnostic on mismatch (per_dir requested + env var set → "unset FORGELM_AUDIT_SECRET" hint) |
| Snippet truncator removes match span | _reverse_pii.py:223 | ✅ 5491572 — _truncate_snippet now takes match_span; centred window preserves the matched identifier |
--audit-dir failures don't follow runtime-error contract |
_reverse_pii.py:311 | ✅ 5491572 — _maybe_audit_logger fail-closed on non-ConfigError; explicit --audit-dir + OSError/ValueError → EXIT_TRAINING_ERROR |
Gemini Code Assist (5 inline finding) — all absorbed
| Comment | File:Line | Status |
|---|---|---|
| UTF-8 decode error handling missing | _reverse_pii.py:315 | ✅ 5491572 — UnicodeDecodeError caught; emits failure audit event + exit 2 |
| Snippet truncation flawed (not match-centred) | _reverse_pii.py:223 | ✅ 5491572 — same fix as CodeRabbit's |
_truncate_snippet rewrite |
_reverse_pii.py:238 | ✅ 5491572 |
_PAIRS registry has no orphan check |
check_bilingual_parity.py:76 | ✅ 5491572 — test_every_tr_mirror_appears_in_pair_registry meta-test added (with documented allowlist for safety_compliance-tr.md whose EN→TR completion is a Wave 4 task) |
| Code fence detection too simplistic — 4-backtick fence closed by 3-backtick line | check_bilingual_parity.py:141 | ✅ cd66c54 — full opener stored; closer must be same marker char AND ≥ opener length per CommonMark §4.5; 2 regression tests added |
SonarCloud (3 issues + 2 hotspots) — all closed
| Rule | File:Line | Status |
|---|---|---|
python:S1172 (MAJOR) — unused output_format parameter |
_reverse_pii.py:335 | ✅ 6ba93be |
python:S2201 (MAJOR) — bytes.decode return value unused |
test_reverse_pii.py:655 | ✅ 6ba93be — round-trip equality assertion added |
| python:S3776 (CRITICAL) — Cognitive Complexity 20 > 15 | config.py:843 | ✅ 6ba93be — _warn_high_risk_compliance refactored into 3 focused helpers; behaviour unchanged |
| python:S5852 (MEDIUM, dos) — heading regex polynomial backtracking | check_bilingual_parity.py:94 | ✅ 6ba93be + 730a015 — split prefix regex + pure-Python deterministic ATX-close trim; zero regex on the trim path |
python:S5443 (LOW) — publicly-writable directory /tmp/model |
test_integration.py:285 | ✅ 730a015 — routed through tmp_path fixture; structured-log payload only, no file written |
SonarCloud Quality Gate: ✅ PASSED (was FAILED — 0 issues, 0 hotspots).
Codacy — partial (auth-gated API)
Codacy's v2/v3 API requires auth; couldn't itemize the 21 issues programmatically. Categorical overlap with SonarCloud's authoritative set (ErrorProne ↔ S1172/S2201, BestPractice ↔ S3776, CodeStyle ~ ruff) suggests the absorption commits cleared the high-severity items. Latest Codacy report (6ba93be) shows 2 high · 18 minor (was 2 high · 19 minor). Remaining minor items deferred — surfaced in the next absorption pass via Codacy UI walkthrough rather than guess-and-fix.
Other bots
- Sourcery: weekly rate limit hit (no review).
- Qodo: monthly free-tier limit hit (no review).
Validation gauntlet (every absorption commit)
ruff format+ruff checkcleanpytest tests/— 1333 → 1346 passed (+13 net), 14 skippedforgelm --config config_template.yaml --dry-rungreenpython3 tools/check_bilingual_parity.py --strict— 9/9 OK (was 8; addedaudit_event_catalog)
PR is ready for human review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@forgelm/cli/subcommands/_reverse_pii.py`:
- Around line 510-525: The implicit defaulting of output_dir inside
_run_reverse_pii_cmd silently breaks salt resolution and stored-digest
invariants when salt_source == "per_dir"; update _run_reverse_pii_cmd so that
when getattr(args, "output_dir", None) is None and salt_source == "per_dir" (or
whenever _resolve_query_form/_resolve_salt will be used) you either (a) raise a
user-facing error asking the operator to supply --output-dir, or (b) emit a
clear WARNING that the corpus directory
(os.path.dirname(os.path.abspath(files[0]))) will be used to create/resolve
.forgelm_audit_salt and that this may break cross-tool correlation; also remove
the unreachable trailing "or '.'" fallback. Reference: _run_reverse_pii_cmd,
salt_source, output_dir, _resolve_query_form, and _resolve_salt.
🪄 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: cfb42d27-b4f4-45ff-8ba1-e2709097b30f
📒 Files selected for processing (24)
.claude/skills/add-test/SKILL.md.claude/skills/add-trainer-feature/SKILL.mdCHANGELOG.mddocs/analysis/code_reviews/closure-plan-202604300906.mddocs/guides/gdpr_erasure-tr.mddocs/guides/gdpr_erasure.mddocs/reference/audit_event_catalog-tr.mddocs/reference/audit_event_catalog.mddocs/usermanuals/en/concepts/data-formats.mddocs/usermanuals/en/reference/json-output.mddocs/usermanuals/tr/concepts/data-formats.mddocs/usermanuals/tr/reference/json-output.mddocs/usermanuals/tr/training/simpo.mdforgelm/cli/__init__.pyforgelm/cli/_parser.pyforgelm/cli/subcommands/_reverse_pii.pyforgelm/config.pyforgelm/webhook.pytests/test_check_bilingual_parity.pytests/test_config.pytests/test_eu_ai_act.pytests/test_integration.pytests/test_reverse_pii.pytools/check_bilingual_parity.py
✅ Files skipped from review due to trivial changes (12)
- docs/usermanuals/en/concepts/data-formats.md
- .claude/skills/add-trainer-feature/SKILL.md
- docs/usermanuals/tr/concepts/data-formats.md
- docs/reference/audit_event_catalog.md
- tests/test_config.py
- docs/reference/audit_event_catalog-tr.md
- docs/usermanuals/tr/training/simpo.md
- docs/usermanuals/tr/reference/json-output.md
- docs/usermanuals/en/reference/json-output.md
- CHANGELOG.md
- docs/guides/gdpr_erasure.md
- docs/analysis/code_reviews/closure-plan-202604300906.md
🚧 Files skipped from review as they are similar to previous changes (4)
- forgelm/webhook.py
- tests/test_eu_ai_act.py
- forgelm/cli/_parser.py
- tools/check_bilingual_parity.py
…drop unreachable fallback `_run_reverse_pii_cmd` previously had two issues at the ``output_dir`` resolution step: * The trailing ``or "."`` fallback in ``getattr(args, "output_dir", None) or os.path.dirname(os.path.abspath(files[0])) or "."`` was unreachable: ``os.path.abspath()`` always returns an absolute path, and ``os.path.dirname()`` of an absolute path is at minimum ``"/"`` — never falsy. Removed. * When ``--salt-source`` was set but ``--output-dir`` was not, the dispatcher silently fell back to the corpus parent directory for salt-file resolution. ``_resolve_salt(corpus_parent)`` then created ``.forgelm_audit_salt`` next to the corpus, breaking the cross-tool correlation invariant: a ``forgelm purge --output-dir=./outputs`` invocation for the same subject computes a different salt and therefore a different digest. Now emits a WARNING naming the resolved fallback dir + the cross-tool correlation consequence so the operator can either supply ``--output-dir`` or accept the scope. No hard refusal — plaintext-mode invocations are common and need no warning, and even hash-mask operators may legitimately scan corpus dirs directly. Two regression tests pin both halves of the fix: * ``test_implicit_output_dir_with_salt_source_warns_about_correlation_risk`` — implicit + ``--salt-source per_dir`` surfaces a WARNING with "Cross-tool" in the message and the salt file lands in the corpus dir as expected. * ``test_implicit_output_dir_without_salt_source_does_not_warn`` — plaintext-mode runs (no ``--salt-source``) do NOT trigger the cross-tool nag; the warning is scoped to hash-mask invocations. Suite 1346 → 1348. Lint clean. Live-tree parity 9/9 OK. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Frozen at 7adecb1. Scopes the second-pass review to the 5 absorption commits since 95e2bf8 (the original 4-agent review snapshot): 5491572 — 4-agent review + 6 inline PR comment findings 6ba93be — 3 SonarCloud + 1 hotspot 730a015 — 2 remaining hotspots cd66c54 — gemini-code-assist fence-length 7adecb1 — implicit --output-dir cross-tool correlation warning Two-question framing: 1. Did each absorption correctly fix the original finding it claims to absorb? ("F-<id> [verified]" / "[partial]" / "[regressed]" bookkeeping in the deliverable.) 2. Did the absorption itself introduce defects? (5491572 rewrote most of `_reverse_pii.py` — same bar as new code.) Same 4-agent split as the original review: code-correctness / privacy-security / standards / test-rigor. Each report saved as `wave3fu-<agent>.md` so the original `wave3-<agent>.md` reports stay readable as the pre-absorption baseline. Re-allowed in .gitignore alongside the original review prompt (public artefact, not internal analysis). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nvergent findings) The 4 wave-3 follow-up review reports identified 5 HIGH-severity regressions in the prior absorption commits plus a set of MEDIUM/LOW sharpening fixes. All convergent items absorbed in this single commit; deferred items documented at the end. HIGH absorptions ================ * F-W3FU-01 (cc) + F-W3FU-S-01 (std) — F-compliance-110 strict-gate OR-across-fields semantics regressed. The 6ba93be cognitive- complexity refactor that absorbed Sonar python:S3776 silently inverted the gate's OR semantics: when ``risk_assessment.risk_category`` AND ``compliance.risk_classification`` disagree (one strict, one not), the resolver picked ``risk_assessment`` first, the strict membership test failed, and the F-compliance-110 ConfigError raise was silently bypassed. Asymmetric-tier YAML reachable from hand- written configs. Fix: split label resolution (display-only) from strict-tier detection (OR across both sibling fields) via ``_risk_tiers`` + ``_is_strict_tier`` + ``_is_unacceptable`` helpers. 4-tier parametrize regression test (``test_asymmetric_strict_tier_still_raises_when_safety_disabled``) + Article-5 banner test pin both the gate AND the unacceptable warning across asymmetric configs. * F-W3FU-01 (priv) + F-W3FU-S-03 (std) — audit-dir default move (``<output_dir>/audit/``) broke cross-tool correlation with ``forgelm purge`` (which writes to ``<output_dir>/audit_log.jsonl`` directly). ``verify-audit`` correlating Article 17 + Article 15 events for the same subject saw two disjoint chains. Fix: revert ``_resolve_audit_dir`` default to ``output_dir`` so both subcommands write into one chain. The cross-tenant write concern that motivated the move is now addressed by the implicit-output-dir warning at the dispatcher entry (which names the resolved fallback dir loudly). Parser help text + CHANGELOG + gdpr_erasure operator guide (EN+TR) updated. * F-W3FU-T-01 — POSIX SIGALRM ReDoS guard had ZERO regression coverage. The 5491572 absorption shipped 251 lines of new code (`_scan_files_with_redos_guard` + `_scan_file_with_alarm`) without pinning any of the three load-bearing contracts (pathological pattern terminates within budget, alarm-reset on success, thread- safety on non-main thread). Fix: 3 focused tests under new ``TestReDoSGuard`` class — one POSIX-only catastrophic-backtracking test that monkeypatches ``_CUSTOM_REGEX_TIMEOUT_S`` to 1s and verifies exit 2 + failure audit event; one outer-alarm-budget preservation test (also pins the F-W3FU-03 fix below); one worker-thread no-op test that exercises ``_scan_files_with_redos_guard`` from a ``threading.Thread`` and asserts no crash. * F-W3FU-02 (cc) + F-W3FU-T-02 — ``_truncate_snippet`` exceeded ``_SNIPPET_MAX_CHARS`` when the match span itself was wider than the budget. The centring math (``ctx = max((budget - match_len) // 2, 0)``) floored at 0 in the degenerate case, returning the full match span unbounded. A ``--type custom --query ".+"`` against a 500-char line emerged as a 502-char snippet. Fix: when ``match_len >= budget``, slice a centred window of the match itself with leading/trailing ellipses. Regression test (``test_match_span_wider_than_budget_still_bounded``) plants a 250-char match in a 264-char line and pins the cap. * F-W3FU-05 (priv) + F-W3FU-T-03 — UnicodeDecodeError failure-path no-leak invariant unpinned. The OSError leg's no-leak assertion did not transfer to the symmetric UnicodeDecodeError test, and the test used a low-entropy ``query="x"`` that would have made any leak invisible. Fix: switch to ``query="alice@example.com"``, add ``"alice@example.com" not in json.dumps(evt)`` assertion, add positive-shape assertion ``evt["query_hash"] == _hash_for_audit(...)`` to pin the salted-hash contract. Test renamed ``test_malformed_utf8_corpus_exits_runtime_error_with_audit_event_and_no_leak``. MEDIUM absorptions ================== * F-W3FU-02 (priv) — ``salt_source_label`` never returned ``"plaintext"`` despite the docstring promise; plaintext-mode scans recorded the audit-hash salt source (``per_dir`` / ``env_var``) and a forensic reviewer could not distinguish hash-mask scans from plaintext ones. Fix: ``_resolve_query_form`` returns ``"plaintext"`` for the scan label in plaintext mode. * F-W3FU-03 (priv) — audit fail-closed ``(OSError, ValueError)`` catch tuple was narrower than the original F-W3-PS-02 ask. A future hardening hook in ``AuditLogger.__init__`` raising RuntimeError / KeyError / etc. would silently re-introduce the silent-drop failure mode. Fix: widen to bare ``Exception`` (with BLE001 noqa + comment) — the contract is "every non-ConfigError fails closed", not "every class we currently know about". * F-W3FU-03 (cc) — ``signal.alarm(N)`` returns the previously- scheduled alarm's remaining seconds. ``_scan_file_with_alarm`` was discarding the return value and calling ``signal.alarm(0)`` in ``finally``, permanently cancelling any outer SIGALRM budget. Fix: capture ``previous_alarm_remaining`` and re-arm in the ``finally`` clause (rounded up to 1s — sub-second remainders cannot survive alarm-syscall granularity). * F-W3FU-04 (cc) — ``signal.signal(SIGALRM, ...)`` raises ``ValueError`` from non-main threads on POSIX. The guard at ``_scan_files_with_redos_guard`` only checked ``os.name == "posix"`` + ``hasattr(SIGALRM)``; a thread-context call would crash. Fix: also require ``threading.current_thread() is threading.main_thread()`` before use. Worker thread test pins the fall-through. * F-W3FU-T-04 — failure-path no-leak test gained the positive-shape hash assertion + base64/hex transformed-form negative-shape assertions. Closes the substring-only-check weakness called out in F-W3-PS-12. * F-W3FU-T-05 — strict-tier ``unacceptable`` integration coverage: ``test_high_risk_without_safety_raises_config_error`` parametrised over both ``high-risk`` and ``unacceptable`` tiers. Pins the YAML-round-trip + Pydantic-validator interaction at the integration layer. * F-W3FU-T-06 — ``test_shorter_opening_fence_closed_by_longer_run`` pins the CommonMark §4.5 ``len(closer) >= len(opener)`` semantics so a hand-edit dropping ``>=`` to ``==`` would surface in CI. LOW / NIT absorptions ===================== * F-W3FU-06 (priv) — implicit-output-dir warning broadened to fire in plaintext mode too (the salt-file side effect is present regardless of scan mode). Tag the message with whether hash-mask is also in play. Test ``test_plaintext_implicit_output_dir_also_warns_about_salt_file`` replaces the previous ``..._does_not_warn`` test (the contract inverted with the broadening). * F-W3FU-06 (cc) — ``_emit_audit_event`` failure-branch chains the original mid-scan exception's diagnostic context into the audit-failure message so a double-fault (corpus read fails AND audit chain write fails) preserves the underlying cause. * F-W3FU-08 (priv) — dropped the unreachable ``salt: Optional[bytes]`` ``None`` branch in ``_hash_for_audit``. Type-tightened to ``salt: bytes``; a future refactor passing ``None`` is now a static error rather than a silent unsalted-hash downgrade. * F-W3FU-S-04 — webhook ``isinstance(timeout, (int, float))`` guard dropped (dead under Pydantic int-validation). Numeric-floor clamp retained. * F-W3FU-S-02 — ``gdpr_erasure.md`` (EN+TR) operator guide gained an "Audit-dir default" paragraph naming the ``<output-dir>/audit_log.jsonl`` fallback and the explicit-override fail-closed behaviour. * F-W3FU-T-09 — ``test_setext_underline_after_atx_heading_does_not_register`` pins setext-after-ATX coverage as a sibling to the existing setext-at-doc-start test. * F-W3FU-T-11 — facade re-exports test strengthened: each name must be ``callable``, not just ``hasattr``. * F-W3FU-T-12 — cross-tool warning substring match made case-insensitive (``"cross-tool" in r.message.lower()``) so a future log-style sweep that lower-cased warning prefixes does not break the test brittlely. Skipped with reason =================== * F-W3FU-T-07 — true purge → reverse-pii driver-level integration test. Helper-shape correlation already pinned at ``test_purge_target_id_matches_reverse_pii_query_hash_on_same_output_dir``. Driver-level test is a Wave 4 task per the original report's classification. * F-W3FU-T-08 — indented fence (CommonMark §4.5 ≤3 leading spaces). ForgeLM docs do not use indented fences; strict no-indent policy is acceptable for the parity tool's curated domain. Defer xfail pin to Wave 4. * F-W3FU-T-10 — ``safety_compliance-tr.md`` allowlist tracking ticket. Comment in test docstring is sufficient for the current cycle; xfail-shape refactor is a Wave 4 nicety. * F-W3FU-S-05 — audit-event catalog row formatting (NIT, F-W3S-08 carry-over). Cosmetic; defer. * F-W3FU-S-06 — parity tool summary message hard-codes ``H2/H3/H4`` even when ``--levels`` is overridden. NIT; defer. * F-W3FU-S-08 — deprecated ``--data-audit`` alias help text references stale ``./audit/`` default. NIT; flagged for future-author awareness only. * F-W3FU-07 (priv) — implicit-output-dir warning severity (WARNING vs ERROR). Judgement call; WARNING is defensible — operator is informed, scan still runs. Promotion to ERROR would break the documented quickstart workflow where operators legitimately scan corpus dirs directly. * F-W3FU-05 (cc) — ``_maybe_audit_logger`` ConfigError fall-through on explicit ``--audit-dir``. Privacy-policy call: the ConfigError-best-effort branch is documented in the docstring as the only "ConfigError is the operator-identity branch" exception to fail-closed; switching to fail-closed on explicit ``--audit-dir`` with ConfigError would force CI runners without ``FORGELM_OPERATOR`` set to refuse the run. Acceptable trade-off; can revisit if a privacy regulator surfaces the gap. Suite 1348 → 1360 tests (+12 net). Lint clean. Dry-run green. Bilingual parity 9/9 OK. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
forgelm/cli/subcommands/_reverse_pii.py (1)
241-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--salt-source=per_dirstill does not work whenFORGELM_AUDIT_SECRETis exported.This path always calls
_resolve_salt(output_dir)first, so a host-levelFORGELM_AUDIT_SECRETforcesresolved_source == "env_var"and the explicitper_dirrequest is rejected. That breaks the advertisedper_dirmode on any environment where the audit secret is set globally, even though the operator asked to read<output_dir>/.forgelm_audit_salt.♻️ Proposed fix
- try: - salt, resolved_source = _resolve_salt(output_dir) - except OSError as exc: - _output_error_and_exit( - output_format, - f"Could not resolve audit salt for {output_dir!r}: {exc}", - EXIT_TRAINING_ERROR, - ) + try: + if salt_source == "per_dir": + salt = _read_persistent_salt(output_dir) + resolved_source = "per_dir" + else: + salt, resolved_source = _resolve_salt(output_dir) + except OSError as exc: + _output_error_and_exit( + output_format, + f"Could not resolve audit salt for {output_dir!r}: {exc}", + EXIT_TRAINING_ERROR, + ) if salt_source is None: return query, "plaintext", salt, "plaintext" - if salt_source != resolved_source: + if salt_source != resolved_source: ...You can keep
_read_persistent_salt()tiny and delegate the shared file-format details to_purge.pyso the two commands stay aligned.Based on learnings from
forgelm/cli/subcommands/_purge.py:112-167,_resolve_salt()always resolves to"env_var"whenFORGELM_AUDIT_SECRETis set, so it cannot implement an explicit per-dir override by itself.🤖 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/_reverse_pii.py` around lines 241 - 279, The code calls _resolve_salt(output_dir) unconditionally which forces resolved_source == "env_var" when FORGELM_AUDIT_SECRET is exported and thus rejects an explicit --salt-source=per_dir; change the flow so that if salt_source == "per_dir" you do not call _resolve_salt but instead read the per-dir salt directly (use the same per-dir reader used by _purge.py / _read_persistent_salt or add a small helper that delegates to the shared per-dir routine), handle OSError the same way (call _output_error_and_exit with EXIT_TRAINING_ERROR), set resolved_source to "per_dir" and continue; keep the existing branch where salt_source is None (call _resolve_salt) and preserve the mismatch check that errors with _output_error_and_exit and EXIT_CONFIG_ERROR when an explicit salt_source disagrees with resolved_source.
🤖 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/gdpr_erasure-tr.md`:
- Around line 175-179: The Markdown link target for the JSON envelope reference
is incorrect; locate the inline link currently written as
[`reference/json-output.md`](`#/reference/json-output`) in the sentence "JSON
envelope şeması için bkz." and replace the href with a repo-relative .md target
that points directly to the JSON output document (use a relative path to the
json-output.md file) so the link navigates correctly from the guide and complies
with docs linking standards.
In `@forgelm/cli/_parser.py`:
- Around line 941-945: The help text for the "--output-dir" argument is
misleading because _resolve_query_form() and _run_reverse_pii_cmd() still
access/create a per-output-dir salt file even when --salt-source is unset;
update the metavar/help for the "--output-dir" argument to state that the
directory may be used to read/create a .forgelm_audit_salt file for audit
hashing and plaintext mode warnings, so operators are informed of that side
effect and not told the flag is only consulted when --salt-source is set.
In `@forgelm/cli/subcommands/_reverse_pii.py`:
- Around line 417-428: The finally block currently re-arms the outer alarm using
previous_alarm_remaining without accounting for elapsed time spent in
_scan_file, which can extend the caller's budget; change the restore logic in
the finally block to measure elapsed = time.monotonic() - start before calling
_scan_file (or capture start at the top), then when previous_alarm_remaining > 0
compute remaining = max(int(math.ceil(previous_alarm_remaining - elapsed)), 1)
and call _signal.alarm(remaining) (while still restoring
_signal.signal(previous_handler) and cancelling the temporary alarm); import
time and math as needed and only perform the adjustment when
previous_alarm_remaining > 0.
In `@forgelm/webhook.py`:
- Around line 100-108: The code in webhook.py currently replaces any timeout < 1
with the model default (default_timeout) instead of clamping to the 1s floor;
change the logic so when timeout < 1 you set timeout = 1 (the minimum allowed)
and update the logger.warning message to state it was clamped to 1s; adjust the
block referencing _WebhookConfig.model_fields, timeout, default_timeout, and
logger to perform the clamp to 1 rather than assigning default_timeout.
In `@tests/test_reverse_pii.py`:
- Around line 664-675: The test currently allows either "UnicodeDecodeError" or
"OSError" in evt["error_class"], which weakens the audit classification; update
the assertion in tests/test_reverse_pii.py (after building access_events and
setting evt) to require the exact class name "UnicodeDecodeError" (e.g., assert
evt["error_class"] == "UnicodeDecodeError"), removing the {"UnicodeDecodeError",
"OSError"} set; keep the subsequent no-leak checks (as_str and query_hash)
unchanged.
---
Duplicate comments:
In `@forgelm/cli/subcommands/_reverse_pii.py`:
- Around line 241-279: The code calls _resolve_salt(output_dir) unconditionally
which forces resolved_source == "env_var" when FORGELM_AUDIT_SECRET is exported
and thus rejects an explicit --salt-source=per_dir; change the flow so that if
salt_source == "per_dir" you do not call _resolve_salt but instead read the
per-dir salt directly (use the same per-dir reader used by _purge.py /
_read_persistent_salt or add a small helper that delegates to the shared per-dir
routine), handle OSError the same way (call _output_error_and_exit with
EXIT_TRAINING_ERROR), set resolved_source to "per_dir" and continue; keep the
existing branch where salt_source is None (call _resolve_salt) and preserve the
mismatch check that errors with _output_error_and_exit and EXIT_CONFIG_ERROR
when an explicit salt_source disagrees with resolved_source.
🪄 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: 08e9fbe5-d7ec-4ddf-a217-7f3d7b108cbd
📒 Files selected for processing (13)
.gitignoreCHANGELOG.mddocs/analysis/code_reviews/wave3-followup-review-prompt.mddocs/guides/gdpr_erasure-tr.mddocs/guides/gdpr_erasure.mdforgelm/cli/_parser.pyforgelm/cli/subcommands/_reverse_pii.pyforgelm/config.pyforgelm/webhook.pytests/test_check_bilingual_parity.pytests/test_eu_ai_act.pytests/test_integration.pytests/test_reverse_pii.py
✅ Files skipped from review due to trivial changes (3)
- docs/guides/gdpr_erasure.md
- .gitignore
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_integration.py
- tests/test_check_bilingual_parity.py
- forgelm/config.py
…on commits
* `gdpr_erasure{,-tr}.md` JSON envelope link — the previous link mixed
a ``.md``-shaped link text with a SPA hash-router href
(``[reference/json-output.md](#/reference/json-output)``). Replaced
with bilingual repo-relative paths
(``../usermanuals/{en,tr}/reference/json-output.md``) so the link
navigates correctly from both GitHub and the SPA viewer.
* `_parser.py` ``--output-dir`` help text — was misleading post-
F-W3-PS-01 ("only consulted when --salt-source is set"); the salt
is now read/created on every invocation for the audit-event hash.
Help text rewritten to name the side effect for both plaintext and
hash-mask modes.
* `_scan_file_with_alarm` outer-alarm preservation — the previous
``F-W3FU-03`` absorption captured ``previous_alarm_remaining`` and
re-armed it in ``finally``, but did NOT subtract the elapsed
wall-clock seconds spent in ``_scan_file``. A nested call would
silently EXTEND the outer caller's deadline by however long the
scan ran. Fixed: capture ``time.monotonic()`` at entry, compute
``remaining = max(int(math.ceil(prev - elapsed)), 1)`` in the
``finally`` clause. Clamp to ≥1 so we never lose the outer alarm
(alarm-syscall granularity is whole seconds; rounding down would
silently shave 0-1s off on every nested call).
* `webhook.py` timeout clamp — the F-W3S-04 absorption replaced the
old literal ``5`` fallback with ``WebhookConfig.model_fields["timeout"].default``
(10s), but the documented contract is "below 1s → clamp to 1s
floor", not "jump to the model default". An operator who set
``timeout: 0`` was getting their budget silently 10x'd. Fixed:
``timeout = 1`` and clamp message updated to "clamping to 1s".
* `tests/test_reverse_pii.py` UnicodeDecodeError assertion — the
failure-path test allowed either ``"UnicodeDecodeError"`` or
``"OSError"`` in ``evt["error_class"]``. The fixture plants
``\xff\xfe`` after a newline which produces ``UnicodeDecodeError``
deterministically when Python's text-mode iterator decodes the
second line; pinning the exact class makes a refactor that
converted it to a generic OSError before logging surface here.
* `_resolve_query_form` per-dir bypass — the previous absorption
refused ``--salt-source=per_dir`` when ``FORGELM_AUDIT_SECRET`` was
exported (because ``_resolve_salt`` always XOR'd the env var in
and the post-resolution mismatch check fired). Refusing was
conservative-but-wrong: an explicit ``per_dir`` choice should
HONOR the operator's intent, ignoring the unrelated env var for
this invocation. Added ``_read_persistent_salt`` helper to
``_purge.py`` (exposes just the on-disk persistent half of the
salt-resolution path) and routed the ``per_dir`` branch through
it. ``env_var`` mode now refuses cleanly when
``FORGELM_AUDIT_SECRET`` is unset (its prerequisite).
``test_per_dir_salt_source_refuses_when_env_var_set`` flipped to
``test_per_dir_salt_source_honored_when_env_var_set`` — pins the
new contract: explicit per_dir wins, scan_mode=hash, salt_source
records "per_dir" in the audit event.
Suite stays at 1360 passed, 14 skipped (the per-dir test was
flipped, not added; new behaviour is covered by the same test in
the inverted shape). Lint clean. Bilingual parity 9/9 OK.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
forgelm/webhook.py (1)
100-101: ⚡ Quick winUpdate
_post_payloaddocstring to match the new timeout fallback source.The code now falls back to
WebhookConfig.timeoutdefault, but the docstring still states a historical 5s default. Keeping these aligned avoids operator/debug confusion.♻️ Suggested docstring update
- * The webhook config defaults ``timeout`` to 5s for historical - compatibility — ``safe_post`` is invoked with ``min_timeout=1.0`` - so an existing operator config that uses ``timeout=5`` keeps - working (every ``http*`` call site outside webhook gets the - stricter 10s floor). + * Timeout fallback comes from ``WebhookConfig.timeout`` (currently 10s); + values below 1s are clamped to 1s before calling ``safe_post`` + with ``min_timeout=1.0``.🤖 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/webhook.py` around lines 100 - 101, Update the _post_payload docstring to reflect the new timeout fallback behavior: state that timeout is derived from WebhookConfig.timeout default (via _WebhookConfig.model_fields["timeout"].default) when self.config.timeout is absent, instead of the historical fixed 5s value; mention the variable name `timeout` and the function `_post_payload` so reviewers can locate and edit the docstring to match the current implementation.tests/test_reverse_pii.py (2)
922-925: ⚡ Quick winDrop the unused tuple element to silence the SonarCloud hint.
files_scannedat line 924 is unpacked but never used; rename to_so the static analysis warning closes.♻️ Proposed fix
- matches, files_scanned = results[0] + matches, _ = results[0] assert len(matches) == 1🤖 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_reverse_pii.py` around lines 922 - 925, The test unpacks results into (matches, files_scanned) but never uses files_scanned; change the unpacking in the test where it does "matches, files_scanned = results[0]" to use a throwaway name (e.g., "matches, _ = results[0]") so the unused variable warning from SonarCloud is silenced; update the occurrence in the test function that asserts len(matches) to reference "matches" unchanged.
869-894: ⚡ Quick win
test_alarm_wrapper_restores_outer_alarm_budgetdoes not pin the budget-decrement contract.The production fix at
_reverse_pii.py:457-459subtractstime.monotonic()elapsed and clamps to ≥1. The current test only assertsremaining > 0, which is satisfied even by a regression that re-introduced the naivesignal.alarm(previous_alarm_remaining)(full original remainder, no subtraction). Strengthen by upper-boundingremainingso a regression that extends the outer budget gets caught.♻️ Proposed test strengthening
try: _signal.alarm(60) # outer budget _scan_file_with_alarm(str(corpus), pattern) remaining = _signal.alarm(0) assert remaining > 0, ( "outer alarm budget must survive the wrapper — F-W3FU-03 " "regression: signal.alarm(0) in finally was cancelling the outer alarm" ) + assert remaining < 60, ( + "outer alarm must be re-armed with the *remaining* budget, " + "not the full original — a regression that re-armed with " + "previous_alarm_remaining (no elapsed-time subtraction) would " + "silently extend the caller's deadline" + ) finally:🤖 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_reverse_pii.py` around lines 869 - 894, The test test_alarm_wrapper_restores_outer_alarm_budget currently only asserts remaining > 0; strengthen it to pin the budget-decrement contract by measuring time.monotonic() before and after calling _scan_file_with_alarm, computing elapsed and an upper bound like max(1, previous_budget - ceil(elapsed)), and then assert that remaining is <= that upper bound (and still > 0); update the test's references to the outer budget (previous_budget = 60), use time.monotonic() and math.ceil to compute the expected maximum remaining, and assert remaining <= expected_max to catch regressions that erroneously extend the outer alarm.docs/guides/gdpr_erasure-tr.md (1)
161-164: 💤 Low valueOptional: clarify ReDoS guard scope (POSIX and main thread).
The line says the SIGALRM budget protects on POSIX systems, but the implementation in
_reverse_pii.py::_scan_files_with_redos_guard(lines 402–407) additionally requiresthreading.current_thread() is threading.main_thread()and degrades to a no-op on worker threads. Operators embeddingforgelm reverse-piifrom a thread (or on Windows) get no guard, which is a meaningful caveat for the documentation audience. Consider noting this so an operator running--type customfrom a worker context understands they own the regex-vetting risk.🤖 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/gdpr_erasure-tr.md` around lines 161 - 164, Documentation currently states the SIGALRM-based ReDoS guard applies on POSIX systems but omits that it's only activated in the main thread; update the text to note that the implementation in _reverse_pii.py::_scan_files_with_redos_guard also checks threading.current_thread() is threading.main_thread() and therefore the alarm-based guard is disabled on worker threads (and unavailable on non-POSIX platforms like Windows), so operators running `forgelm reverse-pii --type custom` from a worker thread or on Windows must assume regex vetting is their responsibility.forgelm/cli/subcommands/_reverse_pii.py (1)
607-735: 💤 Low value
_run_reverse_pii_cmdcognitive complexity is 16 (SonarCloud S3776 ceiling = 15).SonarCloud flags one over the project ceiling. The function already extracts most logic into helpers (
_validate_*,_resolve_*,_maybe_audit_logger,_emit_audit_event), so a small extraction of the success-vs-failure audit emit + scan loop into a single helper (e.g._scan_with_audit(...)) would bring it back under the ceiling without behavioral change. Optional given the rest of the file is well-decomposed; flagged for parity with the project's standing ceiling.🤖 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/_reverse_pii.py` around lines 607 - 735, The function _run_reverse_pii_cmd is one point over the cognitive-complexity ceiling; extract the scanning + audit emit logic (the try/except around _scan_files_with_redos_guard and the subsequent audit success/exception handling) into a new helper (e.g. _scan_with_audit) so _run_reverse_pii_cmd delegates that flow and simply receives matches and files_scanned; the helper should accept the same symbols used here (files, pattern, identifier_type, audit, query_hash, scan_mode, salt_source_label, output_format), call _scan_files_with_redos_guard, emit failure audit events via _emit_audit_event and call _output_error_and_exit on errors exactly as current logic does, and on success emit the success audit event and return (matches, files_scanned) back to _run_reverse_pii_cmd.
🤖 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.
Nitpick comments:
In `@docs/guides/gdpr_erasure-tr.md`:
- Around line 161-164: Documentation currently states the SIGALRM-based ReDoS
guard applies on POSIX systems but omits that it's only activated in the main
thread; update the text to note that the implementation in
_reverse_pii.py::_scan_files_with_redos_guard also checks
threading.current_thread() is threading.main_thread() and therefore the
alarm-based guard is disabled on worker threads (and unavailable on non-POSIX
platforms like Windows), so operators running `forgelm reverse-pii --type
custom` from a worker thread or on Windows must assume regex vetting is their
responsibility.
In `@forgelm/cli/subcommands/_reverse_pii.py`:
- Around line 607-735: The function _run_reverse_pii_cmd is one point over the
cognitive-complexity ceiling; extract the scanning + audit emit logic (the
try/except around _scan_files_with_redos_guard and the subsequent audit
success/exception handling) into a new helper (e.g. _scan_with_audit) so
_run_reverse_pii_cmd delegates that flow and simply receives matches and
files_scanned; the helper should accept the same symbols used here (files,
pattern, identifier_type, audit, query_hash, scan_mode, salt_source_label,
output_format), call _scan_files_with_redos_guard, emit failure audit events via
_emit_audit_event and call _output_error_and_exit on errors exactly as current
logic does, and on success emit the success audit event and return (matches,
files_scanned) back to _run_reverse_pii_cmd.
In `@forgelm/webhook.py`:
- Around line 100-101: Update the _post_payload docstring to reflect the new
timeout fallback behavior: state that timeout is derived from
WebhookConfig.timeout default (via
_WebhookConfig.model_fields["timeout"].default) when self.config.timeout is
absent, instead of the historical fixed 5s value; mention the variable name
`timeout` and the function `_post_payload` so reviewers can locate and edit the
docstring to match the current implementation.
In `@tests/test_reverse_pii.py`:
- Around line 922-925: The test unpacks results into (matches, files_scanned)
but never uses files_scanned; change the unpacking in the test where it does
"matches, files_scanned = results[0]" to use a throwaway name (e.g., "matches, _
= results[0]") so the unused variable warning from SonarCloud is silenced;
update the occurrence in the test function that asserts len(matches) to
reference "matches" unchanged.
- Around line 869-894: The test test_alarm_wrapper_restores_outer_alarm_budget
currently only asserts remaining > 0; strengthen it to pin the budget-decrement
contract by measuring time.monotonic() before and after calling
_scan_file_with_alarm, computing elapsed and an upper bound like max(1,
previous_budget - ceil(elapsed)), and then assert that remaining is <= that
upper bound (and still > 0); update the test's references to the outer budget
(previous_budget = 60), use time.monotonic() and math.ceil to compute the
expected maximum remaining, and assert remaining <= expected_max to catch
regressions that erroneously extend the outer alarm.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bde00414-33ff-48bc-8e35-090de73ca8a5
📒 Files selected for processing (7)
docs/guides/gdpr_erasure-tr.mddocs/guides/gdpr_erasure.mdforgelm/cli/_parser.pyforgelm/cli/subcommands/_purge.pyforgelm/cli/subcommands/_reverse_pii.pyforgelm/webhook.pytests/test_reverse_pii.py
✅ Files skipped from review due to trivial changes (2)
- docs/guides/gdpr_erasure.md
- forgelm/cli/_parser.py
* `gdpr_erasure{,-tr}.md` (EN+TR) — ReDoS guard text amended to name
the main-thread requirement. Previous text said "POSIX 30s SIGALRM
budget" without mentioning that ``_scan_files_with_redos_guard``
also gates on ``threading.current_thread() is threading.main_thread()``,
so worker-thread invocations on POSIX silently fall through to the
unguarded path. Operators running ``forgelm reverse-pii --type
custom`` from a worker thread or on Windows must vet their regex.
* `_run_reverse_pii_cmd` cognitive complexity — extracted
``_scan_with_audit`` helper that owns the full scan + audit-emit
lifecycle (try/except around ``_scan_files_with_redos_guard``,
failure-path audit emission, success-path audit emission). The
dispatcher now delegates and receives ``(matches, files_scanned)``
back. Helper added to ``__all__`` + facade re-exports for symmetry
with the rest of the module surface. No behaviour change; pure
structural refactor to bring the function under Sonar's
cognitive-complexity ceiling.
* `webhook._post_payload` docstring — replaced the historical "5s
default" text with the current contract: ``timeout`` resolves from
``self.config.timeout`` and falls back to
``WebhookConfig.model_fields["timeout"].default`` (10s now), with
sub-1 values clamped to 1s (NOT to the model default — explicit per
the F-W3FU-followup framing).
* `tests/test_reverse_pii.py::test_redos_guard_no_op_on_worker_thread`
— replaced unused ``files_scanned`` with ``_`` to silence Sonar
S1481. The contract pinned by the test is "the worker thread did
not crash on signal.signal", not the per-file accounting; the
unused unpack was incidental.
* `tests/test_reverse_pii.py::test_alarm_wrapper_restores_outer_alarm_budget`
— strengthened beyond ``remaining > 0``. Now measures
``time.monotonic()`` either side of the wrapper, computes
``expected_max = max(int(math.ceil(previous_budget - elapsed)), 1)``
(mirroring the production code's clamp), and asserts both
``remaining > 0`` AND ``remaining <= expected_max``. A regression
that drops the elapsed-time subtraction in the production
``finally`` clause would push ``remaining`` above the bound and
fail the test.
Suite stays at 1360 passed, 14 skipped (the two test edits do not
add cases — one renames a variable, the other tightens an existing
assertion). Lint clean. Bilingual parity 9/9 OK. Dry-run green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced May 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Single integration branch covering three closure-plan phases:
forgelm reverse-pii(GDPR Article 15 right-of-access subcommand) — companion toforgelm purge(Article 17 erasure).tools/check_bilingual_parity.pyCI guard (H2 + H3 + H4 structural parity).Branch:
closure/wave3-integration@95e2bf8. Closure plan: §Faz 24 / §Faz 28 / §Faz 38 marked Wave 3 ✅.What this PR ships
Phase 38 —
forgelm reverse-piiforgelm reverse-pii --query VALUE [--type X] [--salt-source X] JSONL_GLOB....forgelm purge's per-output-dir salt to re-derive the digest, so apurge → reverse-piicycle works on the same digest).email,phone,tr_id,us_ssn,iban,credit_card,custom(regex).data.access_request_querywith the identifier hashed — Article 15 access requests must not themselves leak the subject's data into the audit log.Phase 24 — Bilingual parity
tools/check_bilingual_parity.py(new): replaces the inline H2-only check atci.yml:197-222with an H2 + H3 + H4 structural diff. AST-free, runs in lint job,--strictmode for CI gate.ingestion-tr.md+2 H3 (Markdown-aware splitter, DOCX table preservation — Phase 12 features).architecture-tr.md+4 H3 (results.py,benchmark.py,judge.py,model_card.pymodules).distributed_training-tr.md+3 H3 (Custom DeepSpeed Config, "FSDP-vs-DeepSpeed", LoRA + Distributed) + Multi-Node ↔ Docker reorder.configuration-tr.md+1 H4 (model.multimodal) + evaluation H4 reorder.alignment.md:230"v0.5.1 (Phase 14)" → phase-number reference.--strict.Phase 28 — Curated cleanup
safety.enabled: falsenow raisesConfigError(was warn). EU AI Act Article 9 risk-management evidence cannot be derived from a disabled safety eval.forgelm/config.pyforgelm/config.pydata_audit_report.jsonINFO → WARNING.forgelm/compliance.py_sanitize_md_listhelper + migratedforeseeable_misusebullet build.forgelm/compliance.py\n/\rso multi-line operator prompts don't break the parser.forgelm/deploy.py_is_private_destinationre-export removed fromforgelm.webhook.__all__(no downstream importer found).forgelm/webhook.pyforgelm/trainer.pytests/test_integration_smoke.py→tests/test_integration.py(file is integration test, not smoke).tests/Tier 3 deferred to v0.6.0
F-code-108 (Pydantic _meta.yaml schema), F-business-006 (compliance-export populated bundle), F-business-008 (QMS README rewrite), M-203 (FORGELM_GGUF_CONVERTER manifest), C-52 (
_chunk_semanticimplementation), F-test-009 (pytest-xdist enable), F-test-002 (runtime_smoke.py deletion). Each is >1-PR scope.Test plan
ruff formatcleanruff checkcleanpytest: 1333 passed / 14 skipped (was 1298 → +35 net: +18 reverse-pii, +16 parity tool, +1 high-risk-raise regression).forgelm --config config_template.yaml --dry-rungreenforgelm reverse-pii --helpround-trip via main CLItools/check_bilingual_parity.py --strict: 8/8 doc pairs at parityMigration notes
F-compliance-110 is breaking. Operators with
risk_classification: high-risk(orunacceptable) ANDevaluation.safety.enabled: falsewill hitConfigErroron config load. Mitigation paths:evaluation: { safety: { enabled: true } }.risk_classificationto a non-strict tier (limited-risk/minimal-risk).CHANGELOG
[Unreleased]carries the full migration note.Review
Multi-agent review prompt at
docs/analysis/code_reviews/wave3-review-prompt.md— 4 parallel agents (code-correctness / standards-compliance / test-rigor / privacy-security, the last specialised forreverse-pii's cleartext-PII surface).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
forgelm reverse-piisubcommand for GDPR Article 15 right-of-access corpus scanning (plaintext residual and hash-mask modes).Improvements
Bug Fixes
Documentation