Repository navigation
Wave 2a: Phase 17/18/20/34/37 — consolidated integration (supersedes #23-#27) - #28
Merged
Merged
Conversation
Closure plan §18 deliverable. Design-only PR; no code changes. The document pins down what has been a *de facto* library API for several minor releases — `from forgelm import ForgeTrainer` works, but until now nothing told a downstream consumer which symbols are stable, which are experimental, and which are internal. Phase 19 implements the design without re-deriving any decisions. 12 sections + 16-row implementation plan covering: - Stable/experimental/internal tier split + per-symbol mapping - `py.typed` + `mypy --strict` contract for the public surface - Lazy-import discipline (`import forgelm` MUST NOT pull torch) + the regression test that pins it - Wave 1 round-5 carry-over resolved: `forgelm/cli/__init__.py` lazy `__getattr__` + `_SYMBOL_TO_MODULE` pattern, monkeypatch-safe - Two-segment `__api_version__` separated from `__version__` so CLI patch releases do not inflate the library contract version - Integration test cases Phase 19 ships (`tests/test_library_api.py` + `tests/test_cli_lazy_imports.py`) - Documentation deliverables (reference, guide, notebook, README) - Cross-cutting: logging hygiene, config-from-dict, error propagation at the library boundary, thread/process safety - Five open questions resolved Closes: closure-plan-202604300906.md §7 Phase 18 Unblocks: Phase 19 (Library API implementation) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closure plan §20 deliverable. Design-only PR; no code changes.
Article 17 ("right to erasure") + Article 5(1)(e) ("storage
limitation") map to a single closure plan phase that ships an
operator-actionable subcommand, a retention-policy config block, and
three new audit events. Phase 21 implements; this design pins the
scope so the next phase does not redo legal mapping in code review.
12 sections covering:
- Article 17(1)(a)-(f) triggers mapped to ForgeLM scope
- Seven artefact kinds enumerated with per-kind erasure strategy
(corpus rows / staging dirs / compliance bundles / audit logs /
model adapters / synthetic snapshots / data audit reports)
- `RetentionConfig` Pydantic schema (audit_log_retention_days
default 1825 = 5y per Article 12; staging_ttl_days default 7;
ephemeral_artefact_retention_days default 90; enforce mode
log_only / warn_on_excess / block_on_excess)
- `forgelm purge` flag surface across three modes (--row-id /
--run-id / --check-policy) + atomic-rename guarantees + path-
traversal defence reusing the _approve.py commonpath check
- Three new audit events spec'ed (data.erasure_requested /
_completed / _failed) + how the append-only chain stays valid
post-erasure via Article 17(3)(b) record-keeping exemption
- 11-test plan (closure-plan minimum is 7; +4 defensive)
- Implementation file map for Phase 21 (16 files)
- Three Wave 1 carry-overs absorbed: GH-23 (retention config
shape — top-level retention.* not nested ingestion.retention.*),
GH-13 (`forgelm purge` shipping form), Round-5 concurrent-approve
run lock (filelock pattern, ships in Phase 21 since first concrete
need is concurrent purge of same row)
- Six open questions resolved (config nesting, glob support,
machine-unlearning honesty, interactive confirmation default,
--check-policy exit-code semantics, synthetic-snapshot scope)
- Marketing claim revision for safety_compliance.md GDPR section
Acceptance: ≥400 lines (~460 incl. tables), every Phase 20 task
list item from the closure plan addressed, sign-off checklist
populated.
Closes: closure-plan-202604300906.md §8 Phase 20
Unblocks: Phase 21 (GDPR erasure implementation)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes the Phase 9 discovery gap. forgelm approve / reject ship with
Wave 1 but an operator who walks up to a workstation cold has no way
to ask "which runs are awaiting review?". forgelm approvals fills
the gap.
Two query modes (mutually exclusive, argparse-enforced):
forgelm approvals --pending --output-dir DIR
Lists every run whose audit log carries a
human_approval.required event without a matching terminal
decision. Tabular text or {"success": true, "pending": [...],
"count": N} JSON envelope.
forgelm approvals --show RUN_ID --output-dir DIR
Prints the full approval-gate audit chain (request -> terminal
decision) plus the on-disk staging directory listing. Status
classifies as pending / granted / rejected / unknown.
Implementation:
- forgelm/cli/subcommands/_approvals.py (new) — dispatcher + helpers.
Reuses the audit-log parsing pattern from _approve.py (iter events,
skip malformed, emit one summary warning at end).
- forgelm/cli/_parser.py — _add_approvals_subcommand registrar with
a mutually-exclusive --pending / --show RUN_ID group; --output-dir
required in both modes (no useful default).
- forgelm/cli/_dispatch.py — approvals branch.
- forgelm/cli/__init__.py — facade re-exports for tests / monkeypatch.
Operator docs:
- docs/usermanuals/en/compliance/human-oversight.md "Inspecting
pending runs" section rewritten to match the shipped output (table
format with RUN_ID / AGE / REQUESTED_AT / STAGING columns, the
--output-dir flag the dispatcher actually requires).
- docs/usermanuals/tr/compliance/human-oversight.md mirror updated
in the same shape.
Tests (tests/test_approvals_listing.py — 15 cases, all torch-free):
- empty audit log -> empty list, friendly notice
- three runs, one granted -> two pending in JSON envelope
- rejected run also excluded from pending
- pending summary carries metrics + staging_path + reason
- text output renders the table headers + run id + staging column
- --show pending run emits the chain + staging_contents
- --show granted run status == "granted", chain length == 2
- --show unknown run_id exits 1 with actionable error
- --show with no audit log exits 1
- text show renders the timeline + approver
- subcommand registered (forgelm approvals --help)
- malformed audit lines (non-json + non-dict-root) do not hide a
legitimate pending run
- dispatcher defensive: neither --pending nor --show -> exit 1
- monkeypatch facade resolution: forgelm.cli._collect_pending_runs
is callable + reachable
- caplog assertion: unknown run_id error names the missing id
Coverage 67.95% -> 68.42% (+0.47 from the new module + tests).
Total tests: 1013 -> 1028.
Closes: closure-plan-202604300906.md §9.5 Phase 37 (GH-7)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closure plan §7 Phase 17 deliverable. Adds split-level parallelism
to the audit pipeline while pinning byte-identical output across
worker counts via the test suite.
Implementation:
- forgelm/data_audit/_orchestrator.py
- new module-level _process_split_for_pool wrapper so worker
pickling stays spawn-method safe (lambdas + functools.partial
over keyword-only args do not pickle cleanly across spawn)
- audit_dataset(workers: int = 1) parameter:
workers=1 keeps the pre-Phase-17 single-process path exactly,
workers>1 distributes per-split processing over min(workers,
len(splits)) Pool workers; merge step stays single-threaded so
the final report is byte-identical across worker counts.
- Pre-flight validation: workers must be positive int; rejects
bool / non-int / negative with a typed ValueError.
- forgelm/cli/_argparse_types.py
- new _positive_int type validator (rejects 0 / negatives at
argparse parse time, separate from _non_negative_int which
accepts 0).
- forgelm/cli/_parser.py
- --workers N flag on the audit subparser, default 1, type
_positive_int.
- forgelm/cli/subcommands/_audit.py
- workers param threaded through _run_data_audit + _run_audit_cmd
so the CLI flag reaches audit_dataset.
- forgelm/cli/__init__.py
- re-export _positive_int for tests / monkeypatch.
Determinism contract (the whole point of this phase):
data_audit_report.json is byte-identical between --workers 1 and
--workers 4. The only field that varies is generated_at (wall-clock
timestamp). CI gates that compare the audit-report hash across
runs continue to work after operators switch worker counts.
Tests (tests/test_data_audit_workers.py — 18 cases):
- byte-identical JSON for workers in {2, 4} vs sequential baseline
(parametrised, generated_at stripped before compare)
- per-split lang_sample byte-identical (the field most likely to
diverge under non-deterministic ordering)
- pii_summary / pii_severity identical across worker counts
- near_duplicate_summary identical (with injected duplicate rows)
- total_samples identical
- secrets_summary identical (with injected fake AKIA token)
- single-split corpus tolerates workers > 1 (no parallelism
available, must still produce a valid report)
- default-when-omitted equals workers=1 (backwards compatibility)
- workers in {0, -1, -10} raises typed ValueError
- workers="four" / workers=True raises typed ValueError
- CLI: --workers flag advertised in audit --help with default 1
- CLI: --workers 0 rejected at parse time with clear error
- CLI: --workers -2 rejected at parse time
- CLI: omitted --workers produces same on-disk JSON as --workers 1
(subprocess end-to-end smoke)
Documentation:
- docs/guides/data_audit.md
- Run-it section: --workers 4 example next to existing dedup-
method / quality-filter examples
- CLI reference: --workers N added to the flag list
- dedicated section explaining the determinism contract + how the
speed-up scales with number of splits, not row count
- CHANGELOG.md [Unreleased]: full Phase 17 entry
Test count: 1013 -> 1031 (+18). Coverage 67.94% (unchanged within
rounding; new code adds tests + reduces cli/_argparse_types coverage
gap). ruff format/check clean.
Closes: closure-plan-202604300906.md §7 Phase 17 (F-performance-104)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes ghost-features-analysis-20260502 GH-1 (onboarding bloker — 30 doc references across 8 user-manual files no longer point at a non-existent command). The first command an operator should run after installation. Probes: python.version Python is in the supported window torch.cuda torch + CUDA availability gpu.inventory visible GPUs + per-device VRAM extras.<name> every optional extra (qlora, unsloth, deepspeed, evaluation, wandb, mergekit, ingestion, ingestion-pii-ml, ingestion-scale) hf_hub.reachable HF Hub HEAD probe (skipped under --offline) hf_hub.offline_cache local HF cache size + populated state (--offline-only replacement for the Hub probe) disk.workspace free space on the workspace filesystem (<10 GiB fail, <50 GiB warn, otherwise pass) operator.identity FORGELM_OPERATOR audit-identity hint Three statuses: - pass: invariant holds, nothing to do - warn: operator-actionable but doesn't block (missing optional extra, CPU-only torch, FORGELM_OPERATOR unset) - fail: ForgeLM cannot work this way (Python <3.10, no torch, unresolvable operator identity) Exit codes follow the public contract: - 0 (EXIT_SUCCESS): every check passed (warns are OK) - 1 (EXIT_CONFIG_ERROR): at least one fail - 2 (EXIT_TRAINING_ERROR): a probe itself crashed (operator-actionable bug, surfaced as runtime-error class so CI/CD retry logic treats it the same way) Implementation: - forgelm/cli/subcommands/_doctor.py (new, ~430 lines): probes, renderers, exit-code mapper, top-level dispatcher. Heavy deps (torch, huggingface_hub) are imported lazily inside individual check functions so `forgelm doctor` runs on a brand-new machine where torch is not yet installed without crashing. One probe crashing does not abort the rest of the report. - forgelm/cli/_parser.py: _add_doctor_subcommand registrar with --offline + --output-format flags. - forgelm/cli/_dispatch.py: doctor dispatch branch. - forgelm/cli/__init__.py: facade re-exports for tests / monkeypatch. Tests (tests/test_doctor.py — 38 cases, all torch-free): TestPythonVersionCheck (4) Python 3.10 → warn, 3.11 → pass, 3.12 → pass, 3.9 → fail TestTorchCudaCheck (3) no torch → fail; CPU-only torch → warn; torch + CUDA → pass TestGpuInventoryCheck (3) no torch → fail; no CUDA → warn (count=0); 2 GPUs → pass (with VRAM enumeration validated) TestOptionalExtraCheck (2) present module passes; missing module warns with install hint TestHfHubReachableCheck (2) network outage warns (not fails); HTTP 200 passes TestHfCacheOfflineCheck (3) missing cache → warn; populated cache → pass; empty cache → warn TestDiskSpaceCheck (3) free space query smoke; <10 GiB → fail; 30 GiB → warn TestOperatorIdentityCheck (2) FORGELM_OPERATOR set → pass; unset (with getuser fallback) → warn TestExitCodeMapping (4) all-pass → 0; warn-only → 0; any-fail → 1; crashed-probe → 2 TestRenderers (3) text summary line; JSON envelope shape; success=True with no fails TestProbeCrashIsolation (1) one crashing probe does not abort the run; converted to fail with crashed=True marker TestCheckPlan (2) --offline uses cache probe (not Hub probe); all extras in plan TestDispatcher (3) text output prints summary; JSON envelope structure; exit code resolved from result list TestDoctorCLISmoke (2) `forgelm doctor --help` advertises --offline + --output-format; `forgelm doctor --offline --output-format json` end-to-end smoke TestFacadeReExports (1) every test-touched helper reachable via forgelm.cli Documentation: - docs/usermanuals/en/getting-started/first-run.md sample output updated to match the shipped table format. - TR mirror first-run.md updated in the same shape. - CHANGELOG.md [Unreleased] Phase 34 entry. Existing user-manual references (30 across 8 files in installation, first-run, troubleshooting, air-gap, cli reference) were already in place; they now point at a real working command instead of a ghost feature. The closure plan §10 Faz 30 Task A still has the larger doc sweep on its list; this PR ships the minimum doc updates that keep the user-manuals correct in the meantime. Test count: 1013 -> 1051 (+38). Coverage 67.94% -> 68.59%. ruff format/check clean. Closes: closure-plan-202604300906.md §9.5 Phase 34 ghost-features-analysis-20260502.md GH-1 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes the three HIGH findings from the multi-agent review and the six MEDIUMs that have a clean doc-only fix: - F-23-01 (HIGH): rewrote §4.1 ¶2 to acknowledge the existing test_lazy_imports.py only covers forgelm.trainer / forgelm.model; bare `import forgelm` is verified manually today and Phase 19 task #7 closes the gap with a real subprocess regression test. - F-23-02 (HIGH): added a TYPE_CHECKING import block to the §4.2 __getattr__ sketch so mypy --strict sees real signatures (not Any), and replaced the if/elif chain with a _LAZY_SYMBOLS dict + caching via globals()[name] = value (gemini-code-assist comments addressed in the same edit: maintainability + per-access caching). - F-23-03 (HIGH): replaced the "raise AttributeError on forgelm._http" test with a stronger and import-side-effect-immune assertion ("_http" not in dir(forgelm) and not in __all__). Documented in §2.3 that internal = absent from public surface, not import-protected. - F-23-04 (MEDIUM): §12 sign-off note honestly acknowledges that no existing test imports the new stable names and Phase 19 task #7 ships the coverage; the table claim was aspirational. - F-23-05 (MEDIUM): §4.3 reframes the cli/__init__.py lazy migration as hygiene, not perf; explicitly notes _dispatch eagerly imports six submodules so a real CLI invocation defeats the laziness anyway. - F-23-06 (MEDIUM): §5.2 names the CI trigger (release-* branch push) and explicitly documents the script as a no-op for v0.5.5 because there is no prior __api_version__-bearing baseline. - F-23-07 (MEDIUM): added a tier-completeness audit table covering every top-level module + sub-package (Stable / Experimental / Internal) so reach-ins to forgelm.safety / forgelm.judge / forgelm.ingestion are not silently load-bearing. - F-23-08 (MEDIUM): replaced the §2.1 audit_dataset signature (`**kwargs` placeholder) and verify_audit_log signature (missing hmac_secret) with the real signatures audited from forgelm/data_audit/_orchestrator.py and forgelm/compliance.py. - BOT (sourcery): "spec'es" typo in CHANGELOG.md replaced with "specifies". Reviewer recommendation said "pre-Phase-19 must land before Phase 19 starts"; that's exactly what this commit does — design + Phase 19 implementation plan are now consistent with the audited live code. Closes (review IDs): F-23-01 .. F-23-08 + sourcery-typo Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes the three CRITICAL findings + four HIGH findings from the multi-agent review and the bot comments that have a clean doc-only fix. CRITICAL fixes: - F-24-01 (CRITICAL): added §3.1 "Prior-state acknowledgement" that resolves the staging_ttl_days collision with the already-shipped EvaluationConfig.staging_ttl_days. Phase 21 deprecates the evaluation field via DeprecationWarning + alias-forward to the new retention.staging_ttl_days; removal targeted v0.7.0 per docs/standards/release.md cadence. - F-24-02 (CRITICAL): same §3.1 entry resolves the closure-plan §15.5 GH-23 conflict between top-level retention.* (this design) and ingestion.retention.raw_documents.ttl_days (legacy GDPR docs). Top-level retention.* wins; Phase 21 updates the user-manual gdpr.md page in the same PR; closure-plan GH-23 entry will be amended when Phase 21 ships. - F-24-03 (CRITICAL): added §5.4 "Personal-data minimisation across every audit-event field" — per-field classification + hash policy for category-3 (personal-data-bearing) fields. target_id in row mode is hashed via SHA-256(salt + value); error_message is routed through the existing _http._mask_secrets_in_text + PII regex mask helpers; everything else is genuinely operational. Phase 21 ships test_target_id_hashed_in_audit_event regression. HIGH fixes: - F-24-04 (HIGH): §4.2 flag table revised — line-number fallback for --row-id REFUSED; directory mode REFUSED; multi-row matches in a single file aborted unless --row-matches=all opt-in; new --yes flag required for unattended use (interactive without --yes prompts on TTY, exits EXIT_CONFIG_ERROR on non-TTY). - F-24-05 (HIGH): §4.3 exit codes aligned with the project-wide 0/1/2/3/4 contract; --check-policy returns code 3 (EXIT_EVAL_FAILURE) when violations exist (was: always 0); code 4 (EXIT_AWAITING_APPROVAL) for the interactive-confirm path. - F-24-06 (HIGH): §4.4 atomicity rewritten with strict event ordering (data.erasure_requested FIRST before any deletion, then rewrite, then data.erasure_completed). Step-5 and step-6 failure recovery paths spelled out with named regression test test_partial_commit_emits_failed_event. - F-24-08 (HIGH): §6 marketing-claim revision now carries an explicit scope-limitation paragraph naming what the operator remains responsible for (replicas, downstream consumers, webhook receivers, legal-hold copies). New data.erasure_warning_external_copies event fires when a webhook target is configured. MEDIUM fixes (selected high-value): - F-24-09 (MEDIUM): §3.2 audit_log_retention_days docstring rewritten with accurate Article 12(2) (6-month floor) + Article 11 (10-year high-risk) + GDPR Article 30 (≥3y) reasoning; default 1825d justified as middle-of-the-road horizon. - F-24-11 (MEDIUM): §5.1 audit-event catalog grew from 3 to 6 events — data.erasure_warning_memorisation, _warning_synthetic_data_present, _warning_external_copies are now properly specified (previously referenced 3× without spec). BOT fixes (gemini, qodo, sourcery): - gemini-mtime: §3.3 new section "mtime caveat (and the in-band age field)" — retention check prefers the audit-log timestamp where available (HMAC-signed, can't be touch'd), falls back to mtime with age_source notation in the JSON output. - qodo-line-count: §12 sign-off updated with actual line count (445 vs claimed ≈460); v2 additions enumerated. - sourcery-typo: "spec'es" → "specifies" in design intro + CHANGELOG. DEFERRED (not in this PR): - F-24-07 (HIGH) full §8 file-map enumeration (16 → 20 files): folded into Phase 21 implementation PR description per its closer scope. - F-24-10 (MEDIUM) acquire_run_lock spec + Windows fallback: still Phase 22 ISO/SOC 2 design scope per closure-plan §15.5 Round-5 carry-over; Phase 20 references it but does not specify it. - F-24-12 (MEDIUM) TR mirror file map: Phase 30 final doc sweep scope; Phase 21 ships EN side first. - F-24-13 (MEDIUM) distributed-corpus deletion: explicitly out of scope per §11; already documented. - F-24-14 (MEDIUM): --yes flag now in §4.2 table; no-tty behaviour spelled out. Closes (review IDs): F-24-01..06, 08, 09, 11 + gemini-mtime, qodo-line-count, sourcery-typo Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes the two HIGH findings + four bot suggestions. HIGH fixes: - F-25-01 (HIGH security): _staging_dir_for_event now reuses _staging_path_inside_output_dir from _approve.py to refuse staging_path values that escape output_dir. A tampered audit log pointing staging_path at /etc no longer leaks a directory listing through _staging_contents. Two new regression tests (TestApprovalsPathTraversalGuard, --show + --pending modes). - F-25-02 (HIGH refactor): consolidated the audit-log JSONL parser into a new shared module forgelm/cli/subcommands/_audit_log_reader.py. _approve.py's _find_human_approval_required_event + _find_human_approval_decision_event and _approvals.py's _iter_audit_events all now delegate to iter_audit_events / find_latest_event_for_run there. Future malformed-line policy fixes land in one place. XPR-01 cross-PR observation closed (Phase 21 forgelm purge will use the same module). MEDIUM fixes: - F-25-03 (MEDIUM correctness): latest-wins semantics for re-staged runs. _collect_pending_runs walked the log with setdefault (first required wins) — inconsistent with _approve.py's latest-wins decision-event pattern. Now both use latest-wins (track the latest required line + latest terminal-decision line per run_id; a run is pending iff its latest required > latest decision). New regression: TestApprovalsLatestWinsRunIdReuse exercises the re-staged-after-rejection scenario. Bot-suggested coverage (4 tests + TR typo): - test_show_rejected_run_status_rejected: covers the rejected --show path classifier branch. - test_pending_table_renders_missing_when_staging_absent: covers the MISSING staging cell rendering when create_staging=False. - test_pending_empty_audit_log_json_envelope: pins the JSON envelope shape for the no-audit-log-yet case. - test_show_unknown_run_id_json_envelope: pins the structured error envelope from _output_error_and_exit on unknown run_id under JSON output. - test_show_robust_to_malformed_lines: --show inherits the same malformed-line tolerance as --pending (auto via shared _audit_log_reader). - TR typo: "1 kodu ile" → "1 koduyla" (sourcery nit). DEFERRED (out of this PR's scope): - F-25-04 (MEDIUM doc): JSON envelope schema page — docs/reference/cli_json_schemas.md is XPR-03 cross-PR concern; same page should also document Phase 34 doctor envelope. Better as one consolidated docs PR after this batch. - F-25-05 (MEDIUM test): hardcoded timestamp parametrisation — noise-to-signal too low for this PR's blast radius; absorbed into Wave 2b factory growth (XPR-03 recommendation). - F-25-06 (MEDIUM contract): defensive "neither flag set" branch — kept as belt-and-suspenders; argparse exit code 2 vs our 1 divergence is intentional (operator misuse vs internal contract violation). - LOW / NIT findings (07-14): not regressions; kept on the backlog. Tests: 1028 → 1036 (+8). ruff format/check clean. Coverage 68.42% → 68.68% (+0.26). Closes (review IDs): F-25-01, F-25-02, F-25-03 + 4 bot tests + TR typo Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes the two CRITICAL findings, three HIGH findings, and one
HIGH bot comment.
CRITICAL fixes:
- F-26-01 (CRITICAL): test_lang_sample_byte_identical was vacuous
— both sides returned None because lang_sample is the in-memory
_StreamingAggregator field, never serialised. Renamed to
test_languages_top3_byte_identical and now compares the actual
on-disk languages_top3 list (the persisted derivative operators
and CI gates can see).
- F-26-02 (CRITICAL): test_audit_json_byte_identical_to_sequential
compared parsed dicts (tolerates key ordering / whitespace /
Unicode normalisation / float repr drift), not bytes. Now adds
_file_sha256() helper that strips generated_at textually and
hashes the rest — exactly what an Article 10 governance-bundle CI
gate compares. Dict equality kept as a secondary looser check so
both layers regress-trip simultaneously.
HIGH fixes:
- F-26-03 (HIGH): _orchestrator now pins multiprocessing context to
"spawn" (was: default Pool() = fork on Linux + spawn on macOS /
Windows; Linux CI never exercised the spawn path). Spawn gives
every worker a fresh interpreter — no shared
langdetect.DetectorFactory state, no copied open file handles.
- F-26-04 (HIGH): new TestWorkersWithMinHash exercises the
byte-identical contract on dedup_method="minhash" + workers > 1
(the path operators explicitly recommend for >50K-row corpora).
Skipped when datasketch optional extra is not installed.
- F-26-05 (HIGH): new TestWorkersErrorPropagation —
test_sequential_split_failure_propagates (per-split failure
bubbles up unchanged) + test_parallel_path_does_not_silently_complete
(a missing-file split surfaces inline as read_failed without
hanging or killing the whole report).
- F-26-06 (HIGH): new test_split_iteration_order_pinned asserts
list(report.splits.keys()) == ["train", "validation", "test"] so a
future refactor that swaps the dict for a set or alphabetises
silently breaks the byte-identical contract.
MEDIUM / NIT:
- gemini bot comment: removed nested _sequential_outcomes function
in favour of a single generator expression in the else branch
(cleaner + same semantics).
- Added INFO log on entering parallel path so operators can verify
--workers 4 actually engaged (F-26-08).
- sourcery bot: new test_cli_workers_non_integer_rejected_at_parse_time
exercises the _positive_int "invalid integer" error path.
- qodo bot: docs/guides/data_audit-tr.md updated with --workers
example + dedicated section (was: only EN side updated in original
Phase 17 PR).
DEFERRED (out of this PR's scope):
- F-26-07 (MEDIUM): effective_workers upper-bound — the
min(workers, len(splits_paths)) clamp already caps at split count
(typically 3); 1M-process loophole only triggers if a future
refactor adds per-row parallelism. Logged for that follow-up.
- F-26-09 (MEDIUM): _process_split_for_pool relocation — keeps
spawn-method picklability concerns local to the orchestrator;
splitting would require careful re-export.
- F-26-10 (MEDIUM): fake AKIA test fixture — gitleaks did not flag
in pre-commit; revisited if it ever does.
- F-26-11 (MEDIUM): component-invariant tests calling output_dir —
TestWorkersDeterminism tests already exercise the on-disk path
via _audit_to_canonical_json; component tests are deliberately
in-memory for speed.
- F-26-12..16 LOW/NIT: backlog.
Tests: 1031 → 1035 (+4 net: +6 new tests, -2 vacuous removed).
Skipped: 13 → 14 (datasketch-gated minhash×workers test).
ruff format/check clean. Coverage 67.94% → 67.91% (within rounding).
Closes (review IDs): F-26-01..06 + gemini-sequential + sourcery-non-int
+ qodo-tr-guide
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes the two HIGH findings + four bot comments. HIGH fixes: - F-27-01 (HIGH): GH-1 closure was incomplete — only 2 of 10 doc files had been updated to match the shipped output. This commit updates the rest: * docs/usermanuals/en/getting-started/installation.md — sample "what doctor reports" rewritten to enumerate the actual probe set + JSON envelope shape + --offline flag. * docs/usermanuals/en/operations/air-gap.md — sample --offline output replaced with the real table format (was: aspirational text "ForgeLM 0.5.2 (offline mode verified)" + "Llama Guard 3 8B available locally" — none of which the dispatcher actually produces). * docs/usermanuals/tr/* mirrors updated in lock-step. troubleshooting.md was already accurate (only references the command name, no sample output to drift). - F-27-02 (HIGH): HF Hub probe partial migration toward forgelm._http discipline. HF_ENDPOINT env var is now respected via a new _resolve_hf_endpoint() helper (mirrors huggingface_hub's own resolution) so self-hosted mirrors / corp-proxy installs no longer produce false-warn results. Probe carries a dedicated "forgelm-doctor/Phase-34" User-Agent so server logs identify it. Full migration to forgelm._http.safe_get is deferred to XPR-02 cross-PR work (requires adding safe_get to _http.py). Bot fixes (gemini, qodo): - gemini bot: HF cache resolution honours HF_HUB_CACHE > HF_HOME/hub > ~/.cache/huggingface/hub. Was: pointed at HF_HOME directly, which is the parent of multiple cache trees (hub + datasets + spaces) — wrong directory. - gemini bot: HF_ENDPOINT respected (covered above with F-27-02). - qodo bot: _OPTIONAL_EXTRAS list audited against pyproject.toml. Original used aspirational names ("deepspeed", "evaluation", "wandb", "mergekit") that did not match the published extras ("distributed", "eval", "tracking", "merging") — install hints were unactionable. Also added "export" extra to the probe list. - qodo bot: _check_operator_identity now respects FORGELM_ALLOW_ANONYMOUS_OPERATOR=1 the same way AuditLogger does. Without the opt-in: fail (matches AuditLogger's hard-refuse). With the opt-in: warn (matches AuditLogger's "anonymous@host" fallback). Doctor's verdict now matches actual training-time behaviour. MEDIUM fixes: - F-27-03 (MEDIUM perf): _check_hf_cache_offline walk is now depth-capped (4 levels) + file-capped (5 000 files); cap-hit is recorded in extras["walk_truncated"] so downstream tooling can detect partial-scan results. Prevents 30+s scans on NFS-mounted 50+ GiB caches. - F-27-04 (MEDIUM correctness): dropped the dead socket.timeout except branch. socket.timeout is an alias for TimeoutError since Python 3.10 (project floor) and urllib.request.urlopen wraps timeouts as URLError anyway. - F-27-05 (MEDIUM security): added _DOCTOR_SECRET_ENV_NAMES allow- list + _mask_env_value_for_audit helper. FORGELM_OPERATOR is not on the list (operator identity, not secret) but the discipline is pinned for any future probe surfacing HF_TOKEN / FORGELM_AUDIT_SECRET. Tests added (TestHfEndpointResolution, TestOperatorIdentityAnonymousOptIn, TestSecretEnvMasking, plus updated TestHfCacheOfflineCheck for the HF_HUB_CACHE > HF_HOME/hub > default precedence). +7 tests (38 → 45 in test_doctor.py). DEFERRED (out of this PR's scope): - F-27-06 (MEDIUM contract): JSON envelope schema page — docs/reference/cli_json_schemas.md is XPR-03 cross-PR concern. - F-27-07 (MEDIUM): "skip" status for HF Hub probe under --offline — current behaviour swaps the probe rather than emitting a "skip" row; pinning the swap behaviour explicitly is a separate follow-up. - F-27-08..15 LOW/NIT: backlog. Tests: 1051 → 1058 (+7). ruff format/check clean. Coverage 68.59% → 68.74% (+0.15). Closes (review IDs): F-27-01, F-27-02, F-27-03, F-27-04, F-27-05 + gemini-HF_ENDPOINT, gemini-HF_HOME, + qodo-extras, qodo-anonymous-opt-in Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
… PR #26) # Conflicts: # CHANGELOG.md
…was PR #25) # Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
ruff format collapsed a multi-line def in TestWorkersErrorPropagation that crossed the line-length threshold after the merge. No semantic change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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
|
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:
📝 WalkthroughWalkthroughWave 2a Round-2 integration delivers three new CLI subcommands ( ChangesCore CLI Infrastructure & Features
HTTP Policy & Standards
Documentation, Guides & Design
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
This was referenced May 4, 2026
Review Summary by QodoWave 2a: Phase 17/18/20/34/37 consolidated integration with GDPR erasure design and audit workers parallelism
WalkthroughsDescription **Wave 2a consolidated integration** bundling five previously-scattered phase PRs (#23–#27) into a
single review surface with all Round-1 multi-agent review fixes.
**Core implementations:**
• **Phase 34** (forgelm doctor): Environment diagnostics subcommand with 11 probes (Python,
torch/CUDA, GPU, extras, HF Hub, disk, operator identity), structured _CheckResult objects,
text/JSON output, and defensive per-probe exception handling with secret masking
• **Phase 37** (forgelm approvals): Approval discovery subcommand with --pending mode (list
awaiting runs) and --show RUN_ID mode (display full audit chain), including path-traversal defence
and latest-wins re-staging semantics
• **Phase 17** (audit --workers N): Split-level parallelism via multiprocessing.Pool with
spawn-method pinning; data_audit_report.json byte-identical across worker counts (only
generated_at differs)
• **Wave 2a infra**: Shared _audit_log_reader.py module extracting JSONL parser into single source
of truth, eliminating duplication across _approve.py, _approvals.py, and future Phase 21
_purge.py
**Design documents:**
• **Phase 18** (Library API design): 525-line specification pinning public Python surface with three
stability tiers (Stable/Experimental/Internal), lazy-import invariant, py.typed + mypy --strict
contract, and 16-row Phase 19 task plan
• **Phase 20** (GDPR Article 17 erasure): 445-line design for forgelm purge with RetentionConfig
Pydantic schema, six new audit events, three purge modes, and 11-test strategy
**Test coverage:**
• Phase 34: 11-probe unit tests, text/JSON renderers, exit-code mapping, crash isolation, CLI smoke
tests
• Phase 37: Synthetic JSONL fixtures, --pending/--show modes, malformed-line robustness,
path-traversal guard (F-25-01), re-staging semantics (F-25-03)
• Phase 17: Byte-hash equality across workers {1,2,4}, per-split invariants, split-iteration order
pinning, MinHash × workers, error propagation in sequential/parallel paths
• All Round-1 review fixes integrated (F-26-01/02/03/04/05/06, HF_ENDPOINT resolution, HF_HOME
handling, FORGELM_ALLOW_ANONYMOUS_OPERATOR opt-in)
**Documentation updates:**
• EN + TR user manuals: forgelm doctor tabular output, --output-format json, --offline air-gap
mode, forgelm approvals discovery commands
• EN + TR guides: Phase 17 --workers parallelism, determinism contract, split-level scaling
• CHANGELOG: Comprehensive Wave 2a section (~130 lines) across all five phases plus infra
consolidation
Diagramflowchart LR
A["Phase 18<br/>Library API Design"] --> D["Public Surface<br/>Stability Tiers"]
B["Phase 20<br/>GDPR Erasure Design"] --> E["RetentionConfig<br/>+ forgelm purge"]
C["Phase 17<br/>Audit Workers"] --> F["Split-level<br/>Parallelism"]
G["Phase 34<br/>forgelm doctor"] --> H["11 Probes<br/>Diagnostics"]
I["Phase 37<br/>forgelm approvals"] --> J["Approval Discovery<br/>--pending/--show"]
K["Wave 2a Infra<br/>_audit_log_reader"] --> L["Shared JSONL<br/>Parser"]
F --> M["Byte-identical<br/>JSON Output"]
H --> N["Text + JSON<br/>Output Formats"]
J --> N
D --> O["Phase 19<br/>Integration Tests"]
E --> P["Phase 21<br/>forgelm purge"]
File Changes1. forgelm/cli/subcommands/_doctor.py
|
Code Review by Qodo
1.
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 minor |
| ErrorProne | 2 high |
| Security | 1 high |
| CodeStyle | 96 minor |
🟢 Metrics 417 complexity · 19 duplication
Metric Results Complexity 417 Duplication 19
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.
Self-contained prompt that downstream review agents pick up cold. Lists the diff scope, what each phase ships, the Round-1 carry-over baseline, the cross-phase XPR checks, the per-phase scrutiny areas, and the required Markdown report skeleton. Frozen against integration HEAD 5a57286. Follows the same allow-list-via-`-f` pattern as `foundation-pr-review-prompt.md` — review prompts are intentionally exposed so external review agents (and human reviewers) can read them without internal access. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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 significant enhancements to the forgelm CLI and its underlying architecture. Key additions include a new forgelm approvals subcommand for managing human approval requests and a forgelm doctor subcommand for comprehensive environment diagnostics. The forgelm audit command now supports parallel processing via a --workers flag, with a strong emphasis on maintaining determinism. Architectural improvements include a shared audit-log JSONL reader to centralize parsing logic. The pull request also includes detailed design documents for a new Library API and GDPR Article 17 erasure capabilities, along with extensive updates to user manuals and guides to reflect these new features and improved CLI outputs. The review comments highlight a minor improvement opportunity related to Python version comparison best practices in the _check_python_version function.
|
|
||
| Wave 2a Round-1 review: | ||
|
|
||
| - F-27-04: dropped the ``socket.timeout`` ``except`` branch. |
There was a problem hiding this comment.
The check for sys.version_info in _check_python_version uses sys.version_info < (3, 10), but sys.version_info is a named tuple and comparing it directly to a tuple is deprecated in newer Python versions. It is safer to compare sys.version_info[:2].
Suggested change
| - F-27-04: dropped the ``socket.timeout`` ``except`` branch. | |
| if sys.version_info[:2] < (3, 10): |
Collaborator
Author
There was a problem hiding this comment.
Addressed in 2148a49 — pinned to a (major, minor) 2-tuple slice. Note: the CPython tuple-vs-namedtuple comparison was never actually deprecated (the docs allow it), but the slice form makes intent clearer and is robust against future tuple-shape changes upstream, so it shipped as a NIT-level cleanup.
| detail=f"Python {label} is below the supported floor (>=3.10). Upgrade Python to >=3.11.", | ||
| extras={"version": label, "minimum": "3.10", "recommended": "3.11"}, | ||
| ) | ||
| if version < (3, 11): |
There was a problem hiding this comment.
Similarly, use sys.version_info[:2] for the version comparison to avoid potential issues with named tuple comparisons.
Suggested change
| if version < (3, 11): | |
| if sys.version_info[:2] < (3, 11): |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![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)
tests/test_data_audit_workers.py (1)
546-577:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis fixture can bypass the parallel failure path entirely.
Lines 555-559 remove
validation.jsonlbefore the audit starts, so split discovery can just drop that split instead of scheduling a worker that fails on read. The assertion then explicitly accepts that outcome, which means this test can pass without covering the worker-error path named in the docstring.As per coding guidelines, tests must follow
docs/standards/testing.mdstandards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_data_audit_workers.py` around lines 546 - 577, The test test_parallel_path_does_not_silently_complete_on_split_failure currently unlinks validation.jsonl before calling audit_dataset, allowing split discovery to drop the split and bypass the worker error path; instead preserve the file so the split is scheduled and make it fail during processing by corrupting its contents (e.g., open the validation.jsonl returned by _seed_three_split_corpus and overwrite with invalid/corrupt bytes that will cause the streaming reader to raise a decode/OSError), then call audit_dataset(workers=2) and keep the existing assertions against report.splits (train/test present and validation either has "error" or sample_count==0) so the test exercises the per-split read failure path handled inside _process_split.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md`:
- Around line 407-409: The CLI exit-code behavior for --check-policy is
inconsistent: Q5 declares it should return 0 (report-only) while §4.3 ties the
same condition to EXIT_EVAL_FAILURE (3); pick one canonical behavior and make
both places consistent (either change Q5 to state non-zero EXIT_EVAL_FAILURE (3)
or change §4.3 to state exit 0 for report-only), update the Phase 21 implementer
guidance and any references to EXIT_EVAL_FAILURE to match the chosen behavior,
and ensure the wording mentions the public contract per
docs/standards/error-handling.md so CI consumers and implementers (e.g., Phase
21, --check-policy, EXIT_EVAL_FAILURE) have an unambiguous spec.
- Line 370: The file map and sign-off checklist incorrectly state "Three new
event rows" for docs related to §5.1; update all references to reflect the
actual six new audit events defined in §5.1. Search for the string/entry for
`docs/reference/audit_event_catalog.md` and any occurrences of "Three new event
rows" or the sign-off checklist count, and change those to "Six new event rows"
(or "six new audit events") so the file map, checklist, and any summary tables
consistently match §5.1; ensure you update the corresponding mention at the
other noted location (around the duplicate at 435-435) as well.
In `@docs/analysis/code_reviews/library-api-design-202605021414.md`:
- Around line 348-349: The tests currently require that accessing the public
re-export ForgeTrainer triggers an import of torch, which violates the library's
lazy-import rule; update the tests so that test_lazy_import_no_torch verifies
that importing forgelm does not add "torch" to sys.modules and
test_lazy_import_torch_loads_on_trainer_access is changed to assert that merely
referencing forgelm.ForgeTrainer remains metadata-only (i.e., still does not
import torch), then move the positive assertion that "torch" appears in
sys.modules into a real training execution test (e.g., when instantiating/using
ForgeTrainer to run training or call a method that needs torch) so that torch is
only imported during actual runtime usage rather than on attribute lookup.
In `@docs/usermanuals/tr/getting-started/installation.md`:
- Line 107: Replace the incorrect extra name "distributed" with "deepspeed" in
the Turkish installation text so it matches the English page and the rest of
this file (referenced as extras.deepspeed and the "Dağıtık eğitim /
`[deepspeed]`" section); update the sentence listing optional extras to use
`deepspeed` instead of `distributed` and ensure the pip hint remains `pip
install 'forgelm[<isim>]'`.
In `@forgelm/cli/_parser.py`:
- Around line 655-659: Top-level help epilog is missing the newly-registered
subcommands from _add_doctor_subcommand and _add_approvals_subcommand; update
the parser's epilog string (the top-level help text constructed when creating
the main ArgumentParser) to include entries for "doctor" and "approvals" so they
appear in `forgelm --help`. Locate where the main ArgumentParser or its epilog
is defined in this module, append short descriptive lines for the doctor and
approvals subcommands consistent with the existing epilog format, and ensure any
references to the subcommand list or help generation include those two names.
In `@forgelm/cli/subcommands/_approvals.py`:
- Around line 309-322: The code currently treats the most-recent decision as
authoritative in _classify_chain but elsewhere selects the first
human_approval.required event (staging/inspection code that picks a required
event) which returns a stale required for reused run_ids; update both places to
pick the most-recent matching event by iterating the chain in reverse instead of
using the first match: in _classify_chain keep the "last decision wins" logic by
computing decisions from reversed(chain) or taking decisions[-1] after
reversing, and change the staging/inspection selection (the code that currently
picks the first event where e.get("event")==_EVT_HUMAN_APPROVAL_REQUIRED) to
search reversed(chain) and take the first match there (i.e., the most recent
required event) so --show and --pending use the same latest-wins ordering.
In `@forgelm/cli/subcommands/_audit_log_reader.py`:
- Around line 38-84: iter_audit_events currently silently skips malformed JSON
lines which is unsafe for approval decision checks; add a strict parsing mode by
adding a parameter (e.g. strict: bool = False) to iter_audit_events and, when
strict is True, raise an informative exception (ValueError or a small custom
AuditLogError) including audit_log_path and line_number whenever json.loads
fails or the parsed root is not a dict; preserve the existing silent-skip
behavior when strict is False, and update the approval callers
(_find_human_approval_required_event and _find_human_approval_decision_event) to
call iter_audit_events(..., strict=True) so they fail fast on corrupted decision
records.
In `@tests/test_doctor.py`:
- Around line 161-162: Replace exact float equality in tests/test_doctor.py with
approximate comparisons: change the two assertions that check
result.extras["devices"][0]["vram_gib"] == 24.0 and
result.extras["devices"][1]["vram_gib"] == 80.0 to use pytest.approx (e.g.,
result.extras["devices"][0]["vram_gib"] == pytest.approx(24.0)); ensure pytest
is imported at the top of the test file if not already. This targets the GPU
inventory check exercised by _check_gpu_inventory and makes the test robust to
minor floating-point rounding.
---
Outside diff comments:
In `@tests/test_data_audit_workers.py`:
- Around line 546-577: The test
test_parallel_path_does_not_silently_complete_on_split_failure currently unlinks
validation.jsonl before calling audit_dataset, allowing split discovery to drop
the split and bypass the worker error path; instead preserve the file so the
split is scheduled and make it fail during processing by corrupting its contents
(e.g., open the validation.jsonl returned by _seed_three_split_corpus and
overwrite with invalid/corrupt bytes that will cause the streaming reader to
raise a decode/OSError), then call audit_dataset(workers=2) and keep the
existing assertions against report.splits (train/test present and validation
either has "error" or sample_count==0) so the test exercises the per-split read
failure path handled inside _process_split.
🪄 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: 6ec0e51a-22be-4420-84a0-fb935aecb6bc
📒 Files selected for processing (27)
CHANGELOG.mddocs/analysis/code_reviews/gdpr-erasure-design-202605021414.mddocs/analysis/code_reviews/library-api-design-202605021414.mddocs/analysis/code_reviews/wave2a-round2-review-prompt.mddocs/guides/data_audit-tr.mddocs/guides/data_audit.mddocs/usermanuals/en/compliance/human-oversight.mddocs/usermanuals/en/getting-started/first-run.mddocs/usermanuals/en/getting-started/installation.mddocs/usermanuals/en/operations/air-gap.mddocs/usermanuals/tr/compliance/human-oversight.mddocs/usermanuals/tr/getting-started/first-run.mddocs/usermanuals/tr/getting-started/installation.mddocs/usermanuals/tr/operations/air-gap.mdforgelm/cli/__init__.pyforgelm/cli/_argparse_types.pyforgelm/cli/_dispatch.pyforgelm/cli/_parser.pyforgelm/cli/subcommands/_approvals.pyforgelm/cli/subcommands/_approve.pyforgelm/cli/subcommands/_audit.pyforgelm/cli/subcommands/_audit_log_reader.pyforgelm/cli/subcommands/_doctor.pyforgelm/data_audit/_orchestrator.pytests/test_approvals_listing.pytests/test_data_audit_workers.pytests/test_doctor.py
| - Python sürümü (>=3.10 zorunlu, >=3.11 önerilen) | ||
| - torch + CUDA varlığı (CPU-only `warn`'dur, `fail` değil) | ||
| - GPU envanteri (cihaz başına VRAM, GiB) | ||
| - Opsiyonel extras: `qlora`, `unsloth`, `distributed`, `eval`, `tracking`, `merging`, `export`, `ingestion`, `ingestion-pii-ml`, `ingestion-scale` — eksik extra'lar tam `pip install 'forgelm[<isim>]'` ipucu ile `warn`'lanır |
Contributor
There was a problem hiding this comment.
Same distributed → deepspeed rename needed here as in the EN counterpart.
The TR installation page mirrors the EN page's incorrect extra name. The actual extra is deepspeed (as shown in extras.deepspeed in the doctor output examples and documented in the Dağıtık eğitim / [deepspeed] section of this same file).
📝 Proposed fix
-- Opsiyonel extras: `qlora`, `unsloth`, `distributed`, `eval`, `tracking`, `merging`, `export`, `ingestion`, `ingestion-pii-ml`, `ingestion-scale` — eksik extra'lar tam `pip install 'forgelm[<isim>]'` ipucu ile `warn`'lanır
+- Opsiyonel extras: `qlora`, `unsloth`, `deepspeed`, `eval`, `tracking`, `merging`, `export`, `ingestion`, `ingestion-pii-ml`, `ingestion-scale` — eksik extra'lar tam `pip install 'forgelm[<isim>]'` ipucu ile `warn`'lanır📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Opsiyonel extras: `qlora`, `unsloth`, `distributed`, `eval`, `tracking`, `merging`, `export`, `ingestion`, `ingestion-pii-ml`, `ingestion-scale` — eksik extra'lar tam `pip install 'forgelm[<isim>]'` ipucu ile `warn`'lanır | |
| - Opsiyonel extras: `qlora`, `unsloth`, `deepspeed`, `eval`, `tracking`, `merging`, `export`, `ingestion`, `ingestion-pii-ml`, `ingestion-scale` — eksik extra'lar tam `pip install 'forgelm[<isim>]'` ipucu ile `warn`'lanır |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/usermanuals/tr/getting-started/installation.md` at line 107, Replace the
incorrect extra name "distributed" with "deepspeed" in the Turkish installation
text so it matches the English page and the rest of this file (referenced as
extras.deepspeed and the "Dağıtık eğitim / `[deepspeed]`" section); update the
sentence listing optional extras to use `deepspeed` instead of `distributed` and
ensure the pip hint remains `pip install 'forgelm[<isim>]'`.
Collaborator
Author
There was a problem hiding this comment.
False positive — bot reversed the rename direction. Per pyproject.toml line 66:
distributed = [
"deepspeed>=0.14.0",The extra name is distributed; deepspeed is the dep inside the extra. So pip install 'forgelm[distributed]' is the correct invocation; pip install 'forgelm[deepspeed]' would 404. Round-1 already corrected both EN (L73) and TR (L73) installation pages with an explicit parenthetical: 'Extra adı distributed; çektiği gerçek bağımlılık deepspeed>=0.14.0'.
Contributor
There was a problem hiding this comment.
@cemililik, you're absolutely right — I apologize for the false positive. The pip extra name is distributed (as defined in pyproject.toml and explicitly noted in the TR installation page: "Extra adı distributed; çektiği gerçek bağımlılık deepspeed>=0.14.0"). My suggestion to replace it with deepspeed was incorrect and would have introduced a broken pip install invocation. The existing distributed in the extras list is correct as-is.
✏️ Learnings added
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 28
File: docs/usermanuals/tr/getting-started/installation.md:109-109
Timestamp: 2026-05-04T13:05:13.484Z
Learning: In the `cemililik/ForgeLM` repo, the pip optional-extra name for distributed training is `distributed` (i.e., `pip install 'forgelm[distributed]'`), not `deepspeed`. The `deepspeed>=0.14.0` is the underlying PyPI dependency *inside* that extra, as defined in `pyproject.toml`. The `extras.deepspeed` label that appears in `forgelm doctor` output is the probe/display name, not the pip extra name. Never suggest renaming `distributed` → `deepspeed` in pip install commands or extras lists for this project.
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: 18
File: docs/usermanuals/en/data/pii-ml.md:8-8
Timestamp: 2026-04-29T20:40:44.227Z
Learning: In this repo’s user manual markdown files under docs/usermanuals/**/*.md, links of the form `#/<section>/<page>` (e.g., `#/data/pii-masking`) are intentional SPA hash-router route identifiers, not in-page markdown fragment anchors. They are parsed at runtime by `site/js/guide.js` (via `parseHash`). Therefore, reviewers should NOT suggest converting these to relative `.md` paths (e.g., `./pii-masking.md`), since `tools/build_usermanuals.py` does not rewrite link targets and such relative URLs will render as broken static URLs in the SPA and break navigation. If markdownlint flags them with MD051 (link-fragments), treat MD051 as a known false positive for these hash links and suppress MD051 at the Codacy project level rather than rewriting links.
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 18
File: docs/usermanuals/tr/data/pii-ml.md:8-8
Timestamp: 2026-04-29T20:40:46.342Z
Learning: In ForgeLM user manual markdown files under `docs/usermanuals/**/*.md`, links that use the SPA hash-router format `#/<section>/<page>` (e.g., `#/data/pii-masking`, `#/operations/air-gap`) are intentional route identifiers parsed at runtime by `site/js/guide.js` (`parseHash`). Treat them as SPA routes (not standard in-page markdown anchors) and avoid suggesting replacements with standard markdown anchors or relative `.md` paths, because `tools/build_usermanuals.py` does not rewrite link targets and relative `.md` hrefs would break navigation to non-existent static URLs. If you see markdownlint MD051 warnings for these links, don’t flag them as errors; they are expected false positives.
Round-2 multi-agent review of PR #28 surfaced 52 findings across four specialist agents (correctness / design-doc / standards / test-rigor) plus 9 maintainer inline comments. This commit lands the full fix sweep so Wave 2a is mergeable to development with zero blockers. Code (10 files, ~440 LOC): - forgelm/_http.py: NEW safe_get(method=GET|HEAD) mirroring safe_post's scheme/SSRF/timeout-floor/secret-mask discipline. - forgelm/cli/subcommands/_doctor.py: HF Hub probe migrated from raw urllib.request.urlopen to safe_get with HEAD->GET-on-405 fallback; HF_HUB_OFFLINE env detection promotes to offline mode; secret-env mask extended with 5 third-party tokens; JSON envelope summary now includes crashed count. - forgelm/cli/subcommands/_approvals.py: _classify_chain now uses latest-required-vs-latest-decision indices (re-staged runs correctly classify as pending, not stale granted/rejected); _run_approvals_show picks latest required event via reversed(chain); pending sort gains line-number tiebreaker. - forgelm/cli/subcommands/_audit_log_reader.py: NEW AuditLogParseError + strict mode parameter; find_latest_event_for_run defaults to strict=True for decision-guard fail-fast. - forgelm/cli/subcommands/_approve.py: approve/reject wrap audit-log reads in try/except AuditLogParseError, emit EXIT_CONFIG_ERROR with actionable repair message instead of silent skip. - forgelm/cli/_parser.py: epilog adds doctor + approvals subcommands; --offline help text lists full HF cache resolution chain + env-var implicit-true behaviour. - forgelm/data_audit/_orchestrator.py: INFO log when --workers N is silently clamped to fewer splits. Tests (3 files, ~820 LOC): - tests/test_doctor.py: pytest.approx for VRAM exact-floats; probe- crash sandwich [ok_a, crash, ok_b]; HF Hub tests rewritten to patch forgelm._http.safe_get; new TestDispatcherStrictExit (pass=0 / fail=1 / crashed=2 / end-to-end secret masking / HF_HUB_OFFLINE env); TestHfCacheWalkBoundaries (file-cap exact / file-cap+1 / depth-cap); TestDoctorSecretEnvNames parametrised over 9 names. - tests/test_data_audit_workers.py: _strip_generated_at regex anchored to top-level position; languages_top3 test gated on pytest.importorskip("langdetect"); _positive_int parametrised over 7 invalid forms; TestWorkersSpawnPinning spy on get_context; TestWorkersClamp; default-vs-explicit workers=1 asdict equality; test_parallel_path_does_not_silently_complete_on_split_failure rewritten to corrupt validation.jsonl (was unlink — bypassed worker error path entirely). - tests/test_approvals_listing.py: monotonic _next_default_ts() so sort tests have real signal; TestApprovalsPendingSortOrder; TestFormatAge parametrised over 12 boundaries; TestClassifyChainLatestWins (5 tests pinning re-stage semantics); TestStagingPathTraversalSandwich with sentinel fallback + caplog assertion (fixes "test cannot distinguish working guard from no-op" gap); TestShowUsesLatestRequired; TestAuditLogReaderStrictMode; TestApproveStrictModeOnCorruptLog. CI: - .github/workflows/ci.yml: NEW lint-http-discipline step greps forgelm/ for requests.*(...), urllib.request.urlopen(...), httpx.*(...) outside _http.py; fails on any hit. Pattern is paren-anchored so docstring prose is not flagged. Standards (NEW + amended): - docs/standards/architecture.md: NEW "HTTP discipline" section codifying the safe_post/safe_get chokepoint + grep acceptance gate. - docs/standards/error-handling.md: rewritten to document the SHIPPED {success: false, error: "..."} envelope (replaces aspirational 5-key form); cross-link to new json-output.md. - docs/usermanuals/{en,tr}/reference/json-output.md (NEW): locked schema for every CLI subcommand's --output-format json envelope. Documentation: - library-api-design + gdpr-erasure-design: 17 doc fixes (CI trigger per release.md:259, [eval] not [evaluation], lazy-import test semantics, signature accuracy, CI trigger alignment, exit-code contradictions resolved, marketing-copy targets corrected, audit events 3 -> 6 enumerated, deprecation tracking-issue rule). - closure-plan-202604300906.md §8 line 700: 3 -> 6 audit events. - docs/usermanuals/{en,tr}/getting-started/installation.md: [deepspeed] -> [distributed] (matches pyproject reality); [all] aggregate doesn't exist -> explicit comma-separated example. Verification: pytest --tb=short -q # 1154 passed, 14 skipped, 0 failed ruff check forgelm/ tests/ # All checks passed ruff format --check ... # 131 files already formatted forgelm doctor # exit 0; success: true; safe_get probe works forgelm doctor --offline # hf_hub.offline_cache swap works HF_HUB_OFFLINE=1 forgelm doctor # env detection works forgelm approvals --pending # exit 0 forgelm --dry-run # exit 0 HTTP discipline grep guard # empty (gate would not fire) Full delta with skip rationale: docs/analysis/code_reviews/wave2a-round2-fix-summary-20260504.md (local-only; gitignored alongside the four agent reports). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
forgelm/cli/subcommands/_doctor.py (3)
858-859: 💤 Low valueRemove the dead
_maybe_unusedfunction —Optionalis unused elsewhere in this file.
Optionalappears infrom typing import Any, Callable, Dict, List, Optional, Tupleonly because_maybe_unusedreferences it. With Python 3.10+ (the project floor),X | NonereplacesOptional[X]throughout. Removing the function and theOptionalimport cleans up the module surface without behaviour change.-from typing import Any, Callable, Dict, List, Optional, Tuple +from typing import Any, Callable, Dict, List, Tuple- -def _maybe_unused() -> Optional[None]: # pragma: no cover — kept for typing import alignment - return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/cli/subcommands/_doctor.py` around lines 858 - 859, Remove the dead helper _maybe_unused and the unused Optional import: delete the function named _maybe_unused() and remove Optional from the typing import list (the line that currently reads from typing import Any, Callable, Dict, List, Optional, Tuple), ensuring no other code in this module depends on Optional (replace any remaining usages with X | None if needed).
410-488: ⚡ Quick winExtract the bounded-walk loop into a helper to fix the SonarCloud cognitive-complexity failure (CC=20 vs limit=15).
The inner walk loop (depth guard + file-cap guard + symlink skip + OSError) accounts for most of the complexity. Pulling it into
_walk_hf_cache_boundedreduces_check_hf_cache_offlineto a straightforward guard + call + result construction, and makes the walk logic independently testable.♻️ Proposed refactor
def _walk_hf_cache_bounded(cache_dir_abs: str) -> tuple[int, int, bool]: """Bounded walk of the HF cache directory. Returns ``(file_count, total_bytes, walk_truncated)``. Stops when depth exceeds ``_HF_CACHE_WALK_DEPTH`` or file count reaches ``_HF_CACHE_WALK_FILE_LIMIT`` so an NFS-mounted 50+ GiB cache doesn't stall the doctor probe. """ total_bytes = 0 file_count = 0 walk_truncated = False base_depth = cache_dir_abs.rstrip(os.sep).count(os.sep) for root, dirs, files in os.walk(cache_dir_abs): depth = root.count(os.sep) - base_depth if depth > _HF_CACHE_WALK_DEPTH: dirs[:] = [] walk_truncated = True continue for filename in files: if file_count >= _HF_CACHE_WALK_FILE_LIMIT: walk_truncated = True break full = os.path.join(root, filename) if os.path.islink(full): continue try: total_bytes += os.path.getsize(full) file_count += 1 except OSError: continue if file_count >= _HF_CACHE_WALK_FILE_LIMIT: break return file_count, total_bytes, walk_truncatedThen
_check_hf_cache_offlinereplaces lines 444–469 with:- total_bytes = 0 - file_count = 0 - walk_truncated = False - cache_dir_abs = os.path.abspath(cache_dir) - base_depth = cache_dir_abs.rstrip(os.sep).count(os.sep) - for root, dirs, files in os.walk(cache_dir_abs): - ... # (the entire walk block) + cache_dir_abs = os.path.abspath(cache_dir) + file_count, total_bytes, walk_truncated = _walk_hf_cache_bounded(cache_dir_abs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/cli/subcommands/_doctor.py` around lines 410 - 488, Extract the bounded os.walk loop from _check_hf_cache_offline into a new helper _walk_hf_cache_bounded(cache_dir_abs: str) that returns (file_count, total_bytes, walk_truncated); move base_depth calculation and all depth/file-cap, symlink skip and OSError handling into that helper so its cognitive complexity contains the walking logic, then replace the loop in _check_hf_cache_offline with a single call to _walk_hf_cache_bounded(cache_dir_abs) and use its returned values to compute cache_gib, detail and the _CheckResult; ensure the helper signature and returned tuple match the rest of the function to keep types and behavior unchanged.
39-70: ⚡ Quick winDefine probe-name constants to fix 7 SonarCloud CI failures and align with the existing
_STATUS_*pattern.SonarCloud flags seven
[failure]-level issues for string literals ("python.version","torch.cuda","gpu.inventory","hf_hub.reachable","hf_hub.offline_cache","disk.workspace","operator.identity") used 3–6 times each across the check functions and_build_check_plan. The module already centralises status vocabulary via_STATUS_PASS / _STATUS_WARN / _STATUS_FAIL— the same pattern should cover probe names.♻️ Proposed fix
_STATUS_PASS = "pass" _STATUS_WARN = "warn" _STATUS_FAIL = "fail" + +# Probe-name vocabulary. Mirrors the _STATUS_* pattern so a future rename +# lands in one place rather than N scattered check functions. +_PROBE_PYTHON_VERSION = "python.version" +_PROBE_TORCH_INSTALLED = "torch.installed" +_PROBE_TORCH_CUDA = "torch.cuda" +_PROBE_GPU_INVENTORY = "gpu.inventory" +_PROBE_HF_HUB_REACHABLE = "hf_hub.reachable" +_PROBE_HF_HUB_OFFLINE_CACHE = "hf_hub.offline_cache" +_PROBE_DISK_WORKSPACE = "disk.workspace" +_PROBE_OPERATOR_IDENTITY = "operator.identity"Then replace the string literals in each check function and in
_build_check_plan, e.g.:- name="python.version", + name=_PROBE_PYTHON_VERSION,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forgelm/cli/subcommands/_doctor.py` around lines 39 - 70, Add module-level string constants for the probe names (e.g. _PROBE_PYTHON_VERSION = "python.version", _PROBE_TORCH_CUDA = "torch.cuda", _PROBE_GPU_INVENTORY = "gpu.inventory", _PROBE_HF_HUB_REACHABLE = "hf_hub.reachable", _PROBE_HF_HUB_OFFLINE_CACHE = "hf_hub.offline_cache", _PROBE_DISK_WORKSPACE = "disk.workspace", _PROBE_OPERATOR_IDENTITY = "operator.identity") and use those constants everywhere the corresponding literal is currently used (in the individual check functions and in _build_check_plan) so the probe names are centralized the same way _STATUS_PASS/_STATUS_WARN/_STATUS_FAIL are; update references in any functions that construct or compare probe keys to use the new _PROBE_* symbols.tests/test_approvals_listing.py (1)
108-114: ⚡ Quick winUse a strict namespace instead of
MagicMockfor CLI args.
MagicMockmanufactures attributes on demand, so these tests can keep passing if_run_approvals_cmdstarts reading a typoed or newly added arg. A real namespace makes CLI contract drift fail fast.Suggested fix
+import argparse from datetime import datetime, timedelta, timezone from pathlib import Path -from unittest.mock import MagicMock, patch +from unittest.mock import patch @@ -def _build_args(output_dir: Path, *, pending: bool = False, show: str | None = None) -> MagicMock: +def _build_args(output_dir: Path, *, pending: bool = False, show: str | None = None) -> argparse.Namespace: """Build a MagicMock ``args`` namespace mimicking argparse output.""" - args = MagicMock() - args.pending = pending - args.show = show - args.output_dir = str(output_dir) - return args + return argparse.Namespace( + pending=pending, + show=show, + output_dir=str(output_dir), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_approvals_listing.py` around lines 108 - 114, The helper _build_args currently returns a MagicMock which can silently allow missing or misspelled CLI attributes; replace MagicMock with a strict namespace (e.g., types.SimpleNamespace or argparse.Namespace) so attributes must be explicitly present. Update the return type and construction inside _build_args to create a real namespace with .pending, .show, and .output_dir set from the function parameters (keep the existing signature and ensure output_dir is converted to str). This will make tests fail if _run_approvals_cmd reads a non-existent or misspelled argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/analysis/code_reviews/closure-plan-202604300906.md`:
- Around line 1919-1927: The PR introduces Phase‑21 retention fields
(`raw_documents_retention_days`) and purge kinds but conflicts with the Phase‑20
design; reconcile by adding a top-level RetentionConfig and wiring it through
ForgeConfig and CLI, i.e., add ForgeConfig.retention: Optional[RetentionConfig]
= None, implement class RetentionConfig with fields
audit_log_retention_days:int=1825, staging_ttl_days:int=7,
ephemeral_artefact_retention_days:int=90, raw_documents_retention_days:int=90,
update purge CLI to accept --kind {raw_documents,staging,artefacts}, deprecate
EvaluationConfig.staging_ttl_days by alias‑forwarding it to
retention.staging_ttl_days with a DeprecationWarning (keep alias until v0.7.0),
and update the YAML example in docs/usermanuals/en/compliance/gdpr.md (lines
~51–64) and its TR mirror to use retention.raw_documents_retention_days; ensure
related acceptance tests/assertions reflect the new top‑level schema and purge
kinds.
In `@docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md`:
- Line 56: The markdown table row contains an unescaped pipe in the cell text
"compliance/*.json|yaml" which breaks column alignment; update the cell in the
document (the string `compliance/*.json|yaml` in
docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md) by either
escaping the pipe (`\|`) or wrapping the pattern in inline code so the table
retains its four columns and conforms to the docs/standards guidelines.
- Line 448: Update the checklist line that currently reads "- [x] Document is
≥400 lines. (Line count = 445 actual at v2 — review-pass added §3.1
prior-state, §3.3 mtime caveat, §5.4 audit-PII minimisation, §4.4 commit
ordering recovery paths, scope-limitation paragraph + 3 new warning events.)" so
it no longer reports an outdated exact line count; either recalculate and
replace "445" with the current actual line count or remove the parenthetical
exact-count fragment entirely and leave the checklist as a generic "≥400 lines"
acceptance item, ensuring the rest of the checklist text remains unchanged.
- Around line 341-342: Update the incorrect relative link target
"gdpr_erasure.md" in the file
docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md so it points to
the real guide at docs/guides/gdpr_erasure.md; replace the link target used in
the sentence "See [`docs/guides/gdpr_erasure.md`](gdpr_erasure.md)..." with the
correct relative path "../../guides/gdpr_erasure.md" so the link resolves from
the current directory to the guides file.
In `@docs/usermanuals/tr/reference/cli.md`:
- Line 95: Metindeki yazım/akıcılık hatalarını düzelt: "`--workers N` split
düzeyinde paraleliz;" ifadesinde "paraleliz" kelimesini uygun Türkçe
biçimlerinden biriyle değiştir (tercihen "paralellik" veya "paralelizm") ve
ayrıca belgede geçen "kimlik hata" ifadesini "kimlik hatası" olarak güncelle;
değişiklikleri ilgili Markdown cümlesinde (CLI açıklamasını içeren satırdaki
`--workers N` metni ve belgedeki "kimlik hata" geçen metin) yaparak tutarlı
Türkçe kullanım sağlayın.
In `@forgelm/_http.py`:
- Around line 233-244: Update the module docstring that currently says "for
outbound POST requests" and only references requests.post to reflect that this
module now handles safe_get (and other non-POST calls); change the opening
sentence to cover outbound HTTP calls including GET and HEAD, mention the
safe_get function by name, and expand the contributor-facing grep example to
include requests.get and requests.head (and/or safe_get) so readers see GET/HEAD
calls are routed through this chokepoint; ensure any examples or wording that
imply "POST only" are adjusted to "HTTP requests (GET/HEAD/POST)" and reference
the safe_get symbol where appropriate.
In `@forgelm/cli/subcommands/_approve.py`:
- Around line 241-252: The helper function _output_error_and_exit is annotated
as returning None which makes mypy think control can continue after it; import
NoReturn from typing and change the signature of _output_error_and_exit to
return -> NoReturn so the typechecker knows it always exits; update the import
line to include NoReturn and change the def _output_error_and_exit(...)
annotation accordingly (this will also fix the possibly-unbound variable
warnings for required_event and decision_event in _approve.py and the similar
cases in _run_reject_cmd).
In `@forgelm/cli/subcommands/_audit_log_reader.py`:
- Around line 75-145: iter_audit_events currently exceeds the cognitive
complexity limit due to nested per-line parsing; extract the per-line logic into
a small private helper (e.g. _parse_nonempty_line) that takes (audit_log_path,
line_number, raw_line, strict) and returns either (event_dict) or raises
AuditLogParseError or signals skip, then simplify iter_audit_events to open the
file, iterate lines, call _parse_nonempty_line for non-empty lines and handle
its return/exception to either yield (line_number, event) or increment
skipped_lines; update logger.warning and error behavior unchanged and keep all
existing symbols (iter_audit_events, AuditLogParseError, logger) intact.
---
Nitpick comments:
In `@forgelm/cli/subcommands/_doctor.py`:
- Around line 858-859: Remove the dead helper _maybe_unused and the unused
Optional import: delete the function named _maybe_unused() and remove Optional
from the typing import list (the line that currently reads from typing import
Any, Callable, Dict, List, Optional, Tuple), ensuring no other code in this
module depends on Optional (replace any remaining usages with X | None if
needed).
- Around line 410-488: Extract the bounded os.walk loop from
_check_hf_cache_offline into a new helper _walk_hf_cache_bounded(cache_dir_abs:
str) that returns (file_count, total_bytes, walk_truncated); move base_depth
calculation and all depth/file-cap, symlink skip and OSError handling into that
helper so its cognitive complexity contains the walking logic, then replace the
loop in _check_hf_cache_offline with a single call to
_walk_hf_cache_bounded(cache_dir_abs) and use its returned values to compute
cache_gib, detail and the _CheckResult; ensure the helper signature and returned
tuple match the rest of the function to keep types and behavior unchanged.
- Around line 39-70: Add module-level string constants for the probe names (e.g.
_PROBE_PYTHON_VERSION = "python.version", _PROBE_TORCH_CUDA = "torch.cuda",
_PROBE_GPU_INVENTORY = "gpu.inventory", _PROBE_HF_HUB_REACHABLE =
"hf_hub.reachable", _PROBE_HF_HUB_OFFLINE_CACHE = "hf_hub.offline_cache",
_PROBE_DISK_WORKSPACE = "disk.workspace", _PROBE_OPERATOR_IDENTITY =
"operator.identity") and use those constants everywhere the corresponding
literal is currently used (in the individual check functions and in
_build_check_plan) so the probe names are centralized the same way
_STATUS_PASS/_STATUS_WARN/_STATUS_FAIL are; update references in any functions
that construct or compare probe keys to use the new _PROBE_* symbols.
In `@tests/test_approvals_listing.py`:
- Around line 108-114: The helper _build_args currently returns a MagicMock
which can silently allow missing or misspelled CLI attributes; replace MagicMock
with a strict namespace (e.g., types.SimpleNamespace or argparse.Namespace) so
attributes must be explicitly present. Update the return type and construction
inside _build_args to create a real namespace with .pending, .show, and
.output_dir set from the function parameters (keep the existing signature and
ensure output_dir is converted to str). This will make tests fail if
_run_approvals_cmd reads a non-existent or misspelled argument.
🪄 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: 0c2dffe9-402a-4c38-88d1-5477b97324ef
📒 Files selected for processing (23)
.github/workflows/ci.ymlCHANGELOG.mddocs/analysis/code_reviews/closure-plan-202604300906.mddocs/analysis/code_reviews/gdpr-erasure-design-202605021414.mddocs/analysis/code_reviews/library-api-design-202605021414.mddocs/standards/architecture.mddocs/standards/error-handling.mddocs/usermanuals/en/getting-started/installation.mddocs/usermanuals/en/reference/cli.mddocs/usermanuals/en/reference/json-output.mddocs/usermanuals/tr/getting-started/installation.mddocs/usermanuals/tr/reference/cli.mddocs/usermanuals/tr/reference/json-output.mdforgelm/_http.pyforgelm/cli/_parser.pyforgelm/cli/subcommands/_approvals.pyforgelm/cli/subcommands/_approve.pyforgelm/cli/subcommands/_audit_log_reader.pyforgelm/cli/subcommands/_doctor.pyforgelm/data_audit/_orchestrator.pytests/test_approvals_listing.pytests/test_data_audit_workers.pytests/test_doctor.py
✅ Files skipped from review due to trivial changes (3)
- forgelm/cli/_parser.py
- CHANGELOG.md
- forgelm/cli/subcommands/_approvals.py
Comment on lines
+1919
to
1927
| | GH-023 (`ingestion.retention.raw_documents.ttl_days` config) | Medium | **Top-level `retention.*` altında absorbe** (Faz 20 design §10 Q1 kararı) — nested `ingestion.retention.*` shape'i REDDEDİLDİ; policy ingestion artefact'larının ötesini kapsadığı için top-level. Faz 21 `RetentionConfig`'ine `raw_documents_retention_days` field'ı eklenir + `docs/usermanuals/en/compliance/gdpr.md` YAML örneği yeniden yazılır | Faz 21 (genişletme) | | ||
| | GH-024 (`FORGELM_RESUME_TOKEN`) | Low | **Doc-only — kaldır** (kodda kullanılmıyor) | **Faz 30 Task O** | | ||
| | GH-025 (`FORGELM_CACHE_DIR`) | Low | **Doc-only — kaldır + `HF_HOME` canonical işaretle** | **Faz 30 Task O** | | ||
| | GH-026 (`ingestion.retention` config; duplicate of GH-023) | Medium | (GH-023 ile aynı — Faz 21 extend) | Faz 21 | | ||
| | GH-026 (`ingestion.retention` config; duplicate of GH-023) | Medium | (GH-023 ile aynı — top-level `retention.*` altında absorbe) | Faz 21 | | ||
| | GH-027 (alias of GH-024) | Low | (GH-024 ile aynı) | Faz 30 Task O | | ||
| | GH-028 (alias of GH-025) | Low | (GH-025 ile aynı) | Faz 30 Task O | | ||
|
|
||
| **Ek görev:** Faz 21 kapsamı **GH-023 absorption** ile genişletilir — `ForgeConfig`'a `ingestion: IngestionConfig` top-level key + `IngestionConfig.retention.raw_documents.ttl_days` Pydantic field eklenir; `_purge --kind raw_documents` flag'ı bu retention policy'yi enforce eder. Faz 21 acceptance'a yeni satır: "Config: `ingestion.retention.raw_documents.ttl_days: int = 90` field çalışır + Pydantic Literal sweep tarafından kapsanır + GDPR docs'taki YAML örneği geçerli." | ||
| **Ek görev:** Faz 21 kapsamı **GH-023 absorption** ile genişletilir — Faz 20 design §10 Q1 kararına göre `ForgeConfig`'a top-level `retention: Optional[RetentionConfig] = None` field eklenir (nested `ingestion.retention.*` shape REDDEDİLDİ; gerekçe: policy ingestion artefact'larının ötesini — audit log + staging + ephemeral artefact — kapsar). `RetentionConfig` field'ları: `audit_log_retention_days: int = 1825`, `staging_ttl_days: int = 7`, `ephemeral_artefact_retention_days: int = 90`, `raw_documents_retention_days: int = 90` (GH-023'ün koruduğu eski `ingestion.retention.raw_documents.ttl_days` semantiği). `_purge --kind {raw_documents,staging,artefacts}` flag'ı policy'yi enforce eder. **Aynı PR'da:** `docs/usermanuals/en/compliance/gdpr.md` lines 51-64 YAML örneği yeniden yazılır + TR mirror — eski `ingestion.retention.raw_documents.ttl_days` shape `retention.raw_documents_retention_days`'e değiştirilir; `EvaluationConfig.staging_ttl_days` (v0.5.5'te shipped) deprecation cadence'ı uygulanır (alias-forward to `retention.staging_ttl_days`, v0.7.0'da kaldır). Faz 21 acceptance'a yeni satırlar: "Config: top-level `retention.*` block çalışır + 4 field Pydantic-validate; `evaluation.staging_ttl_days` DeprecationWarning + alias-forward; GDPR docs YAML örneği yeni shape'i gösteriyor + TR mirror eşleşiyor." | ||
|
|
Contributor
There was a problem hiding this comment.
Resolve the Phase‑21 contract drift on retention schema and purge kinds.
These lines add raw_documents_retention_days and --kind raw_documents, but the Phase 20 design doc in this PR still specifies a narrower purge-kind surface and retention schema. Please unify both docs to one canonical contract before implementation starts, otherwise Phase 21 acceptance/test scope is ambiguous.
As per coding guidelines, "Documentation must follow docs/standards/documentation.md and docs/standards/localization.md standards".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/analysis/code_reviews/closure-plan-202604300906.md` around lines 1919 -
1927, The PR introduces Phase‑21 retention fields
(`raw_documents_retention_days`) and purge kinds but conflicts with the Phase‑20
design; reconcile by adding a top-level RetentionConfig and wiring it through
ForgeConfig and CLI, i.e., add ForgeConfig.retention: Optional[RetentionConfig]
= None, implement class RetentionConfig with fields
audit_log_retention_days:int=1825, staging_ttl_days:int=7,
ephemeral_artefact_retention_days:int=90, raw_documents_retention_days:int=90,
update purge CLI to accept --kind {raw_documents,staging,artefacts}, deprecate
EvaluationConfig.staging_ttl_days by alias‑forwarding it to
retention.staging_ttl_days with a DeprecationWarning (keep alias until v0.7.0),
and update the YAML example in docs/usermanuals/en/compliance/gdpr.md (lines
~51–64) and its TR mirror to use retention.raw_documents_retention_days; ensure
related acceptance tests/assertions reflect the new top‑level schema and purge
kinds.
Collaborator
Author
There was a problem hiding this comment.
Rejected — Phase 21 scope leak. raw_documents_retention_days and --kind raw_documents are spec'd in Phase 20 design (this PR, design-only) and scheduled for Phase 21 implementation (separate PR). Implementing them in the integration PR would leapfrog Phase 21 and break the design-only / implementation-only split this wave was structured around.
The "contract drift" the bot perceives is actually the spec describing what Phase 21 will implement; the closure-plan §15.5 v2.5 entry (this same line range) explicitly calls out the absorption decision and aligns with the Phase 20 design. No change needed in this PR.
Contributor
There was a problem hiding this comment.
@cemililik, understood and acknowledged. You're right — the closure-plan §15.5 v2.5 entry is spec language describing what Phase 21 will implement; it does not represent implementation happening in this PR. The design-only / implementation-only split is intentional per this Wave's structure, and flagging the spec description as "contract drift" was incorrect on my part. I'll retract the concern.
✏️ Learnings added
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 28
File: docs/analysis/code_reviews/closure-plan-202604300906.md:1919-1927
Timestamp: 2026-05-04T13:05:51.890Z
Learning: In cemililik/ForgeLM, the closure-plan document (`docs/analysis/code_reviews/closure-plan-202604300906.md`) is a living design/planning document. When its §15.5 or phase-table entries describe retention fields, purge kinds, config shapes, or acceptance criteria that a *future* phase (e.g., Phase 21) will implement, this is intentional spec language — not "contract drift" or "scope leak". Each Wave is structured with design-only PRs (e.g., Phase 20) describing what implementation PRs (e.g., Phase 21, separate PR) will deliver. Do not flag closure-plan language about future phase scope as needing immediate implementation or as a doc/code inconsistency.
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: 28
File: docs/usermanuals/tr/getting-started/installation.md:109-109
Timestamp: 2026-05-04T13:05:13.484Z
Learning: In the cemililik/ForgeLM repo documentation, use the correct pip optional-extra name for distributed training: `distributed` (e.g., `pip install 'forgelm[distributed]'`). Do not suggest renaming `distributed` to `deepspeed` in any pip install commands or extras lists. Document `deepspeed>=0.14.0` only as the underlying PyPI dependency that is included inside the `distributed` extra (as defined in `pyproject.toml`). If `forgelm doctor` shows an `extras.deepspeed` label, treat it as a probe/display name rather than the pip extra name.
Round-2 inline review-bot pass on PR #28. Six findings shipped fixes, one rejected as Phase 21 scope leak (RetentionConfig wiring belongs in the next PR), two rejected as false positives with rationale recorded in the doctor module docstring + CHANGELOG. Code fixes: - _doctor.py _check_hf_cache_offline: OSError on getsize tracked as unreadable_count, surfaced in detail/extras, downgrades verdict to warn instead of silently producing a misleading total. - _doctor.py _render_text glyphs migrated from Unicode (U+2713/U+2717) to ASCII (+, x). Header em-dash (U+2014) caught by the new test_text_output_is_pure_ascii regression test, replaced with hyphen. - _doctor.py _check_python_version pinned to (major, minor) 2-tuple slice for clarity. - _doctor.py extracted _walk_hf_cache_bounded helper, added _PROBE_* string constants for the 8 probe names, removed dead _maybe_unused + the Optional import it kept alive. - _approvals.py _collect_pending_runs sort key uses _safe_timestamp_key which returns "" for any non-string timestamp value. Three regression tests cover string + int + list timestamp shapes. - _approve.py + _approvals.py _output_error_and_exit re-typed -> NoReturn. - _audit_log_reader.py iter_audit_events extracted per-line parsing into _parse_nonempty_line so the iterator stays at one level of nesting. - _http.py module docstring rewritten to cover GET/HEAD/POST. - tests _build_args MagicMock → SimpleNamespace (misspelled attrs now raise AttributeError instead of silently returning a Mock). Doc fixes: - gdpr-erasure-design table cell pipe escaped, hard-coded "445 lines" count replaced with a "recheck with wc -l" note, gdpr_erasure.md link fixed to ../../guides/gdpr_erasure.md (Phase 21 deliverable). - TR cli.md: paraleliz → paralellik sağlar; kimlik hata → kimlik hatası; operator → operatör. Verification: ruff check + format clean, pytest 1158 passed / 14 skipped, coverage 69.57% (≥ 40% required). No semantic change to any shipped behaviour beyond the bug fixes called out above. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SonarCloud 5 issues + 2 hotspots + CodeRabbit 9 inline comments verified.
Six findings shipped fixes; eight rejected as stale or false positives
with rationale recorded in CHANGELOG.
Sonar fixes:
- _http.py:297 (S5332) http://-in-rejection-message: split scheme tokens
so the rule doesn't trip on the branch that *enforces* it.
- _doctor.py:444 (S3776) _walk_hf_cache_bounded complexity 16: extracted
per-directory _accumulate_files_in_dir helper.
- _approve.py:233 (S3776) _run_approve_cmd complexity 16: extracted
_read_required_event_for_approve (parse + missing + double-decision).
- _dispatch.py:23 (S3776) _dispatch_subcommand complexity 16: dispatch
table dict replaces if/elif chain; verify-audit and chat stay
special-cased.
- _parser.py:541 (S1192) duplicated --output-dir help string: extracted
_OUTPUT_DIR_HELP module constant.
- test_data_audit_workers.py:104 (S6326) regex `^ ` → `^ {2}`.
CodeRabbit fix:
- _approvals.py: explicit os.access(..., os.R_OK) check after the
isfile gate in both --pending and --show. Without it,
iter_audit_events swallows OSError-on-open and yields nothing —
a chmod-broken audit log would masquerade as healthy "no pending
approvals" and an operator could miss a real pending decision.
Two regression tests cover both modes.
Stale findings rejected (with citations in CHANGELOG):
- gemini sys.version_info[:2] — already shipped 2148a49.
- qodo "doctor uses non-secret env-vars" — same rationale as round 2.
- CodeRabbit gdpr-erasure event count "3 vs 6" — already 6 throughout;
the "+ 3 new warning events" parenthetical refers to Round-2 delta.
- CodeRabbit --check-policy exit code — already resolved at L196 + L199.
- CodeRabbit ForgeTrainer lazy access — bot misread; test asserts lazy.
- CodeRabbit TR distributed→deepspeed — bot misread pyproject; the
extra IS named "distributed" (deepspeed is the dep inside).
- CodeRabbit "add doctor + approvals to help epilog" — already present.
- CodeRabbit --show latest-wins — fixed in 6d09e0b (Round-1 Phase 37).
- CodeRabbit "add strict parse path" — already shipped Round-1.
- CodeRabbit pytest.approx — already in place at test_doctor.py:161-162.
Verification: ruff format + check clean, pytest 1160 passed (+2 new
readability regression tests) / 14 skipped, coverage 69.72%.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md (3)
75-81: 💤 Low valueOptional: Reword the consecutive
If ...bullets in §3.1 to reduce style lint noise.LanguageTool is flagging “three successive sentences begin with the same word” around the dual-set window bullets in §3.1. This is style-only, but if you want to quiet lint, consider swapping one or more bullets to start with “When …” / “In the case where …” / “If … AND …” consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md` around lines 75 - 81, The three consecutive bullets in §3.1 under "Conflict-resolution semantics (dual-set window v0.5.5–v0.6.x)" all start with "If", triggering style lint warnings; edit the four bullets referring to evaluation.staging_ttl_days and retention.staging_ttl_days so they vary their leading phrase (e.g., use "When only … is set", "If only … is set", "In the case where both are set with identical values", "If both are set with different values") while preserving the exact field names (evaluation.staging_ttl_days, retention.staging_ttl_days), the described behaviors (alias-forward + DeprecationWarning, canonical path/no warning, DeprecationWarning + canonical value, raise ConfigError naming both keys/values), and the v0.7.0 removal target text.
261-262: 💤 Low valueOptional: Tighten “by mistake” wording in §5.3.
LanguageTool flags “by mistake” as wordy. A shorter alternative like “accidentally” or “in error” keeps intent while reducing style warnings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md` around lines 261 - 262, Summary: Tighten wording in §5.3 by replacing “by mistake” with a more concise alternative. Update the sentence that reads "If the justification field includes something that *might* be PII (e.g. operator pastes the data subject's email into the justification by mistake)..." to use "accidentally" or "in error" instead of "by mistake", and make the same change in the operator guide quote that references `--justification`; ensure phrasing remains clear and consistent across §5.3 and the operator guide.
136-138: 💤 Low valueOptional: Clarify “consumer” subject vs grammatical person in §3.3.
LanguageTool suggests “who” for a person vs object, but “consumer” may be a system/consumer rather than a person. If you want to silence the hint without changing meaning, rephrase to avoid the person/object ambiguity (e.g., “a consumer that distrusts mtime” → “a consumer can filter…” or “a downstream consumer can filter…”).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md` around lines 136 - 138, The sentence in §3.3 ("The `--check-policy` JSON output includes `age_source ∈ {audit, mtime}` per artefact so a consumer that distrusts mtime can filter for `audit`-sourced ages only.") uses "that" in a way flagged by LanguageTool; reword to avoid the person/object ambiguity by making the consumer clearly a system/actor — e.g. change to "The `--check-policy` JSON output includes `age_source ∈ {audit, mtime}` per artefact; a consumer can filter for `audit`-sourced ages if it distrusts `mtime`." or "a downstream consumer can filter for `audit`-sourced ages," updating the sentence in §3.3 accordingly.docs/usermanuals/tr/reference/cli.md (2)
95-96: MD051 burada “hash-router” linkleri için beklenen/yanıltıcı olabilir—linki bozma.Line 95’teki
[# Ver i Denetimi](#/data/audit)gibi#/<section>/<page>biçimi bu repo docs’larında SPA hash-router route identifier’ı olarak kullanılıyor. Bu nedenle MD051 “link fragments should be valid” uyarısını görürseniz link hedefini relative.md/fragment anchor’a çevirmeyin; bunun yerine MD051’i bu hash-router desenleri için lint pipeline tarafında bastırmayı tercih edin. citeturn0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/usermanuals/tr/reference/cli.md` around lines 95 - 96, The MD051 lint warning is a false positive because the docs intentionally use SPA hash-router route identifiers like "#/data/audit" (seen in the link `[# Ver i Denetimi](`#/data/audit`)`); do not rewrite the link to a relative .md anchor—instead suppress MD051 for this occurrence by adding a markdownlint directive (e.g. disable-next-line or disable around this link) so the hash-router pattern is preserved; reference the link fragment "#/data/audit" to locate the exact spot and ensure the suppression targets MD051 only.
55-265: ⚡ Quick winReduce repeated MD014 noise for shell examples (disable/enable around examples).
markdownlint-cli2MD014 “commands-show-output” uyarıları birçok```shellbloğunda tekrar ediyor. Her örneğe çıktı satırı eklemek yerine, örnek blokları kapsayacak tek seferlik birmarkdownlint-disable MD014/markdownlint-enable MD014yerleştirmek pratik.💡 Proposed approach
+<!-- markdownlint-disable MD014 --> ```shell $ forgelm --config configs/run.yaml --dry-run # doğrula $ forgelm --config configs/run.yaml --fit-check # VRAM kontrolü $ forgelm --config configs/run.yaml # eğit @@ $ forgelm --config configs/run.yaml # Eğitim, export ve deploy config üretimi hep birlikte gerçekleşir.+
</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/usermanuals/tr/reference/cli.mdaround lines 55 - 265, The markdownlint
MD014 warnings are noisy across manyshellexample blocks; surround the
group of shell examples with a single suppression instead of adding output
comments to each block: insert immediately
before the firstshellblock (the first CLI example starting with "$
forgelm --config configs/run.yaml --dry-run") and insert after the final closing(the last shell example in the file, the pattern showing "# Eğitim, export ve deploy..."), leaving all existingshell``` blocks unchanged so markdownlint will ignore
the commands-show-output rule for the whole section.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md:
- Around line 269-272: Update the §5.4 table row "target_id (row mode)" to
remove the "or line number" fallback and state the canonical source is the JSONL
idfield only (operators must pre-populate stable IDs viaforgelm audit --add-row-ids), reflecting §4.2; explicitly note that line-number fallback is
rejected forforgelm purge --row-id, and keep the emission rule (store as
SHA-256(salt + value) with salt rules) intact so implementers know exactly how
target_idis produced and constrained.In
@docs/usermanuals/tr/reference/cli.md:
- Line 44: Satırdaki açıklamada "ya da" kullanımından kaynaklanan lint uyarısını
düzeltmek için docs/usermanuals/tr/reference/cli.md içindeki--output DIR
satırını bulun ve seçenek açıklamasındaki “./audit/ya da./compliance/”
ifadesini “./audit/veya./compliance/” olarak değiştir; referans için
değiştirilecek metinde geçen--output DIR,./audit/ve./compliance/
sembollerini kontrol ederek doğru yerde güncelleme yap.- Line 73: Update the Turkish wording in the CLI docs for the command "forgelm
doctor": replace the phrase "operator kimliği" with the correct Turkish
orthography "operatör kimliği" so the sentence reads correctly alongside other
occurrences; locate the string in the "forgelm doctor" doc text (the sentence
mentioning Python version, torch/CUDA/GPU, extras, HF Hub/offline cache, disk
space, operator/operatör identity and audit-secret) and make the single-word
replacement ensuring consistency with repo-wide usage.In
@forgelm/cli/_dispatch.py:
- Around line 87-90: The current KeyboardInterrupt handler re-raises for
non-guarded commands which bypasses the project's exit-code contract; change the
else-path to call sys.exit(...) with the designated public contract exit code
instead of re-raising. Specifically, in the except KeyboardInterrupt block that
checks _SIGINT_GUARDED_SUBCOMMANDS and uses EXIT_TRAINING_ERROR for guarded
commands, replace the "raise" with sys.exit(EXIT_CONTRACT_ERROR) (or the
module's canonical public non-zero exit constant); if that constant doesn't
exist, add a descriptive constant (e.g., EXIT_CONTRACT_ERROR) to the same
constants set and use it so all SIGINT paths route through the defined exit-code
mapping.
Nitpick comments:
In@docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md:
- Around line 75-81: The three consecutive bullets in §3.1 under
"Conflict-resolution semantics (dual-set window v0.5.5–v0.6.x)" all start with
"If", triggering style lint warnings; edit the four bullets referring to
evaluation.staging_ttl_days and retention.staging_ttl_days so they vary their
leading phrase (e.g., use "When only … is set", "If only … is set", "In the case
where both are set with identical values", "If both are set with different
values") while preserving the exact field names (evaluation.staging_ttl_days,
retention.staging_ttl_days), the described behaviors (alias-forward +
DeprecationWarning, canonical path/no warning, DeprecationWarning + canonical
value, raise ConfigError naming both keys/values), and the v0.7.0 removal target
text.- Around line 261-262: Summary: Tighten wording in §5.3 by replacing “by
mistake” with a more concise alternative. Update the sentence that reads "If the
justification field includes something that might be PII (e.g. operator pastes
the data subject's email into the justification by mistake)..." to use
"accidentally" or "in error" instead of "by mistake", and make the same change
in the operator guide quote that references--justification; ensure phrasing
remains clear and consistent across §5.3 and the operator guide.- Around line 136-138: The sentence in §3.3 ("The
--check-policyJSON output
includesage_source ∈ {audit, mtime}per artefact so a consumer that distrusts
mtime can filter foraudit-sourced ages only.") uses "that" in a way flagged
by LanguageTool; reword to avoid the person/object ambiguity by making the
consumer clearly a system/actor — e.g. change to "The--check-policyJSON
output includesage_source ∈ {audit, mtime}per artefact; a consumer can
filter foraudit-sourced ages if it distrustsmtime." or "a downstream
consumer can filter foraudit-sourced ages," updating the sentence in §3.3
accordingly.In
@docs/usermanuals/tr/reference/cli.md:
- Around line 95-96: The MD051 lint warning is a false positive because the docs
intentionally use SPA hash-router route identifiers like "#/data/audit" (seen in
the link[# Ver i Denetimi](#/data/audit)); do not rewrite the link to a
relative .md anchor—instead suppress MD051 for this occurrence by adding a
markdownlint directive (e.g. disable-next-line or disable around this link) so
the hash-router pattern is preserved; reference the link fragment "#/data/audit"
to locate the exact spot and ensure the suppression targets MD051 only.- Around line 55-265: The markdownlint MD014 warnings are noisy across many
shellexample blocks; surround the group of shell examples with a single
suppression instead of adding output comments to each block: insert immediately before the firstshellblock
(the first CLI example starting with "$ forgelm --config configs/run.yaml
--dry-run") and insert after the final
closing(the last shell example in the file, the pattern showing "# Eğitim, export ve deploy..."), leaving all existingshell``` blocks unchanged so
markdownlint will ignore the commands-show-output rule for the whole section.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `9eeeb9c7-3109-425a-8ae2-dd743e8d2c6e` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 59f8d8c72a75d55d10ffada3d68166adb9d3553d and 5a408833bb3978966511075d3d0d0e0c81a16b21. </details> <details> <summary>📒 Files selected for processing (13)</summary> * `CHANGELOG.md` * `docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md` * `docs/usermanuals/tr/reference/cli.md` * `forgelm/_http.py` * `forgelm/cli/_dispatch.py` * `forgelm/cli/_parser.py` * `forgelm/cli/subcommands/_approvals.py` * `forgelm/cli/subcommands/_approve.py` * `forgelm/cli/subcommands/_audit_log_reader.py` * `forgelm/cli/subcommands/_doctor.py` * `tests/test_approvals_listing.py` * `tests/test_data_audit_workers.py` * `tests/test_doctor.py` </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * forgelm/cli/subcommands/_audit_log_reader.py * forgelm/cli/subcommands/_doctor.py </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (7)</summary> * forgelm/cli/_parser.py * tests/test_doctor.py * forgelm/cli/subcommands/_approve.py * tests/test_data_audit_workers.py * CHANGELOG.md * forgelm/cli/subcommands/_approvals.py * tests/test_approvals_listing.py </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
CodeRabbit round-4 inline batch — 8 findings verified, all 8 fixed (no rejections this round; the suggestions were either right or constructive). Code: - _dispatch.py SIGINT handler now sys.exit(EXIT_TRAINING_ERROR) for every subcommand, not just chat / quickstart. A bare `raise` would let Python convert SIGINT into shell-shaped 130 (= 128+SIGINT), which is outside the public 0/1/2/3/4 contract and would surprise CI/CD scripts. _SIGINT_GUARDED_SUBCOMMANDS deleted as dead code. Docs: - gdpr-erasure-design §5.4 target_id source: removed "or line number" fallback (was already rejected at the CLI per §4.2). - gdpr-erasure-design §3.1 dual-set bullets: varied "If" leads (When only / When only / In the case where / If) so the prose- style lint stops flagging the four-in-a-row pattern. Behaviour spec unchanged. - gdpr-erasure-design §3.3: "consumer that distrusts mtime" reworded to remove the person/object ambiguity. - gdpr-erasure-design §5.3: "by mistake" → "accidentally". - TR cli.md L44: "ya da" → "veya" (formal register). - TR cli.md L73: "operator kimliği" → "operatör kimliği". Tooling: - New .markdownlint.json at repo root pins project stance on MD051 (SPA hash-router routes are intentional, not invalid anchors) and MD014 (shell examples deliberately omit output to avoid version- drift rot). Single-config approach beats scattering inline directives in every Markdown file. Verification: ruff check + format clean, pytest 1160 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
docs/usermanuals/tr/reference/cli.md (1)
223-223:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTürkçe terimi tutarlılaştırın:
operator→operatör.Line 223’teki ifade, dokümanın diğer kısımlarındaki Türkçe kullanım ile uyumsuz.
💡 Önerilen düzeltme
-| `FORGELM_OPERATOR` | Audit event'lerinde kaydedilen operator kimliği (`getpass.getuser()@hostname`'i override eder) | +| `FORGELM_OPERATOR` | Audit event'lerinde kaydedilen operatör kimliği (`getpass.getuser()@hostname`'i override eder) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/usermanuals/tr/reference/cli.md` at line 223, Satırdaki Türkçe terim uyumsuzluğunu düzeltmek için tablo entradasındaki `FORGELM_OPERATOR` açıklamasında geçen "operator" kelimesini tüm dokümanda kullanılan biçime uygun olarak "operatör" ile değiştir; hedef metin: "`FORGELM_OPERATOR` | Audit event'lerinde kaydedilen operatör kimliği (`getpass.getuser()@hostname`'i override eder)`" olarak güncelle ve benzer kullanım varsa aynı terim tutarlılığı için diğer açıklamalarda da kontrol et.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 461-462: Add a blank line immediately before the heading "###
Added — Wave 2a / Phase 34 — `forgelm doctor` env-check subcommand" so the
heading is separated from the preceding list content; edit the CHANGELOG entry
where that exact heading appears and insert one empty line above it to satisfy
MD022.
In `@docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md`:
- Around line 259-261: The documentation currently contradicts itself about
handling of target_id between sections §5.3 and §5.4: §5.3 says event bodies may
keep target_id (examples like row_id=42), while §5.4 requires hashing row-mode
target_id. Update the doc to unify semantics by deciding whether target_id is
treated as an operational identifier (cleartext) or PII-derived (hashed in
row-mode), then change the wording in both §5.3 and §5.4 to the chosen policy;
explicitly reference "target_id" and the "row-mode" behavior and add a short
note in the operator guide statement about justification handling to avoid
ambiguity (e.g., if target_id must be hashed in row-mode, state that clearly
alongside the justification guidance).
- Line 154: Clarify and reconcile the contradictory deletion contracts: decide
whether compliance artefacts (compliance/*.json) are tied to the entire
<output_dir> lifetime or can be targeted with the purge command; then update the
text around the sentence referencing "their lifetime is the run's lifetime" and
the examples referencing "forgelm purge --run-id ... --kind artefacts" so they
match. If compliance artefacts are deletable via the CLI, change the statement
about requiring deletion of the whole <output_dir> to explain that targeted
deletion is supported (describe that forgelm purge --run-id <id> --kind
artefacts will remove compliance/*.json for that run), or if they must be
deleted only by removing the whole output_dir, remove/alter the purge examples
and note that the operator must manually delete <output_dir>; ensure both the
phrase "compliance/*.json", the reference to "<output_dir>", and the "forgelm
purge --run-id ... --kind artefacts" example are consistent.
In `@forgelm/cli/_dispatch.py`:
- Around line 51-52: The direct sys.exit call for the "verify-audit" branch
bypasses the surrounding try/except KeyboardInterrupt handling; change the
branch so it does not call sys.exit immediately but instead runs
_cli_facade._run_verify_audit_cmd(args) inside the main try/except (e.g., assign
its return to a variable or fall through to the common exit path) and then call
sys.exit(...) from the controlled exit point so a KeyboardInterrupt is caught
and your public exit-code contract (handled by the try/except that wraps the
main dispatch) is preserved.
---
Duplicate comments:
In `@docs/usermanuals/tr/reference/cli.md`:
- Line 223: Satırdaki Türkçe terim uyumsuzluğunu düzeltmek için tablo
entradasındaki `FORGELM_OPERATOR` açıklamasında geçen "operator" kelimesini tüm
dokümanda kullanılan biçime uygun olarak "operatör" ile değiştir; hedef metin:
"`FORGELM_OPERATOR` | Audit event'lerinde kaydedilen operatör kimliği
(`getpass.getuser()@hostname`'i override eder)`" olarak güncelle ve benzer
kullanım varsa aynı terim tutarlılığı için diğer açıklamalarda da kontrol et.
🪄 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: 9e50266e-5c1c-47b8-9972-4c45cf9093fe
📒 Files selected for processing (5)
.markdownlint.jsonCHANGELOG.mddocs/analysis/code_reviews/gdpr-erasure-design-202605021414.mddocs/usermanuals/tr/reference/cli.mdforgelm/cli/_dispatch.py
✅ Files skipped from review due to trivial changes (1)
- .markdownlint.json
…e + 1 dup)
Round-5 review pass surfaced load-bearing defects the prior 4 rounds
missed. 11 of 13 findings shipped fixes; 1 deferred (F-R5-07: TOCTOU
on the readability check is sub-microsecond and iter_audit_events
already logs at ERROR which --quiet's WARNING floor surfaces).
Code:
- F-R5-01: approve / reject now share is_audit_log_readable() with
approvals listing. Chmod-broken audit_log.jsonl previously
surfaced as "verify the run_id" — wrong debugging path on the
Article 14 critical path. Helper extracted to _audit_log_reader.py.
- F-R5-02 + Inline D: verify-audit moved into the dict-table dispatch
so SIGINT during a long verify-of-100K-events lands on
EXIT_TRAINING_ERROR (= 2) instead of bubbling up as Python's
shell-shaped 130 (= 128+SIGINT). Dispatcher docstring rewritten to
acknowledge chat REPL's graceful Ctrl-C → exit 0.
- F-R5-03: added FORGELM_AUDIT_TEST_WORKER_RAISES injection point in
_process_split_for_pool + regression test. The previous parallel-
failure test exercised _process_split's internal catches; the
orchestrator's pool.map re-raise branch was dead in tests.
- F-R5-04: _walk_hf_cache_bounded gained an onerror callback +
walk_errors counter. Chmod-broken cache root (which os.walk
silently skipped) now surfaces as fail when no files visible.
- F-R5-06: json.dumps in _render_json gained default=str so a future
probe author surfacing a Path / datetime / bytes value cannot crash
the JSON envelope contract.
- F-R5-08: corrected misleading # noqa: F401,E402 → # noqa: E402 on
the _iter_audit_events alias (F401 was unnecessary; symbol IS used
in-module at lines 97 + 151).
Docs:
- Inline A: blank line before "### Added — Wave 2a / Phase 34" CHANGELOG
heading (MD022).
- Inline B: gdpr-erasure-design §5.3 reconciled with §5.4 — target_id
handling clarified per target_kind (run mode = clear, row mode =
hashed); operator-facing row_id=42 examples re-framed as "what the
operator typed", not "what lands in the chain".
- Inline C: gdpr-erasure-design §3.4 reconciled with §4.1 —
compliance/*.json IS deletable via forgelm purge --kind artefacts;
the previous "requires deleting the whole output_dir" claim
contradicted the purge-mode table.
- F-R5-05: gdpr-erasure-design §5.4 tightened salt-fallback semantics.
Per-output-dir salt is persistent (.forgelm_audit_salt, mode 0600);
new salt_source field ("env_var" / "per_dir") on every row-mode
erasure event makes salt-source changes detectable in the chain
instead of silently producing "different subject" misreadings.
TR docs:
- Duplicate E: TR cli.md L223 operator → operatör on the
FORGELM_OPERATOR row (Round-2 fixed L224, missed L223).
Verification: ruff check + format clean, pytest 1161 passed (+1 new
parallel-raise regression test), coverage stable.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…76 ceiling Round-5 readability-gate addition pushed _run_reject_cmd from 14 → 17 cognitive complexity (Sonar ceiling 15). Extracted two helpers in _approve.py: - _assert_audit_log_readable_or_exit: shared by approve / reject / approvals-pending / approvals-show (4 call sites collapsed). - _read_required_event_for_reject: reject-flavoured twin of _read_required_event_for_approve, same rationale (separate operator copy beats parameter-list ballooning). Behaviour-preserving — only structural change. ruff clean, pytest 1161 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
May 5, 2026
10 faz status update: Wave 2a (PR #28, merged 2026-05-04): - Faz 17 (audit --workers N) ⏳ → ✅ - Faz 18 (Library API design) ⏳ → ✅ - Faz 20 (GDPR design) ⏳ → ✅ - Faz 34 (forgelm doctor) ⏳ → ✅ - Faz 37 (forgelm approvals) ⏳ → ✅ Wave 2b (PR #30, ready to merge 2026-05-05): - Faz 16 (Pydantic description= migration + AST guard) ⏳ → ✅ - Faz 19 (Library API impl: lazy facade + py.typed) ⏳ → ✅ - Faz 21 (GDPR purge + RetentionConfig + 6 audit events) ⏳ → ✅ - Faz 35 (air-gap cache-models / cache-tasks) ⏳ → ✅ - Faz 36 (verify-annex-iv / verify-gguf / safety-eval) ⏳ → ✅ Plus: status legend entries for Wave 2a + 2b, kapanış özet paragraflarının eklenmesi, version log v2.6 (Wave 2a) + v2.7 (Wave 2b) entry'leri. Kalan iş (Wave 3+): 7 faz + 1 release. - Faz 22 (ISO 27001/SOC 2 design) - Faz 23 (ISO 27001/SOC 2 impl) - Faz 24 (bilingual TR mirror sweep) - Faz 26 (QMS bilingual mirror) - Faz 28 (Major+Minor cleanup) - Faz 30 (final docs + site finalization) - Faz 38 (forgelm reverse-pii GDPR Art. 15) - Faz 33 (v0.5.5 PyPI publish — POST-WAVE) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 5, 2026
PR #30 merged to development at commit b05edb5 (2026-05-05). Closure plan updated to reflect post-merge state: - Status legend: target branch corrected from "main" → "development" for PRs #21, #28, #30 (the project merges into development first; development → main happens at release time). - Wave 2b kapanış özeti: merge commit SHA recorded; suite count delta noted (1160 → 1298, +138 net tests across the wave). - Version log v2.7: rewritten from "merge öncesi" → "merge sonrası" with merge commit SHA + Wave 3 immediate-batch readiness note (Faz 22 + 24 + 38 dependency-free). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks done
6 tasks done
This was referenced May 8, 2026
This was referenced May 10, 2026
cemililik
added a commit
that referenced
this pull request
Jun 15, 2026
…R#63-review)
_validate_rubric let an empty positional {} placeholder slip past the unexpected/missing checks, then crashed .format() with IndexError mid-eval. Add an explicit 'if "" in field_roots' guard with an actionable message. CodeRabbit #28.
Co-Authored-By: Claude Opus 4.8 (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
Consolidated integration PR for Wave 2a of the closure plan (v0.5.5 cycle).
Bundles the five previously-scattered phase PRs (#23, #24, #25, #26, #27) into
a single review surface and lands all of the Wave-2a Round-1 multi-agent
review fixes on top.
The five original PRs are superseded by this one and will be closed
without merge.
What ships
closure/faz18-library-api-designclosure/faz20-gdpr-erasure-designclosure/faz17-workers-determinismclosure/faz37-approvals-listingclosure/faz34-doctor-subcommandPlus a Wave-2a infra extraction:
forgelm/cli/subcommands/_audit_log_reader.py— single source of truthfor the audit-log JSONL parser used by
_approve.py,_approvals.py, and(Phase 21)
forgelm purge.Phase-by-phase highlights
Phase 18 — Library API design (
docs/analysis/code_reviews/library-api-design-202605021414.md)12 sections + 16-row task plan that pin the public Python surface for
downstream consumers. Defines stable / experimental / internal tiers,
documents the lazy-import invariant (
import forgelmdoes not pull torch),specifies
py.typed+mypy --strict, resolves the Wave-1 round-5 lazy__getattr__carry-over forforgelm/cli/__init__.py, and locks thePhase-19 integration test surface.
Phase 20 — GDPR Article 17 erasure design (
docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md)12 sections + 11-test plan + file map for the future
forgelm purge. Mapsevery Article 17(1) trigger to a concrete ForgeLM action; specifies the new
RetentionConfigPydantic block (Article 5(1)(e) storage limitation);specifies six new audit events (
data.erasure_requested/data.erasure_completed/data.erasure_failed+ three operator-warningevents); resolves three Wave-1 carry-overs (GH-23, GH-13, Round-5
concurrent-approve lock); supplies the marketing-claim replacement copy for
safety_compliance.md.Phase 17 —
forgelm audit --workers Ndeterminismmultiprocessing.Pool(spawn-method, pinnedin code).
data_audit_report.jsonis byte-identical across worker counts (onlygenerated_atdiffers and is stripped textually before SHA-256comparison).
_process_split_for_poolso worker pickling staysspawn-method safe.
_positive_intvalidator rejects0/ negatives at parsetime; library-side
audit_dataset(workers=...)raises typedValueErrorfor
0 / -1 / bool / str.{1, 2, 4},per-split
languages_top3, identical PII / secrets / near-dup / totalcounts, split-iteration order pinned (
train→validation→test),single-split corpus tolerance, MinHash × workers (gated on
datasketchextra), and error-propagation in both sequential and parallel paths.
docs/guides/data_audit*.md).Phase 37 —
forgelm approvalslisting subcommandforgelm approvals --pending [--output-dir DIR]lists every run whoseaudit log carries a
human_approval.requiredevent without a matchingterminal decision. Tabular text or JSON envelope.
forgelm approvals --show RUN_ID --output-dir DIRprints the fullapproval chain (request → terminal decision) plus on-disk staging layout.
staging_pathfrom the audit logis run through
_staging_path_inside_output_dirbefore anyos.listdir.run_id, secondhuman_approval.requiredafter a prior decision).ghost-features-analysis-20260502(feat: Phase 10 post-training completion (v0.4.0) #7).Phase 34 —
forgelm doctorenv-check subcommandin
pyproject.toml, HF Hub reachability, workspace disk space, and theFORGELM_OPERATORaudit-identity hint.--offlineskips the HF Hub probe and inspects the local cache(
HF_HUB_CACHE>HF_HOME/hub>~/.cache/huggingface/hub); respectsHF_ENDPOINTfor self-hosted mirrors.contract.
lazily so
forgelm doctorruns on a brand-new machine before torch isinstalled.
probe-crash isolation + facade re-exports + HF cache precedence +
operator-identity anonymous opt-in + secret env masking.
ghost-features-analysis-20260502Welcome to ForgeLM Discussions! #1 onboarding blocker (30doc references across 8 files in
docs/usermanuals/no longer point at anon-existent command).
Wave-2a Round-1 review fixes
Each phase has a
fix(phase-NN): address review-bot + reviewer findingscommit on top of the original
feat/docscommit:93fd9ad— tier completeness audit, lazy-importcaching example, real
audit_dataset/verify_audit_logsignatures,CI trigger spec.
074e533— prior-state acknowledgement (resolvesstaging_ttl_dayscollision + retention shape), mtime caveat, personaldata minimisation table, row-id refusal, exit-code alignment, strict
commit ordering, scope-limitation paragraph.
e537151— pinned spawn context, try/except aroundpool.map with logger context, generator-expression refactor, file-hash
determinism contract, split-iteration order test, error-propagation
tests for both paths, TR docs.
6d09e0b— extracted shared_audit_log_reader.py,path-traversal guard via
_staging_path_inside_output_dir, latest-winssemantics, +8 tests.
9511e1d— corrected extras names againstpyproject.toml,HF_HUB_CACHEprecedence +HF_ENDPOINTrespect,cap-walked HF cache (depth 4, 5K files), removed dead
socket.timeoutbranch, secret-env masking for audit,
FORGELM_ALLOW_ANONYMOUS_OPERATOR,air-gap + installation doc updates.
Cross-PR observations (XPR-01 through XPR-06) from the multi-agent review
are also addressed in this consolidation: XPR-01 audit-log parser
proliferation closed by the new shared module; XPR-02 HTTP discipline,
XPR-03 JSON envelope schema, XPR-04 test rigor, XPR-05 documentation
drift, XPR-06 schema collisions all fixed in the per-phase fix commits.
Diff stat
26 files, ~4 600 insertions, ~120 deletions. Three new modules (
_doctor.py,_approvals.py,_audit_log_reader.py), one new argparse helper(
_argparse_types.py), three new test files (1 729 LOC of tests).Verification
ruff format .— 1 file reformatted (one over-long def intests/test_data_audit_workers.py), included in5a57286.ruff check .—All checks passed!pytest tests/ -q—1103 passed, 14 skippedin 67 s. Coverage 69.39 %(>= 40 % required).
Test plan
closure/wave2a-integration.forgelm --config config_template.yaml --dry-runfrom a cleancheckout.
forgelm doctorfrom a clean checkout (text +--output-format json).forgelm doctor --offlinehonoursHF_HUB_CACHEandHF_ENDPOINT.forgelm audit --workers 4on a 3-split corpus → byte-identicaldata_audit_report.jsonvs--workers 1.forgelm approvals --pendingon a workspace with a granted run + apending run → only the pending run listed.
forgelm approvals --show <run_id>for a re-staged run → latesthuman_approval.requiredsurfaces as pending.below).
Closes
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
forgelm doctor— environment diagnostics with--offlineand--output-format jsonsupportforgelm approvals— list pending approvals (--pending) and view run details (--show RUN_ID)forgelm audit --workers N— parallel split processing for faster auditsHF_HUB_OFFLINE/TRANSFORMERS_OFFLINEsupportDocumentation
doctor,approvals, and--workersfeatures