Repository navigation
Conversation
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>
Reviewer's GuideDesign-only PR that adds a detailed GDPR right-to-erasure analysis & design document and registers it in the changelog, defining the future Sequence diagram for forgelm purge --row-id corpus erasure flowsequenceDiagram
actor Operator
participant PurgeCLI
participant ErasureService
participant RunLock
participant FileSystem
participant ComplianceModule
participant AuditLogger
Operator->>PurgeCLI: forgelm purge --row-id id --corpus path [--dry-run]
PurgeCLI->>PurgeCLI: parse_flags_and_validate()
PurgeCLI->>Operator: prompt_confirm_if_needed()
Operator-->>PurgeCLI: confirm
PurgeCLI->>RunLock: acquire_run_lock(derived_run_id)
RunLock-->>PurgeCLI: lock_acquired
PurgeCLI->>ComplianceModule: emit_erasure_requested(row, id, fields)
ComplianceModule->>AuditLogger: append_event(data.erasure_requested, payload)
PurgeCLI->>ErasureService: purge_row(id, path, justification, dry_run)
alt dry_run
ErasureService->>FileSystem: scan_jsonl(path) without write
ErasureService-->>PurgeCLI: planned_changes
PurgeCLI-->>Operator: print_dry_run_report
else real_delete
ErasureService->>FileSystem: open source_jsonl(path)
ErasureService->>FileSystem: create NamedTemporaryFile
ErasureService->>FileSystem: stream_rows_skip_matching_id
ErasureService->>FileSystem: fsync_and_atomic_replace
ErasureService-->>PurgeCLI: result(bytes_freed, files_modified, pre_line_number)
PurgeCLI->>ComplianceModule: emit_erasure_completed(row, id, fields)
ComplianceModule->>AuditLogger: append_event(data.erasure_completed, payload)
end
RunLock-->>PurgeCLI: release_lock
PurgeCLI-->>Operator: exit_code 0 or 1 or 2
opt failure_during_delete
ErasureService-->>PurgeCLI: raise_error(error_class, error_message)
PurgeCLI->>ComplianceModule: emit_erasure_failed(row, id, fields)
ComplianceModule->>AuditLogger: append_event(data.erasure_failed, payload)
PurgeCLI-->>Operator: exit_code 2
end
Sequence diagram for ForgeTrainer.train retention pre-flight checksequenceDiagram
participant Operator
participant ForgeTrainer
participant ForgeConfig
participant RetentionConfig
participant FileSystem
participant ComplianceModule
participant AuditLogger
Operator->>ForgeTrainer: train()
ForgeTrainer->>ForgeConfig: load_config()
ForgeConfig-->>ForgeTrainer: config_with_retention
alt retention_config_set
ForgeTrainer->>RetentionConfig: get_horizons()
ForgeTrainer->>FileSystem: scan_output_dir_for_artefacts()
FileSystem-->>ForgeTrainer: artefact_list_with_mtime
ForgeTrainer->>ForgeTrainer: classify_violations(artefact_list, retention_config)
ForgeTrainer->>ComplianceModule: record_erasure_event(policy_check, null, justification, dry_run, fields)
ComplianceModule->>AuditLogger: append_event(data.erasure_requested, payload)
ComplianceModule->>AuditLogger: append_event(data.erasure_completed, payload)
alt enforce == block_on_excess and violations_exist
ForgeTrainer-->>Operator: raise ConfigError and abort
else enforce == warn_on_excess and violations_exist
ForgeTrainer-->>Operator: log_warnings
ForgeTrainer->>ForgeTrainer: continue_to_training
else no_violations_or_log_only
ForgeTrainer->>ForgeTrainer: continue_to_training
end
else retention_not_set
ForgeTrainer->>ForgeTrainer: skip_retention_check
end
ForgeTrainer-->>Operator: training_results
Class diagram for RetentionConfig, ForgeConfig, and purge-related componentsclassDiagram
class RetentionConfig {
+int audit_log_retention_days
+int staging_ttl_days
+int ephemeral_artefact_retention_days
+str enforce
}
class ForgeConfig {
+RetentionConfig retention
+str output_dir
+str config_path
+load_from_file(path)
}
class ForgeTrainer {
+ForgeConfig config
+train()
-_check_retention_policy()
}
class PurgeCLI {
+run(argv)
-_handle_row_mode(row_id, corpus_path, justification, dry_run, yes)
-_handle_run_mode(run_id, kind, output_dir, justification, dry_run, yes)
-_handle_policy_mode(config_path, output_format)
}
class ErasureService {
+purge_row(row_id, corpus_path, justification, dry_run)
+purge_run_staging(run_id, output_dir, justification, dry_run)
+purge_run_artefacts(run_id, output_dir, justification, dry_run)
+check_policy(config)
-_rewrite_jsonl_atomically(corpus_path, row_id)
-_delete_staging_dir(staging_path)
-_delete_artefact_allowlist(output_dir)
}
class ComplianceModule {
+record_erasure_event(target_kind, target_id, justification, dry_run, fields)
+emit_erasure_requested(target_kind, target_id, fields)
+emit_erasure_completed(target_kind, target_id, fields)
+emit_erasure_failed(target_kind, target_id, error_class, error_message, fields)
}
class AuditLogger {
+append_event(event_name, payload)
+verify_audit_log(path)
}
class RunLock {
+acquire_run_lock(run_id)
}
RetentionConfig <.. ForgeConfig : uses
ForgeConfig <.. ForgeTrainer : uses
ForgeConfig <.. PurgeCLI : loads
PurgeCLI --> ErasureService : delegates
ErasureService --> ComplianceModule : records_events
ComplianceModule --> AuditLogger : appends
ErasureService --> RunLock : acquires
Flow diagram for forgelm purge subcommand modes and flagsflowchart TD
A[start forgelm_purge_invocation] --> B{flags}
B -->|--row-id and --corpus| C[corpus_row_mode]
B -->|--run-id and --kind| D[run_scoped_mode]
B -->|--check-policy| E[policy_check_mode]
B -->|invalid_combination| Z[exit 1 config_error]
C --> C1[validate_row_id_and_corpus_path]
C1 --> C2[prompt_confirmation_unless_yes]
C2 -->|dry_run| C3[scan_jsonl_and_report]
C2 -->|real_delete| C4[acquire_run_lock]
C4 --> C5[emit_data_erasure_requested]
C5 --> C6[rewrite_jsonl_atomically]
C6 --> C7[emit_data_erasure_completed]
C7 --> C8[release_lock_and_exit_with_code]
D --> D1[validate_run_id_kind_output_dir]
D1 --> D2[prompt_confirmation_unless_yes]
D2 -->|kind=staging| D3[delete_staging_directory_with_safety_checks]
D2 -->|kind=artefacts| D4[delete_artefact_allowlist]
D3 --> D5[emit_erasure_events_and_exit]
D4 --> D5
E --> E1[load_config_and_retention_block]
E1 --> E2[scan_output_dir_for_violations]
E2 --> E3[print_text_or_json_report]
E3 --> E4[exit 0_even_if_violations]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoPhase 20: GDPR right-to-erasure analysis & design document
WalkthroughsDescription• Ships Phase 20 GDPR right-to-erasure analysis & design document • Maps Article 17 triggers to ForgeLM scope with 7-artefact taxonomy • Specifies RetentionConfig Pydantic schema for storage-limitation policy • Defines forgelm purge subcommand with three modes and atomicity guarantees • Documents three new audit events and Article 17(3)(b) chain-integrity preservation • Supplies 11-test plan and 16-file implementation map for Phase 21 • Resolves three Wave 1 carry-overs and six open design questions Diagramflowchart LR
A["Article 17 Triggers"] -->|mapped to scope| B["7 Artefact Kinds"]
B -->|erasure strategy| C["forgelm purge Subcommand"]
C -->|three modes| D["Row / Run / Policy"]
D -->|audit trail| E["3 New Audit Events"]
E -->|chain integrity| F["Append-Only Log"]
B -->|retention horizon| G["RetentionConfig Schema"]
G -->|enforcement| H["Pre-flight Gate"]
C -->|atomicity| I["Atomic Rename Pattern"]
File Changes1. CHANGELOG.md
|
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="CHANGELOG.md" line_range="22-24" />
<code_context>
+ scope of Phase 21's `forgelm purge` implementation. Maps every Article
+ 17(1) trigger to a ForgeLM action; enumerates the seven artefact kinds
+ that may carry personal data; specifies the `RetentionConfig` Pydantic
+ block (Article 5(1)(e) storage limitation); spec'es the three new
+ audit events (`data.erasure_requested` / `data.erasure_completed` /
+ `data.erasure_failed`); resolves three Wave 1 carry-overs (GH-023, GH-013,
</code_context>
<issue_to_address>
**issue (typo):** Consider correcting the typo "spec'es" to "specs" or "specifies".
This change will improve clarity and align the wording with standard usage.
```suggestion
17(1) trigger to a ForgeLM action; enumerates the seven artefact kinds
that may carry personal data; specifies the `RetentionConfig` Pydantic
block (Article 5(1)(e) storage limitation); specifies the three new
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive design document for implementing GDPR Article 17 (right-to-erasure) and Article 5(1)(e) (storage limitation) within ForgeLM. The design specifies a new forgelm purge subcommand, a RetentionConfig schema for automated policy checks, and an updated audit event catalog to track erasure actions while maintaining chain integrity. Feedback on the design highlights critical security and reliability concerns: the potential for PII leakage by storing operational identifiers in the clear within the audit log, the unreliability of using filesystem modification times (mtime) for retention enforcement, and the risk of unintended data loss when using line numbers as a fallback identifier across multiple files.
|
|
||
| ### 5.3 Identifier hashing in audit events | ||
|
|
||
| Event bodies carry the `target_id` (e.g. `row_id=42` or `run_id=fg-abc123`). These are **operational identifiers**, not PII themselves; they can stay in the clear. |
There was a problem hiding this comment.
Storing target_id in the clear within the audit log poses a compliance risk if the identifier itself contains personal data (e.g., an email address used as a row key). Since the audit log is exempt from erasure under Article 17(3)(b), it should not become a secondary repository for the PII being deleted. The design should specify that the target_id must be hashed (e.g., using SHA-256) when recorded in the audit log to satisfy the data minimization principle (Article 5(1)(c)).
| ### 3.3 What "violation" means | ||
|
|
||
| For each retention horizon: | ||
| - Compare artefact `mtime` against `now - <horizon>_days`. |
There was a problem hiding this comment.
Relying on filesystem mtime for retention enforcement is unreliable, as modification timestamps can be reset by common operations like file moves, backups, or manual "touches" without changing the actual data age. For run-scoped artefacts, the design should instead use the creation timestamp recorded in the audit_log.jsonl for the associated run_id. This provides a tamper-resistant temporal anchor for enforcing the Article 5(1)(e) storage limitation.
| | `--row-id <id>` | corpus mode | str | Row to delete. Format: stable id field already in the JSONL row (e.g. `"id"` or `"row_id"` key); falls back to **line number** if the JSONL has no id field. | | ||
| | `--corpus <path>` | corpus mode | str | Path to the JSONL. May be a file or a directory containing `train.jsonl` / `validation.jsonl` / `test.jsonl` (purge scans all matching files; `--row-id` may match in zero or more of them). | |
There was a problem hiding this comment.
Using line numbers as a fallback for --row-id is inherently fragile, particularly as the --corpus flag allows specifying a directory where the tool "scans all matching files". If an operator provides a line number, the tool might inadvertently delete that line from every file in the directory (e.g., train.jsonl, validation.jsonl, test.jsonl), leading to unintended data loss. For a compliance-focused tool, the design should either mandate a unique identifier or, if line numbers are supported, restrict the operation to a single file and include a content-verification step (e.g., a hash of the expected row) to ensure the correct data is erased.
| ```text | ||
| forgelm purge --row-id <id> --corpus <path> # corpus-row erasure | ||
| forgelm purge --run-id <id> --kind {staging,artefacts} # run-scoped erasure | ||
| forgelm purge --check-policy [--config <path>] # retention dry-run | ||
| ``` | ||
|
|
||
| A single invocation does exactly one of the three. Combining flags is a `ConfigError`. | ||
|
|
||
| ### 4.2 Flag semantics | ||
|
|
||
| | Flag | Required when | Type | Purpose | | ||
| |---|---|---|---| | ||
| | `--row-id <id>` | corpus mode | str | Row to delete. Format: stable id field already in the JSONL row (e.g. `"id"` or `"row_id"` key); falls back to **line number** if the JSONL has no id field. | | ||
| | `--corpus <path>` | corpus mode | str | Path to the JSONL. May be a file or a directory containing `train.jsonl` / `validation.jsonl` / `test.jsonl` (purge scans all matching files; `--row-id` may match in zero or more of them). | | ||
| | `--run-id <id>` | run mode | str | Run id matching `<output_dir>/audit_log.jsonl`'s top-level `run_id`. | | ||
| | `--kind {staging,artefacts}` | run mode | enum | Which derived artefacts to erase. `staging` = `final_model.staging.*`; `artefacts` = `compliance/`, `data_audit_report.json`, derived JSONL snapshots. | | ||
| | `--output-dir <dir>` | run mode | str | Where to look for the run. Default `./` (current dir). | |
There was a problem hiding this comment.
2. Purge kind spec inconsistent 🐞 Bug ≡ Correctness
The design doc says operators can act on logs in-tool, but the forgelm purge --run-id --kind
interface only allows {staging,artefacts}, and the closure plan expects
{staging,artefacts,logs}. This spec mismatch makes Phase 21’s intended CLI surface ambiguous.
Agent Prompt
## Issue description
The design specifies `logs` as operator-actionable, but the `forgelm purge --run-id --kind` enum omits it, while the closure plan includes it. This leaves the Phase 21 CLI contract unclear.
## Issue Context
- §2 defines operator-actionable kinds as including `logs`.
- §4 defines `--kind` as `{staging,artefacts}`.
- The closure plan’s Phase 20 tasks list `{staging,artefacts,logs}`.
## Fix Focus Areas
- docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md[48-62]
- docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md[125-145]
- docs/analysis/code_reviews/closure-plan-202604300906.md[665-668]
## What to change
Choose one consistent contract and update all references:
- Option A: Add `logs` to `--kind` and define precisely what gets regenerated/deleted.
- Option B: Remove `logs` from the operator-actionable list and explicitly state that log-derivative regeneration is handled elsewhere (different flag/subcommand), so `--kind` stays `{staging,artefacts}`.
Also reconcile the closure plan wording accordingly (or update the design to match the plan if the plan is authoritative).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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>
|
Wave 2a Round-1 multi-agent review — fixes pushedAll CRITICAL + HIGH findings from the multi-agent review (and the relevant inline bot comments) have been addressed in the latest commit on this branch. See the commit message for the per-finding mapping (review IDs F-24-*, gemini bot IDs, qodo bot IDs, sourcery bot IDs). Per-PR test counts:
ruff format/check clean across all five PRs. Cross-PR XPR-01 concern (audit-log parser proliferation) closed in PR #25 via the new Items deferred to follow-up PRs (with rationale) are listed under DEFERRED in each commit message. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Kicking off a fresh review of the GDPR erasure design document now. [review] |
|
Superseded by #28 (consolidated Wave 2a integration). All commits from this branch are merged into |
… secret (PR#63-review) #24: export_compliance_artifacts rollback swallowed os.remove/os.replace failures with except OSError: pass; collect and logger.error them before re-raising the original promotion error. #25: strict-HMAC guard rejected only None, letting an empty-string secret through; gate on 'not hmac_secret'. CodeRabbit #24,#25. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>



Summary
Design-only PR. Ships the Phase 20 deliverable from the closure plan: a 460-line analysis document (`docs/analysis/code_reviews/gdpr-erasure-design-202605021414.md`) that pins the scope of Phase 21's `forgelm purge` implementation so the next phase does not redo legal mapping in code review.
What this resolves
ForgeLM today has no operator-actionable tool for GDPR Article 17 erasure requests or Article 5(1)(e) storage-limitation policy. `docs/guides/safety_compliance.md` claims "GDPR-aware" without backing implementation; `docs/usermanuals/en/compliance/gdpr-erasure.md` references a `forgelm purge` subcommand that doesn't exist. Phase 21 closes the gap; this design tells Phase 21 exactly what to delete, what to record, what to preserve, and what to refuse.
Document structure
Test plan
Risk
None. Pure design document. No runtime / API / dependency surface touched.
Closes
Unblocks
🤖 Generated with Claude Code
Summary by Sourcery
Documentation: