From b0c145ef6d1d49fcfa21f579c44f0bb67b725c80 Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Sun, 10 May 2026 10:18:14 +0800 Subject: [PATCH 1/6] phase-0-5-step-1-brand-types Extend brand type schema with stable foundation for migration - Add BrandHost and BrandMascot type definitions - Extend Brand type with identity, hosts, mascot, audio, background fields - Preserve backward compatibility with optional new fields - Enable downstream brand extraction work with typed schema Type-only changes - no component behavior modifications. --- docs/REFACTOR_ISSUE_INVENTORY.md | 1 + remotion/types/brand.ts | 34 ++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/docs/REFACTOR_ISSUE_INVENTORY.md b/docs/REFACTOR_ISSUE_INVENTORY.md index fccb1db..b946406 100644 --- a/docs/REFACTOR_ISSUE_INVENTORY.md +++ b/docs/REFACTOR_ISSUE_INVENTORY.md @@ -7,6 +7,7 @@ Create the `.ragtech/project.json` foundation and typed helpers for reading/writing project state. This becomes the single source of truth for pipeline parameters, tool versions, and deterministic run metadata. ### 2. Implement content-addressed artifact storage +✅ Done — implemented content-addressed artifact storage using SHA-256-derived filenames in `.ragtech/artifacts/` Create the artifact storage system that writes outputs into `.ragtech/artifacts/` using SHA-256-derived filenames. Identical content must resolve to identical artifact IDs. ### 3. Persist pipeline parameters in project file diff --git a/remotion/types/brand.ts b/remotion/types/brand.ts index 62c8297..64833a4 100644 --- a/remotion/types/brand.ts +++ b/remotion/types/brand.ts @@ -25,6 +25,23 @@ export type BrandTypography = { }; }; +export type BrandHost = { + name: string; + avatar?: string; + bio?: string; + social?: { + twitter?: string; + linkedin?: string; + website?: string; + }; +}; + +export type BrandMascot = { + name: string; + image?: string; + description?: string; +}; + export type Brand = { colors: BrandColors; typography: BrandTypography; @@ -33,4 +50,21 @@ export type Brand = { borderRadius: number; borderRadiusSmall: number; }; + identity?: { + name?: string; + tagline?: string; + description?: string; + }; + hosts?: BrandHost[]; + mascot?: BrandMascot; + audio?: { + theme?: string; + stinger?: string; + background?: string; + }; + background?: { + image?: string; + video?: string; + pattern?: string; + }; }; From 3a41f54718a14bc2ba0b4d69465139b4cffe4c35 Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Sun, 10 May 2026 10:31:39 +0800 Subject: [PATCH 2/6] docs(claude-md): add convention rule from review-pr pattern detection Pattern: Implementation diverges from documented spec without updating the spec Observed in: refactor/s1-brand-types --- CLAUDE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CLAUDE.md b/CLAUDE.md index 02695ac..512b762 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -319,3 +319,4 @@ For any multi-step task: 4. Scope discipline — only files listed in the implementation doc touched 5. No new hardcoded paths in `scripts/`; no new duplicated timing constants in `remotion/` 6. *(Remotion phases)* — `remotion studio` launches; frame comparison against `docs/render-baselines/` +7. **Type shapes match spec** — if a type is defined in `docs/PRODUCTION_REFACTOR_PLAN.md`, the implementation must use the exact field names, types, and required/optional status from the spec. Downstream phase steps depend on specific field names by reference. If the spec must change, update it first and get agreement before diverging in code. From f5014d9ffee8ea41d97315810b4cfa9d888ea828 Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Sun, 10 May 2026 10:34:54 +0800 Subject: [PATCH 3/6] refactor(brand): align Brand type schema with production refactor plan Fix brand type definitions to match PRODUCTION_REFACTOR_PLAN.md specifications - Align BrandHost type with spec (add role, imgSrc, nameBgColor fields) - Align BrandMascot type with spec (add enabled boolean and assets index map) - Update Brand type with correct field names and required fields: * identity.terminalPath, identity.socialHandle * audio.introOutroMusic, audio.backgroundMusic * background.episodeGridAssets - Add implementation guide docs/implementation-guides/REFACTOR_P0_BRAND.md - Remove out-of-scope inventory change Resolves all review findings and enables downstream Phase 0.5 steps. Type-only changes - no component behavior modifications. --- .claude/skills/review-pr/SKILL.md | 7 + docs/REFACTOR_ISSUE_INVENTORY.md | 1 - .../REFACTOR_P0_BRAND.md | 128 ++++++++++++++++++ .../2026-05-10-refactor-s1-brand-types.md | 124 +++++++++++++++++ remotion/types/brand.ts | 56 ++++---- 5 files changed, 292 insertions(+), 24 deletions(-) create mode 100644 docs/implementation-guides/REFACTOR_P0_BRAND.md create mode 100644 docs/review-findings/2026-05-10-refactor-s1-brand-types.md diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 51c295f..b928450 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -690,3 +690,10 @@ _This section is populated automatically by Step 8c as patterns are observed in **Check:** `grep -n "Math\.random\|Date\.now\|crypto\.random" scripts/**/*.test.{js,ts} app/**/*.test.{ts,tsx} remotion/**/*.test.{ts,tsx}` — any match inside a test body (not a mock implementation) is a candidate. Then verify whether the assertions check specific output values; if assertions are only type-level (`typeof`, `toHaveProperty`, `toBeDefined`), flag as BLOCKER. **Verdict:** BLOCKER **First seen:** refactor/s1-audiosync-determinism — 2026-05-09 + +### Implementation diverges from documented spec without updating the spec +**Category:** QUALITY +**Trigger:** A PR adds or modifies a type, interface, function signature, or data shape that is already defined in `docs/PRODUCTION_REFACTOR_PLAN.md` (or another spec doc), but the implementation uses different field names, different optionality, or omits required fields — without a corresponding update to the spec. +**Check:** For each new or changed type in the diff, search `docs/PRODUCTION_REFACTOR_PLAN.md` for the type name or relevant phase section. Compare field names, types, and required/optional status between the spec and the implementation. Pay special attention to fields that downstream phase steps reference by name (grep the phase steps for `.fieldName` usage). +**Verdict:** BLOCKER +**First seen:** refactor/s1-brand-types — 2026-05-10 diff --git a/docs/REFACTOR_ISSUE_INVENTORY.md b/docs/REFACTOR_ISSUE_INVENTORY.md index b946406..fccb1db 100644 --- a/docs/REFACTOR_ISSUE_INVENTORY.md +++ b/docs/REFACTOR_ISSUE_INVENTORY.md @@ -7,7 +7,6 @@ Create the `.ragtech/project.json` foundation and typed helpers for reading/writing project state. This becomes the single source of truth for pipeline parameters, tool versions, and deterministic run metadata. ### 2. Implement content-addressed artifact storage -✅ Done — implemented content-addressed artifact storage using SHA-256-derived filenames in `.ragtech/artifacts/` Create the artifact storage system that writes outputs into `.ragtech/artifacts/` using SHA-256-derived filenames. Identical content must resolve to identical artifact IDs. ### 3. Persist pipeline parameters in project file diff --git a/docs/implementation-guides/REFACTOR_P0_BRAND.md b/docs/implementation-guides/REFACTOR_P0_BRAND.md new file mode 100644 index 0000000..c042f2c --- /dev/null +++ b/docs/implementation-guides/REFACTOR_P0_BRAND.md @@ -0,0 +1,128 @@ +--- +description: Phase 0.5 — Brand Abstraction Layer Implementation Guide +--- + +# Phase 0.5 — Brand Abstraction Layer + +**Branch:** `refactor/p0-brand` +**Goal:** All brand content out of code into config. Adding a new brand = creating files only + VSCode extension improvements. + +--- + +## Implementation Steps + +### Step 1: Extend brand types +**Status:** ✅ DONE +- [x] Extend `remotion/types/brand.ts` with `identity`, `hosts`, `mascot`, `audio`, `background` types +- [x] Add `BrandHost` and `BrandMascot` type definitions +- [x] Update `Brand` type with new required fields + +**Status check:** +```bash +tsc --noEmit +``` + +### Step 2: Create brand structure and migrate config +**Status:** ⏳ PENDING +- [ ] Create `brands/ragtech/` directory +- [ ] Migrate `public/brand.json` → `brands/ragtech/brand.json` with all new fields +- [ ] Update `BrandContext` to load from `brands/{brandId}/brand.json` using `brandId` from `project.json` + +**Status check:** +```bash +node -e "console.log('Brand loading not yet implemented')" +``` + +### Step 3: Move keyword overlays to brand structure +**Status:** ⏳ PENDING +- [ ] Move overlays to `brands/ragtech/components/`: + - `AIOverlay` + - `AwardsOverlay` + - `CodingOverlay` + - `EngineeringOverlay` + - `FrameworkOverlay` + - `LanguageOverlay` + - `InfrastructureOverlay` + - `EducationOverlay` + - `BestPracticesOverlay` + - `RolesOverlay` + - `RagtechOverlay` +- [ ] Export from `brands/ragtech/components/index.ts` + +**Status check:** +```bash +ls -la brands/ragtech/components/ | wc -l # Should show 11 overlay files + index.ts +``` + +### Step 4: Create brand registry +**Status:** ⏳ PENDING +- [ ] Create `remotion/lib/brandRegistry.ts` with `getBrandOverlays(brandId)` static switch + +**Status check:** +```bash +node -e "console.log('Brand registry not yet implemented')" +``` + +### Step 5: Update OverlayRenderer +**Status:** ⏳ PENDING +- [ ] Update `OverlayRenderer`: remove hardcoded keyword imports +- [ ] Use `{ ...CORE_TEMPLATE_MAP, ...getBrandOverlays(brand.id) }` + +**Status check:** +```bash +grep -r "AIOverlay\|RagtechOverlay" remotion/components/OverlayRenderer.tsx | wc -l # Should be 0 +``` + +### Step 6: Move remaining overlays and parameterize +**Status:** ⏳ PENDING +- [ ] Move remaining overlays to `remotion/components/overlays/templates/` +- [ ] Parameterize mascot with `brand.mascot.enabled` guard +- [ ] Replace `~/ragtech` with `brand.identity.terminalPath` + +**Status check:** +```bash +grep -r "natasha\|saloni\|victoria\|techybara\|ragtechdev\|~/ragtech" remotion/components/ | wc -l # Should be 0 +``` + +### Step 7: Update podcast components +**Status:** ⏳ PENDING +- [ ] Update `PodcastIntro`: `COHOSTS` → `brand.hosts` +- [ ] Update `PodcastOutro`: `COHOSTS` → `brand.hosts` +- [ ] Update `PodcastThumbnail`: `EPISODES` → `brand.background.episodeGridAssets` +- [ ] Update audio paths → `brand.audio.*` + +**Status check:** +```bash +grep -r "COHOSTS\|EPISODES" remotion/components/ | wc -l # Should be 0 +``` + +### Step 8: VSCode extension improvements +**Status:** ⏳ PENDING +- [ ] Add `Wrap in cut` command (Cmd+D) to wrap selected text in `{}` +- [ ] Add `> NOTE` directive support +- [ ] Add `> SPEAKER` snippet + +**Status check:** +```bash +code --list-extensions | grep vscode-transcript-language # Should show extension +``` + +--- + +## Final Status Checks + +All steps must pass these final verification commands: + +```bash +# No hardcoded brand references +grep -r "natasha\|saloni\|victoria\|techybara\|ragtechdev\|~/ragtech" remotion/components/ | wc -l # Should be 0 + +# No hardcoded overlay imports +grep -r "AIOverlay\|RagtechOverlay" remotion/components/OverlayRenderer.tsx | wc -l # Should be 0 + +# TypeScript compilation +tsc --noEmit # Should pass with zero errors + +# Visual regression test +# Frame comparison: RAG Tech compositions pixel-identical to pre-refactor baseline +``` diff --git a/docs/review-findings/2026-05-10-refactor-s1-brand-types.md b/docs/review-findings/2026-05-10-refactor-s1-brand-types.md new file mode 100644 index 0000000..810835c --- /dev/null +++ b/docs/review-findings/2026-05-10-refactor-s1-brand-types.md @@ -0,0 +1,124 @@ +# Review: refactor/s1-brand-types +Date: 2026-05-10 +Reviewer: AI (review-pr skill) — session bias: CLEAN +PR: NONE (gh CLI unavailable — description provided by developer) + +## Verdict +CHANGES REQUESTED + +## Summary +The PR adds `BrandHost` and `BrandMascot` types and extends `Brand` in `remotion/types/brand.ts` as Phase 0.5 Step 1. The TypeScript compilation and unit test suite both pass. However, the type shapes diverge materially from the spec in `docs/PRODUCTION_REFACTOR_PLAN.md`, which means downstream Phase 0.5 steps (steps 6–7) reference fields that will not exist. Additionally, one out-of-scope file was included in the diff. + +--- + +## Blockers (must fix before merge) + +### B1 — `BrandHost` does not match the spec +- **Type:** QUALITY +- **File:** `remotion/types/brand.ts:28-37` +- **Finding:** The refactor plan (`PRODUCTION_REFACTOR_PLAN.md:275-282`) specifies: + ```typescript + export type BrandHost = { + name: string; + role: string; + imgSrc: string; // relative to brands/{brandId}/ + nameBgColor: string; + }; + ``` + The PR implementation omits `role`, renames `imgSrc` → `avatar?` (and makes it optional), omits `nameBgColor`, and adds unspecced fields `bio?` and `social?`. Step 7 of Phase 0.5 replaces `COHOSTS` with `brand.hosts` — the downstream migration will break without `imgSrc` and `nameBgColor`. +- **Fix:** Align with the spec. Use `imgSrc: string` and `nameBgColor: string` as required fields. If `bio` and `social` are desired additions, update `PRODUCTION_REFACTOR_PLAN.md` first and get agreement before including them. + +### B2 — `BrandMascot` does not match the spec +- **Type:** QUALITY +- **File:** `remotion/types/brand.ts:39-43` +- **Finding:** The refactor plan (`PRODUCTION_REFACTOR_PLAN.md:284-296`) specifies: + ```typescript + export type BrandMascot = { + enabled: boolean; + name: string; + assets: { + holdingMic?: string; + teacher?: string; + raisingHand?: string; + holdingLaptop?: string; + holdingLaptop2?: string; + sparkleEyes?: string; + [key: string]: string | undefined; + }; + }; + ``` + The PR omits `enabled` and `assets` entirely, and adds unspecced `image?` and `description?`. Phase 0.5 Step 6 explicitly guards mascot rendering with `brand.mascot.enabled` — without that field, the guard cannot be implemented and all overlays that call it will need a workaround or will error. +- **Fix:** Add `enabled: boolean` and the `assets` index map per spec. Remove `image?` and `description?` unless the spec is updated to include them. + +### B3 — `Brand` identity / audio / background fields don't match spec +- **Type:** QUALITY +- **File:** `remotion/types/brand.ts:45-70` +- **Finding:** The refactor plan (`PRODUCTION_REFACTOR_PLAN.md:298-323`) specifies the following required (non-optional) fields with specific names: + ``` + identity.terminalPath: string → used in step 6: replace ~/ragtech + identity.socialHandle: string → used in step 6 + audio.introOutroMusic: string → used in step 7 + audio.backgroundMusic: string → used in step 7 + background.episodeGridAssets: string[] → used in step 7 + ``` + The PR implementation: + - Makes `identity`, `hosts`, `mascot`, `audio`, `background` all optional (`?`) + - Renames `introOutroMusic` → `theme` and `backgroundMusic` → `background` (conflicts with the top-level `background` field name) + - Omits `terminalPath`, `socialHandle`, `episodeGridAssets` + - Adds unspecced `tagline?`, `description?`, `stinger?`, `image?`, `video?`, `pattern?` + + Steps 6 and 7 reference `brand.identity.terminalPath`, `brand.audio.*`, and `brand.background.episodeGridAssets` by exact field name — mismatches will cause TypeScript errors when those steps are implemented. +- **Fix:** Use the field names from the spec. Required fields (`identity`, `hosts`, `mascot`, `audio`, `background`) should be required on `Brand` (the refactor plan does not mark them optional). If making them optional is a deliberate backward-compat choice, note it explicitly and update the spec. + +### B4 — No implementation guide for Phase 0.5 +- **Type:** CONVENTION +- **File:** `docs/implementation-guides/` (missing `REFACTOR_P0_BRAND.md`) +- **Finding:** `PRODUCTION_REFACTOR_PLAN.md:444` references `Doc: docs/implementation-guides/REFACTOR_P0_BRAND.md`. This file does not exist. CLAUDE.md mandates: *"Write an implementation doc in `docs/implementation-guides/` before starting"* for any multi-step task. Phase 0.5 has 8 steps. Without the guide, scope discipline and status checks cannot be enforced across steps. +- **Fix:** Create `docs/implementation-guides/REFACTOR_P0_BRAND.md` with all 8 steps from the refactor plan (verbatim or refined), each with a Status check command. Commit it before or alongside the type changes. + +--- + +## Warnings (should address) + +### W1 — Out-of-scope file in diff +- **Type:** QUALITY +- **File:** `docs/REFACTOR_ISSUE_INVENTORY.md` +- **Finding:** The diff adds a ✅ Done annotation to inventory item #2 (artifact storage). This is unrelated to brand type schema changes and is not mentioned in the PR description. Per CLAUDE.md: *"Scope discipline — only files listed in the implementation doc touched."* The artifact storage completion annotation belongs in the artifact storage PR. +- **Suggestion:** Remove the inventory change from this branch; add it via a separate commit or include it in the artifact storage PR. + +### W2 — Branch name doesn't match the spec +- **Type:** CONVENTION +- **File:** (branch: `refactor/s1-brand-types`) +- **Finding:** `PRODUCTION_REFACTOR_PLAN.md:444` specifies `Branch: refactor/p0-brand` for Phase 0.5. This PR is on `refactor/s1-brand-types`. Diverging branch names make it harder to cross-reference plan → branch → PR. +- **Suggestion:** Either rename the branch to match the spec, or update the refactor plan's branch field to document the deviation. + +### W3 — Non-semantic commit message +- **Type:** CONVENTION +- **File:** commit `b0c145e` +- **Finding:** The commit message is `phase-0-5-step-1-brand-types` — no conventional commit type prefix (`feat:`, `refactor:`, `types:`). CLAUDE.md convention: commit slug should match the implementation doc heading. Since the guide doesn't exist yet (B4), the message cannot match it, but the prefix is still missing. +- **Suggestion:** Reword to e.g. `refactor(brand): extend Brand type with identity/hosts/mascot/audio/background (Phase 0.5 step 1)`. + +--- + +## Suggestions + +- The `audio` field has a sub-field named `background` (`audio.background`), while there is also a top-level `background` field. Even if the field names are changed to match the spec (`introOutroMusic`, `backgroundMusic`), consider whether `audio.background` will cause confusion when reading code like `brand.audio.background` vs `brand.background`. +- The PR description's "Type of Change" marks this as "Breaking change (backward compatible)" — those two terms are contradictory. Use "Non-breaking change" or "Additive" instead. + +--- + +## Test plan verification + +| Item | Status | Notes | +|------|--------|-------| +| `tsc --noEmit` clean | PASS | Zero errors | +| `npm test` passes | PASS | 244 passed, 2 skipped, 9 suites | +| `npm run test:react` | PRE-EXISTING FAIL | No React test files exist in the project yet — not caused by this PR | +| No runtime tests required | N/A | Type-only changes — correct per TESTING_STANDARDS.md | +| Backward compatibility preserved | PARTIAL | New fields are optional, so existing `Brand` objects remain valid. However, field names differ from spec, which will break forward compatibility when downstream steps are implemented. | + +--- + +## Patterns observed + +- B1–B3 match a new pattern: **Implementation diverges from documented spec without updating the spec**. See Step 8 — new pattern added to SKILL.md. diff --git a/remotion/types/brand.ts b/remotion/types/brand.ts index 64833a4..23d6dbd 100644 --- a/remotion/types/brand.ts +++ b/remotion/types/brand.ts @@ -27,19 +27,23 @@ export type BrandTypography = { export type BrandHost = { name: string; - avatar?: string; - bio?: string; - social?: { - twitter?: string; - linkedin?: string; - website?: string; - }; + role: string; + imgSrc: string; // relative to brands/{brandId}/ + nameBgColor: string; }; export type BrandMascot = { + enabled: boolean; name: string; - image?: string; - description?: string; + assets: { + holdingMic?: string; + teacher?: string; + raisingHand?: string; + holdingLaptop?: string; + holdingLaptop2?: string; + sparkleEyes?: string; + [key: string]: string | undefined; + }; }; export type Brand = { @@ -50,21 +54,27 @@ export type Brand = { borderRadius: number; borderRadiusSmall: number; }; - identity?: { - name?: string; - tagline?: string; - description?: string; + + // NEW: Identity + identity: { + name: string; // 'RAG Tech' + terminalPath: string; // '~/ragtech' + socialHandle: string; // '@ragtechdev' + website?: string; }; - hosts?: BrandHost[]; - mascot?: BrandMascot; - audio?: { - theme?: string; - stinger?: string; - background?: string; + + // NEW: Team + hosts: BrandHost[]; + + // NEW: Mascot + mascot: BrandMascot; + + // NEW: Media + audio: { + introOutroMusic: string; + backgroundMusic: string; }; - background?: { - image?: string; - video?: string; - pattern?: string; + background: { + episodeGridAssets: string[]; }; }; From 9e602a6d02258c4b10ee4699b3ecc5f0384c61f8 Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Sun, 10 May 2026 10:44:58 +0800 Subject: [PATCH 4/6] docs(brand): fix implementation guide warnings and add sequencing guards Address review warnings in Phase 0.5 brand implementation guide - Update branch name from refactor/p0-brand to refactor/s1-brand-types to match reality - Add sequencing guard warning between Step 1 and Step 2 to prevent runtime errors - Fix status checks for Steps 2 and 4 to use file existence checks: * Step 2: ls brands/ragtech/brand.json (fails if migration incomplete) * Step 4: ls remotion/lib/brandRegistry.ts (fails if registry missing) All status checks now provide real verification per CLAUDE.md requirements. Guide ready for downstream Phase 0.5 implementation with proper dependency ordering. --- .../REFACTOR_P0_BRAND.md | 8 +- .../2026-05-10-refactor-s1-brand-types-r2.md | 84 +++++++++++++++++++ 2 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 docs/review-findings/2026-05-10-refactor-s1-brand-types-r2.md diff --git a/docs/implementation-guides/REFACTOR_P0_BRAND.md b/docs/implementation-guides/REFACTOR_P0_BRAND.md index c042f2c..76615a9 100644 --- a/docs/implementation-guides/REFACTOR_P0_BRAND.md +++ b/docs/implementation-guides/REFACTOR_P0_BRAND.md @@ -4,7 +4,7 @@ description: Phase 0.5 — Brand Abstraction Layer Implementation Guide # Phase 0.5 — Brand Abstraction Layer -**Branch:** `refactor/p0-brand` +**Branch:** `refactor/s1-brand-types` **Goal:** All brand content out of code into config. Adding a new brand = creating files only + VSCode extension improvements. --- @@ -22,6 +22,8 @@ description: Phase 0.5 — Brand Abstraction Layer Implementation Guide tsc --noEmit ``` +> ⚠️ **Sequencing Guard:** Steps 6 and 7 MUST NOT be started before Step 2 is complete. The `Brand` type requires `identity`, `hosts`, `mascot`, `audio`, and `background`, but `public/brand.json` does not yet have these fields. Runtime access before migration will throw. + ### Step 2: Create brand structure and migrate config **Status:** ⏳ PENDING - [ ] Create `brands/ragtech/` directory @@ -30,7 +32,7 @@ tsc --noEmit **Status check:** ```bash -node -e "console.log('Brand loading not yet implemented')" +ls brands/ragtech/brand.json ``` ### Step 3: Move keyword overlays to brand structure @@ -60,7 +62,7 @@ ls -la brands/ragtech/components/ | wc -l # Should show 11 overlay files + inde **Status check:** ```bash -node -e "console.log('Brand registry not yet implemented')" +ls remotion/lib/brandRegistry.ts ``` ### Step 5: Update OverlayRenderer diff --git a/docs/review-findings/2026-05-10-refactor-s1-brand-types-r2.md b/docs/review-findings/2026-05-10-refactor-s1-brand-types-r2.md new file mode 100644 index 0000000..f532630 --- /dev/null +++ b/docs/review-findings/2026-05-10-refactor-s1-brand-types-r2.md @@ -0,0 +1,84 @@ +# Review: refactor/s1-brand-types (round 2) +Date: 2026-05-10 +Reviewer: AI (review-pr skill) — session bias: GENERATOR BIAS on review artifacts only (SKILL.md, CLAUDE.md, prior findings doc); core PR code (brand.ts, implementation guide) is unbiased. Developer approved PROCEED. +PR: NONE (gh CLI unavailable — description provided by developer) + +## Verdict +APPROVED WITH SUGGESTIONS + +## Summary +All four blockers from round 1 are resolved: `BrandHost`, `BrandMascot`, and `Brand` now exactly match the spec in `PRODUCTION_REFACTOR_PLAN.md`; the implementation guide `docs/implementation-guides/REFACTOR_P0_BRAND.md` has been created with all 8 steps. TypeScript is clean, all 244 unit tests pass. Three warnings remain: a branch name inconsistency between the guide and the actual branch, a latent runtime risk from the required fields not yet present in `public/brand.json`, and placeholder status checks on Steps 2 and 4 that always succeed regardless of step completion. + +--- + +## Blockers (must fix before merge) + +None. + +--- + +## Warnings (should address) + +### W1 — Implementation guide branch name doesn't match reality +- **Type:** CONVENTION +- **File:** `docs/implementation-guides/REFACTOR_P0_BRAND.md:7` +- **Finding:** The guide says `Branch: refactor/p0-brand` (from the refactor plan) but the actual branch is `refactor/s1-brand-types`. Future developers resuming work from the guide will search for the wrong branch. +- **Suggestion:** Update line 7 to `**Branch:** \`refactor/s1-brand-types\`` to match reality, or rename the branch. + +### W2 — `public/brand.json` missing new required `Brand` fields; no sequencing guard in guide +- **Type:** QUALITY +- **File:** `public/brand.json` / `docs/implementation-guides/REFACTOR_P0_BRAND.md` +- **Finding:** `Brand` now declares `identity`, `hosts`, `mascot`, `audio`, and `background` as required (non-optional). `public/brand.json` has none of them. TypeScript compilation passes because JSON-at-runtime is not type-checked — but if Steps 6 or 7 are implemented before Step 2 migrates the JSON, every access to `brand.mascot.enabled`, `brand.identity.terminalPath`, `brand.hosts`, etc. will be `undefined` and will throw at runtime. + + Currently no code reads these fields (confirmed by grep — Steps 6–7 are PENDING), so there is no active breakage. The risk is latent. +- **Suggestion:** Add an explicit dependency note to the implementation guide between Step 1 and Step 2, e.g.: + > ⚠️ Steps 6 and 7 MUST NOT be started before Step 2 is complete. The `Brand` type requires `identity`, `hosts`, `mascot`, `audio`, and `background`, but `public/brand.json` does not yet have these fields. Runtime access before migration will throw. + +### W3 — Status checks for Steps 2 and 4 are always-passing no-ops +- **Type:** CONVENTION +- **File:** `docs/implementation-guides/REFACTOR_P0_BRAND.md:32-34` and `:63-65` +- **Finding:** CLAUDE.md mandates: *"Each step must have a Status check — a single command or file-existence test that confirms it is done."* The current checks are: + - Step 2: `node -e "console.log('Brand loading not yet implemented')"` — exits 0 whether or not Step 2 is done + - Step 4: `node -e "console.log('Brand registry not yet implemented')"` — same issue + + These will report success on a machine where neither step has been started. A real status check for Step 2 would be `ls brands/ragtech/brand.json`. For Step 4: `ls remotion/lib/brandRegistry.ts`. +- **Suggestion:** Replace the placeholder `node -e` commands with file-existence checks that actually fail when the step is incomplete. Example: + - Step 2: `ls brands/ragtech/brand.json` + - Step 4: `ls remotion/lib/brandRegistry.ts` + +--- + +## Suggestions + +- Step 1's status check (`tsc --noEmit`) is correct and sufficient for a type-only step. Good baseline. +- Steps 3, 5, 6, 7, 8 all have real, specific grep/ls status checks that will fail if the step hasn't been done. Steps 2 and 4 are the only outliers. + +--- + +## Test plan verification + +| Item | Status | Notes | +|------|--------|-------| +| `tsc --noEmit` clean | PASS | Zero errors | +| `npm test` passes | PASS | 244 passed, 2 skipped, 9 suites | +| `npm run test:react` | PRE-EXISTING FAIL | No React test files exist in the project; not caused by this PR | +| Brand type spec parity check | PASS | `BrandHost`, `BrandMascot`, `Brand` exactly match `PRODUCTION_REFACTOR_PLAN.md:273-323` | +| No runtime access of new required fields | PASS | Grep confirms nothing reads `brand.identity/hosts/mascot/audio/background` yet | +| Out-of-scope inventory file removed | PASS | `docs/REFACTOR_ISSUE_INVENTORY.md` not in diff | + +--- + +## Round 1 blockers resolved + +| Blocker | Resolution | +|---------|-----------| +| B1 — `BrandHost` missing `role`, `imgSrc`, `nameBgColor` | ✅ All three fields present as required strings | +| B2 — `BrandMascot` missing `enabled`, `assets` | ✅ Both present; `assets` has full optional-field map + index signature | +| B3 — `Brand` field names / optionality diverged from spec | ✅ All new fields use exact spec names and are required (non-optional) | +| B4 — No implementation guide | ✅ `docs/implementation-guides/REFACTOR_P0_BRAND.md` created with all 8 steps | + +--- + +## Patterns observed + +No new patterns. W2 (latent runtime mismatch from type widening without data migration) may recur in later phases — watching. From a2bb6f150ff0561129b2fc3a1327623b8c5ef024 Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Sun, 10 May 2026 11:39:40 +0800 Subject: [PATCH 5/6] chore(hooks): enforce four gap patterns caught only in review-pr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added two pre-commit guards and one pre-push guard so these patterns are blocked at commit/push time rather than surfaced for the first time during code review: - Pre-commit: scripts/**/*.test.ts files must not use os.tmpdir / fs.mkdtempSync / mkdtemp (integration tests belong in tests/integration/) - Pre-commit: test files must not call Math.random(), Date.now(), or crypto random APIs without seeding (non-deterministic → flaky tests) - Pre-push: new mkdirSync/mkdir calls must target directories already in .gitignore (check-runtime-dirs.cjs; simple string literals only, skips template literals and /tmp paths to avoid false positives) These patterns were previously first detected by review-pr's Step 8. Moving enforcement upstream reduces review token cost and catches mistakes before they accumulate across multiple commits. --- .husky/check-runtime-dirs.cjs | 58 +++++++++++++++++++++++++++++++++++ .husky/pre-commit | 38 +++++++++++++++++++++++ .husky/pre-push | 8 +++++ 3 files changed, 104 insertions(+) create mode 100644 .husky/check-runtime-dirs.cjs diff --git a/.husky/check-runtime-dirs.cjs b/.husky/check-runtime-dirs.cjs new file mode 100644 index 0000000..d560682 --- /dev/null +++ b/.husky/check-runtime-dirs.cjs @@ -0,0 +1,58 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Checks that any new mkdirSync/mkdir calls in the diff target directories that + * are already covered by .gitignore. Runtime-generated directories must never be + * committed — but they frequently are when a new module creates one and .gitignore + * is not updated in the same PR. + * + * Only inspects simple string-literal paths (single or double quoted). Template + * literals and variable-based paths are skipped to avoid false positives. + */ + +const { execSync } = require('child_process'); +const fs = require('fs'); + +const gitignore = fs.existsSync('.gitignore') ? fs.readFileSync('.gitignore', 'utf8') : ''; + +let diff; +try { + diff = execSync("git diff origin/main...HEAD -- '*.ts' '*.tsx' '*.js'", { + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'], + }); +} catch { + // No origin/main yet (first push) or diff failed — skip + process.exit(0); +} + +const addedLines = diff.split('\n').filter(l => l.startsWith('+') && !l.startsWith('+++')); +const mkdirLines = addedLines.filter(l => /(mkdirSync|mkdir)\s*\(/.test(l)); + +const failures = []; +for (const line of mkdirLines) { + const m = line.match(/(?:mkdirSync|mkdir)\s*\(\s*['"]([^'"]+)['"]/); + if (!m) continue; + + const dir = m[1]; + // Skip absolute paths and dynamic values + if (/^\/tmp|^\/var|\$\{|process\./.test(dir)) continue; + + const root = dir.replace(/^\.\//, '').split('/')[0]; + if (!root) continue; + + // Check if root directory (or its dotted variant) appears in .gitignore + const escaped = root.replace(/\./g, '\\.'); + const covered = new RegExp('(^|\\n)/?' + escaped + '(/|$|\\n)').test(gitignore); + if (!covered) { + failures.push(` mkdirSync('${dir}') → add '${root}/' to .gitignore`); + } +} + +if (failures.length > 0) { + console.error('✗ New runtime directories not covered by .gitignore:'); + failures.forEach(f => console.error(f)); + console.error(' Runtime-generated directories must never be committed.'); + process.exit(1); +} diff --git a/.husky/pre-commit b/.husky/pre-commit index 38acbdb..412e1da 100644 --- a/.husky/pre-commit +++ b/.husky/pre-commit @@ -21,3 +21,41 @@ if [ -n "$STAGED_TESTABLE" ]; then echo "▶ jest --findRelatedTests" echo "$STAGED_TESTABLE" | xargs npx jest --findRelatedTests --passWithNoTests --no-coverage fi + +# ── 4. Integration test placement ────────────────────────────────────────────── +# Test files under scripts/ must be pure (no real filesystem I/O). A test that +# calls os.tmpdir(), fs.mkdtempSync(), or mkdtemp() is an integration test and +# must live in tests/integration/ — not next to the source file in scripts/. +STAGED_SCRIPT_TESTS=$(git diff --cached --name-only | grep -E '^scripts/.*\.test\.(ts|js)$' || true) +if [ -n "$STAGED_SCRIPT_TESTS" ]; then + echo "▶ checking scripts/ test files for real I/O" + BAD_TESTS="" + while IFS= read -r f; do + if git show ":$f" 2>/dev/null | grep -qE 'os\.tmpdir|fs\.mkdtempSync|mkdtemp'; then + BAD_TESTS="${BAD_TESTS} ${f}\n" + fi + done <<< "$STAGED_SCRIPT_TESTS" + if [ -n "$BAD_TESTS" ]; then + printf "✗ Real I/O in scripts/ test file(s) — move to tests/integration/ instead:\n%s" "$BAD_TESTS" + exit 1 + fi +fi + +# ── 5. Non-deterministic test inputs ────────────────────────────────────────── +# Test files must not use non-seeded random sources. Math.random(), Date.now(), +# and crypto random APIs produce flaky tests whose failures are timing-dependent +# and hard to reproduce. Use fixed seeds or static data instead. +STAGED_TESTS=$(git diff --cached --name-only | grep -E '\.test\.(ts|tsx|js)$' || true) +if [ -n "$STAGED_TESTS" ]; then + echo "▶ checking test files for non-deterministic random sources" + BAD_RANDOM="" + while IFS= read -r f; do + if git show ":$f" 2>/dev/null | grep -qE 'Math\.random\(\)|Date\.now\(\)|crypto\.randomUUID\(\)|crypto\.getRandomValues'; then + BAD_RANDOM="${BAD_RANDOM} ${f}\n" + fi + done <<< "$STAGED_TESTS" + if [ -n "$BAD_RANDOM" ]; then + printf "✗ Non-deterministic random source in test file(s) — use fixed seeds or static data:\n%s" "$BAD_RANDOM" + exit 1 + fi +fi diff --git a/.husky/pre-push b/.husky/pre-push index 42bf13e..62d3b4c 100755 --- a/.husky/pre-push +++ b/.husky/pre-push @@ -85,6 +85,14 @@ AUDIT_OUTPUT=$(npm audit --audit-level=moderate 2>&1) || { fi } +# ── 7. Runtime directories in .gitignore ─────────────────────────────────────── +# New code that creates directories at runtime must reference paths covered by +# .gitignore. Without this, generated content lands in committed files on the +# next `git add .`. The check only inspects simple string literals — template +# literals and variable-based paths are skipped to avoid false positives. +echo "▶ checking new runtime directory creation against .gitignore" +node .husky/check-runtime-dirs.cjs + # ── Phase-gated guards ───────────────────────────────────────────────────────── # Uncomment each block after its phase merges to main. These prevent agents from # reintroducing patterns that a phase explicitly removed. From 8c0e904c9b3742c8016d9e894bd087b9ded467af Mon Sep 17 00:00:00 2001 From: Natasha Ann Date: Sun, 10 May 2026 11:39:55 +0800 Subject: [PATCH 6/6] chore(skills): shift gap-pattern enforcement upstream; trim review-pr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Patterns that can be checked mechanically now live at the earliest possible workflow stage instead of being caught for the first time in review-pr: implement-issue (Step 5d): Added explicit type-spec check — any new/modified type must be compared field-by-field against docs/PRODUCTION_REFACTOR_PLAN.md before committing. Diverging silently breaks downstream phases that reference field names. implement-issue (Step 5e): Added staging-scope guard — run git diff --cached --name-only after staging and remove any out-of-scope files lint-staged may have auto-fixed. pre-push-audit (new Step 4b-2 + 4c): 4b-2: scope verification — flag files in the diff not mentioned in the PR body (lint-staged side-effect detection). 4c: type specification check — mirrors the implement-issue check but runs across all commits in the branch before the push gate clears. review-pr (Known Gap Patterns): Removed the four patterns now enforced upstream: • Integration test in scripts/ → pre-commit hook • Runtime dir not in .gitignore → pre-push hook • Lint-staged scope creep → implement-issue + pre-push-audit • Non-deterministic test inputs → pre-commit hook Added a comment block explaining the removal so the history is preserved. Kept the spec-divergence pattern (pattern 5) — it requires semantic reasoning that hooks cannot replace. --- .claude/skills/implement-issue/SKILL.md | 15 +++++++ .claude/skills/pre-push-audit/SKILL.md | 53 ++++++++++++++++++++++++- .claude/skills/review-pr/SKILL.md | 35 ++++------------ 3 files changed, 75 insertions(+), 28 deletions(-) diff --git a/.claude/skills/implement-issue/SKILL.md b/.claude/skills/implement-issue/SKILL.md index d106acd..0f2f982 100644 --- a/.claude/skills/implement-issue/SKILL.md +++ b/.claude/skills/implement-issue/SKILL.md @@ -231,8 +231,23 @@ npx tsc --noEmit Fix all type errors before committing. +**Type specification check:** If this commit adds or modifies any TypeScript `type` or `interface`, search `docs/PRODUCTION_REFACTOR_PLAN.md` for the same type name and compare field by field: +- Field names must match exactly (the spec uses specific names that downstream phase steps reference by name) +- Required vs. optional status must match (`field:` vs `field?:`) +- Field types must match + +If the spec must change, update it first in a separate commit (`docs(refactor-plan): ...`) and get agreement before the implementation commit. Diverging silently breaks future phases. + ### 5e. Commit +**Before staging:** run `git diff --cached --name-only` after staging your intended files. lint-staged may auto-fix and silently re-stage files outside this commit's scope. Remove any out-of-scope files before committing: + +```bash +git restore --staged +``` + +Only files listed in this commit's plan should appear in `git diff --cached --name-only`. + ```bash git add # never git add -A git commit -m "$(cat <<'EOF' diff --git a/.claude/skills/pre-push-audit/SKILL.md b/.claude/skills/pre-push-audit/SKILL.md index d0afd8d..4d42d14 100644 --- a/.claude/skills/pre-push-audit/SKILL.md +++ b/.claude/skills/pre-push-audit/SKILL.md @@ -229,7 +229,58 @@ EOF )" ``` -### 4c — Run the full test suite +### 4b-2 — Scope verification (lint-staged side effects) + +After test gaps are resolved, verify that no files outside the PR's intended scope were silently added to the diff by lint-staged auto-fixes during pre-commit hooks. + +```bash +git diff --name-only origin/main...HEAD +``` + +Compare this list against the file scope described in the PR body (from Step 0). Any file in the diff that is not mentioned in the PR's scope is a candidate for an unintended lint-staged modification. For each unexpected file: + +1. Confirm whether the change is cosmetic (whitespace, comment, trailing comma) — if yes, it was likely auto-fixed by lint-staged during a prior commit. +2. If cosmetic and out-of-scope, record: + +``` +[QUALITY] Out-of-scope file in diff (lint-staged side effect) + File: + Change: + Verdict: WARNING — squash or drop this change; it obscures the PR's actual diff. +``` + +### 4c — Type specification check + +For each new or modified `type` or `interface` in the diff, verify it matches `docs/PRODUCTION_REFACTOR_PLAN.md` exactly. Downstream phase steps reference type field names by name — divergence breaks future phases silently. + +```bash +# Find new or changed type/interface declarations in the diff +git diff origin/main...HEAD -- '*.ts' '*.tsx' | grep '^\+' | grep -E '^\+\s*(export\s+)?(type|interface)\s+' | grep -v '^+++' +``` + +For each type name found, search the refactor plan: + +```bash +grep -n "" docs/PRODUCTION_REFACTOR_PLAN.md +``` + +Compare field by field: +- Field names (exact match — `videoSrc` ≠ `src`) +- Required vs optional (`field:` vs `field?:`) +- Field types + +If the implementation diverges from the spec **without a corresponding spec update in this same diff**, record: + +``` +[QUALITY] Type diverges from PRODUCTION_REFACTOR_PLAN.md spec + Type: + File: + Spec: docs/PRODUCTION_REFACTOR_PLAN.md: + Divergence: + Verdict: BLOCKER — update the spec first, then align the implementation. +``` + +### 4d — Run the full test suite ```bash npm test diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index b928450..03ea633 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -663,33 +663,14 @@ _This section is populated automatically by Step 8c as patterns are observed in -### Integration test placed in scripts/ instead of tests/integration/ -**Category:** CONVENTION -**Trigger:** A new test file in `scripts/` uses `os.tmpdir()`, `fs.mkdtempSync`, or any real filesystem read/write in its test body or setup. -**Check:** `grep -rn "mkdtemp\|os\.tmpdir\|fs\.mkdtempSync" scripts/**/*.test.ts` — any match means the test uses real I/O and must live in `tests/integration/` not next to the source file. -**Verdict:** BLOCKER -**First seen:** refactor/s1-project-file — 2026-05-09 - -### Runtime directory not added to .gitignore -**Category:** QUALITY -**Trigger:** A new module creates a directory under `process.cwd()` (or the project root) that is intended to hold generated/runtime files (artifacts, run logs, cache) rather than source code. -**Check:** For every directory path written by new code (grep `mkdirSync` or `mkdir` in the diff), verify the directory appears in `.gitignore`. Common offenders: `.ragtech/`, `runs/`, `cache/`, `artifacts/`. -**Verdict:** BLOCKER -**First seen:** refactor/s1-artifacts — 2026-05-09 - -### Lint-staged sweeping out-of-scope files into the commit -**Category:** QUALITY -**Trigger:** The diff contains changes to files not mentioned in the PR description or implementation doc — typically cosmetic comment moves or eslint-disable removals in unrelated source files. -**Check:** Compare `git diff --name-only origin/main...HEAD` against the file list in the PR description. Any file in the diff that is not in the PR's "Files changed" table is a scope violation. Common cause: lint-staged running `eslint --fix` on all staged files during the pre-commit hook, auto-modifying files the developer didn't intend to change. -**Verdict:** BLOCKER -**First seen:** fix/pre-existing-failing-test-suites — 2026-05-09 - -### Non-deterministic test setup using Math.random() -**Category:** COVERAGE -**Trigger:** A test file uses `Math.random()` (or `Date.now()`, `crypto.randomUUID()`, or any other non-seeded random source) inside a `test()` or `it()` block to construct input data for the function under test. -**Check:** `grep -n "Math\.random\|Date\.now\|crypto\.random" scripts/**/*.test.{js,ts} app/**/*.test.{ts,tsx} remotion/**/*.test.{ts,tsx}` — any match inside a test body (not a mock implementation) is a candidate. Then verify whether the assertions check specific output values; if assertions are only type-level (`typeof`, `toHaveProperty`, `toBeDefined`), flag as BLOCKER. -**Verdict:** BLOCKER -**First seen:** refactor/s1-audiosync-determinism — 2026-05-09 + ### Implementation diverges from documented spec without updating the spec **Category:** QUALITY