# 2026-05-05 Current Repo Full Code Review

## Metadata

- Review date: 2026-05-05
- Reviewed HEAD: `51685d4ec9e8498e1cffecaca49cf57338e93806`
- Worktree before this document: no modified tracked files; untracked `.wave_1b_timestamp.txt`, `error.log`
- Scope: current repository source surface, with focused review on tracked runtime/test/prompt code:
  - `backend/app`
  - `backend/tests`
  - `frontend/src`
  - `scripts/canary`
  - `prompts/_base`
  - key schema/version/config files under `backend/alembic`, `frontend`, and root config files
- Source inventory reviewed:
  - `git ls-files backend/app frontend/src scripts/canary prompts/_base backend/tests`: 945 tracked files
  - source-like files in `backend/app frontend/src scripts prompts/_base backend/tests`: 944 files / 162,708 lines
  - top-level tracked split: `backend/app` 229, `backend/tests` 192, `frontend/src` 84, `prompts/_base` 418, `scripts/canary` 22

## Verification Commands

```bash
git status --short
git rev-parse HEAD
rg --files backend/app frontend/src scripts/canary prompts/_base backend/tests -g '!**/__pycache__/**'
backend/.venv/bin/python -m compileall -q backend/app scripts/canary
backend/.venv/bin/python -m pytest backend/tests -q
npm run build  # from frontend/
```

Results:

- Python compile check: passed.
- Backend full test suite: failed.
  - `2536 passed, 35 skipped, 4 deselected, 1 xfailed, 20 failed`
  - Runtime: `373.06s`
- Frontend build: failed.
  - TypeScript global redeclaration in `frontend/src/test-setup.ts`
  - Type-only import violations in `frontend/src/test-utils.tsx`

## Executive Verdict

`NEEDS_REVISION`.

The current G4.5a / RenderPromptCard path looks mostly coherent at the prompt-contract level: v20 is wired in `detail_steps.py`, `version_registry.py`, prompt v20 exists, spatial consistency is present, and G4.5a canary naming/strictness is broadly in place. The repository is not release-clean as a whole because full backend tests fail, frontend build fails, and several runtime paths still have silent fallback or persistence-order risks.

## Blocking

### B1. Frontend production build currently fails

Evidence:

- `frontend/package.json:8` defines build as `tsc -b && vite build`.
- `frontend/src/test-setup.ts:29-35` redeclares `__APP_VERSION__`, `__BUILD_DATE__`, `__BUILD_TAG__`.
- `frontend/src/vite-env.d.ts:3-5` already declares the same globals.
- `frontend/src/test-utils.tsx:5` imports `ReactElement` / `ReactNode` as value imports.
- `frontend/src/test-utils.tsx:8` imports `RenderOptions` / `RenderHookOptions` as value imports.

Observed build errors:

```text
src/test-setup.ts(31,9): error TS2451: Cannot redeclare block-scoped variable '__APP_VERSION__'.
src/test-setup.ts(33,9): error TS2451: Cannot redeclare block-scoped variable '__BUILD_DATE__'.
src/test-setup.ts(35,9): error TS2451: Cannot redeclare block-scoped variable '__BUILD_TAG__'.
src/test-utils.tsx(5,10): error TS1484: 'ReactElement' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
src/test-utils.tsx(5,24): error TS1484: 'ReactNode' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
src/test-utils.tsx(8,18): error TS1484: 'RenderOptions' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
src/test-utils.tsx(8,45): error TS1484: 'RenderHookOptions' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
```

Impact:

- Any CI or deployment path that runs `npm run build` will stop before producing a frontend bundle.
- This is repo-wide blocking even if runtime app code is unaffected, because the declared build command is broken.

Required action:

- Remove duplicate global declarations from `test-setup.ts`, or make them module-scoped without redeclaring globals already in `vite-env.d.ts`.
- Change test utility imports to `import type { ReactElement, ReactNode } from 'react'` and `import type { RenderOptions, RenderHookOptions } from '@testing-library/react'`.

### B2. Backend full test suite fails under current DB path invariant

Evidence:

- `backend/app/models/project.py:149-158` defines `ck_image_asset_file_path_relative`, rejecting `ImageAsset.file_path` values that start with `/`.
- `backend/alembic/versions/005_file_path_relative_check.py:29-34` defines the same check.
- `backend/app/core/database.py:157-173` adds the same check during startup migration.
- `backend/app/core/file_paths.py:136-175` keeps root-external absolute paths unchanged for legacy/test compatibility.
- `backend/.venv/bin/python -m pytest backend/tests -q` failed with 20 tests.

Representative failure:

```text
psycopg2.errors.CheckViolation: new row for relation "image_asset" violates check constraint "ck_image_asset_file_path_relative"
DETAIL: Failing row contains (..., /tmp/test_image.png, ...)
```

Failed groups:

- `backend/tests/test_image_validator.py`
- `backend/tests/test_images_api.py`
- `backend/tests/test_moderation.py::TestTraceAPI`
- `backend/tests/test_pipeline_v3_e2e.py::TestSyncIntegration`
- `backend/tests/test_variation_pipeline.py`

Assessment:

- The DB invariant itself is correct and intentional.
- The test helpers still insert absolute tmp paths outside `settings.projects_dir` / `_resolve_root()`.
- This makes the current full backend suite non-green and hides whether the image API tests have real behavioral regressions beyond fixture drift.

Required action:

- Update test fixture helpers to create image files under the configured project root or store paths through the same relative-path path used by production producers.
- After fixture repair, rerun full backend tests and reclassify any remaining failures.

## Important

### I1. Manual image upload accepts arbitrary bytes and unbounded file size

Evidence:

- `backend/app/api/v1/images.py:457-480` reads the entire upload into memory with `await file.read()`.
- `backend/app/services/image_upload_service.py:104-113` writes the bytes directly to disk.
- `backend/app/services/image_upload_service.py:111-112` derives only the suffix from the supplied filename.
- `backend/app/api/v1/images.py:189-192` later serves full files as `image/png` regardless of actual content.

Impact:

- Non-image payloads can be registered as `ImageAsset` rows.
- Large uploads can consume process memory because there is no server-side size cap.
- Downstream PIL/image consumers may fail later, far from the upload boundary.

Required action:

- Add backend-side size limit for image uploads.
- Validate actual image bytes using PIL or a strict image header check before writing/registering.
- Normalize/allowlist extensions and content types.
- Mirror planning-doc behavior, which already checks suffix and 200MB limit in `backend/app/api/v1/projects.py:210-219`.

### I2. Shot selection commits DB before checkpoint writes and then degrades to warnings

Evidence:

- `backend/app/services/shot_selection_service.py:107-113` updates DB, marks downstream stale, commits, then writes checkpoint files.
- `backend/app/services/shot_selection_service.py:190-222` documents that file-write failure happens after DB commit and returns only warnings.

Impact:

- A filesystem failure can leave DB state changed while `shot_selection` manifest and resume-sensitive checkpoint status remain stale or old.
- API response can still be `ok: True` with warnings, which is easy for UI or operators to miss.
- This is exactly the kind of DB/checkpoint split-brain that later resume/force logic has to compensate for.

Required action:

- Prefer writing checkpoint files to temporary paths first, then commit DB, then atomically promote files.
- If that is too invasive, return non-OK / `AppError` when the primary `shot_selection` manifest write fails. Downstream stale marker failures can remain warnings if explicitly accepted.

### I3. Scene image prompt loading still has legacy silent fallback from malformed `scene_detail`

Evidence:

- `backend/app/services/scene_checkpoint_loaders.py:274-281` returns `[]` when `scene_detail` manifest parse fails.
- `backend/app/services/scene_checkpoint_loaders.py:283-303` returns `[]` when no matching scene/shot is found.
- `backend/app/services/scene_generation_coordinator.py:703-710` falls back to `still_data["still_frame_prompt"]` when the loaded variation list is empty.

Impact:

- A corrupted or drifted `scene_detail` checkpoint can silently generate scene images from the legacy still prompt instead of the contract-rich `t2i_variations`.
- G3/G4 evidence, owned-object sentinel data, and RenderPromptCard-derived prompt constraints may be bypassed on this path.

Required action:

- For current G4 schema checkpoints, distinguish legacy absence from malformed/empty contract data.
- Treat parse failure or pinned scene/shot miss as a contract failure for selected shot generation, not as `[]`.
- Keep fallback only for explicitly identified legacy rows.

### I4. `load_shot_t2i_variations()` uses `scene_index or still_index`

Evidence:

- `backend/app/services/scene_checkpoint_loaders.py:262-263` documents the behavior.
- `backend/app/services/scene_checkpoint_loaders.py:283` implements `target_si = scene_index or still_index`.

Impact:

- If a caller ever passes `scene_index=0`, it is treated as missing and replaced by `still_index`.
- The comment says this preserves old behavior, so this is not an immediate bug if scene indexes are guaranteed 1-based. It is still a brittle contract edge.

Required action:

- Replace truthiness with explicit `scene_index if scene_index is not None else still_index`.
- If 0 is invalid by domain, enforce that with validation instead of truthiness.

### I5. `render_prompt_card.build_render_strategy()` still contains public-helper silent absorbs

Evidence:

- `backend/app/core/steps/render_prompt_card.py:868-874` types `shot_info` as required `Dict[str, Any]`.
- `backend/app/core/steps/render_prompt_card.py:901-917` uses `(shot_info or {})`.
- `backend/app/core/steps/detail_steps.py:1920-1934` protects production legacy scene-level calls by synthesizing `{"staging_not_applicable": True}` before calling the builder.
- `backend/tests/unit/test_render_prompt_card_integration.py:440-472` verifies that the helper path distinguishes shot path vs legacy path.

Assessment:

- Production callsites are guarded.
- The helper itself is weaker than its type/contract and can silently accept `shot_info=None` if called directly.

Required action:

- Add an early `if not isinstance(shot_info, dict): raise AppError(...)` inside `build_render_strategy()`.
- Use direct dict access for required keys or explicit optional-key branches.

### I6. Internal exception messages are returned to clients in selected API paths

Evidence:

- `backend/app/api/deps.py:142-148` wraps unexpected exceptions as `AppError(code="internal_error", message=f"내부 오류: {exc}", status_code=500)`.
- `backend/app/core/errors.py:12-16` returns `exc.message` in the JSON response.
- `backend/app/api/v1/steps.py:75-79` returns `message=str(exc)` when reading a checkpoint fails.

Impact:

- Filesystem paths, JSON parser details, DB messages, or other internal exception text can leak to authenticated clients.
- This is not as severe as public unauthenticated leakage because affected routes require auth/project access, but it is still an avoidable information disclosure pattern.

Required action:

- Log exception details server-side.
- Return generic user-safe messages for `internal_error` and checkpoint read failures.
- Keep structured error codes for UI branching.

### I7. Entity image upload in the frontend bypasses the shared API wrapper

Evidence:

- `frontend/src/api/client.ts:11-43` provides timeout, credentials, JSON error parsing, and `ApiError`.
- `frontend/src/pages/Entities.tsx:350-360` uses raw `fetch()` for upload.
- Unlike `frontend/src/hooks/api/mutations/useCreateEpisode.ts:34-50`, this upload path has no `AbortController`, no `res.ok` check, and no structured error handling.

Impact:

- HTTP 4xx/5xx upload responses are treated as success unless the network request itself rejects.
- The UI proceeds to refresh and regenerate composites even after a failed upload.

Required action:

- Use the shared `api()` wrapper for FormData, or copy the episode upload pattern with timeout and explicit `res.ok` handling.

## Minor / Operational Notes

### M1. Untracked files are present

Evidence:

```text
?? .wave_1b_timestamp.txt
?? error.log
```

Required action:

- Decide whether these are intentional local artifacts.
- Add to `.gitignore` if recurring, or remove before commit if accidental.

### M2. Prompt loader strict version-pack mode is still opt-in

Evidence:

- `backend/app/core/config.py:78-80` sets `prompt_version_pack_strict=False`.
- `backend/app/modules/prompt_loader.py` warns by default and only raises in strict mode.

Assessment:

- This appears intentional: strict mode can be enabled operationally.
- For prompt-contract migration work, default leniency means alignment tests and version registry checks remain important.

### M3. Export HTML modal uses `innerHTML`, but current source content is escaped

Evidence:

- `backend/app/services/export_service.py:759-769` builds modal metadata from `_html_escape(m)`.
- `backend/app/services/export_service.py:791-808` escapes scene paragraphs/headings.
- `backend/app/services/export_service.py:910-913` copies `.meta` content into the modal with `innerHTML`.

Assessment:

- Current `.meta` content is escaped before insertion, so I did not classify this as a vulnerability.
- If future `.meta` fields include raw LLM/user HTML, this becomes an XSS risk.

Required action:

- Prefer DOM text assignment for future additions, or preserve the current invariant that `.meta` only contains escaped/generated markup.

## G4.5a / Prompt Contract Review

Verified OK:

- `backend/app/core/steps/detail_steps.py:104-105`
  - `SCENE_DETAIL_SCHEMA_VERSION = 7`
  - `SCENE_DETAIL_PROMPT_VERSION = "20.202605051240"`
- `backend/app/core/version_registry.py:34`
  - `scene_detail_composer = "1.20.0"`
- `backend/app/core/version_registry.py:125-127`
  - prompt dependency is `scene_detail/v20`
- `prompts/_base/scene_detail/20.202605051240/system.md`
  - 423 lines
  - 23 `##` headings
  - RenderPromptCard primary contract at lines 3-17
  - spatial consistency section at lines 144-155
- `backend/app/core/steps/render_prompt_card.py:437-470`
  - spatial consistency required-key sets exist for camera frame, fg/bg shared anchor, primary framing.
- `backend/app/core/steps/render_prompt_card.py:578-820`
  - spatial consistency dict builder places `body_part_focus_cross_ref` under primary framing close/wide rules, not under camera frame.
- `backend/app/core/steps/render_prompt_card.py:3060-3083`
  - `_card_metadata.lift_status` includes G4.2/G4.3/G4.4/G4.5a keys.
- `scripts/canary/g4_5a_token_count.py:15-19`
  - token script is documented CP-free.
- `scripts/canary/g4_5a_primary_framing.py:402-406`
  - candidate role exits `1` on primary framing violation count > 0.

Residual risk:

- The active scene_detail prompt is still 423 lines, not the earlier 250-line aspirational target.
- This is expected after G4.5a spatial lift, but G4.5b/c should continue moving large legacy prose blocks into deterministic card fields or compressing them with gates.

## API / Security Review

Verified OK:

- Auth is cookie-backed with `httponly=True`, `samesite="lax"`, and `secure` set for HTTPS in `backend/app/api/v1/auth.py:18-25`.
- Production rejects insecure secret/default password/default DB settings in `backend/app/core/config.py:101-140`.
- Project access is centralized in `backend/app/api/deps.py:81-116`.
- File download routes have path traversal guards:
  - image files: `backend/app/api/v1/images.py:151-159`
  - exports: `backend/app/api/v1/exports.py:142-150`
- Project member routes delegate owner/admin checks to `ProjectService`:
  - endpoints: `backend/app/api/v1/projects.py:326-376`
  - service checks: `backend/app/services/project_service.py:265-381`

Risks:

- Manual image upload needs backend byte/type/size validation.
- Internal exception messages are exposed on selected routes.
- Export response header manually includes `filename` in `backend/app/api/v1/exports.py:161-165`; path traversal is guarded, but consider relying on `FileResponse(filename=...)` sanitization rather than a hand-built `Content-Disposition` header.

## Persistence / Checkpoint Review

Verified OK:

- `StepRunner` resume mismatch behavior is strict: schema/config mismatch escalates to force-like rerun.
- `StepRunner` verifies before final save and marks partial on verification failure.
- DB relative path invariant is enforced by model, startup migration, and Alembic.

Risks:

- Shot selection DB/checkpoint write ordering can split state.
- Scene image prompt loading still has legacy empty-list fallback on malformed `scene_detail`.
- Full backend tests are currently blocked by test fixtures that violate the new `ImageAsset.file_path` invariant.

## Frontend Review

Blocking:

- `npm run build` fails due test global redeclaration and type-only import violations.

Risks:

- Entity image upload bypasses the shared API client and ignores non-OK HTTP status.

Verified OK:

- Shared API client has credential inclusion, timeout, and structured `ApiError`.
- Episode upload already has an explicit 120s timeout and non-OK handling, which is the pattern entity image upload should follow.

## Test / Canary Review

Verified:

- Python source compile passed for `backend/app` and `scripts/canary`.
- G4.5a canary script names match the expected family:
  - `g4_5a_camera_frame_consistency.py`
  - `g4_5a_fg_bg_shared_anchor.py`
  - `g4_5a_primary_framing.py`
  - `g4_5a_view_mixing_extension.py`
  - `g4_5a_token_count.py`
- G4.5a primary framing canary has strict candidate exit behavior.
- Token-count canary avoids importing `_g4_5a_common`, preserving CP-free behavior.

Not clean:

- Backend full test suite fails with 20 failures.
- Frontend build fails before Vite bundle generation.

## Recommended Fix Order

1. Fix frontend build failures.
2. Repair backend image-path test fixtures so the full suite can validate product behavior again.
3. Add backend validation for manual image uploads: size, content type, actual image decode, extension allowlist.
4. Change entity image upload frontend path to use `api()` or a proper timeout/status-checking fetch wrapper.
5. Fix shot selection DB/checkpoint ordering or return hard failure on primary manifest write failure.
6. Harden `load_shot_t2i_variations()` for current G4 schema: malformed/missing selected-shot variations should fail, not silently fall back.
7. Harden `build_render_strategy()` against direct `shot_info=None` calls.
8. Sanitize internal error responses.

## Appendix: Broad Static Scan Summary

Patterns scanned across `backend/app`, `frontend/src`, `scripts/canary`, `prompts/_base`, and `backend/tests`:

- silent fallback shapes: `or []`, `or {}`, `.get(..., [])`, `.get(..., {})`
- broad exception handlers: `except Exception`, `pass`, warning-only failure paths
- file IO and persistence: `json.loads`, `read_text`, `write_text`, `atomic_write_json`, `commit`, `rollback`
- browser/security sinks: `innerHTML`, `dangerouslySetInnerHTML`, direct `fetch`, `localStorage`, `sessionStorage`
- process/security sinks: `eval`, `exec`, `subprocess`, `shell=True`, `pickle`, `yaml.load`
- prompt/version anchors: scene_detail prompt version, version registry, prompt loader strictness

Most grep hits were expected in legacy modules, tests, prompt docs, or deliberate compatibility paths. The findings above are the ones that still connect to current runtime, build, or test reliability.