Repository navigation
fix(local-installation): do not offer a local model other programs leave no memory for - #33
oldwinter53 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Approve with nits (2 minor, 1 nit)
Scope: base main, merge-base 36fff89; 1 commit, 8 files reviewed at c338c9c. Loaded coderbook. ponytail-review and karpathy-guidelines were not loaded; their checklists were applied from memory. Ran in a separate session.
Findings
Minor
-
apps/desktop/src/main/local-server.ts:570: "memory in use" is decided from two different upstream fields.
inspectSetup(packages/local-installation/src/setup-plan.ts:192) makes a computer ineligible whenstack.basis === "capacity". The desktop then setsmemoryInUseonly when the reason list containsfree-memory-required. These are two sources of truth for one fact.- Failure scenario: localmodels reports
basis: "capacity"with a different or extra reason code, or with no reason. The package marks the computer ineligible, but the desktop sets nomemoryInUseand hides the model with no note. That is the same unexplained outcome this PR sets out to fix. - Fix: have
inspectSetupemit the decision once as a structured field (for examplelocalModels.memoryInUse: value.stack.available && value.stack.basis === "capacity"). The desktop then reads that field instead of matching the reason code again.
- Failure scenario: localmodels reports
-
apps/desktop/src/shell/setup.html:2130: the new blocked branch has no test.
The blocked state shows the model row, replaces the needs line with "Other apps are using the memory…", disables the toggle and forcesplan.localModel = false.apps/desktop/src/shell/setup-page.test.tshas no local-model case, and the PR says the page was not looked at in a running app.- Failure scenario: a later edit to
renderLocalletsplan.localModelstay true while the row is blocked. Quick Install would then run the planner that this PR shows fails. - Fix: add one setup-page test for
{ state: "checked", eligible: false, memoryInUse: true, model }. It should assert that the toggle is disabled, the note is shown, and the plan summary leaves out "Local model".
- Failure scenario: a later edit to
Nit
packages/local-installation/src/local-models.ts:289: run-on sentence. Whencausehas no closing period, the message reads "…could not create a local model plan: stack: no verified LLM fits this computer Existing resources were preserved."managed-runtimes.ts:81andmodel-gateway.ts:153put the cause last. Either do the same or add the period yourself.
Verified as fine
assertLocalModelsEligiblenow rejects a capacity-basis plan, so the install path and the desktop check agree.setup-plan.test.tscovers this, and the estimate still names the model.- The planner failure path reuses the existing
localmodelsFailurehelper. It redacts output, splits it into log events and drops progress lines, asmanaged-runtimes.tsdoes. No new parsing was added. - The desktop inspection runs once per app run (
local-server.ts:463), so "Quit them, then reopen Linger" really does re-check memory. - When blocked,
plan.localModelis forced to false and the toggle is disabled, so the default plan (localModel: false) still shows "Quick Install". - No other consumer of
localModels.eligibleinapps/orpackages/is affected. The installer's own page hides the model without a note, which the PR states as a known gap. - No new env vars, contracts, migrations or cross-app imports. All text is English. The commit message follows the conventions.
Process notes
- The branch is BEHIND
main, andmainrequires up-to-date branches, so it needs a rebase before merge.
CI
At review time, CodeQL, check (computer-go), check (contracts), check (teamhub), Analyze (actions) and changes passed. check (desktop), check (installer), check (packages), check (teamhub-cli), check (web) and repository-gate were still pending. No tests were run locally; the change does not touch apps/computer/.
Automated review by Claude Code
Automated review of c338c9c by Claude Code.
There was a problem hiding this comment.
Approve with nits (1 minor; 3 previous findings still open: 2 minor, 1 nit)
Scope: follow-up on c338c9c. Base main, merge base 030a1b0; 3 commits (c338c9c, 236054e, merge e75af75), 8 files reviewed, focusing on the new commit 236054e. Helper skills (coderbook, ponytail-review, karpathy-guidelines) were not loaded as separate skills; I reviewed against the review rules directly. Ran in a separate session.
Previous findings
- Not resolved: Memory-in-use is decided from two different upstream fields.
apps/desktop/src/main/local-server.ts:570still keysmemoryInUseon thefree-memory-requiredreason code, whilepackages/local-installation/src/setup-plan.ts:192keys eligibility onbasis === "capacity". - Not resolved: No test for the setup page's disabled branch.
apps/desktop/src/shell/setup.html(blocked) is still not covered;setup-page.test.tsis unchanged. - Not resolved: Missing sentence separator after the cause. The new
refused()helper (packages/local-installation/src/local-models.ts:229) still builds`${text}: ${cause} Existing resources were preserved.`. The new test even asserts"...held by another program Existing resources were preserved.". BecauseInstallerApinow appendsjob.message(local-server.ts:2290), this run-on sentence is now shown on the desktop setup page too.
Findings
Minor
packages/local-installation/src/local-models.ts:283(return captured ? result : { ...result, stderr: tail }) and:257-258. In the uncapturedstack apply --jsonlrun, every non-progress line goes intotail, including JSON lines, andtailis passed tolocalmodelsFailureas stderr with an empty stdout.errorEvent()(localmodels-failure.ts:38) only scans stdout, so a structured error line from upstream is never parsed. Upstream uses this shape for other commands:localmodels.gateway-error/v1andlocalmodels.harness-error/v1. Whenstack applyexits on such a line, the errorcodeis lost, and the cause falls back to "the last 3 stderr lines joined". The user then sees raw JSON in the outcome message, which now also appears on the desktop page, e.g....did not finish: {"schema":"localmodels.stack-error/v1","code":"...","message":"..."} Existing resources were preserved.. The stdout-only{"kind":"progress"filter in the log also no longer applies to these lines. Fix: return JSON lines from the tail asstdoutand plain text lines asstderr, solocalmodelsFailuretreats an apply failure the same way it treats a captured run. Then add a test where apply emits an error JSON line.
Verified as fine
refused()reuses the existinglocalmodelsFailurehelper for bothstack planandstack apply. It does not copy the cause/log logic, and the old inline plan-failure block was removed.- The
tailbuffer is bounded (8000 chars), and per-chunk parsing still forwards byte progress. The redacted output goes only to installer log events; the outcomecausegoes throughline()redaction and the 300-char cap. captureis unchanged for every command; the only change is thatcapturedis now a named variable.InstallerApikeeps its fixed advice forLOCAL_MODEL_FAILURESand appends the installer message only when one exists; the test covers the appended form.- The changes from the first review (eligibility on
basis !== "capacity", thememoryInUsefield, the disabled row with the note) are unchanged sincec338c9c. - Commit
236054efollows the conventional-commit format. Both commit authors are linked to GitHub accounts, and the branch is not behindmain.
Process notes
- None that need action.
CI
- CodeQL,
changes,check (computer-go),check (contracts),check (teamhub): pass. check (desktop),check (installer),check (packages),check (teamhub-cli),check (web),repository-gate: still pending when this review ran.- I ran no tests locally. The finding follows directly from the code:
errorEventreads onlyresult.stdout, and the apply path returns an empty stdout.
Automated review by Claude Code
Automated review of e75af75 by Claude Code.
There was a problem hiding this comment.
Approve with nits (1 minor carried over, 0 new)
Scope: follow-up on e75af75, reviewing head f938c95 against base main (merge base 030a1b0). One new commit and 2 files changed since the previous review (apps/desktop/src/main/local-server.ts, apps/desktop/src/main/local-server.test.ts). I read the installer's job manager (apps/linger-installer/src/server/jobs.ts) for context. I did not load the helper skills (coderbook, ponytail-review, karpathy-guidelines), so I checked the items from the review checklist directly. This review ran in a separate session.
Previous findings
- Minor: a JSON error line from
applyis treated as stderr (packages/local-installation/src/local-models.ts:283): Not resolved.packages/has not changed sincee75af75, andreturn captured ? result : { ...result, stderr: tail };is still there. When the upstream error line is captured as stderr,errorEvent(which reads stdout) misses it, so the person sees raw JSON. Fix: send JSON lines to stdout and text lines to stderr.
Findings
No new findings in the changes since e75af75.
Verified as fine
- Root cause of the new fix.
JobManager.start(apps/linger-installer/src/server/jobs.ts:270-276) returns any existing job for the instance that is stillactive.activeincludeswaiting_user(jobs.ts:25-26). The desktop only reusesqueued/runningjobs (local-server.ts:2204-2213). So before this commit, awaiting_userattempt fell through toPOST /api/jobsand was handed back every time, with the same failure. Cancelling it first (local-server.ts:2218-2222) fixes this at the source. - What cancel does. For a job that is not executing, cancel marks it
canceledand keeps account, data and device (jobs.ts:473-487,apps/linger-installer/AGENTS.md:12). That makes it safe to run before starting a new job, including thefreshpath and a staleRUNTIME_AUTH_REQUIREDjob whose instance login has gone. - Jobs waiting for local-model approval or recovery. A job waiting on
LOCAL_MODELS_APPROVAL_REQUIREDorLOCAL_MODELS_RECOVERY_REQUIREDis now cancelled and restarted rather than resumed. The new job goes back through the approval and replan handling in the same loop, so the behaviour stays correct; it only re-runs the early stages. - Encoding and errors. The job id is URL-encoded, and a failed cancel throws through
#jsonwith the installer's message. - Test.
starts again past an attempt that stopped waiting for the personasserts the cancel, the newlocal-modelsjob and thereadyphase. Without the fix,canceledwould be empty, so the test fails as it should. - Docs.
apps/desktop/AGENTS.mddoes not describe the job-reuse rule, so no doc update is needed. - Conventions. Comments are in English and explain why. The change imports nothing across apps.
Process notes
- The branch is BEHIND
main, which requires up-to-date branches, so it needs a rebase before merge.
CI
At review time, CodeQL, changes, check (computer-go), check (contracts) and check (teamhub) had passed. check (desktop), check (packages), check (installer), check (web), check (teamhub-cli) and repository-gate were still pending. I ran no tests locally because this worktree has no installed dependencies; the changed desktop test is covered by check (desktop).
Automated review by Claude Code
Automated review of f938c95 by Claude Code.
…ave no memory for localmodels 0.1.0.dev31 reports capable hardware as available, with a capacity plan and a free-memory-required reason, while other programs hold the memory. Installing plans against the memory free then, so the planner refused and setup ended on a generic failure. The report now marks such a computer not eligible, the setup page names the model and says to quit other apps, and a refused plan carries the planner's cause. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
The setup page replaced every local model failure with one fixed sentence, and a stopped deployment carried no cause at all because its output is not captured. The page now appends the installer's message, and a refused plan or stopped deployment names the localmodels cause and logs its redacted output. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
The installer answers a new job request with one still waiting for the person. An attempt that stopped on a local model failure stayed in that state on disk, so every later Try Again, across restarts and upgrades, got the same job back and failed at once without running anything. The desktop now cancels such a job before it asks for a new one. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
f938c95 to
cdafb8a
Compare
There was a problem hiding this comment.
Approve (0 findings)
Scope: follow-up on f938c95. Base main, merge-base 90e380d (current origin/main tip, so the branch is up to date). Reviewed 3 commits and 8 files at cdafb8a. Loaded coderbook, ponytail-review and karpathy-guidelines. Ran in a separate session.
Previous findings
The previous review (f938c95, approve with nits) had no findings, so there is nothing to track.
What changed since f938c95
The branch was rebased onto main and the merge commit e75af75 was dropped. git range-diff shows each of the three commits as patch-identical (=) to its earlier version:
66d7224fix(local-installation): do not offer a local model other programs leave no memory forfef2c1bfix(local-installation): say why a local model deployment stoppedcdafb8afix(desktop): start a new installation past an attempt left waiting
main gained commits in between, but they only touch apps/node-services (runner-gateway adaptive limits), apps/teamhub (voice live canvas), acceptance scenarios and docs/model-scheduling.md. None of these overlap or interact with packages/local-installation or apps/desktop. No code changed since the previous review, so there was nothing new to review.
Findings
None.
Verified as fine
- The three PR commits are byte-identical to the previously reviewed ones (
git range-diff). - The rebase dropped the merge commit, so history is linear on top of
main. - The commits that arrived from
maindo not touch any file this PR changes.
Process notes
- The branch is up to date with
main. Merge state is BLOCKED only on pending checks and the required review.
CI
changes,repository-gate: pending at review time.Analyze (actions),CodeQL: pass.- No tests run locally. Nothing changed since the previous review, and this PR does not touch
apps/computer/.
Automated review by Claude Code
Automated review of cdafb8a by Claude Code.
Summary
Since the localmodels pin moved to 0.1.0.dev31,
setup inspectreports capable hardware asavailable: truewithbasis: "capacity"and afree-memory-requiredreason while other programs hold the memory the model needs. The plan in that report assumes those programs are closed. The installer read onlyavailable, offered the local model, andstack plan, which uses the memory free at that moment, then refused. Setup ended on "This computer can't run the local model right now" with no cause anywhere.inspectSetupreports such a computer as not eligible, keeping the reason and the model the hardware could serve.Verification
darwin-arm64kit on a Mac with 2.4 GB available:setup inspectreturnedavailable: truewithfree-memory-required, and the samestack planthe installer runs exited 2 with "no verified LLM fits".inspectSetupagainst that kit on the same Mac returnseligible: false,reasons: ["free-memory-required"], modellfm3-1.7b.@linger/local-installation: typecheck, lint,setup-plan.test.tsandlocal-models.test.ts.@linger/desktop: typecheck, lint,local-server.test.ts(89 passed).@linger/linger-installer: typecheck.Risks
🤖 Generated with Claude Code