Fix stale backend after update, plus title truncation and hour timestamps#65
Merged
Conversation
`podcli update` downloaded and swapped only the launcher binary. The embedded Python backend was extracted solely by `podcli setup`, and `ensureRuntime` only ran setup when `runtime/backend/cli.py` was absent. An updated launcher therefore kept driving whatever backend the release that first provisioned it had installed. The drift was invisible: main.go exports PODCLI_VERSION into the backend, and backend/version.py reads it before its own fallback, so a stale backend reported the launcher's version as its own. It also silently pinned dependencies. ensureBackendDeps reads requirements-runtime.txt from the backend tree and stamps on that file's hash, so a stale tree matched its old stamp and deps never reinstalled, even though the check ran on every invocation. - Stamp the extracted backend with the launcher version, reusing the marker name provision already writes for the studio and Remotion bundles. An unstamped tree is treated as stale, so existing installs self-heal on first run. - Re-extract on version mismatch from every path that executes Python: the runtime commands, the other engine verbs, and the mcp server (stderr-only, so the JSON-RPC channel on stdout stays clean). - Extract before ensureBackendDeps, so a refreshed tree yields a refreshed dep set. - Add `setup --refresh` to re-provision the version-bound artifacts without pulling a model, and run it from `update` against the newly installed binary, since the new backend is embedded in that binary rather than the running one. - Extract before any network provisioning in setup, so an offline or interrupted run still leaves a backend matching the launcher. - Report backend drift in `doctor`. Release 2.4.1.
The version lived in three independent places: the git tag, package.json, and a hardcoded default in main.go. The default had rotted to 2.2.1 while the project shipped 2.4.0, so any build without the release workflow's -ldflags reported a version two minors stale. package.json is now the single source. `go generate` writes cli/VERSION from it and the launcher embeds that. A test fails when VERSION drifts from package.json, and the release workflow refuses to build a tag that disagrees with it. Drops -ldflags "-X main.Version": the linker applies -X only to a constant-initialized string and would silently ignore an embed-initialized one.
Suggested clip titles were cut to 55 characters with a raw slice before being stored, so cards showed "...on an automot" and "...is evolving, bu". The cut was a display concern that had leaked into the data layer: the stored title, the render payload, and the clip history all carried the shortened text, and `.clip-title` already had `text-overflow: ellipsis` that never got a chance to run. Titles are now stored whole. Where a fixed width genuinely applies, truncate_title cuts on a word boundary instead: - claude_suggest keeps the full title (whitespace normalized). - The render payload no longer slices to 40 chars. clip_generator already slugifies and caps the filename on its own, so the slice only corrupted the title stored in clip history. - The CLI's heuristic fallback, which builds a title out of raw transcript text, uses truncate_title. So does terminal output, where the width is real. - .clip-title wraps instead of clipping to one line. Separately, fmt() divided seconds by 60 without rolling into hours, so a clip an hour into an episode read "78:31". HighlightsPage carried its own copy of the same bug; it now uses the shared helper.
📝 WalkthroughWalkthroughThis PR establishes package.json as the single source of truth for versioning, generating cli/VERSION and embedding it into the Go binary, with backend extraction stamping/refresh logic and CI/release validation. Separately, it centralizes title cleaning/truncation in a shared utility, removes 40-character title truncation in exports/UI, and improves time formatting. ChangesVersion stamping and backend refresh
Estimated code review effort: 3 (Moderate) | ~30 minutes Title cleanup and formatting
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/cli.py (1)
2002-2003: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the import out of the loop.
truncate_titleis imported on every iteration of the nestedwin_size/window loop. Functionally harmless (module caching), but inconsistent with the top-level import style used forclean_titleinclaude_suggest.py.♻️ Proposed fix
- if not title: - title = text[:60].strip() - from utils.text import truncate_title - title = truncate_title(title) + if not title: + title = text[:60].strip() + title = truncate_title(title)Add
from utils.text import truncate_titlenear the top of the file alongside other module-level imports.🤖 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 2002 - 2003, The `truncate_title` import inside the nested `win_size`/window loop in `backend/cli.py` should be hoisted to the module level to match the existing import style used elsewhere. Move `from utils.text import truncate_title` up with the other top-level imports and keep the loop body only calling `truncate_title(title)` so the import is no longer repeated on every iteration.cli/gen-version.sh (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider skipping the write when VERSION is already current.
The script unconditionally overwrites
VERSION, which touches the file's mtime even when content is unchanged. This can trigger unnecessary rebuilds in build systems that watch file timestamps. A quick guard before writing would makego generateidempotent:♻️ Optional optimization
printf '%s\n' "$ver" > "$here/VERSION"Could become:
+if [ "$(cat "$here/VERSION" 2>/dev/null)" = "$ver" ]; then + echo "version $ver (unchanged)" + exit 0 +fi printf '%s\n' "$ver" > "$here/VERSION"🤖 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 `@cli/gen-version.sh` around lines 6 - 8, The gen-version.sh script always rewrites VERSION even when the version is unchanged. Update the version generation flow around the existing ver assignment and VERSION write so it first checks the current contents of "$here/VERSION" and skips the write when it already matches ver, keeping go generate idempotent and avoiding unnecessary mtime changes.
🤖 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 `@cli/main.go`:
- Around line 45-50: Both the MCP path in main and the non-runtime branch in
runEngine refresh the backend but then skip ensureBackendDeps, which leaves the
Python environment out of sync after backend requirements change. Update those
flows so they invoke ensureBackendDeps immediately after refreshBackend,
matching the existing ensureRuntime sequence and preserving the clear setup
error path. If adding EnsurePython to the MCP path, verify it is stdout-safe
there; otherwise keep its output off the JSON-RPC channel.
---
Nitpick comments:
In `@backend/cli.py`:
- Around line 2002-2003: The `truncate_title` import inside the nested
`win_size`/window loop in `backend/cli.py` should be hoisted to the module level
to match the existing import style used elsewhere. Move `from utils.text import
truncate_title` up with the other top-level imports and keep the loop body only
calling `truncate_title(title)` so the import is no longer repeated on every
iteration.
In `@cli/gen-version.sh`:
- Around line 6-8: The gen-version.sh script always rewrites VERSION even when
the version is unchanged. Update the version generation flow around the existing
ver assignment and VERSION write so it first checks the current contents of
"$here/VERSION" and skips the write when it already matches ver, keeping go
generate idempotent and avoiding unnecessary mtime changes.
🪄 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: 5c3cbc1e-ede5-4631-aaa5-7c9a65b3df26
📒 Files selected for processing (21)
.github/workflows/release.ymlRELEASE.mdbackend/cli.pybackend/services/claude_suggest.pybackend/utils/text.pycli/VERSIONcli/gen-version.shcli/internal/backend/embed.gocli/internal/backend/embed_test.gocli/internal/update/update.gocli/main.gocli/version.gocli/version_test.gopackage.jsonsrc/ui/client/EpisodeWorkspace.jsxsrc/ui/client/HighlightsPage.tsxsrc/ui/client/lib.test.tssrc/ui/client/lib.tssrc/ui/public/css/styles.csssrc/ui/web-server.tstests/test_text_utils.py
Comment on lines
+45
to
+50
| // Safe on this path despite stdout being the JSON-RPC channel: it only writes | ||
| // to stderr. Skipping it would strand MCP-only clients on a stale backend. | ||
| if err := refreshBackend(); err != nil { | ||
| fmt.Fprintln(os.Stderr, "podcli:", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing ensureBackendDeps after refreshBackend on MCP and non-runtime paths.
Both the mcp path (line 47) and the runEngine non-runtime else if branch (line 162) call refreshBackend but skip ensureBackendDeps. The refreshBackend comment itself states it "Must run before ensureBackendDeps: the dep stamp hashes the backend's own requirements-runtime.txt, so a stale tree pins a stale dep set." After a backend refresh that changes requirements-runtime.txt, the Python venv still holds the old packages, so engine.RunMCP / engine.Run can fail with a confusing ImportError rather than the clear "run podcli setup" message that ensureBackendDeps provides.
ensureRuntime (line 107) correctly calls both. The podcli update flow is also safe because refreshRuntime shells out to setup --refresh, which installs deps. The gap only bites when a new binary is installed outside the update flow (e.g., manual replacement) and the user immediately runs an MCP or non-runtime command.
Verify that provision.EnsurePython does not write to stdout before adding it to the MCP path — stdout is the JSON-RPC channel there. If it does, redirect or suppress stdout for that call.
Proposed fix for runEngine non-runtime path
if wantsRuntime(args) {
if err := ensureRuntime(); err != nil {
fmt.Fprintln(os.Stderr, "podcli:", err)
return 1
}
- } else if err := refreshBackend(); err != nil {
- fmt.Fprintln(os.Stderr, "podcli:", err)
- return 1
+ } else {
+ if err := refreshBackend(); err != nil {
+ fmt.Fprintln(os.Stderr, "podcli:", err)
+ return 1
+ }
+ if err := ensureBackendDeps(); err != nil {
+ fmt.Fprintln(os.Stderr, "podcli:", err)
+ return 1
+ }
}Proposed fix for MCP path (if EnsurePython is stdout-safe)
if err := refreshBackend(); err != nil {
fmt.Fprintln(os.Stderr, "podcli:", err)
os.Exit(1)
}
+ if err := ensureBackendDeps(); err != nil {
+ fmt.Fprintln(os.Stderr, "podcli:", err)
+ os.Exit(1)
+ }
code, err := engine.RunMCP()Also applies to: 162-164
🤖 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 `@cli/main.go` around lines 45 - 50, Both the MCP path in main and the
non-runtime branch in runEngine refresh the backend but then skip
ensureBackendDeps, which leaves the Python environment out of sync after backend
requirements change. Update those flows so they invoke ensureBackendDeps
immediately after refreshBackend, matching the existing ensureRuntime sequence
and preserving the clear setup error path. If adding EnsurePython to the MCP
path, verify it is stdout-safe there; otherwise keep its output off the JSON-RPC
channel.
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.
podcli updatenever updated the backendupdatedownloaded and swapped only the launcher binary. The embedded Python backend was extracted solely bypodcli setup, andensureRuntimeran setup only whenruntime/backend/cli.pywas missing. An updated launcher kept driving whatever backend the release that first provisioned it had installed.The drift was invisible.
main.goexportsPODCLI_VERSIONinto the backend andbackend/version.pyreads it before its own fallback, so a stale backend reported the launcher's version as its own.It also silently pinned dependencies.
ensureBackendDepsreadsrequirements-runtime.txtfrom the backend tree and stamps on that file's hash, so a stale tree matched its old stamp and deps never reinstalled, even though the check ran on every invocation. This is the likely shape of the "missing questionary on Windows" class of report.Fix
provisionalready writes for the studio and Remotion bundles. An unstamped tree counts as stale, so existing installs self-heal.mcpserver (stderr-only, so the JSON-RPC channel on stdout stays clean). Previouslymcpnever calledensureRuntimeat all, stranding MCP-only clients.ensureBackendDeps, so a refreshed tree yields a refreshed dep set.setup --refreshto re-provision version-bound artifacts without pulling a model, and run it fromupdateagainst the newly installed binary, since the new backend is embedded in that binary rather than the running one.doctor.Version had three sources of truth
The git tag,
package.json, and a hardcodedvar Version = "2.2.1"inmain.gowere independent. The default had rotted two minors behind while the project shipped 2.4.0.package.jsonis now the single source:go generatewritescli/VERSIONand the launcher embeds it. A test fails when they drift, and the release workflow refuses to build a tag that disagrees. Drops-ldflags "-X main.Version", which the linker applies only to a constant-initialized string and would have silently ignored.Clip titles were truncated, timestamps never reached hours
Suggested titles were cut to 55 chars with a raw slice before storage, so cards showed
...on an automot. A display concern had leaked into the data layer: the stored title, the render payload, and clip history all carried the shortened text. Titles are stored whole now;truncate_titlecuts on a word boundary where a fixed width genuinely applies.fmt()divided seconds by 60 without rolling into hours, so a clip an hour into an episode read78:31.HighlightsPagehad its own copy of the same bug and now uses the shared helper.Verification
go build,go vet,go test ./...clean.backendpackage tests cover the stale/unstamped/mismatch cases.PODCLI_HOMEwith a stale unstamped backend: a v2.4.1 launcher re-extracts on first run, is idempotent on the second, leaves a repo checkout untouched, refreshes on themcppath without writing to stdout, and lands the backend before the network stalls.setup --refreshverified to download no model.package.jsonbump propagates topodcli versionaftergo generate, and that drift fails the test.tsc --noEmit, 61 vitest tests, and the Python suites pass.Releases 2.4.1.
Summary by CodeRabbit
Bug Fixes
New Features