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 51c295f..03ea633 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -663,30 +663,18 @@ _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 + + +### Implementation diverges from documented spec without updating the spec **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. +**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-audiosync-determinism — 2026-05-09 +**First seen:** refactor/s1-brand-types — 2026-05-10 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. 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. diff --git a/docs/implementation-guides/REFACTOR_P0_BRAND.md b/docs/implementation-guides/REFACTOR_P0_BRAND.md new file mode 100644 index 0000000..76615a9 --- /dev/null +++ b/docs/implementation-guides/REFACTOR_P0_BRAND.md @@ -0,0 +1,130 @@ +--- +description: Phase 0.5 — Brand Abstraction Layer Implementation Guide +--- + +# Phase 0.5 — Brand Abstraction Layer + +**Branch:** `refactor/s1-brand-types` +**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 +``` + +> ⚠️ **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 +- [ ] 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 +ls brands/ragtech/brand.json +``` + +### 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 +ls remotion/lib/brandRegistry.ts +``` + +### 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-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. 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 62c8297..23d6dbd 100644 --- a/remotion/types/brand.ts +++ b/remotion/types/brand.ts @@ -25,6 +25,27 @@ export type BrandTypography = { }; }; +export type BrandHost = { + name: string; + role: string; + imgSrc: string; // relative to brands/{brandId}/ + nameBgColor: string; +}; + +export type BrandMascot = { + enabled: boolean; + name: string; + assets: { + holdingMic?: string; + teacher?: string; + raisingHand?: string; + holdingLaptop?: string; + holdingLaptop2?: string; + sparkleEyes?: string; + [key: string]: string | undefined; + }; +}; + export type Brand = { colors: BrandColors; typography: BrandTypography; @@ -33,4 +54,27 @@ export type Brand = { borderRadius: number; borderRadiusSmall: number; }; + + // NEW: Identity + identity: { + name: string; // 'RAG Tech' + terminalPath: string; // '~/ragtech' + socialHandle: string; // '@ragtechdev' + website?: string; + }; + + // NEW: Team + hosts: BrandHost[]; + + // NEW: Mascot + mascot: BrandMascot; + + // NEW: Media + audio: { + introOutroMusic: string; + backgroundMusic: string; + }; + background: { + episodeGridAssets: string[]; + }; };