Repository navigation
release: v0.5.5 — Waves 1-5 closure + site UX overhaul - #38
Merged
Merged
Conversation
The Wave 2a / Wave 4 closure cycle authored four design specifications
("Phase 18 Library API", "Phase 20 GDPR Right-to-Erasure", "Faz 22
ISO 27001 / SOC 2 Type II Alignment", and the Phase 14/15
data_audit + cli sub-package split) and dropped them in
``docs/analysis/code_reviews/`` next to the closure plan and wave
review artefacts. They are forward-looking design specs, not review
artefacts — they describe what the implementation should look like,
not what it shipped at a particular commit. The right home is
``docs/design/`` (already the canonical location for ``blackwell_optimized.md``
and ``wizard_mode.md``).
This commit moves them with ``git mv`` (history preserved), strips
closure-cycle provenance from the file headers + sign-off sections,
and updates every non-wave / non-closure reference to point at the
new path.
## Renames
* ``library-api-design-202605021414.md`` → ``docs/design/library_api.md``
* ``gdpr-erasure-design-202605021414.md`` → ``docs/design/gdpr_erasure.md``
* ``iso27001-soc2-alignment-202605052315.md`` → ``docs/design/iso27001_soc2_alignment.md``
* ``split-design-data_audit-cli-202604300906.md`` → ``docs/design/data_audit_cli_split.md``
## In-document changes
For each moved file:
* Headed with a one-paragraph **Scope** blockquote stating what the
document covers + which implementation files / config keys / tests
it tracks. Old "Document ID / Status: Draft / Author: Closure Wave
2a / Companion: closure-plan / Implements (next phase) / Base
commit" provenance metadata removed; the doc is now framed as a
living spec, not a closure-cycle artefact.
* "Sign-off checklist (Phase N acceptance)" sections in
``library_api.md`` and ``gdpr_erasure.md`` replaced with
"Implementation status" sections that list the as-shipped surface
in v0.5.5 (Pydantic models, CLI subcommands, audit events, test
files, doc files) — turning a one-shot wave acceptance gate into a
reader-useful pointer set.
* "Wave 1 round-5 carry-over" subsection title in ``library_api.md``
renamed to "CLI facade lazy migration" (closure provenance dropped;
technical content unchanged).
* Trailing closure footers (``*End of Phase 18 design —
library-api-design-202605021414*`` etc.) removed.
* Closing reference list in ``iso27001_soc2_alignment.md`` swapped
from a closure-plan link to direct pointers at the QMS templates,
audit-event catalog, and the two ISO/SOC 2 reference mappings.
* All ``../../`` relative paths repaired to ``../`` to reflect the
new ``docs/design/<name>.md`` depth (was ``docs/analysis/code_reviews/<name>.md``,
one level deeper).
## Reference updates (16 files outside the code_reviews/ tree)
* CHANGELOG.md — 18 path updates (link refs in PR-39 / PR-30 / PR-28
entries) plus 4 inline narrative updates that referenced the old
bare names.
* docs/guides/{library_api,library_api-tr}.md — design-source link.
* docs/guides/{iso_soc2_deployer_guide,iso_soc2_deployer_guide-tr}.md
— cross-reference + bottom-of-file resource list.
* docs/guides/{gdpr_erasure,gdpr_erasure-tr}.md — bottom resources.
* docs/qms/{statement_of_applicability,statement_of_applicability-tr}.md
— header cross-reference.
* docs/reference/compliance_summary.md — ISO 27001 / SOC 2 alignment
bullet.
* docs/reference/{iso27001_control_mapping,iso27001_control_mapping-tr}.md
— header + bottom-of-file design-doc pointer.
* docs/reference/{library_api_reference,library_api_reference-tr}.md
— header + bottom-of-file design-doc pointer.
* docs/reference/{soc2_trust_criteria_mapping,soc2_trust_criteria_mapping-tr}.md
— header + bottom-of-file design-doc pointer.
* docs/usermanuals/{en,tr}/reference/library-api.md — design rationale
link in operator how-to.
* forgelm/__init__.py — package-facade docstring pointer.
* forgelm/_version.py — ``__api_version__`` docstring pointer.
* forgelm/cli/subcommands/_purge.py — Phase 21 implementation header.
* tests/test_library_api.py — module docstring pointer.
* tests/test_gdpr_erasure.py — module docstring pointer.
## Wave / closure artefacts (intentionally untouched)
24 files under ``docs/analysis/code_reviews/`` still reference the
old paths from inside their bodies (closure-plan, foundation-pr-review,
master-review-opus, opusreview-pr29, pr29-master-review-A1/A2/A3/A8,
pr29-master-review-consolidated, pr29-master-review-prompt,
wave1-review-prompt, wave2a-round2-{design-doc,fix-summary,review-prompt,
standards}, wave2b-final-{merge-review-prompt,public-api,standards},
wave4-{code-correctness,followup-review-prompt,privacy-security,
review-prompt,standards}, wave4fu-{privacy-security,standards}). These
are frozen-in-time review artefacts; their broken paths are part of
the historical record and rewriting them would falsify the audit
trail.
## Verification
| Check | Result |
|---------------------------------------------|--------|
| ``check_anchor_resolution --strict`` | ✅ 268/268 (was 264; +4 new design files) |
| ``check_bilingual_parity --strict`` | ✅ 40/40 |
| ``check_cli_help_consistency --strict`` | ✅ 423/423 |
| ``check_site_claims --strict`` | ✅ artefacts/templates/GPU/version |
| ``ruff check`` (touched source) | ✅ all checks passed |
| ``pytest tests/test_library_api.py + test_gdpr_erasure.py`` | ✅ 62/62 |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…compliance)
The hero block previously shipped a single technical pitch ("YAML in.
Fine-tuned model out."). That copy reads to engineers but invisibly to
two adjacent audiences: non-technical users who would benefit from the
wizard, and compliance / regulated-industry buyers for whom the EU AI
Act story is the load-bearing differentiator. This commit replaces the
single-pitch left column with a three-slot rotating slider so each
audience meets ForgeLM in their own language within the first viewport.
The right-column animated terminal stays put — it is part of the brand
identity and reads as cross-cutting truth across all three pitches.
The cross-cutting meta strip (EU AI Act ready · CPU-friendly audit ·
Air-gap capable · CI/CD integrated) likewise stays visible across
slides because those four facts hold regardless of which persona is
reading.
## Mechanics
* **Three slides:**
- Engineer (default, identical copy to the prior single pitch).
- Beginner — "No YAML? No problem." pointing at ``quickstart.html?wizard=open``
(Stage B will land the modal wizard the deep-link opens).
- Compliance — "EU AI Act ready by default." pointing at ``compliance.html``.
* **Auto-rotate 8s**, hover-pause + focus-pause + visibilitychange
pause (no CPU burn on background tabs).
* **Keyboard:** ←/→ inside the slider; the slider is keyboard-focusable
via ``tabindex=-1`` so screen-reader users can land on it
programmatically without it being a tab stop.
* **Touch:** horizontal swipe ≥ 50 px with vertical-dominant guard so
scrolling the page doesn't trip the slider.
* **Pagination dots:** three pills; active dot expands and runs a
linear progress fill (CSS animation pinned to the 8s timer; pauses
in lockstep when the slider pauses).
* **Prev/next chevrons:** ghost by default; revealed on hover or
keyboard focus inside the slider; hidden on screens < 720 px (touch
users get the swipe gesture instead).
* **Reduced-motion fallback:** every slide stacks linearly, controls
disappear, the auto-rotate timer is a no-op. Linear reading order
preserved.
* **ARIA:** slides are ``role=group aria-roledescription=slide``; the
pagination dot row is ``role=tablist`` with each dot ``role=tab
aria-controls aria-selected`` to the slide it pages to. Active slide
removes ``hidden``; inactive slides set it (so screen readers don't
read all three at once between transitions).
## Files
* ``site/index.html`` — left-column ``<div>`` replaced with
``.hero-personas`` container holding 3 ``.hero-slide`` blocks +
pagination + arrows + the unchanged badge / meta strip. Right column
``.hero-terminal`` and grid template untouched.
* ``site/css/style.css`` — new ``Hero rotating slider`` section
inserted between ``Hero meta`` and ``Hero terminal``: ~180 lines
covering slide cross-fade, dot pagination + 8s progress fill,
prev/next chevron visibility, mobile arrow hide, reduced-motion
stack-linear branch.
* ``site/js/main.js`` — ``initHeroSlider()`` (~140 lines) wired into
the ``DOMContentLoaded`` initialiser. Same defensive idioms as
``initHeroTyper`` (matchMedia early return, no-op when slider absent).
* ``site/js/translations.js`` — 12 new keys × 6 locales = 72 new
entries across the per-page home-block ``Object.assign`` calls
(``engineer.title/lead/cta1/cta2``, ``beginner.title/lead/cta1/cta2``,
``compliance.title/lead/cta1/cta2``, ``slider.label``,
``dot.engineer/beginner/compliance``, ``prev``, ``next``).
The legacy ``home.hero.{title,lead,cta1,cta2}`` keys are kept so
external references (if any) don't break; the live HTML now binds to
the persona-namespaced keys exclusively.
## Verification
| Check | Result |
|-------|--------|
| ``check_anchor_resolution --strict`` | ✅ 268/268 |
| ``check_bilingual_parity --strict`` | ✅ 40/40 |
| ``check_cli_help_consistency --strict`` | ✅ 423/423 |
| ``check_site_claims --strict`` | ✅ artefacts/templates/GPU/version |
| ``check_site_chrome_parity`` | ✅ EN ↔ TR lockstep at 353 keys (was 341) |
The DE/FR/ES/ZH 42-key chrome backlog is unchanged from the prior
state — Stage A's 12 new keys land in EN+TR active tiers and in the
four deferred locales identically (no new drift introduced).
This is Stage A of the home / quickstart UX upgrade. Stage B (modal
YAML wizard on quickstart) follows in a separate commit so the hero
slider can be reviewed in isolation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stage B of the home / quickstart UX upgrade (Stage A landed the home hero rotating slider in 35f2b65). The site now ships an interactive modal wizard that mirrors the CLI ``forgelm --wizard`` flow but lives entirely in the browser: walks the operator through 7 steps, generates a working ``quickstart-generated.yaml``, and offers copy / download / run-snippet delivery. The existing "Five steps. No notebook." page structure is preserved untouched — the wizard is a complementary modal-as-app, not a replacement. ## Steps (7 total) 1. **Welcome** — experience-level toggle (Beginner / Expert). Beginner mode shows a tutorial paragraph on every step; Expert mode shows short hints only. The detail level is also re-toggleable per-step from the header so the operator can flip mid-flow. 2. **Use case** — six template cards (Customer support / Code copilot / Domain expert / GRPO math / Turkish medical Q&A / Custom). Selecting a template auto-fills the model + dataset + format on subsequent steps; everything stays overrideable. 3. **Base model** — four preset cards (Llama-3.1-8B-Instruct, Qwen2.5-7B-Instruct, Mistral-7B-Instruct-v0.3, Phi-3-mini-4k-instruct) plus a Custom option that takes a free-form HuggingFace model ID. 4. **Dataset** — radio for HuggingFace / local JSONL / local PDF (with the ingest-first note baked into the YAML preamble), path / name input, format toggle (prompt+response vs messages), and prompt / response field-name inputs. 5. **Training params** — QLoRA toggle, LoRA r slider (4-64), LoRA alpha slider (8-128), epochs (1-5), learning rate dropdown (1e-4 / 5e-5 / 3e-5 / 1e-5), batch size (1 / 2 / 4 / 8). 6. **Evaluation & EU AI Act compliance** — risk classification radio (minimal / limited / high / unacceptable / unknown), auto-revert + max-loss threshold, Llama Guard safety eval, audit log toggle, human-approval gate toggle, Annex IV export toggle. **High-risk and unacceptable selections force-enable the audit log + human approval + Annex IV toggles** and surface a red callout banner explaining the Article 9-17 obligations. 7. **Review** — final live YAML (syntax-highlighted), 3 delivery actions (copy to clipboard, download as ``quickstart-generated.yaml``, reset wizard) plus a ``forgelm --config ... --dry-run`` snippet. ## Triggers * ``[data-wizard-open]`` clicks open the modal: - ``page-header-aside`` link "Open the wizard ↗" (new, sits under the existing "Install, pick a template, validate, train, deploy." lead). - ``cta-card`` button "Launch the wizard" (replaces the old README link as the primary CTA; "CLI version (forgelm --wizard)" + "Browse all features" remain as secondary / tertiary). * URL parameter ``?wizard=open`` auto-opens the modal on page load. Used by the home-page Beginner-slide CTA wired in Stage A (``index.html`` → ``quickstart.html?wizard=open``). Combined with ``&step=N`` (future) it can deep-link to a specific step. * The URL is ``replaceState``-pushed when the modal opens / closes so a refresh round-trips back into the same state. ## State & persistence * In-memory state object mirrored to ``localStorage`` under ``forgelm.wizard.state``. Schema-versioned (``state.v = 1``) so future shape changes can clear stale snapshots. * Every input change saves; every step transition saves. * On reopen, the wizard surfaces a "Resume?" pathway implicitly — the saved step is restored. Reset button on Step 7 clears localStorage with a confirm dialog. * High-risk auto-coercion runs on every render so toggling risk back to a lower tier doesn't accidentally leave audit + approval off; the upward transition force-enables, downward keeps the operator's choices. ## Modal mechanics * ``role="dialog" aria-modal="true" aria-labelledby="wizard-title"``; the outer wrapper carries ``hidden`` when closed so the modal is removed from the accessibility tree entirely. * Focus trap on Tab / Shift+Tab cycles within the dialog. * Escape key closes; backdrop click closes; the X button in the header closes. Return focus to the triggering element on close. * The page background is marked ``aria-hidden`` while the modal is open and ``document.body.style.overflow = "hidden"`` prevents background scroll. * Reduced-motion: dialog enter animation disabled, progress-bar transition disabled, slide animations no-op. ## Layout * **Desktop (≥ 901 px):** 1080 px × 720 px centered dialog, 60/40 split (step pane left, YAML preview right). Both panes scroll independently. * **Tablet (901-720 px):** Same shape, slightly tighter padding. * **Mobile (≤ 900 px):** Fullscreen takeover. YAML preview collapses into an accordion under the step pane (default collapsed; tap to expand). Step name in the footer hides to make room for prev/next. ## YAML serialisation Browser-side string-builder (no YAML library dependency). Output is structurally aligned with ``config_template.yaml`` so it passes ``forgelm --config quickstart-generated.yaml --dry-run`` cleanly. Inline syntax tinting paints comments / keys / strings / booleans / numbers / null in distinct colours via a small line-walk regex. ## Files * ``site/quickstart.html`` — page-header sub-CTA, body-end modal scaffold (~75 lines), ``<script src="js/wizard.js" defer>`` include. Existing 5-step content untouched. * ``site/css/style.css`` — new ``Wizard modal`` section (~520 lines): backdrop + dialog + header progress + 60/40 body split + YAML pane chrome + footer + step-content building blocks (cards / sliders / toggles / callouts / actions / snippet) + resume banner + validation error pin + mobile fullscreen + YAML accordion + reduced-motion fallback. Plus ``page-header-aside`` + ``link-cta`` helper styles for the page-header sub-CTA. * ``site/js/wizard.js`` (NEW, ~880 lines) — state machine, step renderers (one per step), YAML serialiser, syntax tinter, localStorage persistence, focus trap, URL sync, copy/download handlers, mobile YAML accordion, reduced-motion early return. * ``site/js/translations.js`` — 129 new keys × 6 locales = 774 new entries (engineer / beginner / compliance is from Stage A; this commit adds the wizard surface). Per-step structure: title + short name + description + tutorial paragraph + control labels + hints + validation messages + use-case / model preset cards + EU AI Act risk tier callouts. Each locale is fully translated — the wizard tutorial paragraphs are not English fallbacks. ## Verification | Check | Result | |-------|--------| | ``check_anchor_resolution --strict`` | ✅ 268/268 | | ``check_bilingual_parity --strict`` | ✅ 40/40 | | ``check_cli_help_consistency --strict`` | ✅ 423/423 | | ``check_site_claims --strict`` | ✅ artefacts/templates/GPU/version | | ``check_site_chrome_parity`` | ✅ EN ↔ TR lockstep at 482 keys (was 353 after Stage A; +129 wizard keys per active-tier locale) | The DE/FR/ES/ZH 42-key chrome backlog from prior locale work is unchanged — the new 129 wizard keys land identically across all 6 locales (no new drift introduced). Manual browser testing TBD — the dialog passes structural / lint gates but a multi-locale + multi-theme + reduced-motion + mobile walkthrough is recommended before promoting the changes to ``main``. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…il + prominent CTA Stage B follow-up addressing the four issues raised on the wizard. Schema audit (the YAML the wizard emitted was invalid against Pydantic): - ModelConfig: name_or_path (was model_name_or_path); load_in_4bit belongs to model, not "model.use_qlora" sibling. - LoraConfigModel: now emits a top-level lora: block with r/alpha/ dropout/bias/target_modules. Previously folded into model:. - DataConfig: drop format / prompt_field / response_field — extra="forbid" rejects them; row shape is auto-detected. Single tutorial block in the UI explains the auto-detect. - TrainingConfig: trainer_type lives under training:, not as a top-level trainer: key. Drop top-level trainer:. - EvaluationConfig: no enabled flag — the block being present is the enable signal. require_human_approval is the Article 14 gate. - SafetyConfig: classifier (was classifier_path). - ComplianceMetadataConfig: drop audit_log/annex_iv_export toggles — the audit log is always written, the Annex IV bundle is produced from the compliance.* metadata. Added provider_name / system_name / intended_purpose inputs (surfaced for high-risk/unacceptable tiers). Verified: both the high-risk full-block path and the limited-risk minimal path dry-run cleanly through the real ForgeConfig. Modal scroll fix: - .wizard-body grid container and .wizard-step-pane grid item both needed min-height: 0 plus max-height: 100% on the pane. Without that, flex/grid children take their content's intrinsic height and overflow-y: auto never fires. Step panes are now scrollable on every step (welcome / use-case / model / dataset / training / compliance / review) on desktop and mobile. - Reset pane.scrollTop = 0 on prev/next navigation so the operator always sees the new step's title. Per-field beginner detail (Step 5 + Step 6): - New .wizard-row-detail block with steel-bright left border, rendered beneath the hint when state.detailsVisible is true. - Six training rows (qlora / lora_r / lora_alpha / epochs / lr / batch) and four eval rows (auto_revert / max_loss / safety / human_approval) now ship 2-3-sentence plain-language primers explaining what the field MEANS, not just what range to pick. Prominent wizard CTA (quickstart.html): - New .wizard-spotlight banner above the manual five-step grid: eyebrow + title + lead + 2 CTAs + 3 meta items + a 3-stage visual diagram (Answer 7 questions → Get a valid YAML → Run it). - .steps-intro separator routes the eye from the spotlight down into the manual steps. Translations: - EN/TR (active tier): full coverage of the new keys (autodetect, always-emitted info, approval/eval/training detail blocks, provider metadata inputs, spotlight CTA copy) — 41 distinct wizard keys resolve in both locales. - DE/FR/ES/ZH (deferred tier): same structural updates; legacy keys removed in lockstep so site_chrome_parity reports no new drift beyond the existing GDPR/ISO backlog. Verified with the seven-gate gauntlet: - ruff format + ruff check: pass - pytest: 1479 passed, 14 skipped - forgelm --config config_template.yaml --dry-run: valid - check_anchor_resolution.py --strict: 268 docs OK - check_bilingual_parity.py --strict: 40 doc pairs in lockstep - check_cli_help_consistency.py --strict: 423 invocations OK - check_site_chrome_parity.py --strict: EN/TR in lockstep - check_site_claims.py --strict: artefacts + templates + version OK - A wizard-emitted YAML (high-risk full + limited-risk minimal) was fed to the real ForgeConfig validator; both pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Root cause of "use-case / model / dataset steps won't scroll": - .wizard-dialog used max-height, not height. With only max-height, flex: 1 on .wizard-body resolves its flex-basis: 0% against an indefinite parent. Body then sizes to content. Step pane inside grows with body. overflow-y: auto on the pane never fires because there's nothing to overflow against. - The earlier min-height: 0 fix was necessary but not sufficient. min-height: 0 only matters once the parent itself has a definite size to constrain against — which max-height alone doesn't deliver in this flex context. Fix: replace `max-height: min(720px, calc(100vh - 4rem))` with `height: min(720px, calc(100vh - 4rem))`. Now the dialog is always exactly that tall, body has a definite 720px-ish frame, and the step pane's overflow-y: auto fires on every step (use-case, model, dataset, training, compliance — all now scroll). Also bumped .wizard-step-pane bottom padding from 1.4rem to 2rem (symmetric) and added scrollbar-gutter: stable so the last item in a scrolled pane has the same breathing room as the first and the layout doesn't reflow when scroll appears. Removed the now-redundant max-height: 100% from the pane (the grid track auto-sizes correctly once the dialog has a definite frame). Mobile path was already correct — `height: 100vh` on the dialog at @media (max-width: 900px) is already definite, so scroll worked on small viewports already. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…croll The actual bug under the wizard scroll issue, after two earlier attempts that were necessary but not sufficient: A grid container with no ``grid-template-rows`` creates an implicit ``auto`` row, and ``auto`` resolves to ``max-content``. So even though .wizard-body had a definite size from the flex parent (.wizard-dialog now has height: min(720px, ...)) and ``min-height: 0`` to allow shrinking, the grid row INSIDE was still auto-sizing to the step pane's content. The pane filled its track, the track was as tall as the content, and ``overflow-y: auto`` on the pane had nothing to overflow against — it scrolled relative to its own content-sized height, i.e. not at all. Fix: set ``grid-template-rows: minmax(0, 1fr)`` on .wizard-body. ``minmax(0, 1fr)`` pins the row to the body's definite height (min: 0 lets it shrink, 1fr fills available space), the step pane fills 1fr, and scroll fires whenever content exceeds the row height. Mobile rule updated for parity: ``grid-template-rows: minmax(0, 1fr) auto`` (was ``1fr auto``) so the step-pane row has the same min: 0 floor. This is the third commit chasing the same symptom and the comment block in style.css now spells out all three requirements (definite dialog height, min-height: 0 on body, minmax(0, 1fr) on the grid row) so this stops being a recurring bug. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bundles a UI/UX audit pass over the marketing site, focused on the class of bugs that hide content when the user toggles to light mode. Light-theme contrast — dark surfaces using page-text variables: - .terminal-bar, .terminal-body .cmd, .terminal-body .out - .code-block-header, .code-block-header .copy-btn - .wizard-yaml-header, .wizard-yaml-copy (default + hover) - .wizard-yaml-output .yaml-null - .hero-slider-prev / .hero-slider-next These selectors all paint onto a *theme-locked dark surface* (the code chrome stays dark in both light and dark page themes), so they need theme-locked tokens too — --code-comment / --code-text instead of --text-muted / --text-secondary / --text which flip to dim greys or near-black in light mode and disappear against the dark gradient. The visible symptom on the home page was the literal forgelm command becoming invisible in the hero terminal under light mode. Light-theme contrast — accent tokens below WCAG 1.4.3 AA on light: - inline code in body copy, .use-case-tag, .article-num, .guide-content td:first-child code Swap --ember-bright / --steel-bright (which pass on dark but drop below 4.5:1 on light card surfaces) for the corresponding -deep tokens via a [data-theme="light"] rule. Same semantic colour, AA compliance. A11y wiring (HTML + JS): - nav-hamburger gets aria-controls="primary-nav" pointing at the matching id on .nav-menu so screen readers announce the controlled region. - theme-toggle gets aria-pressed="false" initial state; main.js flips it to true whenever light mode is engaged so the button's state is announced correctly. - Three guide.search.label / guide.sidebar.label / guide.progress.label i18n keys added (EN + TR + DE/FR/ES/ZH) so the search input, sidebar nav and reading-progress bar carry proper aria-labels. Print: @media print hides the wizard-modal so a left-open wizard doesn't print over the page. All eight pages (index, quickstart, features, guide, compliance, contact, privacy, terms) carry the same nav set; chrome parity gate remains in EN-TR lockstep. Bilingual + anchor + CLI-help gates pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eld validation Wizard scope expansion from ~30% to full ForgeLM feature surface, plus the UX polish needed to keep 9 steps + 7 accordions navigable. WIZARD STRUCTURE — 8 → 9 steps: - New "Operations & advanced settings" step (#8) lifts notifications, monitoring, synthetic data, and retention out of the compliance step. Compliance was getting too tall to scan in one viewport; Operations now hosts the four accordions on its own. - Spotlight + step-counter strings updated 8 → 9 across all 6 locales. NEW BUILDYAML SURFACE (Bundles 4-7): - evaluation.benchmark — lm-evaluation-harness gate (tasks list + min_score). Conditionally emitted when benchmarkEnabled. - evaluation.llm_judge — LLM-as-Judge scoring (judge_model + judge_api_key_env + min_score). Conditionally emitted. - data.governance — Article 10 inputs (collection_method, annotation_process, known_biases, personal_data_included, dpia_completed) inside a dataset-step accordion. - webhook — Slack/Teams/Discord notifications. Now gated by an explicit ``webhookEnabled`` toggle: off → no webhook block in YAML, on → URL is required. URL accepts ``env:VAR_NAME`` for secrets; buildYaml branches on the prefix to emit ``url`` vs ``url_env``. - monitoring — Article 12+17 post-market hooks (endpoint, metrics_export, alert_on_drift). Auto-coerced for high-risk tiers. - synthetic — teacher → student data generation (teacher_model, seed_file, api_key_env). - retention — GDPR Art. 5+17 horizons (audit_log, staging, ephemeral, raw_documents) + enforce mode literal (log_only/warn_on_excess/block_on_excess). PER-FIELD VALIDATION (19 conditional-required inputs): - Always required: model.name_or_path (Custom preset only) + format check (org/repo); data.dataset_name_or_path. - Feature-enabled: webhook.url, monitoring.endpoint, synthetic teacher_model + seed_file + api_key_env, benchmark.tasks, judge.api_key_env (each required only when its parent toggle is on). - High-risk + unacceptable: compliance.{provider_name, system_name, intended_purpose} (Annex IV §1), risk_assessment.{intended_use, foreseeable_misuse, mitigation_measures} (Article 9), governance.{ collection_method, annotation_process, known_biases} (Article 10). Validation messages explain WHY (which choice triggered the rule) AND WHAT to do (fill the field or undo the choice). Example: "EU AI Act Annex IV §1 requires the provider's legal entity name for high-risk systems. Fill it in or lower the risk classification." All 19 messages translated across EN / TR / DE / FR / ES / ZH. DEFAULT-CLEAN UX: - attemptedSteps wiped on every loadState() so a fresh open never shows pre-painted errors from a previous session. - On Next-on-invalid: every accordion containing an invalid field auto-opens, the renderer paints inline errors + .is-invalid borders, and keyboard focus jumps to the first invalid input (preventScroll + smooth scrollIntoView so the input lands centred without a jarring jump). ACCORDION UX REWRITE: - SVG line icons replace the previous unicode arrow chevron. Per-accordion identity icons (governance=document, risk=shield, eval=check-circle, webhook=bell, monitoring=heartbeat, synthetic=sparkles, retention=clock). - Header restructured as a 3-column grid (icon | titles | chevron) so a long hint wraps cleanly without pushing the chevron off-axis. - Open state gets an ember-tinted border + soft shadow + ember icon fill. Chevron rotates 180°. - Focus-visible ring on the header button for keyboard users. - Closed = ``display: none`` on the body wrapper. (Two earlier attempts — grid-template-rows: 0fr → 1fr and max-height: 0 — both failed in real browsers; the body kept its min-content size and every closed accordion rendered as a tall empty frame.) - Override the global ``section { padding: 5rem 0 }`` rule on .wizard-accordion so the section element doesn't grow 80px of vertical padding inside the wizard pane (the actual cause of the "huge gray frame around closed accordions" bug). TR TRANSLATION QUALITY PASS (20 fixes): - Academic words simplified: paradigma → yöntem, muhakeme → akıl yürütme, kanonik → standart. - Mixed-language friction smoothed: VEYA → ya da, feedback → geri bildirim, "Uzunluk-normalize" → "Uzunluk-dengeli". - Grammar cleanups: policy'i → policy'yi (i-uyumu), log-probabilities'leri → log-probability'leri (avoid double plural). - Sentence structure tightened where the EN → TR translation strained. I18N COVERAGE: - 728 EN keys, 728 TR keys — active-tier in lockstep. - DE / FR / ES / ZH carry every wizard key (213 distinct refs including dynamic concat enumerations) so site_chrome_parity reports no new drift beyond the existing GDPR/ISO backlog. - 19 validation error messages in all 6 locales. VERIFIED: - ruff check + JS parse (translations.js + wizard.js) - Doc gates: anchor_resolution, bilingual_parity, cli_help_consistency, site_chrome_parity (EN↔TR lockstep), site_claims — all green. - Schema fuzz: a fully-loaded wizard YAML (every Bundle 4-7 surface populated, high-risk + Annex IV + risk_assessment + governance + webhook + monitoring + synthetic + retention) was fed to the real Pydantic ForgeConfig validator — passes cleanly. - 6 trainer types × multiple risk tiers fuzzed earlier — 20/20 valid. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…GH + 9 MEDIUM + 2 LOW) Synthesis of the six-agent site audit (docs/analysis/code_reviews/ 2026-05-07_site_full_review.md). Each finding was verified against the code first; 7 reported issues were false positives and skipped (documented in the commit body), 1 major refactor (translations per-locale split) was deliberately deferred. CRITICAL — 3 confirmed: - Wizard dialog aria-labelledby="wizard-title" pointed at a non-existent id (orphan reference; screen readers announced "dialog" with no name). Fixed by adding id="wizard-title" to the rendered step title in all three render sites in wizard.js. - 6 of 8 pages had no <main> wrapper around primary content. Wrapped every page (except guide.html which already had it) so assistive tech, search engines, and skip-links can locate the main region. - All 8 pages declared <link hreflang="tr"> pointing at the same URL as the EN canonical. The site uses runtime JS i18n on a single English-baseline URL, so the alternate misled crawlers. Removed the false hreflang and added an HTML comment explaining the choice. HIGH — 8 fixed, 1 deferred: - index.html had three <h1> tags (one per hero slide). Demoted slides 2 + 3 to <h2>; CSS .hero h1 generalized to .hero h1, .hero h2 so the visual treatment is identical. - All early <script src="js/translations.js"> and js/i18n.js references in <head> are now defer-loaded (8 pages). HTML parsing no longer blocks on script execution. - Added a skip-to-main-content link on every page (visually hidden, reveals on first Tab) with a CSS .skip-link rule honoring focus. i18n key common.skip_to_main added to all 6 locales. - JSON-LD blocks (SoftwareApplication + Organization) added to the <head> of all 8 pages. Google rich-snippet eligibility unlocked. - Helper consolidation: tableForLang / tr / escapeHtml lived in three to four files each. New js/_shared.js exposes them via window.ForgeLMShared; consumers (main.js, wizard.js, i18n.js, guide.js) keep small fallback aliases for paranoia. - 15× transition: all (var(--transition) plus two 0.15s/0.12s deviations) replaced with an explicit background-color, border-color, color, transform, box-shadow, opacity list. Eliminates layout-thrash transitions on hover. - Canonical breakpoint tokens documented (--bp-sm/md/lg/xl) on :root. Existing media queries are NOT migrated en masse — every breakpoint was chosen for a specific component layout, and a bulk-rename would risk visual regressions across the whole site. - Deferred: per-locale translations split (~2 days of build-script + async-loader work; tracked for v0.6.x). MEDIUM — 11 fixed: - Footer drift fixed: the rich 4-column footer-grid (Brand / Product / Community / Legal) now appears on all 8 pages. Inline style="width:28px;..." extracted to .footer-brand-mark + .footer-brand-icon CSS classes. - 15 inline style="" attributes extracted into 11 utility CSS classes (section-alt, text-center-mt, container-flush, stack-narrow, cta-card-compact, quickstart-wizard-h2/p, compliance-cookbook, contact-section/form-intro, legal-content-mt). The 11 remaining inline styles are all data-typer-step opacity:0 (animation seed, intentional) plus the wizard-progress-fill width:0% (JS-set). - meta.desc.home trimmed below 160 chars in all 6 locales (EN 179 → 145; TR/DE/FR/ES/ZH all rewritten to fit). - og:locale:alternate added for de_DE / fr_FR / es_ES / zh_CN on all 8 pages (previously only tr_TR). - 140 decorative SVGs marked aria-hidden="true" (every <svg> across all 8 HTML files; 18 already had it). Screen readers no longer announce the redundant "graphic" labels. - Hardcoded macOS dot colors (#ff5f56 / #ffbd2e / #27c93f) tokenized to --dot-red / --dot-yellow / --dot-green. - localStorage save failures (private mode + quota-exceeded) now surface a role=status / aria-live=polite banner inside the wizard modal instead of failing silently. New i18n key wizard.storage.unavailable in all 6 locales. - Wizard step counter element gained aria-current="step" + aria-live="polite" so screen readers announce step changes. - Print stylesheet's 14× !important documented as deliberate (industry standard for print resets; alternative would be massive selector duplication or a body[data-printing] hook). - @import url(fonts.googleapis.com/css2) removed from style.css (it was render-blocking after style.css parse). Replaced by an HTML <link rel="stylesheet"> in each page's <head>, fetched in parallel with style.css. - Two filter: blur(40px) decorative pseudo-elements reduced to 24px (~30% paint cost saving, ~80% visual effect retained). LOW — 2 fixed: - 59 <article class="feature-card"> (15 in index.html, 44 in features.html) converted to <div>. Hero slider's three <article class="hero-slide"> kept (genuinely standalone, semantic). - main.js smooth-scroll anchor handler swapped pushState for replaceState so multiple anchor clicks don't bloat the history stack with intermediate scroll positions. False positives (NOT fixed; rationale documented in the audit report): - Light-mode code-block contrast — code surfaces are theme-locked dark by design (--code-bg + --code-text both pinned in [data-theme= "light"]); previous commit eea260a already fixed the real page-text-on-code-bg contrast issues. - Wizard focus management — Esc-close, Tab/Shift+Tab focus trap, and trigger-focus restore are all already implemented in wizard.js:946-973. - wizard.js loaded everywhere — only loaded on quickstart.html. - Hero slider + wizard CPU contention — slider lives on index.html, wizard on quickstart.html; never co-exist. - prefers-reduced-motion JS ignored — main.js:271,395 already check matchMedia and bail out before starting animations. - theme-toggle aria-label — already declared aria-label="Toggle theme" on the button. Verified: - ruff: clean - 5 doc gates green: anchor_resolution (268 docs), bilingual_parity (40 pairs), cli_help_consistency (423 invocations), site_chrome_parity (EN-TR 730 keys lockstep — was 728), site_claims (artefacts + templates + GPU profile + PyPI version). - All six site JS files (_shared, main, i18n, translations, wizard, guide) parse as valid JS via Node Function constructor. 15 files changed, ~1290 insertions, ~460 deletions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
Contributor
📝 WalkthroughWalkthroughThis PR consolidates v0.5.5 release artifacts by migrating design documents from analysis/code-reviews to canonical docs/design paths, completing the GDPR erasure feature with a purge CLI subcommand implementation, upgrading the torch dependency to 2.3.0, and redesigning the public website with improved i18n script loading, footer restructuring, hero carousel, and expanded CSS utilities. ChangesDocumentation & Design Consolidation
GDPR Erasure Purge Implementation
Website Redesign & Frontend Modernization
Sequence DiagramsequenceDiagram
participant Client as Browser Client
participant Shared as ForgeLMShared
participant I18n as i18n Module
participant Guide as Guide Module
participant Main as Main Module
Client->>Shared: load js/_shared.js (deferred)
Shared->>Shared: init helpers (tableForLang, tr, escapeHtml)
Client->>I18n: load translations & i18n.js (deferred)
I18n->>Shared: call tableForLang(all, lang)
Shared->>I18n: return locale table or undefined
I18n->>Client: apply translations to DOM
Client->>Guide: load guide.js (deferred if present)
Guide->>Shared: use escapeHtml()
Client->>Main: load main.js (deferred)
Main->>Shared: use tr(key)
Main->>Main: initHeroSlider()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 48 high |
| Security | 52 high |
🟢 Metrics 520 complexity · 38 duplication
Metric Results Complexity 520 Duplication 38
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reorganizes project documentation by moving design files to a dedicated directory and updating all internal references across guides, manuals, and source code. It also significantly upgrades the project website, adding structured data for SEO, improving accessibility through skip links and WCAG contrast fixes, and introducing a persona-based hero slider and an interactive configuration wizard. Review feedback identifies a recurring error in CSS transition shorthand syntax that prevents smooth animations, as well as logic issues in the hero slider where manual attribute toggling interferes with CSS transitions and the rotation timer resumes while keyboard focus is still present.
| font-size: 0.92rem; | ||
| font-weight: 500; | ||
| transition: all var(--transition); | ||
| transition: background-color, border-color, color, transform, box-shadow, opacity var(--transition); |
There was a problem hiding this comment.
The transition shorthand syntax used here is incorrect for applying a shared duration and timing function to multiple properties. In CSS, when properties are comma-separated in the shorthand, each one is treated as a separate transition definition. Properties without an explicit duration (like background-color, border-color, color, transform, and box-shadow in this list) will default to 0s, causing them to change instantly. Only opacity will transition as intended. This pattern is repeated throughout the file (e.g., lines 446, 486, 507, 568, 1053, 1098, 1290, 1311, 2612, 2662, 2983, 3310, 3389, 3465).
Suggested change
| transition: background-color, border-color, color, transform, box-shadow, opacity var(--transition); | |
| transition: background-color var(--transition), border-color var(--transition), color var(--transition), transform var(--transition), box-shadow var(--transition), opacity var(--transition); |
Comment on lines
+261
to
+269
| slides.forEach(function (s, i) { | ||
| var active = i === index; | ||
| s.classList.toggle('is-active', active); | ||
| if (active) { | ||
| s.removeAttribute('hidden'); | ||
| } else { | ||
| s.setAttribute('hidden', ''); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Manipulating the hidden attribute here conflicts with the CSS transitions defined for the hero slides. The hidden attribute applies display: none, which causes the outgoing slide to disappear instantly, bypassing the fade-out transition. Since the CSS already manages visibility and opacity via the .is-active class (including a delay on visibility to allow for the fade), the hidden attribute manipulation should be removed from the active slide logic to enable the intended cross-fade effect.
slides.forEach(function (s, i) {
s.classList.toggle('is-active', i === index);
});
Comment on lines
+323
to
+328
| slider.addEventListener('mouseleave', start); | ||
| slider.addEventListener('focusin', stop); | ||
| slider.addEventListener('focusout', function (e) { | ||
| // Only resume if focus actually left the slider entirely. | ||
| if (!slider.contains(e.relatedTarget)) start(); | ||
| }); |
There was a problem hiding this comment.
The slider timer resumes on mouseleave even if the component still has keyboard focus (e.g., if a user is navigating links within a slide). This can lead to unexpected slide changes while a user is interacting with the content. The timer should only resume if neither hover nor focus is present within the component.
Suggested change
| slider.addEventListener('mouseleave', start); | |
| slider.addEventListener('focusin', stop); | |
| slider.addEventListener('focusout', function (e) { | |
| // Only resume if focus actually left the slider entirely. | |
| if (!slider.contains(e.relatedTarget)) start(); | |
| }); | |
| slider.addEventListener('mouseleave', function () { | |
| if (!slider.matches(':focus-within')) start(); | |
| }); | |
| slider.addEventListener('focusin', stop); | |
| slider.addEventListener('focusout', function (e) { | |
| if (!slider.contains(e.relatedTarget)) start(); | |
| }); |
| touchStartY = null; | ||
| }); | ||
|
|
||
| show(0); |
There was a problem hiding this comment.
When removing the hidden attribute manipulation from the show function to fix transition issues, you must ensure that all slides are made visible (by removing the hidden attribute present in the initial HTML) during initialization, so they can be controlled solely by the .is-active class and CSS visibility rules.
Suggested change
| show(0); | |
| slides.forEach(function (s) { s.removeAttribute('hidden'); }); | |
| show(0); |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/reference/soc2_trust_criteria_mapping.md (1)
100-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse canonical config key instead of deprecated alias.
The config key
evaluation.approval_retention_daysis deprecated and forwards to the canonicalretention.staging_ttl_days. Prefer the canonical key in documentation.📝 Proposed fix
-| P4.1 Use, retention, and disposal | `evaluation.approval_retention_days`; `forgelm purge --check-policy` | +| P4.1 Use, retention, and disposal | `retention.staging_ttl_days`; `forgelm purge --check-policy` |Based on learnings: "Document that the deprecated config key evaluation.approval_retention_days forwards to the canonical retention.staging_ttl_days in forgelm/config.py:1023; prefer the canonical key in docs and config references."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reference/soc2_trust_criteria_mapping.md` at line 100, Replace the deprecated config key in the table entry: change `evaluation.approval_retention_days` to the canonical `retention.staging_ttl_days` (the deprecated alias forwards to this canonical key), so update the P4.1 row to reference `retention.staging_ttl_days` instead of `evaluation.approval_retention_days`.docs/design/iso27001_soc2_alignment.md (1)
360-360:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse canonical config key instead of deprecated alias.
The config key
evaluation.approval_retention_daysis deprecated and forwards to the canonicalretention.staging_ttl_days. Prefer the canonical key in documentation.📝 Proposed fix
-| P4.1 Use, retention, and disposal | `evaluation.approval_retention_days` + `forgelm purge --check-policy` retention audit | +| P4.1 Use, retention, and disposal | `retention.staging_ttl_days` + `forgelm purge --check-policy` retention audit |Based on learnings: "Document that the deprecated config key evaluation.approval_retention_days forwards to the canonical retention.staging_ttl_days in forgelm/config.py:1023; prefer the canonical key in docs and config references."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/iso27001_soc2_alignment.md` at line 360, Replace the deprecated config key evaluation.approval_retention_days with the canonical key retention.staging_ttl_days in the documentation entry for P4.1; update the table cell to reference retention.staging_ttl_days and, if helpful, add a brief parenthetical note that evaluation.approval_retention_days currently forwards to retention.staging_ttl_days to aid readers and ensure all doc references use the canonical key.forgelm/cli/subcommands/_purge.py (2)
1033-1055:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScan compliance bundles individually in the retention report.
This treats
output_dir/complianceas one artefact, but purge/delete logic already models compliance bundles as per-file, run-scoped outputs. A single fresh file can keep the directory mtime new and hide older expired bundles, while an old genesis timestamp can make a fresh bundle look stale.Suggested direction
horizons: List[Tuple[str, str, int, Optional[str]]] = [ ("audit_log", audit_log_path, retention.audit_log_retention_days, None), ("staging_dir", os.path.join(output_dir, "final_model.staging"), retention.staging_ttl_days, None), - ( - "compliance_bundle", - os.path.join(output_dir, "compliance"), - retention.ephemeral_artefact_retention_days, - None, - ), ( "data_audit_report", os.path.join(output_dir, "data_audit_report.json"), retention.ephemeral_artefact_retention_days, None, ), ] @@ horizons.extend(_discover_per_run_staging_horizons(output_dir, retention.staging_ttl_days)) + horizons.extend( + _discover_compliance_bundle_horizons( + output_dir, + retention.ephemeral_artefact_retention_days, + ) + ) horizons.extend(_discover_raw_documents_horizons(output_dir, retention.raw_documents_retention_days))The helper should emit one tuple per compliance file and, where the filename embeds a run id, carry that
run_idso_resolve_artefact_age()can use the per-run audit timestamp.
962-974:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch
yaml.YAMLErrorin the strict--check-policyconfig load path.The helper
_maybe_load_configexplicitly documents and catches_yaml.YAMLError(for YAML parse failures), and re-raises it whenstrict=True. However, the caller at lines 963–974 only catches(ConfigError, OSError), allowing malformed YAML to propagate uncaught and bypass the documented error envelope via_output_error_and_exit.Suggested fix
def _run_purge_check_policy(args, output_format: str) -> None: """Handle ``forgelm purge --check-policy [--config <path>]``. @@ - from forgelm.config import ConfigError + import yaml as _yaml + from forgelm.config import ConfigError @@ - except (ConfigError, OSError) as exc: + except (ConfigError, OSError, _yaml.YAMLError) as exc: # OSError covers FileNotFoundError + permission denied; ConfigError # wraps every Pydantic ValidationError raised by ``load_config``. # Any other class would be a contract violation in the loader and🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forgelm/cli/subcommands/_purge.py` around lines 962 - 974, The strict config load path currently catches only ConfigError and OSError causing yaml.YAMLError to escape; update the except clause in the try/except around the call to _maybe_load_config to also catch yaml.YAMLError (e.g. except (ConfigError, OSError, yaml.YAMLError) as exc) and pass the caught exception into _output_error_and_exit exactly as is so malformed YAML triggers the same formatted error handling path; keep references to _maybe_load_config and _output_error_and_exit so reviewers can locate the change.
🧹 Nitpick comments (3)
site/guide.html (1)
84-84: ⚡ Quick winSkip-link target
#guide-contentneedstabindex="-1"for reliable keyboard focusThe skip link activates correctly visually (scrolls to
<main id="guide-content">), but withouttabindex="-1"on the target, Chromium and Safari do not reliably move keyboard focus to the element — users pressing Tab after activation land back in the nav rather than the guide content.🔧 Proposed fix
- <main class="guide-content" id="guide-content"></main> + <main class="guide-content" id="guide-content" tabindex="-1"></main>The same fix applies to
<main id="main-content">inprivacy.html,quickstart.html, and any other page that carries a skip link.Also applies to: 156-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@site/guide.html` at line 84, The skip-link target referenced by the anchor href="#guide-content" needs to be programmatically focusable: add tabindex="-1" to the <main id="guide-content"> element so keyboard focus reliably moves there after activating the skip link; apply the same fix to <main id="main-content"> used by privacy.html, quickstart.html and any other pages that include a skip link targeting those IDs.site/index.html (2)
147-148: 💤 Low valueHeading rank when a non-default slide is active.
Slide 1 uses
<h1>and slides 2-3 use<h2>. After the user rotates past slide 1 (or whendata-slide-indexis initialized from the URL?wizard=open-style deep-link to a non-default slide), the live document has no<h1>and starts at<h2>, which hurts document outline for SR users and SEO snapshot tooling.Two simple options:
- Keep all three slide titles as
<h1>(only one is visible at a time, since the others arehidden, so there is still effectively a single H1 in the accessibility tree).- Or move the
<h1>to a stable element above the slider (e.g., thehero-badge/eyebrow region) and keep slide titles as<h2>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@site/index.html` around lines 147 - 148, The slider's visible headings can result in no H1 when a non-default slide is active; update the markup so the page always contains a stable H1 outside the slider and make slide titles H2s: add a single <h1> in the hero badge/eyebrow region (e.g., id="hero-main-title") and change each slide title (e.g., the element with id="hero-slide-engineer" and other .hero-slide elements' <h1>) to <h2> so only the stable H1 remains in the document outline while slide titles stay as section headings.
139-211: ⚡ Quick winARIA: tab/tablist roles on the dots don't pair with
role="tabpanel"on the slides.The dot controls are
role="tab"inside arole="tablist"and reference slides viaaria-controls, but each slide isrole="group"witharia-roledescription="slide". Per the ARIA Authoring Practices, a tab should control a tabpanel; mixing these patterns gives screen readers inconsistent cues.Pick one approach:
- Pure carousel — drop
role="tablist"/role="tab"/aria-selectedfrom the dot strip and keep slides asrole="group". Dots become standard buttons.- Tabbed carousel — switch slides to
role="tabpanel"and keep the tab roles on dots.♻️ Tabbed-carousel variant (minimal diff)
- <article class="hero-slide is-active" data-slide-index="0" id="hero-slide-engineer" role="group" aria-roledescription="slide" aria-labelledby="hero-slide-engineer-title"> + <article class="hero-slide is-active" data-slide-index="0" id="hero-slide-engineer" role="tabpanel" aria-roledescription="slide" aria-labelledby="hero-slide-engineer-title"> ... - <article class="hero-slide" data-slide-index="1" id="hero-slide-beginner" role="group" aria-roledescription="slide" aria-labelledby="hero-slide-beginner-title" hidden> + <article class="hero-slide" data-slide-index="1" id="hero-slide-beginner" role="tabpanel" aria-roledescription="slide" aria-labelledby="hero-slide-beginner-title" hidden> ... - <article class="hero-slide" data-slide-index="2" id="hero-slide-compliance" role="group" aria-roledescription="slide" aria-labelledby="hero-slide-compliance-title" hidden> + <article class="hero-slide" data-slide-index="2" id="hero-slide-compliance" role="tabpanel" aria-roledescription="slide" aria-labelledby="hero-slide-compliance-title" hidden>Your
js/main.jsalready implements arrow-key roving tabindex correctly on the dot strip (tabindex="0"on active,tabindex="-1"on inactive), so the JavaScript side is ready for either pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@site/index.html` around lines 139 - 211, The dots are using role="tab" inside a role="tablist" while each slide article uses role="group", causing ARIA mismatch; switch to the tabbed-carousel pattern by changing each slide element (ids like hero-slide-engineer, hero-slide-beginner, hero-slide-compliance) from role="group" to role="tabpanel" (you can keep aria-roledescription="slide" and aria-labelledby as-is), ensure the dot buttons (class hero-slider-dot) keep role="tab" and manage aria-selected and tabindex in JS, and update show/hide logic to toggle hidden/aria-hidden on the corresponding role="tabpanel" panels so screen readers reflect visibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/library_api.md`:
- Around line 529-532: The documentation overstates test coverage by claiming
tests/test_library_api.py covers “train-and-chat journeys”; update the bullet in
docs/design/library_api.md (the Integration tests (§6) line mentioning
tests/test_library_api.py and tests/test_lazy_imports.py) to accurately reflect
that tests/test_library_api.py performs public-surface, lazy-import, and
library-entry checks only (no train/chat journey), and adjust phrasing to match
documentation.md and localization.md standards (use neutral wording and, if
needed, add a note that train-and-chat journeys are not included in this test
file).
In `@docs/reference/library_api_reference.md`:
- Line 9: The docs mention the __api_version__ format as a two-segment version;
update all occurrences in this document that describe __api_version__ (including
the section tied to the linked design contract and other scattered mentions) to
require a three-segment semver (MAJOR.MINOR.PATCH) and adjust examples and
sample values accordingly so they match the runtime/tests contract; ensure
wording follows documentation.md/localization.md style and that any related
examples, validation text, or links referencing the old two-segment format are
changed to the three-segment format.
In `@site/css/style.css`:
- Line 414: The transition shorthand currently lists multiple properties
followed by a single duration token so only the last property (opacity) actually
animates; update each occurrence of the rule (the selector that contains
"transition: background-color, border-color, color, transform, box-shadow,
opacity var(--transition);") to attach the duration/easing to each property
(e.g. background-color var(--transition), border-color var(--transition), color
var(--transition), transform var(--transition), box-shadow var(--transition),
opacity var(--transition)); for the two locations that use literal durations
instead of the token (the rules that currently use explicit "0.15s ease" /
"0.12s ease"), repeat that literal after each property instead of placing it
only after the final property so every property animates.
- Around line 2332-2335: The rule currently sets font-size: 0.78rem and then
immediately resets it with the shorthand font: inherit; to fix, move the
shorthand declaration before the explicit size so the explicit font-size on the
same rule takes precedence (i.e., place font: inherit before font-size:
0.78rem), or alternatively replace font: inherit with the specific
sub-properties you need (e.g., font-family: inherit) to avoid clobbering
font-size; update the rule containing the font-size/cursor/font declarations
accordingly.
In `@site/js/main.js`:
- Around line 25-28: The local alias var tr = (window.ForgeLMShared &&
window.ForgeLMShared.tr) || function(k){return k;} captures the identity
fallback too early (loaded before _shared.js), so change it to a lazy resolver:
replace the immediate-binding alias in main.js with a function that, on each
call, looks up window.ForgeLMShared?.tr and invokes it if present otherwise
returns the key; update any direct usages to call this lazy tr function (the
same name can be kept) so copy button and contact form callbacks get the real
translations once _shared.js loads.
In `@site/quickstart.html`:
- Around line 394-396: The nested <main> element (outer main with id
"main-content" and inner element with class "wizard-step-pane") is invalid and
creates duplicate landmarks; change the inner element used by the wizard from a
<main class="wizard-step-pane" data-wizard-pane aria-live="polite"> to a
non-landmark container such as a <div> (keeping class "wizard-step-pane",
data-wizard-pane and aria-live="polite") so the wizard content remains
accessible without nesting main landmarks and update any references in wizard.js
that query for ".wizard-step-pane" if needed.
---
Outside diff comments:
In `@docs/design/iso27001_soc2_alignment.md`:
- Line 360: Replace the deprecated config key evaluation.approval_retention_days
with the canonical key retention.staging_ttl_days in the documentation entry for
P4.1; update the table cell to reference retention.staging_ttl_days and, if
helpful, add a brief parenthetical note that evaluation.approval_retention_days
currently forwards to retention.staging_ttl_days to aid readers and ensure all
doc references use the canonical key.
In `@docs/reference/soc2_trust_criteria_mapping.md`:
- Line 100: Replace the deprecated config key in the table entry: change
`evaluation.approval_retention_days` to the canonical
`retention.staging_ttl_days` (the deprecated alias forwards to this canonical
key), so update the P4.1 row to reference `retention.staging_ttl_days` instead
of `evaluation.approval_retention_days`.
In `@forgelm/cli/subcommands/_purge.py`:
- Around line 962-974: The strict config load path currently catches only
ConfigError and OSError causing yaml.YAMLError to escape; update the except
clause in the try/except around the call to _maybe_load_config to also catch
yaml.YAMLError (e.g. except (ConfigError, OSError, yaml.YAMLError) as exc) and
pass the caught exception into _output_error_and_exit exactly as is so malformed
YAML triggers the same formatted error handling path; keep references to
_maybe_load_config and _output_error_and_exit so reviewers can locate the
change.
---
Nitpick comments:
In `@site/guide.html`:
- Line 84: The skip-link target referenced by the anchor href="#guide-content"
needs to be programmatically focusable: add tabindex="-1" to the <main
id="guide-content"> element so keyboard focus reliably moves there after
activating the skip link; apply the same fix to <main id="main-content"> used by
privacy.html, quickstart.html and any other pages that include a skip link
targeting those IDs.
In `@site/index.html`:
- Around line 147-148: The slider's visible headings can result in no H1 when a
non-default slide is active; update the markup so the page always contains a
stable H1 outside the slider and make slide titles H2s: add a single <h1> in the
hero badge/eyebrow region (e.g., id="hero-main-title") and change each slide
title (e.g., the element with id="hero-slide-engineer" and other .hero-slide
elements' <h1>) to <h2> so only the stable H1 remains in the document outline
while slide titles stay as section headings.
- Around line 139-211: The dots are using role="tab" inside a role="tablist"
while each slide article uses role="group", causing ARIA mismatch; switch to the
tabbed-carousel pattern by changing each slide element (ids like
hero-slide-engineer, hero-slide-beginner, hero-slide-compliance) from
role="group" to role="tabpanel" (you can keep aria-roledescription="slide" and
aria-labelledby as-is), ensure the dot buttons (class hero-slider-dot) keep
role="tab" and manage aria-selected and tabindex in JS, and update show/hide
logic to toggle hidden/aria-hidden on the corresponding role="tabpanel" panels
so screen readers reflect visibility.
🪄 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: e0e29263-7042-4352-9e34-0c5af2ed472a
📒 Files selected for processing (42)
CHANGELOG.mddocs/design/data_audit_cli_split.mddocs/design/gdpr_erasure.mddocs/design/iso27001_soc2_alignment.mddocs/design/library_api.mddocs/guides/gdpr_erasure-tr.mddocs/guides/gdpr_erasure.mddocs/guides/iso_soc2_deployer_guide-tr.mddocs/guides/iso_soc2_deployer_guide.mddocs/guides/library_api-tr.mddocs/guides/library_api.mddocs/qms/statement_of_applicability-tr.mddocs/qms/statement_of_applicability.mddocs/reference/compliance_summary.mddocs/reference/iso27001_control_mapping-tr.mddocs/reference/iso27001_control_mapping.mddocs/reference/library_api_reference-tr.mddocs/reference/library_api_reference.mddocs/reference/soc2_trust_criteria_mapping-tr.mddocs/reference/soc2_trust_criteria_mapping.mddocs/usermanuals/en/reference/library-api.mddocs/usermanuals/tr/reference/library-api.mdforgelm/__init__.pyforgelm/_version.pyforgelm/cli/subcommands/_purge.pysite/compliance.htmlsite/contact.htmlsite/css/style.csssite/features.htmlsite/guide.htmlsite/index.htmlsite/js/_shared.jssite/js/guide.jssite/js/i18n.jssite/js/main.jssite/js/translations.jssite/js/wizard.jssite/privacy.htmlsite/quickstart.htmlsite/terms.htmltests/test_gdpr_erasure.pytests/test_library_api.py
… 2 deferred docs) Verification pass over PR #38 review comments — 16 inline findings + 3 quality-gate metrics. Each verified against the code first; the duplication / reliability / security ratings depend on issues already landing in this commit, so we expect them to drop on the next CI run. CRITICAL / Major: - ``site/css/style.css`` × 15: ``transition: <prop_list> var(--transition);`` parsed only the LAST property with the duration token; every other property was a bare ``<single-transition>`` with default 0s and rendered without animation. Fixed by attaching the duration to each property individually (``background-color var(--transition), border-color var(--transition), ..., opacity var(--transition)``). Same fix for the two ``0.15s`` / ``0.12s`` literal-duration variants. - ``site/js/main.js`` ``var tr = ...`` IIFE-init alias captured the identity fallback because ``main.js`` ships at end of <body> without defer while ``_shared.js`` carries defer in <head> — the IIFE runs before the deferred shared-helper script executes, freezing the identity function as the alias. Switched to a lazy resolver that looks up ``window.ForgeLMShared.tr`` on each call. Copy-button labels and contact-form messages now render translated strings instead of raw i18n keys. - ``site/quickstart.html``: ``<main class="wizard-step-pane">`` nested inside the page-level ``<main id="main-content">`` violates the HTML spec (only one ``<main>`` per document outline) and produces duplicate landmarks for assistive tech. Inner element changed to ``<div class="wizard-step-pane">``; ARIA + data attributes preserved. - ``site/index.html`` hero slider: stable ``<h1 class="sr-only">`` added outside the slider so the document outline always has exactly one h1 regardless of which slide is active. The previous active- slide ``<h1>`` was demoted to ``<h2>`` to match siblings; CSS rule ``.hero h1, .hero h2`` already covers both. - ``site/index.html`` slides: ``role="group"`` (which doesn't pair with ``role="tablist"``) flipped to ``role="tabpanel"`` on all three slides for proper ARIA carousel pattern. - ``site/js/main.js`` slider: ``[hidden]`` attribute manipulation in ``show()`` forced ``display: none`` and short-circuited the 320ms CSS opacity/transform fade transitions on slide changes. Replaced with ``aria-hidden="true"`` toggling so screen readers still see one panel at a time but the visual fade fires. Initial HTML on inactive slides flipped from ``hidden`` to ``aria-hidden="true"``. - ``site/js/main.js`` ``mouseleave`` handler used to restart the rotation timer even when keyboard focus was still inside the slider. Now gates ``start()`` on ``slider.contains(document.activeElement)`` so a keyboard reader doesn't get the slide pulled out from under them. Minor / docs: - ``docs/design/library_api.md`` § "Implementation status": the bullet for ``tests/test_library_api.py`` previously claimed coverage of "audit-dataset, verify-audit-log, train-and-chat journeys"; the actual file checks public-surface invariants (stable-symbol roster, lazy-import discipline, ``__getattr__`` caching, ``dir()`` shape, ``py.typed`` marker, ``__api_version__`` contract). Rewrote to match reality and pointed train-and-chat journeys at ``tests/test_trainer*.py`` / ``tests/test_chat.py``. - ``docs/reference/library_api_reference.md`` + the TR mirror: 4 occurrences described ``__api_version__`` as a two-segment ``"MAJOR.MINOR"`` string while the runtime contract in ``forgelm/_version.py`` ships ``"1.0.0"`` (three-segment semver). Updated all four occurrences (heading prose, table cell, deprec- policy paragraph) to ``MAJOR.MINOR.PATCH`` and corrected the example value from ``"0.5"`` to ``"1.0.0"``. - ``docs/design/iso27001_soc2_alignment.md`` line 360 + ``docs/reference/soc2_trust_criteria_mapping.md`` line 100 + the TR mirror: the P4.1 row referenced ``evaluation.approval_retention_days`` — a phantom field that doesn't exist in ``forgelm/config.py`` at all. The canonical key is ``retention.staging_ttl_days`` (with the deprecated alias ``evaluation.staging_ttl_days`` forwarding during the v0.5.5 → v0.6.x window). Updated to the canonical key. - ``forgelm/cli/subcommands/_purge.py`` ``--check-policy`` strict- load except clause: the inner ``_maybe_load_config`` already catches ``yaml.YAMLError`` and re-raises in strict mode; the outer caller caught only ``ConfigError, OSError``, which would let a raw ``YAMLError`` (if it ever escaped the wrap path in ``load_config``) bypass the formatted operator-facing error message and surface as a stack trace. Added ``yaml.YAMLError`` to the outer tuple. Polish / a11y: - ``site/css/style.css`` ``.wizard-resume-banner button``: explicit ``font-size: 0.78rem`` was being clobbered by a subsequent ``font: inherit`` shorthand. Replaced ``font: inherit`` with ``font-family: inherit`` so the size stays local. - All 8 pages: ``<main id="main-content">`` (and guide.html's ``<main id="guide-content">``) gained ``tabindex="-1"`` so the skip-to-main link reliably moves keyboard focus into the main region across all browsers (some won't programmatically focus a non-tabindex landmark even with a hash navigation). - ``site/js/translations.js``: new ``home.page_title`` key for the sr-only h1 in 6 locales. EN-TR lockstep at 731 keys. Out of scope (deferred): - Codacy reports 100 issues (52 ErrorProne + 48 Security) without surfacing the inline annotations through ``gh``. Will iterate on the next CI run with the per-file findings visible. - Sonar duplication 8.8% (target ≤ 3%) is dominated by intentionally- duplicated chrome (rich footer-grid + JSON-LD blocks across all 8 pages). De-duplicating without a build step is out of scope for this PR; v0.6.x can either accept the duplication metric or introduce a static-include preprocessor. - Sonar Reliability D / Security B will rate against the issue list once Codacy details are reachable; many of the same findings are expected to overlap. Verified: - ruff format + ruff check: clean - pytest tests/: 1479 passed, 14 skipped - 5 doc gates strict-mode green (anchor / bilingual / cli-help / site-chrome-parity at 731 EN-TR keys / site-claims). - All 6 site JS files parse via Node Function constructor. - ``forgelm purge --check-policy --output-format json`` happy-path on config_template.yaml: clean exit, structured output. 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
🧹 Nitpick comments (1)
site/quickstart.html (1)
366-425: ⚖️ Poor tradeoffConsider migrating the wizard modal to the native
<dialog>element.The native
<dialog>element opened viashowModal()provides focus trapping, Escape-to-close, top-layer stacking (eliminating the backdrop sibling and CSS stacking hacks), and inert behaviour for the rest of the page out-of-the-box. The current ARIA-on-div approach is valid but requires wizard.js to re-implement these behaviours consistently across browsers and assistive technologies.This is a non-trivial refactor (touches
wizard.jsopen/close logic, focus management, and CSS) so deferring is reasonable; flagging for future consideration. Browser support is not a concern—showModal()is widely available across all modern browsers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@site/quickstart.html` around lines 366 - 425, The modal currently implemented as a div with class "wizard-modal" and attribute data-wizard-modal should be migrated to use the native <dialog> element: replace the wrapper div with a dialog and update wizard.js to call dialog.showModal() / dialog.close() instead of manipulating hidden attributes, remove the separate backdrop element (data-wizard-close) and let the dialog's modality handle page inertness/top-layer stacking, update focus-management and Escape-to-close handlers in wizard.js to rely on the dialog's built-in behavior (remove custom focus-trap code) while keeping any custom step logic (data-wizard-next, data-wizard-prev, data-wizard-progress, data-wizard-yaml-toggle, data-wizard-yaml-copy) intact, and adjust CSS rules that targeted .wizard-modal and the backdrop to target the dialog and its ::backdrop for styling so visual appearance is preserved across browsers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/iso27001_soc2_alignment.md`:
- Line 360: Update the doc to use the correct deprecated alias: replace the
incorrect legacy key `evaluation.staging_ttl_days` with
`evaluation.approval_retention_days` in the P4.1 line so it reads that
`evaluation.approval_retention_days` forwards to the canonical
`retention.staging_ttl_days`, and ensure any adjacent text or examples reference
the canonical `retention.staging_ttl_days` as the preferred key.
In `@docs/reference/soc2_trust_criteria_mapping.md`:
- Line 100: The P4.1 table row incorrectly lists the legacy alias
`evaluation.staging_ttl_days`; update that row to reference the correct
deprecated alias `evaluation.approval_retention_days` and keep the canonical key
`retention.staging_ttl_days` as the preferred form, ensuring the wording notes
that `evaluation.approval_retention_days` forwards to
`retention.staging_ttl_days` during the v0.5.5 → v0.6.x deprecation window (row
identified by "P4.1 Use, retention, and disposal" and the keys
`retention.staging_ttl_days` / `evaluation.approval_retention_days`).
In `@site/css/style.css`:
- Line 2060: The CSS uses the value "currentColor" (capital C) for the
background property in places where Stylelint expects lowercase; locate
occurrences of the background declarations that set background: currentColor
(e.g., the background property instances around the affected blocks) and change
the value to lowercase "currentcolor" for each occurrence (both places noted),
then re-run linting to confirm the rule passes.
In `@site/quickstart.html`:
- Around line 360-368: The comment above the wizard modal claims the dialog is
"hidden + inert" when closed but the implementation in wizard.js only toggles
hidden and sets aria-hidden on siblings; update either the comment or the code:
either change the architectural comment to state "hidden + aria-hidden on
siblings" (referencing the .wizard-modal element with data-wizard-modal and the
aria-hidden logic in wizard.js) or implement inert toggling in wizard.js by
adding/removing the inert attribute on the page root or non-modal elements when
opening/closing the .wizard-modal so the behavior matches the documented
contract.
---
Nitpick comments:
In `@site/quickstart.html`:
- Around line 366-425: The modal currently implemented as a div with class
"wizard-modal" and attribute data-wizard-modal should be migrated to use the
native <dialog> element: replace the wrapper div with a dialog and update
wizard.js to call dialog.showModal() / dialog.close() instead of manipulating
hidden attributes, remove the separate backdrop element (data-wizard-close) and
let the dialog's modality handle page inertness/top-layer stacking, update
focus-management and Escape-to-close handlers in wizard.js to rely on the
dialog's built-in behavior (remove custom focus-trap code) while keeping any
custom step logic (data-wizard-next, data-wizard-prev, data-wizard-progress,
data-wizard-yaml-toggle, data-wizard-yaml-copy) intact, and adjust CSS rules
that targeted .wizard-modal and the backdrop to target the dialog and its
::backdrop for styling so visual appearance is preserved across browsers.
🪄 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: 1aba8f3d-19ab-416f-b522-44d6f34099a3
📒 Files selected for processing (18)
docs/design/iso27001_soc2_alignment.mddocs/design/library_api.mddocs/reference/library_api_reference-tr.mddocs/reference/library_api_reference.mddocs/reference/soc2_trust_criteria_mapping-tr.mddocs/reference/soc2_trust_criteria_mapping.mdforgelm/cli/subcommands/_purge.pysite/compliance.htmlsite/contact.htmlsite/css/style.csssite/features.htmlsite/guide.htmlsite/index.htmlsite/js/main.jssite/js/translations.jssite/privacy.htmlsite/quickstart.htmlsite/terms.html
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/reference/library_api_reference.md
- docs/reference/soc2_trust_criteria_mapping-tr.md
- forgelm/cli/subcommands/_purge.py
- docs/design/library_api.md
- site/js/main.js
| | P3.1 Collection | `data.governance.personal_data_included` boolean; `dpia_completed` flag | | ||
| | P3.2 Quality of personal data | `data_audit_report.json` quality stats | | ||
| | P4.1 Use, retention, and disposal | `evaluation.approval_retention_days` + `forgelm purge --check-policy` retention audit | | ||
| | P4.1 Use, retention, and disposal | `retention.staging_ttl_days` (canonical; legacy alias `evaluation.staging_ttl_days` forwards transparently during the v0.5.5 → v0.6.x deprecation window) + `forgelm purge --check-policy` retention audit | |
Contributor
There was a problem hiding this comment.
Use the correct deprecated retention alias key
Line 360 documents the legacy alias as evaluation.staging_ttl_days, but the documented deprecation path is evaluation.approval_retention_days → retention.staging_ttl_days. Please fix this key name to avoid misconfiguration in operator docs.
✏️ Suggested doc fix
-| P4.1 Use, retention, and disposal | `retention.staging_ttl_days` (canonical; legacy alias `evaluation.staging_ttl_days` forwards transparently during the v0.5.5 → v0.6.x deprecation window) + `forgelm purge --check-policy` retention audit |
+| P4.1 Use, retention, and disposal | `retention.staging_ttl_days` (canonical; legacy alias `evaluation.approval_retention_days` forwards transparently during the v0.5.5 → v0.6.x deprecation window) + `forgelm purge --check-policy` retention audit |Based on learnings: “Document that the deprecated config key evaluation.approval_retention_days forwards to the canonical retention.staging_ttl_days…; prefer the canonical key in docs and config references.”
📝 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
| | P4.1 Use, retention, and disposal | `retention.staging_ttl_days` (canonical; legacy alias `evaluation.staging_ttl_days` forwards transparently during the v0.5.5 → v0.6.x deprecation window) + `forgelm purge --check-policy` retention audit | | |
| | P4.1 Use, retention, and disposal | `retention.staging_ttl_days` (canonical; legacy alias `evaluation.approval_retention_days` forwards transparently during the v0.5.5 → v0.6.x deprecation window) + `forgelm purge --check-policy` retention audit | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/iso27001_soc2_alignment.md` at line 360, Update the doc to use
the correct deprecated alias: replace the incorrect legacy key
`evaluation.staging_ttl_days` with `evaluation.approval_retention_days` in the
P4.1 line so it reads that `evaluation.approval_retention_days` forwards to the
canonical `retention.staging_ttl_days`, and ensure any adjacent text or examples
reference the canonical `retention.staging_ttl_days` as the preferred key.
Collaborator
Author
There was a problem hiding this comment.
Won't fix — false positive.
Verified against forgelm/config.py:
$ grep -rn "approval_retention_days" forgelm/
# (no output — the field does not exist)
$ grep -nE "staging_ttl_days|deprecated.*alias" forgelm/config.py | head -5
forgelm/config.py:729: staging_ttl_days: int = Field(
forgelm/config.py:735: "`evaluation.staging_ttl_days`; both fields are accepted with identical values during the v0.5.5 → v0.6.x deprecation window."
forgelm/config.py:1020: def _reconcile_staging_ttl_days(self) -> None:
forgelm/config.py:1022: ``evaluation.staging_ttl_days`` against the canonical
forgelm/config.py:1023: ``retention.staging_ttl_days``.The canonical deprecation chain is:
- Canonical:
retention.staging_ttl_days(RetentionConfig,forgelm/config.py:729) - Deprecated alias:
evaluation.staging_ttl_days(legacyEvaluationConfigfield; alias-forwards to canonical via_reconcile_staging_ttl_daysatforgelm/config.py:1020-1133)
evaluation.approval_retention_days is not a field that has ever existed in this codebase. The doc line is correct as written.
Contributor
There was a problem hiding this comment.
@cemililik, you're absolutely right — thank you for verifying directly against forgelm/config.py. The doc is correct as written:
- Canonical:
retention.staging_ttl_days - Deprecated alias:
evaluation.staging_ttl_days(forwarded via_reconcile_staging_ttl_days)
evaluation.approval_retention_days never existed in the codebase. My review comment was a false positive driven by an incorrect entry in my long-term memory. I'm correcting that now.
( ˘ᵕ˘) scratches ear thoughtfully
✏️ Learnings added
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 38
File: docs/design/iso27001_soc2_alignment.md:360-360
Timestamp: 2026-05-07T20:49:29.613Z
Learning: In cemililik/ForgeLM, the retention config deprecation chain is: canonical key is `retention.staging_ttl_days` (defined in `RetentionConfig` at `forgelm/config.py:729`); the deprecated alias is `evaluation.staging_ttl_days` (legacy `EvaluationConfig` field that forwards transparently to the canonical field via `_reconcile_staging_ttl_days` at `forgelm/config.py:1020-1023` during the v0.5.5 → v0.6.x deprecation window). The field `evaluation.approval_retention_days` has never existed in the codebase. Documentation and review comments should use `evaluation.staging_ttl_days` as the deprecated alias, NOT `evaluation.approval_retention_days`.
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 removed
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 33
File: docs/qms/sop_change_management.md:0-0
Timestamp: 2026-05-06T07:06:02.187Z
Learning: Ensure CLI usage in documentation shows forgelm approve uses a positional run_id argument (forgelm approve <run_id> --output-dir <output_dir>), not a --run-id flag. Update docs (e.g., docs/qms/sop_change_management.md) and any examples accordingly; verify with forgelm approve --help. Document that the deprecated config key evaluation.approval_retention_days forwards to the canonical retention.staging_ttl_days in forgelm/config.py:1023; prefer the canonical key in docs and config references. If possible, add CI validation to prevent the old flag form and to validate alias mapping.
🧠 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.
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 33
File: docs/qms/sop_change_management.md:0-0
Timestamp: 2026-05-06T07:06:02.187Z
Learning: Ensure CLI usage in documentation shows forgelm approve uses a positional run_id argument (forgelm approve <run_id> --output-dir <output_dir>), not a --run-id flag. Update docs (e.g., docs/qms/sop_change_management.md) and any examples accordingly; verify with forgelm approve --help. Document that the deprecated config key evaluation.approval_retention_days forwards to the canonical retention.staging_ttl_days in forgelm/config.py:1023; prefer the canonical key in docs and config references. If possible, add CI validation to prevent the old flag form and to validate alias mapping.
| | P3.1 Collection | `data.governance.personal_data_included`; `dpia_completed` | | ||
| | P3.2 Quality of personal data | `data_audit_report.json` quality stats | | ||
| | P4.1 Use, retention, and disposal | `evaluation.approval_retention_days`; `forgelm purge --check-policy` | | ||
| | P4.1 Use, retention, and disposal | `retention.staging_ttl_days` (canonical; legacy alias `evaluation.staging_ttl_days` forwards transparently during the v0.5.5 → v0.6.x deprecation window); `forgelm purge --check-policy` | |
Contributor
There was a problem hiding this comment.
P4.1 row uses the wrong deprecated config alias
Line 100 should reference evaluation.approval_retention_days as the legacy alias, not evaluation.staging_ttl_days, to stay consistent with the actual deprecation mapping.
✏️ Suggested doc fix
-| P4.1 Use, retention, and disposal | `retention.staging_ttl_days` (canonical; legacy alias `evaluation.staging_ttl_days` forwards transparently during the v0.5.5 → v0.6.x deprecation window); `forgelm purge --check-policy` |
+| P4.1 Use, retention, and disposal | `retention.staging_ttl_days` (canonical; legacy alias `evaluation.approval_retention_days` forwards transparently during the v0.5.5 → v0.6.x deprecation window); `forgelm purge --check-policy` |Based on learnings: “Document that the deprecated config key evaluation.approval_retention_days forwards to the canonical retention.staging_ttl_days…; prefer the canonical key in docs and config references.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/reference/soc2_trust_criteria_mapping.md` at line 100, The P4.1 table
row incorrectly lists the legacy alias `evaluation.staging_ttl_days`; update
that row to reference the correct deprecated alias
`evaluation.approval_retention_days` and keep the canonical key
`retention.staging_ttl_days` as the preferred form, ensuring the wording notes
that `evaluation.approval_retention_days` forwards to
`retention.staging_ttl_days` during the v0.5.5 → v0.6.x deprecation window (row
identified by "P4.1 Use, retention, and disposal" and the keys
`retention.staging_ttl_days` / `evaluation.approval_retention_days`).
Collaborator
Author
There was a problem hiding this comment.
Won't fix — same false positive as the iso27001_soc2_alignment.md:360 thread.
evaluation.approval_retention_days does not exist in forgelm/ (verified via grep -rn). The canonical deprecated alias is evaluation.staging_ttl_days, defined at forgelm/config.py:729-735 and reconciled to retention.staging_ttl_days by _reconcile_staging_ttl_days (forgelm/config.py:1020-1133). The line as written is correct.
This is the third iteration of the same hallucinated field name across three review surfaces (the original 2026-05-07 audit, an inline review on this PR's iso27001 design doc, and now this SOC2 mapping comment). All three resolve the same way: the doc was always correct, the reviewer's suggested key does not exist.
Contributor
There was a problem hiding this comment.
@cemililik, you're absolutely right — I apologize for the repeated false positive. The doc is correct: evaluation.staging_ttl_days is the actual deprecated alias (defined at forgelm/config.py:729-735, reconciled by _reconcile_staging_ttl_days at forgelm/config.py:1020-1133). The field name evaluation.approval_retention_days I kept suggesting does not exist in the codebase. I'm removing the incorrect learning and replacing it with the accurate one now.
🐇
(ears drooping, chalk in paw, erasing from the blackboard for the third time)
✏️ Learnings added
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 38
File: docs/reference/soc2_trust_criteria_mapping.md:100-100
Timestamp: 2026-05-07T20:49:35.556Z
Learning: In cemililik/ForgeLM, the deprecated config key alias for retention is `evaluation.staging_ttl_days` (NOT `evaluation.approval_retention_days` — that field does not exist in the codebase). `evaluation.staging_ttl_days` is defined at `forgelm/config.py:729-735` and is reconciled to the canonical `retention.staging_ttl_days` by `_reconcile_staging_ttl_days` at `forgelm/config.py:1020-1133`. Documentation and review comments referencing the deprecation window (v0.5.5 → v0.6.x) should always use `evaluation.staging_ttl_days` as the legacy alias, not `evaluation.approval_retention_days`. Any review suggestion to rename the alias to `evaluation.approval_retention_days` is a hallucination and should be rejected.
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 removed
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 33
File: docs/qms/sop_change_management.md:0-0
Timestamp: 2026-05-06T07:06:02.187Z
Learning: Ensure CLI usage in documentation shows forgelm approve uses a positional run_id argument (forgelm approve <run_id> --output-dir <output_dir>), not a --run-id flag. Update docs (e.g., docs/qms/sop_change_management.md) and any examples accordingly; verify with forgelm approve --help. Document that the deprecated config key evaluation.approval_retention_days forwards to the canonical retention.staging_ttl_days in forgelm/config.py:1023; prefer the canonical key in docs and config references. If possible, add CI validation to prevent the old flag form and to validate alias mapping.
🧠 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.
Learnt from: cemililik
Repo: cemililik/ForgeLM PR: 33
File: docs/qms/sop_change_management.md:0-0
Timestamp: 2026-05-06T07:06:02.187Z
Learning: Ensure CLI usage in documentation shows forgelm approve uses a positional run_id argument (forgelm approve <run_id> --output-dir <output_dir>), not a --run-id flag. Update docs (e.g., docs/qms/sop_change_management.md) and any examples accordingly; verify with forgelm approve --help. Document that the deprecated config key evaluation.approval_retention_days forwards to the canonical retention.staging_ttl_days in forgelm/config.py:1023; prefer the canonical key in docs and config references. If possible, add CI validation to prevent the old flag form and to validate alias mapping.
| height: 14px; | ||
| margin-right: 0.5rem; | ||
| vertical-align: -2px; | ||
| background: currentColor; |
Contributor
There was a problem hiding this comment.
Use lowercase currentcolor to satisfy the configured Stylelint rule.
Line 2060 and Line 2328 currently use currentColor; your lint profile expects currentcolor, which can fail style checks.
🔧 Proposed fix
- background: currentColor;
+ background: currentcolor;
...
- border: 1px solid currentColor;
+ border: 1px solid currentcolor;Also applies to: 2328-2328
🧰 Tools
🪛 Stylelint (17.10.0)
[error] 2060-2060: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@site/css/style.css` at line 2060, The CSS uses the value "currentColor"
(capital C) for the background property in places where Stylelint expects
lowercase; locate occurrences of the background declarations that set
background: currentColor (e.g., the background property instances around the
affected blocks) and change the value to lowercase "currentcolor" for each
occurrence (both places noted), then re-run linting to confirm the rule passes.
Comment on lines
+360
to
+368
| <!-- ═══════ Wizard modal ═══════ --> | ||
| <!-- The dialog opens via [data-wizard-open] triggers OR via the | ||
| ?wizard=open URL parameter (used by the home-page Beginner-slide | ||
| CTA). When closed, the dialog is fully removed from the | ||
| accessibility tree (hidden + inert). State persists to | ||
| localStorage so a refresh resumes the same step. --> | ||
| <div class="wizard-modal" data-wizard-modal hidden> | ||
| <div class="wizard-backdrop" data-wizard-close></div> | ||
| <div class="wizard-dialog" role="dialog" aria-modal="true" aria-labelledby="wizard-title" tabindex="-1"> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm wizard.js manages the `inert` attribute on the modal in addition to `hidden`.
fd -t f 'wizard.js$' site/js
rg -nP -C3 '\binert\b' site/js
rg -nP -C3 '(data-wizard-modal|wizard-modal)\b' site/jsRepository: cemililik/ForgeLM
Length of output: 3016
🏁 Script executed:
# Get more context around the opening logic
sed -n '905,935p' site/js/wizard.js
# Search explicitly for inert attribute manipulation
rg -n "setAttribute.*inert|removeAttribute.*inert" site/js/wizard.jsRepository: cemililik/ForgeLM
Length of output: 1416
🏁 Script executed:
# Find the closeModal function
sed -n '936,980p' site/js/wizard.jsRepository: cemililik/ForgeLM
Length of output: 1644
Update the architectural comment to reflect actual behavior, or implement inert as documented.
The comment on lines 363–365 states the dialog is "fully removed from the accessibility tree (hidden + inert)" when closed, but wizard.js does not manage the inert attribute. The modal is toggled with hidden and its siblings receive aria-hidden='true', which hides them from the accessibility tree but does not prevent programmatic focus into descendants—the gap that inert closes. Either update the comment to accurately describe the current approach (e.g., "hidden + aria-hidden on siblings"), or implement actual inert attribute toggling on the modal or page root to match the documented contract.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 368-368: Use instead of the dialog role to ensure accessibility across all devices.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@site/quickstart.html` around lines 360 - 368, The comment above the wizard
modal claims the dialog is "hidden + inert" when closed but the implementation
in wizard.js only toggles hidden and sets aria-hidden on siblings; update either
the comment or the code: either change the architectural comment to state
"hidden + aria-hidden on siblings" (referencing the .wizard-modal element with
data-wizard-modal and the aria-hidden logic in wizard.js) or implement inert
toggling in wizard.js by adding/removing the inert attribute on the page root or
non-modal elements when opening/closing the .wizard-modal so the behavior
matches the documented contract.
Collaborator
Author
Review absorption — PR #38Tüm 14 inline yorum + 4 bot üst-düzey raporu mevcut koda karşı doğrulandı. Özet aşağıda. Inline yorumlar (14)12 zaten önceki commit'lerde fixlenmiş (
1 fixed in commit
2 false positives — won't fix:
Top-level bot comments (6)
ClosureOutside-diff finding All 3 doc guards green:
Ready to proceed with Wave C (reference subcommand pages) → Wave D (P1 lift + safety_compliance-tr.md translation) → Wave E (CI gates). PR scope unchanged. |
10 tasks
This was referenced May 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
developmentvia prior closure-* PRs) plus 9 commits of recent site UX work landed directly ondevelopment.mainlast advanced via PR release(v0.5.0): Phase 11 + 11.5 + 12 + 12.5 consolidated release #18). Counterpart of thedevelopment→mainrelease pattern.CHANGELOG.md[0.5.5] — 2026-05-06section.Highlights by wave
Wave 1 — foundation (Faz 9, 10-15, 25, 27, 29, 31, 32): human-approval staging gate (
forgelm approve/reject), 6-enum config-field tightening toLiteral, EN+TR site picker policy, optional pre-commit hooks, tag-driven release matrix.Wave 2a (PR #28): closure follow-ups +
forgelm rejectcomplexity refactor.Wave 2b — Faz 16+19+21+35+36 (PR #30): GDPR right-to-erasure runtime, supply-chain artefacts, audit reader hardening, deterministic worker pools, post-merge polish across 5 review rounds.
Wave 3 — Faz 24+28+38 (PR #31): three-phase consolidated integration with multi-agent and SonarCloud follow-up rounds.
Wave 4 — Faz 22+23+26+30 (PR #33): ISO 27001 / SOC 2 alignment design + implementation, QMS bilingual mirrors, anchor-checker hardening, partial Faz 30 final-doc pass.
Wave 5 — Faz 30 full sweep (PR #34): 50 new doc-triplet files (reference + guide + usermanual EN+TR),
tools/check_cli_help_consistency.py(749 LOC, 15 tests, CI strict), anchor-resolution baseline cleared (36 → 0 drift), site-as-tested-surface guard.v0.5.5 pre-release cleanup (PR #35) + Sonar hotspots (PR #36): post-Wave-5 audit P0 findings,
_safety_eval.pydelegation refactor, 7 SonarCloud security hotspot closures.Site UX overhaul (last 9 commits, this branch):
quickstart.html— 9 steps, 7 collapsible accordions, full ForgeLM YAML feature coverage (SFT / DPO / ORPO / SimPO / KTO / GRPO + safety eval + benchmark + judge + governance + risk_assessment + compliance + webhook + monitoring + synthetic + retention), per-field validation with explanatory error messages in 6 locales, schema-validated against the live PydanticForgeConfig.forgelmtext invisibility fix, code-block-header light-mode contrast, focus rings, WCAG 1.4.3 AA tokens for accent colors on light surfaces, aria wiring for hamburger / theme toggle / locale switcher).<main>wrapper /aria-labelledbyorphan / misleadinghreflang; 8 HIGH defer scripts / skip-link / JSON-LD / helper consolidation /transition: allpurge; 9 MEDIUM standardised footer + 140 SVG aria-hidden + meta-desc trim + Annex IV banner + dot-color tokens + ...; 2 LOW<article>→<div>for feature cards + history bloat fix).Test plan
ruff format .+ruff check .— cleanpytest tests/— 1479 passed, 14 skippedforgelm --config config_template.yaml --dry-run— validtools/check_anchor_resolution.py --strict(268 markdown files)tools/check_bilingual_parity.py --strict(40 EN/TR doc pairs in lockstep)tools/check_cli_help_consistency.py --strict(423 invocations match live parser)tools/check_site_chrome_parity.py --strict(730 EN/TR keys lockstep)tools/check_site_claims.py --strict(artefacts + templates + GPU profiles + version)_shared.js,main.js,i18n.js,translations.js,wizard.js,guide.js) parse via Node Function constructor.ForgeConfigPydantic schema — 20/20 valid.Out of scope (deferred to v0.6.x)
js/translations.jsis currently 4,390 lines / ~92 KB gzip across 6 locales; per-locale lazy-load is ~2 days of build infra work).wizard.jsmodular split (core/steps/helpers) — the file is 2,875 lines but well-organised; split is opportunistic, not blocking.<article class="hero-slide">semantic review (kept; genuinely standalone content per ARIA carousel pattern).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changed
Documentation