Consolidate AI provider selection; add optional remote sync - #146
Merged
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds a unified AI provider layer, Podcli Pro authentication and workspace APIs, cloud synchronization for clips, assets, and knowledge files, new sync commands, and UI surfaces for account, insights, and AI setup status. ChangesUnified AI provider execution
Podcli Pro cloud integration
Workspace synchronization
Workspace UI and status routes
Additional model behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PodcliCLI
participant CloudAPI
participant LocalSync
User->>PodcliCLI: login or run sync
PodcliCLI->>CloudAPI: authenticate or verify session
PodcliCLI->>LocalSync: synchronize local workspace data
LocalSync->>CloudAPI: register, upload, download, or update data
CloudAPI-->>LocalSync: synchronization results
LocalSync-->>PodcliCLI: reports and conflicts
PodcliCLI-->>User: status and exit code
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
-dtw was hardcoded to the base alignment-head preset while the model came from settings, so --fast (tiny.en) aborted whisper-cli with exit 3. Derive the preset from the model file, and omit -dtw for models with no preset. Also repoint two tests at the seams the AI provider consolidation moved: they patched claude_suggest._find_ai_cli_candidates, which no longer gates either path, so they only passed on machines with a real CLI installed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_ai_fallback.py (1)
389-399: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClear the discovery caches between tests.
_find_ai_cli_candidatesnow returns_discover(_discovery_key()), and both_discoverand_lookup_dirsarelru_cached. The cache survives across tests in one process, so a test that expects a fresh probe can silently receive a cached list.test_env_override_prefers_podcli_claude_pathassertsfind_mock.assert_called_once(), which fails if_discoverreturns a cached entry for an identical_discovery_key().Add a
setUptoAICliDiscoveryTeststhat clears both caches.💚 Proposed fix
class AICliDiscoveryTests(unittest.TestCase): + def setUp(self): + ai._discover.cache_clear() + ai._lookup_dirs.cache_clear() + def test_find_cli_resolves_windows_cmd_shim(self):🤖 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 `@tests/test_ai_fallback.py` around lines 389 - 399, Add a setUp method to AICliDiscoveryTests that calls cache_clear() on both the _discover and _lookup_dirs lru-cached functions before each test, then invokes the base test setup if needed. Keep the existing test behavior unchanged while ensuring each test performs fresh CLI discovery.
🧹 Nitpick comments (14)
tests/test_entitlement_chain.py (1)
15-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the temporary directory after each test.
setUpcreates a directory withtempfile.mkdtempfor every test method and never deletes it. Each run leaves eight directories in the system temp folder.♻️ Proposed fix
+import shutil + def setUp(self): self.tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.tmp, ignore_errors=True) patcher = mock.patch.dict(podcli_cloud.paths, {"home": self.tmp})🤖 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 `@tests/test_entitlement_chain.py` around lines 15 - 19, Update setUp to register cleanup of the tempfile.mkdtemp directory via addCleanup, ensuring self.tmp is removed after each test while preserving the existing paths patch cleanup.src/ui/client/AiSetup.tsx (1)
92-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle a clipboard write failure.
navigator.clipboardis undefined when the page is served over plain HTTP from a non-localhost host, andwriteTextrejects when the permission is denied. The current code then throws or produces an unhandled rejection, and the label still changes to "Copied". Await the promise and set the state only on success.🤖 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/client/AiSetup.tsx` around lines 92 - 96, Update the onClick handler in AiSetup to await navigator.clipboard.writeText(INSTALL_COMMAND), handle unavailable or rejected clipboard writes without an unhandled rejection, and call setCopied(true) with its timeout only after the write succeeds.src/services/knowledge-sync.ts (1)
109-111: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winWrite the conflict copy relative to the confined target.
Line 81 computed
targetthroughinsideKnowledge, but line 109 rebuilds the path from the rawpathvalue. The two agree today, so this is not a defect. Using${target}.workspace-${version}keeps the confinement guarantee at a single point and prevents a future edit from reintroducing the traversal.♻️ Proposed fix
- await writeFile(join(paths.knowledge, `${path}.workspace-${version}`), - file.content, "utf-8"); + await writeFile(`${target}.workspace-${version}`, file.content, "utf-8");🤖 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/services/knowledge-sync.ts` around lines 109 - 111, Update the conflict-copy write in the knowledge sync flow to derive its destination from the already confined target produced by insideKnowledge, using target for the workspace-version suffix instead of rebuilding the path from raw path. Keep the existing conflict reporting unchanged.src/services/asset-sync.test.ts (1)
14-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
pull.The mock defines only the functions
pushuses.pullhandles server-provided asset names, writes into.podcli/assets/shared, and registers the result, which is the riskier half of this module. AdddownloadAssetto the mock and a test that a remote name containing path separators cannot write outside the assets folder.🤖 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/services/asset-sync.test.ts` around lines 14 - 20, Extend the podcli-cloud mock with downloadAsset, then add pull coverage that supplies a remote asset name containing path separators and verifies the downloaded file remains within .podcli/assets/shared and is registered correctly. Use the existing pull test setup and assert no write occurs outside the assets folder.src/ui/client/WorkspaceInsights.tsx (1)
73-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a stable key that cannot repeat.
key={line}uses the text content. Two identical guidance or observation strings produce duplicate keys, and React drops one of the rows. Combine the index with the text.Also applies to: 90-92
🤖 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/client/WorkspaceInsights.tsx` around lines 73 - 78, Update the guidance and observation mappings in WorkspaceInsights to use a stable key combining each item’s text with its map index, rather than the text alone. Apply this to both the guidance map around the visible rows and the observation map noted in the comment, while preserving the rendered content.src/services/asset-sync.ts (1)
58-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
skippedmixes two different outcomes.
pushpushes toreport.skippedwhen the local file is missing and when the workspace already holds the same checksum. The caller cannot tell a broken registry entry from a successful no-op. Consider a separatemissinglist. This is optional.🤖 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/services/asset-sync.ts` around lines 58 - 83, Separate missing local files from successful no-op outcomes in the asset sync loop: add/use a dedicated report.missing collection for assets failing existsSync(asset.path), while retaining report.skipped for checksum matches and unchanged uploads. Update the report shape and any consumers of the missing-file result accordingly.src/services/podcli-cloud.ts (1)
119-136: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
uploadClipVideobuffers the whole clip in memory.
readFile(filePath)loads up to 200 MB into the heap before the request starts. A stream body avoids the allocation. Node'sfetchaccepts aReadableStreambody whenduplex: "half"is set. This is optional while clip sizes stay small.🤖 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/services/podcli-cloud.ts` around lines 119 - 136, Update uploadClipVideo to stream the video file instead of passing readFile(filePath) as the request body, using a ReadableStream-compatible file stream and setting fetch’s required duplex option to "half"; preserve the existing size validation, headers, timeout, response handling, and return behavior.backend/services/ai_cli.py (2)
85-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable branch and the unused tag.
kindin the first loop is only ever"prefix"or"root", so theelseat lines 89-90 never runs. The second loop tags every entry"bin"and never reads the tag, which Ruff reports as B007.♻️ Proposed simplification
if kind == "prefix": dirs.append(raw if sys.platform == "win32" else os.path.join(raw, "bin")) - elif kind == "root": + else: # "root" dirs.append(os.path.join(raw, ".bin")) - else: - dirs.append(raw) - for args, kind in ( - (["pnpm", "config", "get", "global-bin-dir"], "bin"), - (["pnpm", "bin", "-g"], "bin"), - (["yarn", "global", "bin"], "bin"), + for args in ( + ["pnpm", "config", "get", "global-bin-dir"], + ["pnpm", "bin", "-g"], + ["yarn", "global", "bin"], ):🤖 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/ai_cli.py` around lines 85 - 96, Remove the unreachable else branch from the first loop, keeping only the prefix and root handling. In the second loop, remove the unused kind tag from each tuple and update iteration accordingly so the command-processing logic no longer binds an unread variable.Source: Linters/SAST tools
291-291: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the optional parameter type explicit.
extra_paths: list[str] = Noneis an implicitOptional, which PEP 484 does not allow. Ruff reports RUF013.♻️ Proposed fix
-def _find_cli(name: str, extra_paths: list[str] = None) -> Optional[str]: +def _find_cli(name: str, extra_paths: Optional[list[str]] = None) -> Optional[str]:🤖 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/ai_cli.py` at line 291, Update the _find_cli function signature to explicitly annotate extra_paths as optional while retaining its current default of None; leave the parameter’s list-of-strings behavior unchanged.Source: Linters/SAST tools
backend/services/content_generator.py (1)
228-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
project_dirfor consistency withgenerate_clip_content.This call omits
project_dir, soai_provider.generatefalls back to the backend package directory.generate_clip_contentpasses the repository root at line 397. The AI CLI subprocess therefore runs with a different working directory for the two paths in this module.♻️ Proposed change
result = ai_provider.generate( prompt, timeout=120, + project_dir=os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", ".."), on_attempt=announce, adapt=_shorten_for_codex, )🤖 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/content_generator.py` around lines 228 - 233, Update the ai_provider.generate call in the surrounding content-generation function to pass the same repository-root project_dir used by generate_clip_content. Reuse the existing project directory value or lookup so both AI CLI execution paths run from the repository root.tests/test_ai_fallback.py (1)
424-460: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the command-construction assertions into their own test.
test_get_ai_cli_status_reports_candidatesasserts the status payload at lines 431-432 and then, from line 433, verifies how_run_ai_commandbuilds its argv. The two behaviors are unrelated, and the method name only describes the first. If the status assertions fail, the argv assertions never run.🤖 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 `@tests/test_ai_fallback.py` around lines 424 - 460, Split the `_run_ai_command` command-construction and subprocess assertion block out of `test_get_ai_cli_status_reports_candidates` into a separate clearly named test, leaving that test focused only on the `get_ai_cli_status()` candidate payload assertions.backend/cli.py (1)
1010-1012: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a boolean name instead of a fake path value.
_ai_cli_pathis only tested for truthiness at lines 1032, 1105 and 1304. Assigning the literal"cloud"makes the value wrong whenever the active provider is a local CLI or the Claude API.ai_provideris also already imported at line 923 in this function.♻️ Proposed rename
- # Per-clip content generation needs any provider, not specifically a binary. - from services import ai_provider - _ai_cli_path = "cloud" if ai_provider.available() else None + # Per-clip content generation needs any provider, not specifically a binary. + _ai_available = ai_provider.available()Then replace
_ai_cli_pathwith_ai_availableat lines 1032, 1105 and 1304.🤖 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 1010 - 1012, Replace the fake path variable _ai_cli_path with a boolean _ai_available based on ai_provider.available(), reusing the existing ai_provider import in the function. Update all truthiness checks at the referenced generation paths to use _ai_available, preserving support for any available provider.backend/services/claude_suggest.py (1)
530-550: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe learned prompt block is lost when the chain falls back to a local backend.
learnedis prepended toinstructiononly.local_prompt=promptis passed unchanged, so every non-cloud backend receives the original prompt without the learned block. For a Pro user whose cloud attempt fails, the local retry silently drops the channel-specific context.If that is intended, the existing comment could say so. If not, prepend
learnedtolocal_promptas well.♻️ Proposed change if the block should survive fallback
learned = podcli_cloud.prompt_block() + local_prompt = prompt if learned: instruction = f"{learned}\n\n{instruction}" + local_prompt = f"{learned}\n\n{prompt}" attempt = ai_provider.generate( instruction, @@ - local_prompt=prompt, + local_prompt=local_prompt, )🤖 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/claude_suggest.py` around lines 530 - 550, Ensure the learned prompt block is preserved for local-backend fallback in the ai_provider.generate call. Update the local_prompt value to include learned before the original prompt when present, while keeping the existing prompt unchanged when learned is empty and leaving the cloud instruction flow intact.backend/services/thumbnail_ai.py (1)
482-491: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused local
_extract_jsonhelper.
_extract_jsonis only defined inbackend/services/thumbnail_ai.pyand has no remaining callers;thumbnail_ai.pyusesai_provider.generate_json()instead.🤖 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/thumbnail_ai.py` around lines 482 - 491, Remove the unused _extract_json helper from thumbnail_ai.py, leaving _ask_ai_for_json and its ai_provider.generate_json flow unchanged.
🤖 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 3495-3498: Wrap the ai_provider.status() lookup in print_banner
with the same try/except protection used for the encoder, assets, presets, and
corrections lookups. If provider discovery or auth-file parsing raises, fall
back to an empty providers result and allow the banner to render normally.
- Around line 3642-3648: Update the email assignment in the sign-in flow to
strip surrounding whitespace from both the prompted value and args.email, while
preserving the existing prompt fallback and validation behavior.
In `@backend/main.py`:
- Around line 796-807: Update the failure messages in the generation failure
branches of the main handler and handle_generate_custom to reflect
provider-based availability rather than requiring a local Claude or Codex
binary. Use wording that guides signed-in Pro users and ANTHROPIC_API_KEY users
appropriately while preserving the existing failure behavior.
In `@backend/services/ai_cli.py`:
- Around line 347-378: Update _discovery_key to include the .env file identity
used by services.env_settings._read_pairs, including its path and a
change-sensitive value such as modification time or contents. Ensure _discover
receives this expanded key so CLI discovery invalidates after Studio settings
writes while preserving existing environment-based caching.
In `@backend/services/ai_provider.py`:
- Around line 134-143: Update the JSON extraction loop in the helper containing
the opener iteration to consider “{” and “[” in ascending order of their
positions in body, rather than fixed “{” then “[” order. Preserve raw_decode
parsing and ValueError fallback, ensuring a top-level array is decoded from its
opening bracket and not from a nested object.
In `@backend/services/claude_suggest.py`:
- Around line 517-528: Update usable to require parsed["clips"] to be a list,
rejecting missing, empty, or non-list values with the existing fallback error
behavior. Preserve acceptance for non-empty clip arrays so suggest_with_claude
only iterates valid clip records.
- Around line 552-557: In the failed-attempt branch around the progress_callback
and error_sink handling, append attempt.error directly instead of passing it to
classify_cli_error. Preserve the existing callback and return None behavior,
relying on the provider layer’s classification from _run_cli.
In `@backend/services/podcli_cloud.py`:
- Around line 24-25: Update api_url() to parse the configured URL and allow only
http and https schemes, rejecting or falling back from any other scheme before
callers invoke urllib.request.urlopen. Keep the existing default URL and
trailing-slash normalization behavior, ensuring all request paths use this
validated result.
- Around line 244-259: Update the CloudError handling around register_clip in
the backfill loop to inspect the error status and immediately abort the loop for
terminal authentication or entitlement statuses 401 and 402. Preserve the
existing failed counter and continue behavior for all other CloudError
instances.
- Around line 145-154: Update _describe to validate that the decoded JSON
payload is a dictionary before calling payload.get("error"), falling back to an
empty dictionary for lists, strings, numbers, or other non-object values. Apply
the same guard to the analogous payload access around line 163 so malformed HTTP
error bodies consistently produce CloudError handling instead of AttributeError.
- Around line 53-61: Update _write_auth to create the auth file descriptor with
restrictive 0o600 permissions at open time, rather than relying on a later
chmod. Preserve the existing JSON-writing behavior and ensure the file is safely
replaced or truncated while retaining the requested mode for pre-existing files.
In `@backend/services/transcription_whispercpp.py`:
- Around line 26-40: Update _QUANT_SUFFIX so it recognizes underscore-qualified
K-quantization suffixes such as q4_k_m while preserving existing quantization
matches. Add a regression case covering ggml-large-v3-q4_k_m.gguf and verify
_dtw_preset_for_model returns the supported large.v3 preset.
In `@src/services/asset-sync.ts`:
- Around line 125-127: Update the asset sync logic around target construction
and manager.register: preserve each asset’s remote path beneath the shared
directory so distinct basenames cannot collide, creating any required parent
directories before writing, and register the asset using entry.kind instead of
inferType(target). Keep the existing download and registration flow otherwise
unchanged.
In `@src/services/clips-history.ts`:
- Around line 131-139: Update the clip synchronization flow in
src/services/clips-history.ts#L131-L139 so cloud_synced becomes true only after
uploadClipVideo succeeds, and false when the upload is unavailable or rejected;
update src/services/clips-history.ts#L349-L352 so backfillCloud counts entries
only when both cloud_id and cloud_video_uploaded are set; add the false-return
upload case in src/services/clips-history-cloud.test.ts#L61-L99 and assert the
entry remains unsynchronized and backfillCloud reports failure.
In `@src/services/knowledge-sync.test.ts`:
- Around line 21-25: Update the beforeEach setup in the knowledge sync tests to
remove the sync state file knowledge-sync.json from tmp before recreating the
knowledge directory. Keep the existing directory cleanup and mock reset,
ensuring each test starts with an empty sync state map.
In `@src/services/knowledge-sync.ts`:
- Around line 51-54: Update localFiles to scan paths.knowledge recursively so it
returns markdown files within nested directories, preserving their relative
paths for the push phase; this must include files such as brand/voice.md created
by insideKnowledge. Keep the existing empty-directory behavior and sorting
intact.
In `@src/services/podcli-cloud.ts`:
- Around line 201-205: Update the direct fetch calls in putKnowledge,
uploadAsset, and downloadAsset to include AbortSignal.timeout, matching
uploadClipVideo’s pattern; use the standard timeout for knowledge requests and
the longer asset-transfer timeout for uploadAsset and downloadAsset so sync
operations cannot remain pending indefinitely.
- Around line 161-167: Update getInsights and getPreferences to return
Promise<Insights | null> and Promise<Preferences | null>, respectively, matching
request’s nullable empty-body result while preserving their existing request
calls.
In `@src/ui/client/AiSetup.tsx`:
- Around line 46-53: The API status handling in AiSetup must reject non-OK
responses and normalize missing providers and candidates to empty arrays before
setStatus; update src/ui/client/AiSetup.tsx lines 46-53 accordingly. In
WorkspaceInsights, update src/ui/client/WorkspaceInsights.tsx lines 44-45 to
access insights.guidance and preferences.titleEdits with optional chaining,
matching the existing preferences?.observations handling.
In `@src/ui/client/ConfigPage.tsx`:
- Around line 173-174: Update ConfigPage’s AiSetup integration so it refreshes
provider status after saveSetting or clearSetting invokes refreshAiCli. Pass a
refreshToken or equivalent trigger from ConfigPage into AiSetup and include it
in AiSetup’s status-fetch useEffect dependencies, preserving the existing
initial fetch behavior.
---
Outside diff comments:
In `@tests/test_ai_fallback.py`:
- Around line 389-399: Add a setUp method to AICliDiscoveryTests that calls
cache_clear() on both the _discover and _lookup_dirs lru-cached functions before
each test, then invokes the base test setup if needed. Keep the existing test
behavior unchanged while ensuring each test performs fresh CLI discovery.
---
Nitpick comments:
In `@backend/cli.py`:
- Around line 1010-1012: Replace the fake path variable _ai_cli_path with a
boolean _ai_available based on ai_provider.available(), reusing the existing
ai_provider import in the function. Update all truthiness checks at the
referenced generation paths to use _ai_available, preserving support for any
available provider.
In `@backend/services/ai_cli.py`:
- Around line 85-96: Remove the unreachable else branch from the first loop,
keeping only the prefix and root handling. In the second loop, remove the unused
kind tag from each tuple and update iteration accordingly so the
command-processing logic no longer binds an unread variable.
- Line 291: Update the _find_cli function signature to explicitly annotate
extra_paths as optional while retaining its current default of None; leave the
parameter’s list-of-strings behavior unchanged.
In `@backend/services/claude_suggest.py`:
- Around line 530-550: Ensure the learned prompt block is preserved for
local-backend fallback in the ai_provider.generate call. Update the local_prompt
value to include learned before the original prompt when present, while keeping
the existing prompt unchanged when learned is empty and leaving the cloud
instruction flow intact.
In `@backend/services/content_generator.py`:
- Around line 228-233: Update the ai_provider.generate call in the surrounding
content-generation function to pass the same repository-root project_dir used by
generate_clip_content. Reuse the existing project directory value or lookup so
both AI CLI execution paths run from the repository root.
In `@backend/services/thumbnail_ai.py`:
- Around line 482-491: Remove the unused _extract_json helper from
thumbnail_ai.py, leaving _ask_ai_for_json and its ai_provider.generate_json flow
unchanged.
In `@src/services/asset-sync.test.ts`:
- Around line 14-20: Extend the podcli-cloud mock with downloadAsset, then add
pull coverage that supplies a remote asset name containing path separators and
verifies the downloaded file remains within .podcli/assets/shared and is
registered correctly. Use the existing pull test setup and assert no write
occurs outside the assets folder.
In `@src/services/asset-sync.ts`:
- Around line 58-83: Separate missing local files from successful no-op outcomes
in the asset sync loop: add/use a dedicated report.missing collection for assets
failing existsSync(asset.path), while retaining report.skipped for checksum
matches and unchanged uploads. Update the report shape and any consumers of the
missing-file result accordingly.
In `@src/services/knowledge-sync.ts`:
- Around line 109-111: Update the conflict-copy write in the knowledge sync flow
to derive its destination from the already confined target produced by
insideKnowledge, using target for the workspace-version suffix instead of
rebuilding the path from raw path. Keep the existing conflict reporting
unchanged.
In `@src/services/podcli-cloud.ts`:
- Around line 119-136: Update uploadClipVideo to stream the video file instead
of passing readFile(filePath) as the request body, using a
ReadableStream-compatible file stream and setting fetch’s required duplex option
to "half"; preserve the existing size validation, headers, timeout, response
handling, and return behavior.
In `@src/ui/client/AiSetup.tsx`:
- Around line 92-96: Update the onClick handler in AiSetup to await
navigator.clipboard.writeText(INSTALL_COMMAND), handle unavailable or rejected
clipboard writes without an unhandled rejection, and call setCopied(true) with
its timeout only after the write succeeds.
In `@src/ui/client/WorkspaceInsights.tsx`:
- Around line 73-78: Update the guidance and observation mappings in
WorkspaceInsights to use a stable key combining each item’s text with its map
index, rather than the text alone. Apply this to both the guidance map around
the visible rows and the observation map noted in the comment, while preserving
the rendered content.
In `@tests/test_ai_fallback.py`:
- Around line 424-460: Split the `_run_ai_command` command-construction and
subprocess assertion block out of `test_get_ai_cli_status_reports_candidates`
into a separate clearly named test, leaving that test focused only on the
`get_ai_cli_status()` candidate payload assertions.
In `@tests/test_entitlement_chain.py`:
- Around line 15-19: Update setUp to register cleanup of the tempfile.mkdtemp
directory via addCleanup, ensuring self.tmp is removed after each test while
preserving the existing paths patch cleanup.
🪄 Autofix
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 Plus
Run ID: efbd875b-1281-410b-b7f7-849a724bd3d3
📒 Files selected for processing (37)
.gitignorebackend/cli.pybackend/main.pybackend/services/ai_cli.pybackend/services/ai_provider.pybackend/services/claude_suggest.pybackend/services/content_generator.pybackend/services/env_settings.pybackend/services/integrations/youtube/learnings.pybackend/services/podcli_cloud.pybackend/services/thumbnail_ai.pybackend/services/transcription_whispercpp.pycli/internal/engine/engine.gocli/main.goscripts/build-studio.shsrc/models/index.tssrc/services/asset-sync.test.tssrc/services/asset-sync.tssrc/services/clips-history-cloud.test.tssrc/services/clips-history.tssrc/services/knowledge-sync.test.tssrc/services/knowledge-sync.tssrc/services/podcli-cloud.tssrc/sync.tssrc/ui/client/AccountChip.tsxsrc/ui/client/AiSetup.tsxsrc/ui/client/AnalyticsPage.tsxsrc/ui/client/ConfigPage.tsxsrc/ui/client/Layout.tsxsrc/ui/client/WorkspaceInsights.tsxsrc/ui/public/css/styles.csssrc/ui/web-server.tstests/test_ai_fallback.pytests/test_entitlement_chain.pytests/test_find_moments.pytests/test_suggest_handler.pytests/test_whispercpp_adapter.py
Twenty threads, and the ones that mattered were about claiming success that had not happened: - `podcli sync` marked a clip synchronised before its video upload, so it could exit happy while every share link played nothing. - Nested knowledge files were pulled once and never pushed again: the local scan was not recursive, so later edits to `brand/voice.md` never went back. - Two workspace assets whose names differed only by folder collided on one local file, and the workspace's own kind was thrown away in favour of guessing from the extension. - The backfill kept hashing and uploading after a 401 or 402 that was going to refuse every remaining clip. - Three fetches had no timeout, so one stalled connection hung a whole sync. - The auth file was created with the default umask and chmodded afterwards, leaving the session token briefly readable by anyone on the machine. - `q4_k_m` models lost `-dtw` because the quantisation suffix pattern missed underscore-qualified names. Covered by a regression case now. - The CLI discovery cache ignored the .env file it also reads, so saving a path in the studio did nothing until the process restarted. The rest are smaller: JSON extraction now tries whichever opener comes first, a model answering with an object where a list belongs is a failed attempt rather than an AttributeError, a cloud 401 no longer tells someone to log into a CLI they do not use, and the settings panels stop disagreeing about whether a CLI was found.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/services/claude_suggest.py (2)
517-528: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve provider fallback when all clips are unusable.
usablereturnsTruefor any non-emptyclipslist. Normalization later drops invalid records at Lines [586-591]. If the first provider returns only short, malformed, or otherwise unusable clips,ai_provider.generatestops there and Lines [619-630] returnNonewithout trying the next provider.Make acceptance use the same semantic validation as normalization, or continue the provider chain when normalization produces no usable clips.
Also applies to: 586-630
🤖 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/claude_suggest.py` around lines 517 - 528, Update the provider acceptance flow centered on usable and the normalization logic around ai_provider.generate so a non-empty clips list is accepted only when it yields at least one semantically valid clip under the existing normalization rules. If normalization removes every clip, reject that provider result and continue to the next provider instead of returning None; preserve the existing behavior for results containing usable clips.
490-504: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject malformed timestamps instead of converting them to zero.
If
startis"invalid"andendis30,_parse_secondsreturns0.0for the start. The code then accepts a 30-second clip from the beginning of the episode. This silently selects the wrong content.Return an invalid marker for parse failures. Discard the record unless both timestamps are finite and non-negative. Preserve the default only for genuinely missing fields.
Also applies to: 575-591
🤖 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/claude_suggest.py` around lines 490 - 504, Update _parse_seconds to return an invalid marker for malformed or unsupported timestamp values instead of 0.0, while retaining the existing default behavior for genuinely missing fields. In the record-processing logic around the start/end timestamp handling, discard records unless both parsed values are finite and non-negative, preventing malformed starts from being treated as episode time zero.
🤖 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/services/claude_suggest.py`:
- Around line 526-528: Strengthen clip validation in the response handling
around the primary and alternate `clips` paths: require every clip to be a
dictionary before normalization, and validate nested scores, segments, and text
fields before accessing or extending them. Skip malformed records safely or
return the existing no-clips result, ensuring values reaching `c.get(...)` and
the alternate clip aggregation cannot be null, strings, or other invalid types.
---
Outside diff comments:
In `@backend/services/claude_suggest.py`:
- Around line 517-528: Update the provider acceptance flow centered on usable
and the normalization logic around ai_provider.generate so a non-empty clips
list is accepted only when it yields at least one semantically valid clip under
the existing normalization rules. If normalization removes every clip, reject
that provider result and continue to the next provider instead of returning
None; preserve the existing behavior for results containing usable clips.
- Around line 490-504: Update _parse_seconds to return an invalid marker for
malformed or unsupported timestamp values instead of 0.0, while retaining the
existing default behavior for genuinely missing fields. In the record-processing
logic around the start/end timestamp handling, discard records unless both
parsed values are finite and non-negative, preventing malformed starts from
being treated as episode time zero.
🪄 Autofix
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 Plus
Run ID: f212f2b2-39da-4afe-9bbd-37ec94bef710
📒 Files selected for processing (15)
backend/cli.pybackend/main.pybackend/services/ai_cli.pybackend/services/ai_provider.pybackend/services/claude_suggest.pybackend/services/podcli_cloud.pybackend/services/transcription_whispercpp.pysrc/services/asset-sync.tssrc/services/clips-history.tssrc/services/knowledge-sync.test.tssrc/services/knowledge-sync.tssrc/services/podcli-cloud.tssrc/ui/client/AiSetup.tsxsrc/ui/client/ConfigPage.tsxtests/test_whispercpp_adapter.py
🚧 Files skipped from review as they are similar to previous changes (11)
- src/ui/client/ConfigPage.tsx
- tests/test_whispercpp_adapter.py
- src/ui/client/AiSetup.tsx
- src/services/knowledge-sync.ts
- backend/services/transcription_whispercpp.py
- src/services/asset-sync.ts
- backend/main.py
- src/services/knowledge-sync.test.ts
- backend/services/podcli_cloud.py
- src/services/clips-history.ts
- src/services/podcli-cloud.ts
Three of them reached further than the review asked:
The recursive knowledge scan used `readdir({ recursive })` and
`dirent.parentPath`, which need Node 20.1 and 20.12. podcli supports 18 and
CI runs 20, so that crash would have found Node 18 users rather than the
build. Walked by hand instead.
Dropping `classify_cli_error` entirely took the "run `claude` once in a
terminal" advice away from the local CLI users it is written for. It is now
skipped only for cloud and API attempts, which is what it was rewriting
wrongly. The CLI path tags an attempt with its engine name rather than "cli",
so the condition asks the question the other way round.
A clip whose rendered file is gone can never be uploaded, so it is settled
rather than reported as failed on every run, which is the unfixable number
this file already refuses to print.
The asset kind list omits a type that is valid at both ends, which reads as an oversight. It is not: 'other' is what anything unrecognised uploads as, so taking it back would turn a local video into 'other' on the round trip.
Two of the earlier fixes stopped at the first call site, and one of them created a new hole: - The "no AI available" message was corrected in one of the two places that print it. `handle_generate_custom` still sent cloud users to install a binary they will never use. - `AiSetup` learned to reject a bad payload; `WorkspaceInsights` reads the same one and still dereferenced arrays the server can omit. - Requiring `clips` to be a non-empty list still admits a null or a string inside it, and the alternate responses were not checked at all, so `.get` on one of those raised out of a path with nothing above it to catch. Clip records, their scores and their segments are now each checked before use.
Merged
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.
AI provider selection. Discovery, invocation and error classification lived across
claude_suggestand its callers, each locating the CLI again. They now shareai_clifor discovery andai_providerfor choosing a backend. Discovery results are cached against the environment they were derived from — each probe shells out to npm, pnpm and yarn for roughly three seconds, and callers ask several times per render.Optional remote sync. Opt-in, off unless configured. Reconciles clip history, assets and knowledge files with a remote workspace and uploads rendered clips. Source video is never uploaded. With sync unconfigured, behaviour is unchanged.
Server-supplied paths are resolved and confined before any write, and knowledge reconciles before pushing so shipped defaults cannot overwrite edited files.
Python 595 pass (3 pre-existing failures on main), TypeScript 243 pass, tsc clean, Go build and vet clean.
Summary by CodeRabbit
New Features
podcli syncfor one-command synchronization.Bug Fixes