Repository navigation
Conversation
There was a problem hiding this comment.
Approve with nits (1 minor, 1 nit)
Scope: base main, merge base f9f9f63; 2 commits, 21 files reviewed at d498f4f. The helper skills (coderbook, ponytail-review, karpathy-guidelines) were not loaded; the review followed the linger-pr-review checklist directly. It ran in a separate session.
Findings
Minor
apps/desktop/src/main/local-server.ts:1075:configurePiProviderdrops the installer's failure reason and logs nothing. The method calls.catch(() => null), and every non-readyresult becomes one fixed string ("Provider setup did not finish…"). The installer gives specific reasons:Invalid runtime credentials,Model fields require an OpenAI-compatible Pi provider, the endpoint/model rejection fromvalidateModel, a 409 while the instance is locked, orlogin.messagefromnativeLogin.status. The person sees none of them, and diagnostics hold no trace. The siblingstartSignIn(local-server.ts:1055-1060) already passeserror.messageand the login result to#showSignIn. Do the same here; none of these messages contain the key. Also add alocal_server.pi_provider_failedwarn with the status or code only, never the credentials.
Nit
apps/desktop/src/main/local-server.ts:2490: the Stop wait loop has no limit and no signal. AfterPOST /cancel, the loop polls withdelay(this.#pollMs)and passes no signal. If the manager never reaches a checkpoint (a job stuck inrunning), the page stays on "Stopping…" and thelocal.cancelIPC promise never resolves.stop()aborting#abortdoes not end the loop either. A stuck job sat on "installing" before this PR too, so nothing got worse. Still, pass{ signal: this.#abort.signal }todelay, and add a comment saying the wait is unbounded on purpose because the manager owns checkpoints.
Verified as fine
- Cancellation ownership. Preparation and the first installer run share one
InstallationAttempt, throughinstallation ??=and theAbortSignal.anyof the module and attempt signals. When a run returns,#installationis cleared, so a later Stop during sign-in, enrollment or agents is a no-op (cancelInstallationchecksphase === "installing"and the attempt).attempt.finish()runs in both thefinallyinrunInstallerand the one in#install, so a pendinglocal.cancelalways resolves once the attempt settles. - Completion race. The cancel path re-reads the job before posting
/cancel. It treats a finished job, or acanceledjob withRUNTIME_AUTH_REQUIREDand an origin, as done, so a Stop that arrives late still hands off to sign-in. Wideningfinished()affects job reuse only when the instance login probe also succeeds, which is the intended resume behaviour. - Phase handling.
#setresets progress only foridle/preparing, sostopping/cancelledkeep the step and bar.#advancereportsstoppingwhile the attempt is aborted.InstallerStopUnconfirmedis shown asfailedwith a warning that the manager may still be running. The desktop never kills the manager or stops services directly. - Credentials. The Pi form posts once over setup IPC to the existing
startInstallerRuntimeLogincontract (contracts/openapi/linger-installer/openapi.yaml; the type is derived from the generatedoperations, nothing was hand-edited). The installer re-validates on its side (apps/linger-installer/src/server/server.ts:451-476). The key is cleared from the input after submit and on runtime change, and it is never added toSetupState. The zod rules match the installer's (no control or space characters, HTTPS or loopback HTTP, no credentials, query or hash in the URL). - Failure messages.
localModelFailureMessagepassesjob.messagethrough the existingredactLocalmodelsOutputhelper (packages/local-installation/src/localmodels-failure.ts), which the package's./*export already exposes.stackFailureSummaryemits only allowlisted cause codes and component labels, never raw upstream text. Guidance is keyed on error codes, not on prose. - Byte progress. It is accepted only when
unit === "bytes", the values are safe non-negative integers, andtotal >= completed. Otherwise the page shows an indeterminate state. - Web.
BROWSER_CONNECTION_UNSUPPORTEDno longer claims the Computer is outdated. The copy is chosen fromplatform,upgrade_requiredand the existingisUpgradeRecommendedhelper. Status errors are retryable, an invalid saved connection stays repairable, and a fresh error overrides cached data (!current || error). New tests cover these branches. - Docs. The desktop, web and local-installation
AGENTS.mdfiles are updated alongside the code. The user-message →provider_readypath is untouched.
Process notes
mergeStateStatusisBEHIND;mainrequires an up-to-date branch, so this needs a rebase before merge.- The PR comes from the fork
oldwinter/linger, and no checks have run on it. A maintainer must approve the fork workflows so CI runs before merge. - Both commits are authored by the linked account
oldwinter.
CI
No checks were reported for this head (0 status checks). Nothing was rerun locally for this review: the desktop and web suites are CI's to run, and the findings above were confirmed from the code. The PR body reports green local runs of make check-desktop check-installer check-web check-packages check-teamhub-cli check-docs and make check-repository-gate, but CI has not confirmed them.
Automated review by Claude Code
Automated review of d498f4f by Claude Code.
d498f4f to
b84180f
Compare
There was a problem hiding this comment.
Approve with nits (0 new findings; 1 P2 and 1 P3 from the previous review remain open)
Scope: follow-up on d498f4f. Base main, merge base b3ef8c2, 2 commits, 21 files. Loaded coderbook, ponytail-review and karpathy-guidelines. Ran in a separate session.
The new head is a clean rebase onto the current main. git range-diff shows that both commits match the previous review exactly (d58df58 → b2c24a0, d498f4f → b84180f, no content changes). Nothing new needed review.
Previous findings
- P2: Pi provider setup drops the installer's failure reason and logs nothing — Not resolved.
apps/desktop/src/main/local-server.ts:1075still swallows the error with.catch(() => null), and:1082still shows only the generic "Provider setup did not finish…" text. A 400 from the installer, such as an invalid key or an unsupported endpoint, never reaches the user or the logs. Fix: pass the installer message through, asstartSignInalready does at:1055-1060, and add onewarnlog. - P3: The stop-wait loop has no limit and ignores the shutdown signal — Not resolved.
apps/desktop/src/main/local-server.ts:2490still callsawait delay(this.#pollMs)without#abort.signal. If the manager gets stuck inrunning, the page stays on "Stopping" and the IPC call never returns. Fix: pass the abort signal. If the loop is meant to have no limit, say so in a comment.
Findings
None new. The two commits have the same content as the previous review.
Verified as fine
- The range-diff of both commits against the previous head shows no content changes.
- The rebase did not change anything in the previous review's conclusions about the installer cancellation checkpoints, the Stop handoff to runtime sign-in, the masked API-key clearing, or the Web capability/error-recovery logic.
Process notes
- Both commits are authored by
oldwinter, a linked GitHub account. - The merge state is BLOCKED while the required checks are still pending.
CI
8 checks pass: computer-go, installer, packages, teamhub, teamhub-cli, contracts, changes and repository-gate. check (desktop) and check (web) were still pending when this review ran. I did not run any tests myself because the code is the same as at the previous review.
Automated review by Claude Code
Automated review of b84180f by Claude Code.
What and why
Make desktop local setup explain what is happening and provide a safe way to recover:
The changes are based on the current Linger
mainand retain its owner-account screen, local server identity checks, and native installation flow.How it was verified
Desktop: 288 passed. Local installation: 334 passed, 4 skipped. Installer: 124 passed. Web: 3155 passed, with types, lint and React Doctor passing. Shared-package checks, TeamHub CLI (879 passed, 14 skipped), daemon cross-builds, repository scripts, commit/format/lint gates, and documentation links also passed.
GitHub CI was triggered, but run 43027631818 is awaiting approval from a repository maintainer for this fork (
action_required). No remote CI jobs have run yet; the local checks above passed.Regression coverage exercises cancellation checkpoints and completion races, Stop acknowledgement before interactive Pi setup, credential validation and clearing, the new owner-account screen, safe stack failure summaries, and browser capability/error recovery.
The earlier independent source review accepted these fixes. This port additionally verifies their interaction with Linger's newer account and server-identity flow. Three-device baseline testing and controlled setup-page replays are prior evidence; newly packaged real model stop/resume and personal provider authentication remain release acceptance work.
Boundary
Updated the owning Desktop, Web, and local-installation guidance. Existing hardware eligibility, installation data layout, account identity, and API contracts are preserved. The user-message-to-
provider_readypath gains no network round trip or file walk; extra status/cancel requests run only during installation recovery.Rollback is a revert of the two commits. The main risk is installer lifecycle/cancellation behavior; dedicated completion-race and runtime-handoff tests cover it.