[codex] Fix managed Ollama runtime extraction#12
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughPins Ollama releases to v0.30.0, requires macOS llama-server in managed runtimes (with an adopted-system-runtime bypass), standardizes chat IPC failure logging to include normalized-history metadata, and extends tests to validate packaged runtime extraction, readiness, and recovery. ChangesOllama Runtime and Chat Logging Enhancement
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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. 🔧 ESLint
ESLint install failed: one or more packages not found in the registry. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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
🤖 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 `@src/main/services/ollama-manager.ts`:
- Around line 465-468: The downloadBinaryUnix function builds an incorrect Linux
URL (uses ollama-linux.tgz) which doesn't exist; change the URL logic in
downloadBinaryUnix to: for darwin keep the existing ollama-darwin.tgz URL, but
for linux pick an architecture suffix from process.arch (map arm64 -> arm64,
otherwise -> amd64) and request ollama-linux-{arch}.tar.zst; also rename
tgzPath/archive variable accordingly and ensure any downstream
filename/extension handling (e.g., extraction logic that references tgzPath) is
updated to use the .tar.zst filename and appropriate extraction flow.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
Run ID: 5335bd4f-d5b6-4638-9fb8-8ef5031326c9
📒 Files selected for processing (3)
src/main/ipc/chat-ipc.tssrc/main/services/__tests__/ollama-onboarding-install.test.tssrc/main/services/ollama-manager.ts
duetCJackson
force-pushed
the
codex/fix-ollama-runtime-sidecars
branch
from
0b4f483 to
413dd88
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/main/services/__tests__/ollama-onboarding-install.test.ts`:
- Around line 212-263: The test's execFileMock currently creates a flat
runtimeDir/ollama which masks the real tar layout; update the mock
implementation in ollama-onboarding-install.test.ts (the execFileMock used when
instantiating OllamaManager) to write both join(rootDir, 'models',
'ollama-runtime', 'bin', 'ollama') and join(rootDir, 'models', 'ollama-runtime',
'lib', 'ollama') (or equivalent nested paths) instead of a single flat file,
then change the assertions that check the extracted binary to assert access on
the bin/ollama path (and/or the lib path) and ensure manager.isReady() checks
align with the new layout so the test verifies the real bin/lib structure rather
than the flat layout.
- Around line 18-32: The functions setArch and loadOllamaManager lack explicit
TypeScript return type annotations; add a return type of void for setArch and
annotate loadOllamaManager with an explicit Promise return type (either a
concrete object type matching the returned bundle or a named interface) so it
satisfies `@typescript-eslint/explicit-function-return-type`; update the
loadOllamaManager signature (the function that returns { OllamaManager,
clearDownloadedComponents, execSyncMock, execFileMock, spawnMock } in tests) to
use a clear exported or inline interface for readability.
In `@src/main/services/ollama-manager.ts`:
- Around line 465-489: downloadBinaryUnix extracts Linux archives with a
bin/ollama layout but getBinaryPath() currently expects runtimeDir/ollama;
update the runtime resolution so Linux uses runtimeDir/bin/ollama (or move the
extracted bin/ollama into runtimeDir) to match where the binary actually lands:
modify getBinaryPath() (and any callers like isReady(), spawn calls, and chmod
invocations in downloadBinaryUnix()) to prefer join(runtimeDir, 'bin', 'ollama')
on non-darwin platforms (or perform a post-extract relocation of bin/* into
runtimeDir and then keep getBinaryPath() unchanged), ensuring
chmod(this.getBinaryPath()), access checks in isReady(), and subsequent spawn
use the real path.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
Run ID: ee316573-cd6e-4677-b324-006749e4d567
📒 Files selected for processing (3)
src/main/ipc/chat-ipc.tssrc/main/services/__tests__/ollama-onboarding-install.test.tssrc/main/services/ollama-manager.ts
duetCJackson
force-pushed
the
codex/fix-ollama-runtime-sidecars
branch
from
413dd88 to
bb43e25
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/services/ollama-manager.ts (1)
549-551:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
llama-serverneeds executable permission when copied from installed runtime.The
copyDirectoryContentsIfMissingmethod only sets executable permission onollama, butllama-serveralso needschmod 0o755on macOS. When adopting from an installed runtime,copyFilemay not preserve the source file's mode, causingllama-serverto be non-executable and model execution to fail despiteisReady()returningtrue.🔧 Proposed fix
await copyFile(sourcePath, destPath) - if (!IS_WIN && entry.name === 'ollama') { + if (!IS_WIN && (entry.name === 'ollama' || entry.name === 'llama-server')) { await chmod(destPath, 0o755) }🤖 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/main/services/ollama-manager.ts` around lines 549 - 551, In copyDirectoryContentsIfMissing, ensure the copied runtime binaries get executable bits set: when iterating entries (check entry.name), in addition to setting chmod(destPath, 0o755) for 'ollama' on non-Windows (IS_WIN), also apply the same chmod for 'llama-server' (macOS/adopted runtime cases) so the copied llama-server is executable; update the condition that references entry.name to include 'llama-server' and call chmod(destPath, 0o755) accordingly.
🤖 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.
Outside diff comments:
In `@src/main/services/ollama-manager.ts`:
- Around line 549-551: In copyDirectoryContentsIfMissing, ensure the copied
runtime binaries get executable bits set: when iterating entries (check
entry.name), in addition to setting chmod(destPath, 0o755) for 'ollama' on
non-Windows (IS_WIN), also apply the same chmod for 'llama-server'
(macOS/adopted runtime cases) so the copied llama-server is executable; update
the condition that references entry.name to include 'llama-server' and call
chmod(destPath, 0o755) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
Run ID: 235e66ad-afa9-49e7-8f31-b12e750987c7
📒 Files selected for processing (3)
src/main/ipc/chat-ipc.tssrc/main/services/__tests__/ollama-onboarding-install.test.tssrc/main/services/ollama-manager.ts
duetCJackson
force-pushed
the
codex/fix-ollama-runtime-sidecars
branch
from
bb43e25 to
2c78b5f
Compare
duetCJackson
force-pushed
the
codex/fix-ollama-runtime-sidecars
branch
from
2c78b5f to
418553e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/services/ollama-manager.ts (1)
193-198:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat a copied lone
ollamabinary as a complete adopted macOS runtime.Line 196 still copies only
which ollamaintoollama-runtime, but Line 197 now bypasses the newllama-serverreadiness check.start()then launches that copied binary fromruntimeDir, so macOS dev fallbacks can still hit the same missing-sidecar failure this PR is fixing for managed downloads. Either keep using the discovered system binary in place, or copy the full runtime bundle instead of a single file.🤖 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/main/services/ollama-manager.ts` around lines 193 - 198, The PR currently copies only the lone system "ollama" executable returned by findSystemOllama() into getBinaryPath() and marks adoptedSystemRuntime=true, which bypasses the new llama-server readiness/sidecar checks in start() and can cause the same missing-sidecar failure; instead either (A) stop copying and use the discovered system binary in-place by storing its path (from findSystemOllama()) and avoiding copyFile/marking adoptedSystemRuntime, or (B) copy the full macOS runtime bundle (not just the single binary) into runtimeDir so the sidecar files are present before setting adoptedSystemRuntime; update canUseSystemRuntimeFallback()/findSystemOllama() handling and ensure start() checks the same readiness for the adopted runtime path you choose.
🤖 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.
Outside diff comments:
In `@src/main/services/ollama-manager.ts`:
- Around line 193-198: The PR currently copies only the lone system "ollama"
executable returned by findSystemOllama() into getBinaryPath() and marks
adoptedSystemRuntime=true, which bypasses the new llama-server readiness/sidecar
checks in start() and can cause the same missing-sidecar failure; instead either
(A) stop copying and use the discovered system binary in-place by storing its
path (from findSystemOllama()) and avoiding copyFile/marking
adoptedSystemRuntime, or (B) copy the full macOS runtime bundle (not just the
single binary) into runtimeDir so the sidecar files are present before setting
adoptedSystemRuntime; update canUseSystemRuntimeFallback()/findSystemOllama()
handling and ensure start() checks the same readiness for the adopted runtime
path you choose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
Run ID: 62c3881d-c604-4c25-b7b7-e6532acbd705
📒 Files selected for processing (3)
src/main/ipc/chat-ipc.tssrc/main/services/__tests__/ollama-onboarding-install.test.tssrc/main/services/ollama-manager.ts
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
Fixes the managed Ollama install path for the refreshed macOS
v0.30.0archive, which now requiresllama-serverand sidecar libraries in addition to theollamabinary.What changed
v0.30.0instead of the moving/latestendpoint.ollamafile.llama-serverbefore treating setup as ready.autodocLog.llama-serverreadiness failure.Root cause
AutoDoc extracted only the
ollamabinary fromollama-darwin.tgz. Ollama refreshed thev0.30.0Darwin artifact on June 1 with a runtime layout that needsllama-serverand sidecar dylibs. The API server could still start and/api/tagscould return OK, but model execution failed withllama-server binary not found.CodeRabbit follow-up
The Linux URL finding was valid against the old non-macOS Unix branch, but Linux is not a supported AutoDoc platform. Rather than add untested Linux support, this PR now makes that path fail clearly as unsupported while preserving the supported macOS/Windows behavior.
Validation
v0.30.0artifact;/api/generatereturned HTTP 200.npm run test:main:run -- src/main/ipc/__tests__/chat-ipc.test.ts src/main/services/__tests__/ollama-onboarding-install.test.tsnpm run test:main:run -- src/main/services/__tests__/ollama-onboarding-install.test.tsnpm run typecheck:node