feat: config profiles, integrations system, and DaVinci Resolve export#1
Open
nmbrthirteen wants to merge 3 commits into
Open
feat: config profiles, integrations system, and DaVinci Resolve export#1nmbrthirteen wants to merge 3 commits into
nmbrthirteen wants to merge 3 commits into
Conversation
Replace ad-hoc `os.path.join(__file__, "..", "..", ".podcli", ...)` patterns across the Python backend with a single `config.paths` module that resolves home/data/cache locations from env vars (PODCLI_HOME, PODCLI_DATA) and a `.podcli-home` marker file. The TypeScript side mirrors the same precedence in `src/config/paths.ts`, and `python-executor.ts` propagates both env vars to the Python subprocess so the two layers always agree. Also: ignore the `.podcli-home` profile marker to prevent absolute paths from leaking into commits.
Three independent but related additions, plus the ProRes 4444 alpha caption overlay that the editor exporter needs. Config profiles - `podcli config <export|import|use|status|migrate>` packages a config home (knowledge, presets, assets, settings) into a portable zip and restores it elsewhere. Asset paths in registries/presets are rewritten to point at the imported copies. Imports back up the existing home before clobbering. - Idempotent legacy-cache migration runs once on startup to move stale `project/.podcli/cache/` content into `data/cache/`. - Equivalent MCP tool `manage_config` and HTTP endpoints under `/api/config`. - Bundle extraction rejects symlinks, absolute paths, and parent-traversal members to prevent zip-slip and out-of-tree writes. Output integrations - New `backend/services/integrations/` package with a registry/manager and a shared FCPXML/IR helper module for editor exporters. - DaVinci Resolve integration (experimental, opt-in via `manage_integrations action=enable name=davinci_resolve`) emits an FCPXML 1.10 project: one compound clip per short on the master timeline, V1 source plus optional V2 alpha caption overlay so cuts and captions remain editable in free or Studio Resolve 20.x. - MCP tools: `manage_integrations`, `export_to_davinci_resolve`. UI page at `/integrations.html` toggles per-integration state. ProRes alpha caption overlay - `remotion/render.mjs --keep-overlay` writes a ProRes 4444 alpha overlay alongside the baked output instead of deleting it. - `clip_generator.generate_clip` exposes `keep_caption_overlay` and returns `caption_overlay_path` + `cropped_source_path` for editor handoff. It also auto-enables the overlay when the davinci_resolve integration is on, so enabling the editor integration is the only knob users have to flip. Also fixes a pre-existing bug exposed by the new `from config.paths import paths` import in cli.py: a local `paths = _thumb_gen(...)` in the thumbnail loop shadowed the module-level dict, so `cmd_process` crashed with UnboundLocalError on any clip that triggered earlier `paths[...]` lookups. Renamed the local to `thumb_paths`.
After Claude/Codex picks moments in `[3/4] Selecting moments...`, persist the result to `<home>/sessions/clips-<cache_hash>.json` keyed by the same video hash used for the transcript cache. A subsequent `podcli process` on the same video reloads the saved suggestions instantly instead of re-running the AI step (which can take several minutes on long episodes and bills tokens each time). `--no-resume` forces regeneration. Session files are only loaded when `top_n` matches; mismatches fall through to a fresh AI call.
📝 WalkthroughWalkthroughThis PR introduces centralized path configuration with environment variables and marker files, adds a plugin framework for editor integrations (DaVinci Resolve), implements portable config profile export/import with legacy migration, extends Remotion rendering to export caption overlays, and refactors MCP tools with improved Zod schemas and error handling. ChangesPath Resolution and Configuration Infrastructure
Integration Framework and DaVinci Resolve
Caption Overlay Export for DaVinci Resolve
Config Bundle Export, Import, and Legacy Migration
Transcript Cache Layout Migration and Knowledge Consolidation
Web API Routes for Config and Integrations
Web UI Pages for Config and Integrations
MCP Tool Handler Integration and Server Wiring
CLI Refactoring for Cache and Knowledge Management
Documentation Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/services/clip_generator.py (1)
900-913:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the overlay artifacts outside
work_dir.
caption_overlay_pathandcropped_source_pathpoint intowork_dir, but thefinallyblock deletes that directory wheneveroutput_diris set. In the normal CLI flow the caller gets paths to files that no longer exist, so the Resolve exporter can't consume them.💡 Proposed fix
- out = { + persisted_overlay_path = None + persisted_cropped_path = None + if keep_caption_overlay and caption_overlay_path and os.path.exists(caption_overlay_path): + base, _ = os.path.splitext(final_path) + persisted_overlay_path = f"{base}_captions.mov" + persisted_cropped_path = f"{base}_cropped.mp4" + shutil.copy2(caption_overlay_path, persisted_overlay_path) + shutil.copy2(cropped_path, persisted_cropped_path) + + out = { "output_path": final_path, "duration": round(duration, 2), "file_size_mb": file_size_mb, "title": title, "start_second": start_second, "end_second": end_second, "caption_style": caption_style, "crop_strategy": crop_strategy, } - if keep_caption_overlay and caption_overlay_path and os.path.exists(caption_overlay_path): - out["caption_overlay_path"] = caption_overlay_path - out["cropped_source_path"] = cropped_path + if persisted_overlay_path and persisted_cropped_path: + out["caption_overlay_path"] = persisted_overlay_path + out["cropped_source_path"] = persisted_cropped_path return outAlso applies to: 915-918
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/clip_generator.py` around lines 900 - 913, The caption overlay and cropped source artifacts (caption_overlay_path and cropped_path) are currently inside work_dir which is removed in the finally block, so persist them to a safe location before work_dir is deleted: if output_dir (or final_path's parent) is set, copy/move caption_overlay_path and cropped_path to a durable location (e.g., the same directory as final_path or a subfolder under output_dir), update the out dict keys ("caption_overlay_path" and "cropped_source_path") to point to the new persisted paths, and only include the persisted paths in the returned out; ensure this logic is applied where out is constructed (the block that builds out and the conditional that adds caption_overlay_path/cropped_source_path) so callers receive valid, non-temporary file paths.src/server.ts (1)
771-824:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the resolved batch inputs in the direct fallback.
You compute
resolvedClips,resolvedVideoPath, andresolvedTranscriptWordsforexport_selected/clip_numbers, but the non-Web-UI fallback still callshandleBatchClips(params). If the UI server is down, this path can export the wrong clips or fail with missing inputs even though resolution already succeeded.Suggested fix
- finalResult = await handleBatchClips(params); + finalResult = await handleBatchClips({ + ...params, + video_path: resolvedVideoPath, + clips: resolvedClips, + transcript_words: resolvedTranscriptWords, + });Also applies to: 895-900
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 771 - 824, The resolved batch inputs (resolvedClips, resolvedVideoPath, resolvedTranscriptWords) computed for export_selected/clip_numbers are not used in the non‑UI fallback; instead handleBatchClips(params) is called which can use stale/missing params. Update the fallback(s) that call handleBatchClips (and the similar block around the 895-900 region) to call handleBatchClips with the resolved values or to replace params' fields before calling it — i.e., pass an object where clips = resolvedClips, video_path = resolvedVideoPath, and transcript_words = resolvedTranscriptWords (falling back to original params only if those resolved variables are still undefined) so the exported batch uses the resolved inputs.src/ui/web-server.ts (1)
1737-1747:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
/api/mcp/exportis stripping clip fields the new export flow depends on.This mapper only preserves a small subset of clip properties and only translates
segments -> keep_segments. That drops already-normalizedkeep_segmentsfromsrc/server.tsand also dropskeep_caption_overlay, so MCP-driven multi-cut exports collapse to single ranges and overlay exports silently lose their sidecar assets.Suggested fix
const styledClips = clips.map((c: any) => ({ + ...c, start_second: c.start_second, end_second: c.end_second, title: (c.title || "clip").slice(0, 40), caption_style: c.caption_style || captionStyle, crop_strategy: c.crop_strategy || cropStrategy, allow_ass_fallback: c.allow_ass_fallback === true || allowAssFallback, - // Preserve multi-cut segments from suggestions - ...(Array.isArray(c.segments) && - c.segments.length > 0 && { keep_segments: c.segments }), + ...(Array.isArray(c.keep_segments) && + c.keep_segments.length > 0 && { keep_segments: c.keep_segments }), + ...(Array.isArray(c.segments) && + c.segments.length > 0 && { keep_segments: c.segments }), + keep_caption_overlay: c.keep_caption_overlay === true, })); @@ { video_path: videoPath, clips: styledClips, transcript_words: transcriptWords, output_dir: paths.output, logo_path: logoPath, outro_path: outroPath, + keep_caption_overlay: req.body.keep_caption_overlay === true, face_map: uiState.transcript?.face_map, },Also applies to: 1771-1778
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/web-server.ts` around lines 1737 - 1747, The mapper in styledClips is dropping clip properties the export flow needs (it converts segments -> keep_segments but overwrites or omits already-normalized keep_segments and omits keep_caption_overlay and other passthrough fields), so update the clips.map transformation (styledClips and the similar block around the other occurrence) to: 1) preserve an existing c.keep_segments if present and only derive keep_segments from c.segments when c.keep_segments is absent, 2) include c.keep_caption_overlay (or default) in the output, and 3) pass-through any other important sidecar/overlay fields rather than stripping them—use the existing c.* values (e.g. keep_segments, keep_caption_overlay, allow_ass_fallback) when available instead of always replacing them.
🧹 Nitpick comments (6)
backend/services/integrations/davinci_resolve/__init__.py (1)
8-8: 💤 Low valueConsider alphabetically sorting
__all__.Ruff suggests sorting
__all__entries alphabetically. While the current order matches import order, alphabetical sorting improves consistency and maintainability.♻️ Proposed fix
-__all__ = ["integration", "emitter"] +__all__ = ["emitter", "integration"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/integrations/davinci_resolve/__init__.py` at line 8, The __all__ export list is unsorted; update the __all__ variable in the module to list its names in alphabetical order (e.g., put "emitter" before "integration") so the __all__ = [...] tuple/ list is alphabetically ordered for consistency and to satisfy linters like Ruff.backend/services/integrations/_shared/media_probe.py (1)
13-50: ⚡ Quick winConsider combining the two ffprobe calls.
Running
ffprobetwice (once for video, once for audio) is less efficient than running once with both stream selections. While this works correctly, combining them could reduce overhead, especially when probing many files.♻️ Example combined approach
cmd = [ "ffprobe", "-v", "error", "-select_streams", "v:0,a:0", "-show_entries", "stream=width,height,r_frame_rate,nb_frames,duration,channels:format=duration", "-of", "json", p, ] data = json.loads(subprocess.check_output(cmd, text=True, timeout=30)) streams = data.get("streams", []) v_stream = next((s for s in streams if s.get("width")), {}) a_stream = next((s for s in streams if s.get("channels")), {})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/integrations/_shared/media_probe.py` around lines 13 - 50, The two separate ffprobe invocations in media_probe.py should be merged into a single ffprobe call: call ffprobe once with "-select_streams", "v:0,a:0" and "-show_entries", "stream=width,height,r_frame_rate,nb_frames,duration,channels:format=duration", parse the single JSON result (replace v_data/a_data usage with a single data variable), then locate v_stream as the stream with width/height/r_frame_rate and a_stream as the stream with channels (e.g., using next over data.get("streams", [])); keep the existing logic that computes width, height, fps, fmt_duration and duration_frames but read those values from v_stream/a_stream in the consolidated data and include a timeout on subprocess.check_output as appropriate.backend/services/integrations/davinci_resolve/cli.py (2)
70-72: 💤 Low valueDefault output directory may not exist.
The default output path assumes
data/export/davinci_resolve/exists. If it doesn't,emitter.emit()will fail when writing the file. Consider ensuring the directory exists or documenting that users should create it.🛡️ Proposed fix
out = args.out or ( REPO_ROOT / "data" / "export" / "davinci_resolve" / f"{args.title.replace(' ', '_')}.fcpxml" ) + out.parent.mkdir(parents=True, exist_ok=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/integrations/davinci_resolve/cli.py` around lines 70 - 72, The default output path assigned to out (out = args.out or (REPO_ROOT / "data" / "export" / "davinci_resolve" / f"{args.title.replace(' ', '_')}.fcpxml")) assumes the parent directory exists and will cause emitter.emit() to fail; before calling emitter.emit() ensure the output file's parent directory exists by creating out.parent with parents=True and exist_ok=True (or document that users must create data/export/davinci_resolve), referencing the variables out, REPO_ROOT, args.title, and the emitter.emit call so the directory creation happens immediately prior to writing the file.
16-18: ⚡ Quick winFragile path calculation using fixed parent depth.
The REPO_ROOT calculation assumes this file is exactly 4 directories deep. If the file is moved or the structure changes, this will break silently.
♻️ More robust alternative
-REPO_ROOT = Path(__file__).resolve().parents[4] +# Find repo root by looking for a marker file +def _find_repo_root(): + p = Path(__file__).resolve() + for parent in [p] + list(p.parents): + if (parent / "backend").is_dir() and (parent / "package.json").is_file(): + return parent + raise RuntimeError("Could not locate repository root") + +REPO_ROOT = _find_repo_root()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/integrations/davinci_resolve/cli.py` around lines 16 - 18, The REPO_ROOT calculation is fragile because parents[4] assumes fixed depth; change it to walk up the directory tree from Path(__file__).resolve() until you find a repository marker (e.g., a ".git" directory or the "backend" folder) and set REPO_ROOT to that parent instead of using parents[4]; then use that computed REPO_ROOT when checking/inserting into sys.path (symbols to update: REPO_ROOT, Path(__file__).resolve(), and the sys.path.insert call) so the import path logic continues to work if files are moved.backend/services/integrations/davinci_resolve/emitter.py (2)
30-31: 💤 Low valueDocument the rationale for ID starting values.
The asset IDs start at 2 and media IDs at 1000 without explanation. A brief comment would help future maintainers understand whether these specific values are significant or arbitrary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/integrations/davinci_resolve/emitter.py` around lines 30 - 31, Add a short explanatory comment above the next_asset and next_media initializations in emitter.py that documents why asset IDs start at 2 and media IDs at 1000 (e.g., reserved ID 1 for root/placeholder, gap for legacy IDs, or arbitrary chosen to avoid collision), or mark them as intentionally arbitrary if no special meaning; reference the variables next_asset and next_media and update their nearby comment or module docstring to capture this rationale for future maintainers.
23-33: ⚡ Quick winConsider validating that
project.shortsis non-empty.The function proceeds directly to building the library even if
project.shortsis empty, which would produce valid FCPXML with no clips. Adding an early check would provide clearer feedback.🛡️ Proposed validation
def emit(project: Project, out_path: Path) -> Path: + if not project.shorts: + raise ValueError("Project must contain at least one short") fmt_id = "r1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/integrations/davinci_resolve/emitter.py` around lines 23 - 33, The emit function should validate that project.shorts is non-empty before building the FCPXML; add an early check at the top of emit (before building fmt_id/resources/compounds) that tests if not project.shorts and then raise a clear exception (e.g., ValueError with a descriptive message) or return an explicit error so callers know there are no clips to emit; update any callers or tests that expect this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/cli.py`:
- Around line 405-410: The session key currently only uses
compute_cache_hash(video_path) so passing a custom transcript (args.transcript)
can reuse stale suggestions; update the resume logic in the code that sets
cache_hash (uses services.transcript_packer.compute_cache_hash) to either (a)
disable resume when args.transcript is present by leaving cache_hash empty/None,
or (b) include the transcript and relevant config in the session key (e.g.,
compute a combined hash of video_path plus the transcript file contents/mtime
and any config flags) so functions that rely on cache_hash will never reuse
suggestions from a different transcript; apply the same change to the other
resume spots noted (the blocks around where cache_hash is computed at the other
occurrences).
In `@backend/config_bundle.py`:
- Around line 496-497: Activating a new home writes the marker but never
refreshes the module-level in-memory paths cache, so functions like
get_active_home() and later home-relative operations still return the old home;
after writing the marker in set_active_home() and any code path that calls
_marker_path().write_text(...) (including import_config(..., activate=True)),
invalidate or reload the cached paths (e.g., update the module-level paths
variable or call the existing path-loading routine) so
config.paths/get_active_home() reflect the new marker immediately in-process;
alternatively remove the import-time caching and make get_active_home() read the
marker dynamically. Ensure you update all activation sites referenced (the
functions that call _marker_path().write_text) to perform this refresh.
In `@backend/main.py`:
- Around line 476-483: The auto-migration call in _maybe_auto_migrate_backend
currently runs inline and will propagate exceptions to request handling; wrap
the import and call to auto_migrate_legacy_if_pending(quiet=True) in a
try/except that catches Exception, logs the exception (contextual message
referencing auto-migrate) and continues without re-raising so migration failures
are best-effort and non-blocking; apply the same try/except pattern to the other
identical call referenced at the other location (the occurrence around line 498)
and keep the existing early-return logic for "manage_config" tasks unchanged.
- Around line 453-454: The current except block returns the full traceback in
the API response; instead, log the full traceback on the server and emit a
sanitized error string to the client. In the except block for emit_result use
logging.exception(...) or a server logger to record traceback.format_exc() (or
call logger.exception(e)) and then call emit_result(task_id, "error",
error=f"{type(e).__name__}: {e}") or a generic message like "Internal server
error" so no stack/paths are sent to clients; update the block around
emit_result and the exception handler to perform server-side logging and a
sanitized emit_result call.
In `@backend/services/encoder.py`:
- Line 173: The docstring comment still hardcodes "data/cache/encoder.json" but
the cache path is now resolved dynamically via paths["cache"]; update the
docstring in backend/services/encoder.py (the comment at the Cached at ... line)
to reference the dynamic cache location (e.g. paths["cache"] joined with the
encoder filename) and note it is keyed by the ffmpeg binary fingerprint so the
documentation matches the actual behavior in this module.
In `@backend/services/integrations/_shared/media_probe.py`:
- Around line 13-21: The subprocess.check_output calls that run ffprobe
(building v_cmd and invoking subprocess.check_output to produce v_data) need a
timeout to avoid hanging; update the call to include a timeout parameter (e.g.,
timeout=30) and handle subprocess.TimeoutExpired if desired, and apply the same
change to the audio probe call that builds a_cmd / produces a_data so both
ffprobe invocations use a timeout.
In `@backend/services/integrations/base.py`:
- Around line 46-47: The register method on IntegrationBase currently overwrites
any existing entry in cls._instances; change register (def register(cls,
integration: IntegrationBase)) to first check if integration.name already exists
in cls._instances and if so raise a clear exception (e.g., ValueError or
RuntimeError) preventing silent overwrite; otherwise proceed to assign
cls._instances[integration.name] = integration so duplicate registrations fail
fast and surface the conflict.
In `@backend/services/integrations/davinci_resolve/integration.py`:
- Around line 64-66: The code currently allows an empty params["shorts"] to
proceed and emit a project with no clips; before calling probe_media and
building shorts (the block that starts with shorts: list[Short] = [] and the
later similar block around the probe_media loop), validate that params["shorts"]
is present and not empty (or that the resulting shorts list after filtering is
non-empty) and raise a clear validation error (e.g., ValueError or a
ValidationError) instead of continuing; apply this check both where
params["shorts"] is consumed and in the second similar loop to fail fast and
return an error response rather than success metadata.
In `@backend/utils/prompt_files.py`:
- Line 25: The module docstring and any function comments referring to
“.podcli/tmp/” are out of date—update them to reflect that temporary files are
now written to os.path.join(paths["home"], "tmp") (the variable base in this
file), so change wording to indicate the temp directory is under the configured
paths["home"]/tmp location (and adjust any troubleshooting/manual cleanup
instructions accordingly).
In `@src/config/paths.ts`:
- Around line 11-22: The resolveHome() function currently calls
readFileSync(homeMarker) without error handling which can crash the app; wrap
the readFileSync(homeMarker, "utf-8") call in a try-catch inside the
existsSync(homeMarker) block (keeping the isAbsolute(marker) check) and on any
exception treat it as a missing/invalid marker by falling back to return
resolve(projectRoot, ".podcli"); reference resolveHome(), homeMarker,
readFileSync, existsSync, and projectRoot when making the change.
In `@src/handlers/integrations.handler.ts`:
- Around line 127-133: The handler currently calls handleManageIntegrations({
action, name }) even when action is "enable" or "disable" and name is empty; add
an upfront guard in the async handler (the anonymous async ({ action, name }) =>
{ ... } block) that checks if action === "enable" || action === "disable" and if
name is falsy, then return a clear MCP error via mcpText (e.g., a descriptive
message like "Missing required 'name' for enable/disable action") instead of
calling handleManageIntegrations; this will fail fast and avoid sending an empty
name to Python.
In `@src/handlers/integrations.routes.ts`:
- Around line 44-57: The export route that calls
executor.execute("manage_config", ...) and then res.download(file, ...) must
delete the temporary bundle file after the response completes—use the
res.download callback or res.on('finish'/'close') to unlink the bundlePath/file
so the ZIPs don't accumulate; likewise, for the import route that reads
req.file.path, ensure you remove the uploaded file in a finally block after
processing (regardless of success/failure) to guarantee cleanup; reference the
export handler around executor.execute and res.download and the import handler
using req.file.path when adding the unlink logic.
In `@src/server.ts`:
- Around line 509-515: The export path in src/server.ts is not forwarding the
keep_caption_overlay flag to the /api/mcp/export endpoint, so Web-UI exports
ignore overlay-enabled renders; update the code that constructs the export
request payload for the Web-UI export path so it includes the
keep_caption_overlay property (preserve its boolean value/default), propagate it
through any intermediate structures or typed interfaces used when calling
/api/mcp/export, and ensure the server handler reading /api/mcp/export consumes
that field; specifically add keep_caption_overlay to the JSON/body sent to
/api/mcp/export wherever export requests are assembled.
In `@src/ui/public/integrations.html`:
- Around line 169-170: The current integration toggle is a non-focusable div
("toggle" element with role="switch" and data-name="${escapeHtml(int.name)}")
and must be replaced or augmented with a keyboard-accessible control: change the
element to a semantic, focusable control (e.g., a <button> or an <input
type="checkbox">) that maintains role="switch" or native semantics, ensure
aria-checked reflects int.enabled, include keyboard event handling (Space/Enter)
to toggle the state, keep the data-name attribute for identification, and ensure
visual classes ("on") are applied when toggled; update any click-only listeners
attached to the old ".toggle" to handle keyboard activation as well.
In `@src/ui/web-server.ts`:
- Around line 1643-1645: The field transcriptWordCount is returning the words
array itself due to misplaced parentheses around (uiState.transcript?.words ??
[].length); change it to return a numeric length instead by using
uiState.transcript?.words.length (or (uiState.transcript?.words ?? []).length)
so transcriptWordCount is always a number; update the expression that sets
transcriptWordCount in web-server.ts to reference
uiState.transcript?.words.length and fall back to 0 when missing.
- Around line 909-919: The current isUploadedFile check uses startsWith on
path.resolve(join(paths.working, "uploads")), which mistakenly matches sibling
directories like "uploads-backup"; change the guard in the web-server logic that
computes resolvedPath and isUploadedFile so you compare against the uploads
directory with a directory boundary (e.g., compute uploadsDir =
path.resolve(join(paths.working, "uploads")) and ensure resolvedPath ===
uploadsDir or resolvedPath.startsWith(uploadsDir + path.sep)) or use
path.relative to verify the file is inside uploads (relative does not begin with
'..'); update the isUploadedFile boolean accordingly where resolvedPath,
isSessionVideo, and paths.working are used.
---
Outside diff comments:
In `@backend/services/clip_generator.py`:
- Around line 900-913: The caption overlay and cropped source artifacts
(caption_overlay_path and cropped_path) are currently inside work_dir which is
removed in the finally block, so persist them to a safe location before work_dir
is deleted: if output_dir (or final_path's parent) is set, copy/move
caption_overlay_path and cropped_path to a durable location (e.g., the same
directory as final_path or a subfolder under output_dir), update the out dict
keys ("caption_overlay_path" and "cropped_source_path") to point to the new
persisted paths, and only include the persisted paths in the returned out;
ensure this logic is applied where out is constructed (the block that builds out
and the conditional that adds caption_overlay_path/cropped_source_path) so
callers receive valid, non-temporary file paths.
In `@src/server.ts`:
- Around line 771-824: The resolved batch inputs (resolvedClips,
resolvedVideoPath, resolvedTranscriptWords) computed for
export_selected/clip_numbers are not used in the non‑UI fallback; instead
handleBatchClips(params) is called which can use stale/missing params. Update
the fallback(s) that call handleBatchClips (and the similar block around the
895-900 region) to call handleBatchClips with the resolved values or to replace
params' fields before calling it — i.e., pass an object where clips =
resolvedClips, video_path = resolvedVideoPath, and transcript_words =
resolvedTranscriptWords (falling back to original params only if those resolved
variables are still undefined) so the exported batch uses the resolved inputs.
In `@src/ui/web-server.ts`:
- Around line 1737-1747: The mapper in styledClips is dropping clip properties
the export flow needs (it converts segments -> keep_segments but overwrites or
omits already-normalized keep_segments and omits keep_caption_overlay and other
passthrough fields), so update the clips.map transformation (styledClips and the
similar block around the other occurrence) to: 1) preserve an existing
c.keep_segments if present and only derive keep_segments from c.segments when
c.keep_segments is absent, 2) include c.keep_caption_overlay (or default) in the
output, and 3) pass-through any other important sidecar/overlay fields rather
than stripping them—use the existing c.* values (e.g. keep_segments,
keep_caption_overlay, allow_ass_fallback) when available instead of always
replacing them.
---
Nitpick comments:
In `@backend/services/integrations/_shared/media_probe.py`:
- Around line 13-50: The two separate ffprobe invocations in media_probe.py
should be merged into a single ffprobe call: call ffprobe once with
"-select_streams", "v:0,a:0" and "-show_entries",
"stream=width,height,r_frame_rate,nb_frames,duration,channels:format=duration",
parse the single JSON result (replace v_data/a_data usage with a single data
variable), then locate v_stream as the stream with width/height/r_frame_rate and
a_stream as the stream with channels (e.g., using next over data.get("streams",
[])); keep the existing logic that computes width, height, fps, fmt_duration and
duration_frames but read those values from v_stream/a_stream in the consolidated
data and include a timeout on subprocess.check_output as appropriate.
In `@backend/services/integrations/davinci_resolve/__init__.py`:
- Line 8: The __all__ export list is unsorted; update the __all__ variable in
the module to list its names in alphabetical order (e.g., put "emitter" before
"integration") so the __all__ = [...] tuple/ list is alphabetically ordered for
consistency and to satisfy linters like Ruff.
In `@backend/services/integrations/davinci_resolve/cli.py`:
- Around line 70-72: The default output path assigned to out (out = args.out or
(REPO_ROOT / "data" / "export" / "davinci_resolve" / f"{args.title.replace(' ',
'_')}.fcpxml")) assumes the parent directory exists and will cause
emitter.emit() to fail; before calling emitter.emit() ensure the output file's
parent directory exists by creating out.parent with parents=True and
exist_ok=True (or document that users must create data/export/davinci_resolve),
referencing the variables out, REPO_ROOT, args.title, and the emitter.emit call
so the directory creation happens immediately prior to writing the file.
- Around line 16-18: The REPO_ROOT calculation is fragile because parents[4]
assumes fixed depth; change it to walk up the directory tree from
Path(__file__).resolve() until you find a repository marker (e.g., a ".git"
directory or the "backend" folder) and set REPO_ROOT to that parent instead of
using parents[4]; then use that computed REPO_ROOT when checking/inserting into
sys.path (symbols to update: REPO_ROOT, Path(__file__).resolve(), and the
sys.path.insert call) so the import path logic continues to work if files are
moved.
In `@backend/services/integrations/davinci_resolve/emitter.py`:
- Around line 30-31: Add a short explanatory comment above the next_asset and
next_media initializations in emitter.py that documents why asset IDs start at 2
and media IDs at 1000 (e.g., reserved ID 1 for root/placeholder, gap for legacy
IDs, or arbitrary chosen to avoid collision), or mark them as intentionally
arbitrary if no special meaning; reference the variables next_asset and
next_media and update their nearby comment or module docstring to capture this
rationale for future maintainers.
- Around line 23-33: The emit function should validate that project.shorts is
non-empty before building the FCPXML; add an early check at the top of emit
(before building fmt_id/resources/compounds) that tests if not project.shorts
and then raise a clear exception (e.g., ValueError with a descriptive message)
or return an explicit error so callers know there are no clips to emit; update
any callers or tests that expect this behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f56e140a-d39d-4b4f-a887-30b5a0c3746f
📒 Files selected for processing (50)
.gitignoreCONTRIBUTING.mdREADME.mdbackend/cli.pybackend/config/paths.pybackend/config_bundle.pybackend/main.pybackend/presets.pybackend/services/asset_store.pybackend/services/claude_suggest.pybackend/services/clip_generator.pybackend/services/content_generator.pybackend/services/corrections.pybackend/services/encoder.pybackend/services/integrations/__init__.pybackend/services/integrations/_shared/__init__.pybackend/services/integrations/_shared/fcpxml.pybackend/services/integrations/_shared/media_probe.pybackend/services/integrations/_shared/timeline_ir.pybackend/services/integrations/base.pybackend/services/integrations/davinci_resolve/README.mdbackend/services/integrations/davinci_resolve/__init__.pybackend/services/integrations/davinci_resolve/cli.pybackend/services/integrations/davinci_resolve/emitter.pybackend/services/integrations/davinci_resolve/integration.pybackend/services/integrations/manager.pybackend/services/thumbnail_ai.pybackend/services/thumbnail_generator.pybackend/services/thumbnail_html.pybackend/services/transcript_packer.pybackend/utils/prompt_files.pyremotion/render.mjsremotion/test-render.mjssrc/config/paths.test.tssrc/config/paths.tssrc/handlers/batch-clips.handler.tssrc/handlers/create-clip.handler.tssrc/handlers/integrations.handler.tssrc/handlers/integrations.routes.tssrc/models/index.tssrc/server.tssrc/services/python-executor.tssrc/ui/public/config.htmlsrc/ui/public/index.htmlsrc/ui/public/integration.htmlsrc/ui/public/integrations.htmlsrc/ui/web-server.tssrc/utils/logger.test.tstests/test_clip_generator.pytests/test_config_bundle.py
Comment on lines
+405
to
+410
| # Cache hash for resume — keyed by video size+mtime, shared with transcript cache. | ||
| cache_hash = "" | ||
| try: | ||
| from services.transcript_packer import compute_cache_hash as _compute_cache_hash | ||
|
|
||
| cache_hash = _compute_cache_hash(video_path) |
There was a problem hiding this comment.
Don't resume suggestions when the transcript input changed.
The session key is derived only from compute_cache_hash(video_path), so podcli process episode.mp4 -t revised.json will happily reuse suggestions generated from an older transcript for the same video. That bypasses transcript edits and can render the wrong moments. At minimum, disable resume when args.transcript is supplied; ideally include transcript/config inputs in the session key.
Also applies to: 425-452, 542-547
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/cli.py` around lines 405 - 410, The session key currently only uses
compute_cache_hash(video_path) so passing a custom transcript (args.transcript)
can reuse stale suggestions; update the resume logic in the code that sets
cache_hash (uses services.transcript_packer.compute_cache_hash) to either (a)
disable resume when args.transcript is present by leaving cache_hash empty/None,
or (b) include the transcript and relevant config in the session key (e.g.,
compute a combined hash of video_path plus the transcript file contents/mtime
and any config flags) so functions that rely on cache_hash will never reuse
suggestions from a different transcript; apply the same change to the other
resume spots noted (the blocks around where cache_hash is computed at the other
occurrences).
Comment on lines
+496
to
+497
| if activate: | ||
| _marker_path().write_text(str(target) + "\n", encoding="utf-8") |
There was a problem hiding this comment.
Activating a new home never refreshes the in-memory path map.
set_active_home() and import_config(..., activate=True) update .podcli-home, but get_active_home() still reads the import-time paths["home"]. In the same Python process, config status and any later home-relative operation stay pointed at the old home until restart. This shows up immediately in the new interactive config flow. Refresh config.paths after activation, or stop caching the active home at module import.
Also applies to: 512-520, 523-538
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/config_bundle.py` around lines 496 - 497, Activating a new home
writes the marker but never refreshes the module-level in-memory paths cache, so
functions like get_active_home() and later home-relative operations still return
the old home; after writing the marker in set_active_home() and any code path
that calls _marker_path().write_text(...) (including import_config(...,
activate=True)), invalidate or reload the cached paths (e.g., update the
module-level paths variable or call the existing path-loading routine) so
config.paths/get_active_home() reflect the new marker immediately in-process;
alternatively remove the import-time caching and make get_active_home() read the
marker dynamically. Ensure you update all activation sites referenced (the
functions that call _marker_path().write_text) to perform this refresh.
Comment on lines
+453
to
+454
| except Exception as e: | ||
| emit_result(task_id, "error", error=f"{type(e).__name__}: {e}\n{traceback.format_exc()}") |
There was a problem hiding this comment.
Do not return full traceback content in API error responses.
This leaks internal paths and stack details to clients. Log traceback server-side and return a sanitized error message.
Proposed fix
try:
result = tool.handler(tool_params)
emit_result(task_id, "success", data=result)
except Exception as e:
- emit_result(task_id, "error", error=f"{type(e).__name__}: {e}\n{traceback.format_exc()}")
+ print(traceback.format_exc(), file=sys.stderr, flush=True)
+ emit_result(task_id, "error", error=f"{type(e).__name__}: {e}")🧰 Tools
🪛 Ruff (0.15.13)
[warning] 453-453: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/main.py` around lines 453 - 454, The current except block returns the
full traceback in the API response; instead, log the full traceback on the
server and emit a sanitized error string to the client. In the except block for
emit_result use logging.exception(...) or a server logger to record
traceback.format_exc() (or call logger.exception(e)) and then call
emit_result(task_id, "error", error=f"{type(e).__name__}: {e}") or a generic
message like "Internal server error" so no stack/paths are sent to clients;
update the block around emit_result and the exception handler to perform
server-side logging and a sanitized emit_result call.
Comment on lines
+476
to
+483
| def _maybe_auto_migrate_backend(task_type: str, params: dict) -> None: | ||
| if task_type == "manage_config": | ||
| action = params.get("action", "status") | ||
| if action == "status" or (action == "migrate" and params.get("dry_run")): | ||
| return | ||
| from config_bundle import auto_migrate_legacy_if_pending | ||
|
|
||
| auto_migrate_legacy_if_pending(quiet=True) |
There was a problem hiding this comment.
Isolate auto-migration failures from normal task execution.
A migration exception here can fail unrelated requests before handler dispatch. This should be best-effort and non-blocking for regular tasks.
Proposed fix
def _maybe_auto_migrate_backend(task_type: str, params: dict) -> None:
if task_type == "manage_config":
action = params.get("action", "status")
if action == "status" or (action == "migrate" and params.get("dry_run")):
return
- from config_bundle import auto_migrate_legacy_if_pending
-
- auto_migrate_legacy_if_pending(quiet=True)
+ try:
+ from config_bundle import auto_migrate_legacy_if_pending
+ auto_migrate_legacy_if_pending(quiet=True)
+ except Exception as e:
+ print(f"Warning: auto-migrate skipped: {e}", file=sys.stderr, flush=True)Also applies to: 498-498
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/main.py` around lines 476 - 483, The auto-migration call in
_maybe_auto_migrate_backend currently runs inline and will propagate exceptions
to request handling; wrap the import and call to
auto_migrate_legacy_if_pending(quiet=True) in a try/except that catches
Exception, logs the exception (contextual message referencing auto-migrate) and
continues without re-raising so migration failures are best-effort and
non-blocking; apply the same try/except pattern to the other identical call
referenced at the other location (the occurrence around line 498) and keep the
existing early-return logic for "manage_config" tasks unchanged.
| """Get full encoder detection info (for UI/logging). | ||
|
|
||
| Cached at .podcli/cache/encoder.json keyed by ffmpeg binary fingerprint. | ||
| Cached at data/cache/encoder.json keyed by ffmpeg binary fingerprint. |
There was a problem hiding this comment.
Update cache-location docstring to match the new dynamic path source.
The text still says data/cache/encoder.json, but the file now resolves via paths["cache"] (env-configurable). Keeping this accurate avoids debugging confusion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/services/encoder.py` at line 173, The docstring comment still
hardcodes "data/cache/encoder.json" but the cache path is now resolved
dynamically via paths["cache"]; update the docstring in
backend/services/encoder.py (the comment at the Cached at ... line) to reference
the dynamic cache location (e.g. paths["cache"] joined with the encoder
filename) and note it is keyed by the ffmpeg binary fingerprint so the
documentation matches the actual behavior in this module.
Comment on lines
+44
to
+57
| app.get("/api/config/export", async (_req, res) => { | ||
| try { | ||
| const bundlePath = join(paths.working, `profile-export-${uuidv4()}.zip`); | ||
| await mkdir(paths.working, { recursive: true }); | ||
| const result = await executor.execute<{ bundle: string }>("manage_config", { | ||
| action: "export", | ||
| bundle_path: bundlePath, | ||
| }); | ||
| const file = result.data?.bundle ?? bundlePath; | ||
| if (!existsSync(file)) { | ||
| res.status(500).json({ error: "Export failed: bundle not created" }); | ||
| return; | ||
| } | ||
| res.download(file, "podcli-profile.zip"); |
There was a problem hiding this comment.
Delete the temporary ZIPs after each request completes.
Both endpoints leave their bundle on disk after the response returns. With the 512MB upload cap, a few imports/exports can leak gigabytes into paths.working/uploadDir and eventually break later requests. Clean the exported file in the res.download() callback and remove req.file.path in a finally block after the import finishes.
Also applies to: 81-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/handlers/integrations.routes.ts` around lines 44 - 57, The export route
that calls executor.execute("manage_config", ...) and then res.download(file,
...) must delete the temporary bundle file after the response completes—use the
res.download callback or res.on('finish'/'close') to unlink the bundlePath/file
so the ZIPs don't accumulate; likewise, for the import route that reads
req.file.path, ensure you remove the uploaded file in a finally block after
processing (regardless of success/failure) to guarantee cleanup; reference the
export handler around executor.execute and res.download and the import handler
using req.file.path when adding the unlink logic.
Comment on lines
+509
to
+515
| keep_caption_overlay: z | ||
| .boolean() | ||
| .optional() | ||
| .default(false) | ||
| .describe( | ||
| "Keep ProRes 4444 alpha caption overlay beside the render (for DaVinci Resolve export). Returns caption_overlay_path and cropped_source_path.", | ||
| ), |
There was a problem hiding this comment.
Forward keep_caption_overlay through the Web-UI export path.
Both tool schemas accept keep_caption_overlay, but the requests sent to /api/mcp/export never include it. With the Web UI running—which is the preferred path—overlay-enabled exports silently behave like normal renders.
Suggested fix
body: JSON.stringify({
video_path: params.video_path,
clips: [
{
start_second: params.start_second,
end_second: params.end_second,
title: (params.title || "clip") as string,
caption_style: params.caption_style || "hormozi",
crop_strategy: params.crop_strategy || "speaker",
allow_ass_fallback: params.allow_ass_fallback === true,
+ keep_caption_overlay: params.keep_caption_overlay === true,
...(keepSegments && { segments: keepSegments }),
},
],
transcript_words: params.transcript_words,
logo_path: params.logo_path || null,
outro_path: params.outro_path || null,
+ keep_caption_overlay: params.keep_caption_overlay === true,
}),
@@
body: JSON.stringify({
video_path: resolvedVideoPath,
clips: resolvedClips,
transcript_words: resolvedTranscriptWords,
+ keep_caption_overlay: params.keep_caption_overlay === true,
}),Also applies to: 603-617, 730-735, 834-838
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server.ts` around lines 509 - 515, The export path in src/server.ts is
not forwarding the keep_caption_overlay flag to the /api/mcp/export endpoint, so
Web-UI exports ignore overlay-enabled renders; update the code that constructs
the export request payload for the Web-UI export path so it includes the
keep_caption_overlay property (preserve its boolean value/default), propagate it
through any intermediate structures or typed interfaces used when calling
/api/mcp/export, and ensure the server handler reading /api/mcp/export consumes
that field; specifically add keep_caption_overlay to the JSON/body sent to
/api/mcp/export wherever export requests are assembled.
Comment on lines
+169
to
+170
| <div class="toggle ${int.enabled ? "on" : ""}" role="switch" | ||
| aria-checked="${int.enabled}" data-name="${escapeHtml(int.name)}"></div> |
There was a problem hiding this comment.
Use a keyboard-accessible control for the integration switch.
The switch is currently a clickable div, which is not keyboard-focusable/operable by default. This blocks non-mouse users from enabling/disabling integrations.
Proposed fix
- <div class="toggle ${int.enabled ? "on" : ""}" role="switch"
- aria-checked="${int.enabled}" data-name="${escapeHtml(int.name)}"></div>
+ <button type="button"
+ class="toggle ${int.enabled ? "on" : ""}"
+ role="switch"
+ aria-checked="${int.enabled}"
+ aria-label="Toggle ${escapeHtml(int.name)} integration"
+ data-name="${escapeHtml(int.name)}"></button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div class="toggle ${int.enabled ? "on" : ""}" role="switch" | |
| aria-checked="${int.enabled}" data-name="${escapeHtml(int.name)}"></div> | |
| <button type="button" | |
| class="toggle ${int.enabled ? "on" : ""}" | |
| role="switch" | |
| aria-checked="${int.enabled}" | |
| aria-label="Toggle ${escapeHtml(int.name)} integration" | |
| data-name="${escapeHtml(int.name)}"></button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/public/integrations.html` around lines 169 - 170, The current
integration toggle is a non-focusable div ("toggle" element with role="switch"
and data-name="${escapeHtml(int.name)}") and must be replaced or augmented with
a keyboard-accessible control: change the element to a semantic, focusable
control (e.g., a <button> or an <input type="checkbox">) that maintains
role="switch" or native semantics, ensure aria-checked reflects int.enabled,
include keyboard event handling (Space/Enter) to toggle the state, keep the
data-name attribute for identification, and ensure visual classes ("on") are
applied when toggled; update any click-only listeners attached to the old
".toggle" to handle keyboard activation as well.
Comment on lines
909
to
919
| const resolvedPath = path.resolve(filePath); | ||
| const isSessionVideo = uiState.videoPath && resolvedPath === path.resolve(uiState.videoPath); | ||
| const isUploadedFile = resolvedPath.startsWith(path.resolve(join(paths.working, "uploads"))); | ||
| const isSessionVideo = | ||
| uiState.videoPath && resolvedPath === path.resolve(uiState.videoPath); | ||
| const isUploadedFile = resolvedPath.startsWith( | ||
| path.resolve(join(paths.working, "uploads")), | ||
| ); | ||
| if (!isSessionVideo && !isUploadedFile) { | ||
| res.status(403).json({ error: "Access denied: path not in allowed directories" }); | ||
| res | ||
| .status(403) | ||
| .json({ error: "Access denied: path not in allowed directories" }); | ||
| return; |
There was a problem hiding this comment.
Tighten the uploaded-file boundary check.
resolvedPath.startsWith(path.resolve(join(paths.working, "uploads"))) also matches sibling paths like .../uploads-backup/foo.mp4, so this new guard can authorize files outside the actual uploads directory.
Suggested fix
const resolvedPath = path.resolve(filePath);
const isSessionVideo =
uiState.videoPath && resolvedPath === path.resolve(uiState.videoPath);
- const isUploadedFile = resolvedPath.startsWith(
- path.resolve(join(paths.working, "uploads")),
- );
+ const uploadsRoot = path.resolve(join(paths.working, "uploads"));
+ const relativeToUploads = path.relative(uploadsRoot, resolvedPath);
+ const isUploadedFile =
+ relativeToUploads !== "" &&
+ !relativeToUploads.startsWith("..") &&
+ !path.isAbsolute(relativeToUploads);
if (!isSessionVideo && !isUploadedFile) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const resolvedPath = path.resolve(filePath); | |
| const isSessionVideo = uiState.videoPath && resolvedPath === path.resolve(uiState.videoPath); | |
| const isUploadedFile = resolvedPath.startsWith(path.resolve(join(paths.working, "uploads"))); | |
| const isSessionVideo = | |
| uiState.videoPath && resolvedPath === path.resolve(uiState.videoPath); | |
| const isUploadedFile = resolvedPath.startsWith( | |
| path.resolve(join(paths.working, "uploads")), | |
| ); | |
| if (!isSessionVideo && !isUploadedFile) { | |
| res.status(403).json({ error: "Access denied: path not in allowed directories" }); | |
| res | |
| .status(403) | |
| .json({ error: "Access denied: path not in allowed directories" }); | |
| return; | |
| const resolvedPath = path.resolve(filePath); | |
| const isSessionVideo = | |
| uiState.videoPath && resolvedPath === path.resolve(uiState.videoPath); | |
| const uploadsRoot = path.resolve(join(paths.working, "uploads")); | |
| const relativeToUploads = path.relative(uploadsRoot, resolvedPath); | |
| const isUploadedFile = | |
| relativeToUploads !== "" && | |
| !relativeToUploads.startsWith("..") && | |
| !path.isAbsolute(relativeToUploads); | |
| if (!isSessionVideo && !isUploadedFile) { | |
| res | |
| .status(403) | |
| .json({ error: "Access denied: path not in allowed directories" }); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/web-server.ts` around lines 909 - 919, The current isUploadedFile
check uses startsWith on path.resolve(join(paths.working, "uploads")), which
mistakenly matches sibling directories like "uploads-backup"; change the guard
in the web-server logic that computes resolvedPath and isUploadedFile so you
compare against the uploads directory with a directory boundary (e.g., compute
uploadsDir = path.resolve(join(paths.working, "uploads")) and ensure
resolvedPath === uploadsDir or resolvedPath.startsWith(uploadsDir + path.sep))
or use path.relative to verify the file is inside uploads (relative does not
begin with '..'); update the isUploadedFile boolean accordingly where
resolvedPath, isSessionVideo, and paths.working are used.
Comment on lines
1643
to
1645
| transcriptWordCount: Array.isArray(uiState.transcript?.words) | ||
| ? uiState.transcript?.words ?? [].length | ||
| ? (uiState.transcript?.words ?? [].length) | ||
| : 0, |
There was a problem hiding this comment.
Return the transcript word count, not the words array.
This ternary currently yields uiState.transcript.words when it exists, so MCP callers receive an array in transcriptWordCount. src/server.ts treats that field as a number, which breaks transcript detection and workflow guidance.
Suggested fix
transcriptWordCount: Array.isArray(uiState.transcript?.words)
- ? (uiState.transcript?.words ?? [].length)
+ ? (uiState.transcript?.words ?? []).length
: 0,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| transcriptWordCount: Array.isArray(uiState.transcript?.words) | |
| ? uiState.transcript?.words ?? [].length | |
| ? (uiState.transcript?.words ?? [].length) | |
| : 0, | |
| transcriptWordCount: Array.isArray(uiState.transcript?.words) | |
| ? (uiState.transcript?.words ?? []).length | |
| : 0, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/web-server.ts` around lines 1643 - 1645, The field transcriptWordCount
is returning the words array itself due to misplaced parentheses around
(uiState.transcript?.words ?? [].length); change it to return a numeric length
instead by using uiState.transcript?.words.length (or (uiState.transcript?.words
?? []).length) so transcriptWordCount is always a number; update the expression
that sets transcriptWordCount in web-server.ts to reference
uiState.transcript?.words.length and fall back to 0 when missing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three connected additions plus the paths cleanup they ride on top of:
os.path.join(__file__, "..", "..", ".podcli", ...)across the Python backend with a singleconfig.pathsmodule. TypeScript mirrors the same env-var precedence (PODCLI_HOME,PODCLI_DATA) and propagates both vars to the Python subprocess. Also ignores the.podcli-homeprofile marker so absolute paths can't leak into commits.podcli config <export|import|use|status|migrate>packages the config home (knowledge, presets, assets, settings) into a portable zip and restores it elsewhere. Asset paths in registries/presets are rewritten to point at the imported copies. Imports back up the existing home before clobbering. MCP toolmanage_configand/api/config/*HTTP endpoints expose the same actions. Idempotent migration moves legacyproject/.podcli/cache/content intodata/cache/on first run. Zip extraction rejects symlinks, absolute paths, and parent-traversal members.backend/services/integrations/introduces a registry/manager for editor exporters. DaVinci Resolve is the first integration (opt-in viamanage_integrations action=enable name=davinci_resolve): emits an FCPXML 1.10 project with one compound clip per short on the master timeline, V1 source + optional V2 alpha caption overlay, editable in free or Studio Resolve 20.x. Configure via/integrations.html. Real-clip flow tested end-to-end against 19 rendered shorts.remotion/render.mjs --keep-overlaykeeps a transparent caption overlay alongside the baked output instead of deleting it.clip_generator.generate_clipreturnscaption_overlay_path+cropped_source_pathfor editor handoff and auto-enables the overlay when the DaVinci integration is on, so enabling the integration is the only toggle users need.[3/4] Selecting moments...now persists results to<home>/sessions/clips-<hash>.json. Subsequent runs ofpodcli processon the same video instant-resume instead of re-running Claude/Codex (saves several minutes + tokens per retry on long episodes).--no-resumeopts out.Also fixes a pre-existing
pathsvariable shadow incmd_processthat the new module-levelfrom config.paths import pathsimport would otherwise unmask (localpaths = _thumb_gen(...)in the thumbnail loop turned the dict access into anUnboundLocalError).Test plan
pytest tests/— 298 passing (incl. newtest_config_bundle.pycovering round-trip, backup-on-failure, zip-slip rejection, legacy cache + presets migration)npx tsc --noEmitcleanexport_to_davinci_resolve→ 17 KB FCPXML written → import path documented inbackend/services/integrations/davinci_resolve/README.md.fcpxmlinto free DaVinci Resolve 20.x and confirm the 19 compound clips render identically to the baked outputsdavinci_resolveintegration → runcreate_clip→ verify_captions.movProRes 4444 alpha overlay lands next to the MP4podcli config export ~/test.zip→podcli config import ~/test.zip --home ~/.podcli-alt --activate→ verify knowledge/presets/assets paths resolve correctly under the new homeNotes on scope
DaVinci Resolve integration is marked experimental in code (
default_enabled = False) and in docs. The FCPXML emitter is intentionally constrained to free-tier Resolve structures — no Studio-only effects, noadjust-blend, no transform keyframes (Resolve mis-translates those). Future editor integrations (Premiere, FCP, CapCut) can subclassIntegrationBaseand reuse the_sharedFCPXML/IR helpers.Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Refactor
.podcli-homemarker and environment variables