fix(update): make the update flow report the truth and stop wedging - #38
Merged
Conversation
The CDN manifest took its version from apps/pythinker-code/package.json and the site autodeploys on every push to main, so a `ci: release packages` merge advertised the next version before — and, when a changeset landed while the version PR was open, without — npm and the GitHub release assets ever getting it. Clients then polled GitHub for assets that did not exist for about six minutes on every launch. Derive the advertised version from the npm dist-tag instead, take publishedAt from npm's own publish timestamp so unrelated site deploys stop re-anchoring the client rollout window, and run the release consistency check on version-bump merges that published nothing — it was gated on a successful publish, so it skipped exactly the case where the version and the published artifacts diverge.
The CDN manifest advertised a version only, so a client had to guess a GitHub asset URL and poll for six minutes when the guess was wrong. `latest.json` now carries the resolved per-platform artifact (url + sha256), copied from the release's own native manifest.json, and the client exposes one predicate over it. A manifest with no `platforms` key, or an unparseable one, still resolves to available: a CDN blip must never stop a working update. A manifest that explicitly omits the running platform is a definitive denial.
A grep for max-time/connect-timeout over install.sh returned 0. A connection that accepted and never answered left the installer running forever, and its pid stays recorded as the active update, so one hung request wedged every update path with no expiry. curl now gets a connect cap, a per-attempt ceiling and a stall guard. The script owns retry, so --retry is deliberately absent: it resets the --max-time counter on every attempt. wget gets -T and nothing else, the one timeout flag BusyBox also understands — GNU's long options abort the install outright on Alpine-class systems. The PowerShell installer bounds the archive with a cancellation token instead. HttpClient.Timeout cannot do that job: its setter throws once the client has sent a request, and the metadata calls run first, and with ResponseHeadersRead it never covered the streaming body at all. Verified through the wired helpers, not by inspection: _fetch and _download_quiet against a black-holed address both abort after 10s with curl exit 28.
…latform The client trusted the manifest version alone, so it advertised, prompted for and background-installed a release that had no build for the running platform. That is what left an installer polling GitHub for six minutes on every launch. One predicate, three call sites: the passive preflight, the TUI /update path and `pythinker upgrade`. Only the native source is gated — npm-family sources install from the registry, where the published version is the artifact, and homebrew installs through its formula, so suppressing those would be a regression. That exemption is the case the tests protect hardest. The preflight still refreshes in the background when it declines, or a client would freeze on the cached answer and never learn about the next release.
…ller install.sh rendered a determinate bar only when stdout was a TTY. The background installer has no TTY, and in that branch the script did not merely skip rendering — it called a single blocking curl and reported nothing at all, which is why an update in flight looked identical to one that was wedged. The non-animated branch now polls the same way the animated one does and emits one newline-terminated line per state on stderr, the stream the parent already pipes. The parent reads stderr by line: progress lines are parsed and recorded on the active install record, and are kept out of the failure tail, or a long download would evict the very error text that buffer exists to preserve. percent, transferred and total travel together, so an unknown size degrades to indeterminate instead of showing a fabricated percentage. Proven by execution against a local throttled HTTP server, with a known and an unknown Content-Length; nothing renders it yet.
…rompt bar The footer carried a progress slice that nothing dispatched and nothing tested: no producer, no consumer, rendered into the activity row above the composer. It is now the update slice, rendered in the status row under the composer where the request was for it to appear, reusing the context gauge's own bar glyphs so the two read as one design. An availability chip becomes a live download bar and then a restart prompt: `↑ v0.11.0`, `↓ v0.11.0 ▰▰▰▱▱▱▱▱ 42%`, `↑ v0.11.0 restart to apply`. An unknown download size renders without a bar rather than inventing a percentage. The mapping from persisted state to the slice is a pure function; the poll that feeds it resolves the install source once, reads both state files off the render path every two seconds, and dispatches only when the result changes.
Two compatibility paths, both now actively harmful, both deleted rather than
improved.
The client fell back to the plain-text /latest endpoint whenever latest.json
failed to parse. That endpoint carries no per-platform artifact data, so the
fallback turned "cannot verify this platform has a build" into "verified" and
re-opened the hole the platforms key exists to close. It also could not fail
independently: both files come from the same generator in the same deploy, and
the manifest schema already tolerates unknown fields and a malformed platforms
value without failing the parse. A bad manifest now keeps the cached answer
instead of silently downgrading to an unverifiable one. /latest is still
published for install.sh and for clients shipped before the manifest.
apps/site/public/ held byte-identical copies of install.sh and install.ps1,
which is how a one-sided edit ships silently. There is one checked-in installer
now; the site root path keeps working because build-cdn places the file there.
fetchLatestFromCdn returned {latest, manifest} where latest was always
manifest.version, so the wrapper type is gone with it.
A live pid used to hold either lease forever with no ceiling, so a recycled pid wedged every update path permanently and nothing expired it. The rule now lives in one place. Two files each carried their own copy of isProcessRunning and their own age arithmetic, which is how they drifted apart: the lock file capped a pid-less lease at 30 minutes while the active record capped it at 6 hours, and neither capped a live one at all. isLeaseFresh takes the ceilings as arguments, so the two leases keep their different pid-less windows without keeping different implementations. hasFreshActiveInstall moves next to the record it reads, so the foreground upgrade command can ask the same question instead of growing a third copy. The preflight test mocked the whole install-state module, which silently replaced the predicate under test with undefined; it now fakes only the file IO.
The background installer's terminal state write lives in the parent process, and the product tells the user to close that terminal. When they do, the active record stays behind, no failure is recorded, and the attempt counter never advances — so the version that cannot succeed is retried on every launch. A real state file showed attempts: 1 after hours of retrying. Startup now reconciles an active record whose lease has expired into one more recorded failure and clears it, which lets the existing threshold park the version. No new counter, no bookkeeping at spawn time. failureAttemptsFor moves next to the record it reads, so install-state owns the lease rule, the failure counter and the reconciliation together, and preflight is left as the orchestrator that calls them.
`pythinker upgrade` imported neither the lock nor the install state, and the preflight prompt path fell through a guard that returns before the active-record check. Either could run a foreground install while a detached background one was already writing the same executable, and neither recorded its outcome — so a stale active record kept misleading the state machine afterwards. Both now refuse when an install is in flight, take the lock only after the prompt resolves (holding it across an interactive wait would block the background path for as long as the prompt sits unanswered), release it in a finally, and write lastSuccess or lastFailure with the same shapes and the same attempt counter the background path uses. hasFreshActiveInstall is imported, not injected: a pure predicate that never varies per call site does not need a seam, and the test was passing the real one anyway.
The reported screenshot: the banner said v0.11.0 was available, /update answered "Update to v0.10.0 already in progress". Two surfaces, two versions, no explanation, so the update read as stuck. The in-progress result now carries both — installingVersion, named so it cannot be mistaken for the target, plus the target when it is strictly newer — and the notice becomes "Installing v0.10.0 — v0.11.0 will follow" with a body that says the running install finishes first. The single-version case keeps its wording. Not implemented on purpose: killing the running installer to switch targets. Its lease is now bounded and startup reconciles an abandoned one, so the wait is finite; killing a live installer that is writing the executable is the most dangerous edit in the report for the smallest gain.
The preflight started a background install from the cached target and only then refreshed and decided again. A real rollout log shows the cached path selecting 0.10.0 twice while latest.json already advertised 0.11.0 — so the app launched a multi-minute installer against a version it was about to stop advertising. The second decision was already the correct one, so it is now the only one: the cached decision just answers "is anything worth refreshing for", and everything after the bounded refresh uses the refreshed target and the refreshed manifest, falling back to the cached pair when the refresh fails or times out. The duplicate install attempt is deleted, and with it refreshInBackground — the bounded refresh has always just run on every surviving path. Costs up to one second before a background install starts when an update is pending. That wait was already paid on this path, just later.
The chip is the surface that produced the reported confusion: it read updates/latest.json directly, was computed once at startup and never recomputed, and knew nothing about the install source or about whether the version it advertised had a build for this platform — so it announced v0.11.0 while /update was talking about v0.10.0. It was also blocking IO in the render path: the gutter re-renders every child every frame, and each frame called readFileSync. The footer now carries a live update chip in the status row under the prompt, computed from real install state and gated on installability. The fix for two surfaces disagreeing is one surface, so the chip and the subtitleChip option it was the only producer of are gone, along with the border branch that existed to place it.
The staged rollout can hold an update back for as long as its batch plan says. That is right for an ordinary release and wrong for one a client cannot skip — a protocol change, a revoked credential, a service that no longer answers the old client. There was no way past the delay. latest.json may now carry minRequiredVersion. A client below it gets the target regardless of rollout eligibility, with reason 'required' so the decision log and telemetry still say where the device sat in the plan, and the footer labels it `↑ v0.11.0 required`. The floor lives in one predicate next to the other manifest questions, so the rollout and the footer cannot disagree about who is below it. An unreadable declaration answers false: a value we cannot parse must not escalate an update on its own. Setting it is a policy decision, so it is a constant in build-cdn.mjs rather than an env var — it should be visible in the diff that makes it.
A version parks once the failure counter hits the threshold — the background lifecycle refuses to touch it again. /update answered with a bare manual command that mentioned neither the failures nor their reason, even though the installer's own error text was sitting in lastFailure.message. That is the last piece of "feels stuck, nothing happened": nothing was running, nothing would run, and nothing said so. /update now reports the failure with its attempt count, the recorded reason and the command to run by hand. The reason is collapsed to one line and truncated, because the recorded value is up to 2 KB of installer stderr. No counter reset and no retry: re-running an install that already failed twice is what burned six minutes per launch in the reported case.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe update flow now uses validated CDN manifests, platform-specific artifact checks, minimum-version enforcement, shared install leases, persisted progress and failures, installer network timeouts, and footer-based TUI status reporting. ChangesUpdate flow reliability
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TUI
participant UpdateCache
participant InstallState
participant Footer
TUI->>UpdateCache: poll cached manifest
TUI->>InstallState: poll persisted install state
TUI->>Footer: derive update status
Footer-->>TUI: render availability or progress below prompt
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
commit: |
… bytes Two seams were verified only by inference. Nothing in the suite drove startUpdateStatusPolling through pollUpdateStatus to dispatchFooter, so the status-row chip could have been dead code with every test still passing — a startup test now drives the real poller against real state files and asserts the rendered row. And the line reader had only ever parsed hand-written fixtures, so it now pins the exact bytes a real install.sh run emitted, which also records the throttle's real behaviour: three lines in one chunk write the first update and the terminal one, not the middle.
Resolves apps/site/scripts/build-cdn.mjs by keeping both sides: main's RELEASE_VERSION guard and strict publishedAt from the #37 review, plus this branch's resolvePlatformArtifacts, MIN_REQUIRED_VERSION and the manifest fields that name each platform's artifact. Verified by running the merged script: it writes 0.9.2 with npm's own publish time and six platform entries with real checksums.
Contributor
Author
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
apps/pythinker-code/test/tui/commands/update-preferences.test.ts (1)
350-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a multi-line failure reason.
renderUpdateFailureReasonexists to collapse an installer stderr tail onto one line. The current long-reason fixture is a single line ofxcharacters, so the\s+collapse is never exercised. A real recordedmessagecontains newlines. Add a short multi-line fixture that stays under 160 characters, so the test asserts the collapse without also asserting truncation.💚 Proposed additional test
+ it('collapses a multi-line recorded reason onto one line', async () => { + const host = makeHost(); + mocks.startManualUpdate.mockResolvedValue({ + status: 'failed', + version: '0.10.0', + attempts: 2, + failedAt: '2026-08-05T08:00:00.000Z', + message: 'npm ERR! code EACCES\nnpm ERR! syscall mkdir\n\n npm ERR! path /usr/local/lib ', + command: 'npm install -g `@pythoughts/pythinker-code`@0.10.0', + }); + + await handleUpdateCommand(host, ''); + + expect(host.showError).toHaveBeenCalledWith( + 'Update to v0.10.0 failed after 2 attempts.\n' + + 'Reason: npm ERR! code EACCES npm ERR! syscall mkdir npm ERR! path /usr/local/lib\n' + + 'To update manually, run: npm install -g `@pythoughts/pythinker-code`@0.10.0', + ); + });🤖 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 `@apps/pythinker-code/test/tui/commands/update-preferences.test.ts` around lines 350 - 369, Add a separate test case near the existing long-reason test in the update command tests using a short failure message containing newline characters and remaining under 160 characters. Mock startManualUpdate with that message, invoke handleUpdateCommand, and assert showError receives the reason collapsed to a single line without the truncation suffix.Source: Path instructions
apps/pythinker-code/src/tui/runtime/footer/footer-model.ts (1)
472-474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe update item is the only status item with no
statusLinegate.Every other item in
selectStatusItemsis behind a flag:showModel,showContextBar,showGit,showModes,showElapsed,showGoal,showBackgroundTasks. The update item is pushed unconditionally and takes the first position, so a user who trimmed the status line cannot suppress it. Add a flag toStatusLineConfigand gate the item, or confirm that always-on is the intended product decision for the required/failed states.♻️ Proposed gate
const items: string[] = []; - const update = formatUpdate(state.update); - if (update !== null) items.push(update); + if (statusLine.showUpdate) { + const update = formatUpdate(state.update); + if (update !== null) items.push(update); + }Add
showUpdatetoStatusLineConfigand default it totrueinDEFAULT_STATUS_LINE_CONFIGso existing configurations keep the new indicator.🤖 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 `@apps/pythinker-code/src/tui/runtime/footer/footer-model.ts` around lines 472 - 474, Update the status-line configuration to add a showUpdate flag, defaulting it to true in DEFAULT_STATUS_LINE_CONFIG, and gate the update item in selectStatusItems before pushing the formatted value. Preserve existing update formatting and behavior when the flag is enabled.apps/pythinker-code/src/cli/update/cdn.ts (1)
50-54: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider dropping only the malformed platform entry, not the whole record.
z.record(...).catch(undefined)fails as a unit. One bad entry erases every valid entry, andmanifestArtifactAvailabilitythen reports'available'for every platform. The documented intent is to keep the manifest usable, but a per-entry filter keeps the artifact gate working for the platforms that did parse.♻️ Proposed per-entry filter
platforms: z - .record(z.string(), UpdateManifestPlatformSchema) - .readonly() + .record(z.string(), z.unknown()) + .transform((raw) => { + const entries = Object.entries(raw).flatMap(([key, value]) => { + const parsed = UpdateManifestPlatformSchema.safeParse(value); + return parsed.success ? [[key, parsed.data] as const] : []; + }); + return entries.length === 0 ? undefined : Object.freeze(Object.fromEntries(entries)); + }) .optional() .catch(undefined),🤖 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 `@apps/pythinker-code/src/cli/update/cdn.ts` around lines 50 - 54, Update the platforms schema in UpdateManifestPlatformSchema so malformed individual platform entries are discarded while valid entries remain available, instead of allowing the record-level catch to replace the entire record with undefined. Preserve the optional behavior for a missing platforms field and ensure manifestArtifactAvailability can still evaluate successfully parsed platforms.apps/pythinker-code/test/cli/upgrade.test.ts (1)
365-365: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the refusal message, not just that stderr is non-empty.
expect(stderr.join('')).not.toBe('')passes for any stderr output. The test would still pass if the code wrote an unrelated error and never reachedrefuseForegroundInstall. The sibling test at Line 346 asserts specific content.💚 Proposed stronger assertion
- expect(stderr.join('')).not.toBe(''); + expect(stderr.join('')).toContain('another update install is already in progress');🤖 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 `@apps/pythinker-code/test/cli/upgrade.test.ts` at line 365, Strengthen the upgrade refusal test by replacing the generic non-empty stderr assertion with an assertion that stderr contains the specific refusal message emitted by refuseForegroundInstall. Match the sibling test’s content-based assertion style so unrelated stderr output cannot satisfy the test.apps/pythinker-code/src/cli/update/preflight.ts (2)
626-639: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBound the
partialline buffer.
partialaccumulates every byte that arrives without a newline. The installers emit newline-terminated lines, so the practical growth is zero. A malfunctioning installer that writes a long unterminated stream to stderr would grow this string without limit for the whole download.Cap
partialat the tail window so the reader stays allocation-bounded regardless of what the child writes.♻️ Proposed bound on the partial buffer
stream.on('data', (chunk: string) => { const lines = (partial + chunk).split('\n'); partial = lines.pop() ?? ''; + // A child that never emits a newline must not grow this buffer forever. + if (partial.length > INSTALLER_STDERR_TAIL_CHARS) { + partial = partial.slice(-INSTALLER_STDERR_TAIL_CHARS); + } for (const line of lines) {🤖 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 `@apps/pythinker-code/src/cli/update/preflight.ts` around lines 626 - 639, Bound the unterminated-line buffer in the stream data handler by truncating `partial` to at most `INSTALLER_STDERR_TAIL_CHARS` after updating it from `lines.pop()`. Preserve the existing newline splitting and progress parsing behavior while ensuring `partial` cannot grow beyond the tail window.
1252-1252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReconciliation makes the inferred-success branch in
showPendingBackgroundInstallNoticeunreachable.
reconcileAbandonedInstallruns beforeshowPendingBackgroundInstallNoticeand clears everyactiverecord that is not a fresh lease. The notice function's second branch requiresactive !== nulland!hasFreshActiveInstall(state)at Lines 410-415. After reconciliation, no state can satisfy both conditions, so Lines 416-441 never execute.The renamed test at Line 1760 of
apps/pythinker-code/test/cli/update/preflight.test.tsconfirms the new behavior: a stale active record now produces a recorded failure, not an inferred success notice.Remove the dead branch so the notice function states only the reachable rule.
🤖 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 `@apps/pythinker-code/src/cli/update/preflight.ts` at line 1252, Remove the unreachable inferred-success branch from showPendingBackgroundInstallNotice, including the active-record and !hasFreshActiveInstall(state) path. Keep only the reachable notice behavior after reconcileAbandonedInstall clears stale active records, and preserve the existing recorded-failure handling for stale installs.apps/pythinker-code/src/cli/sub/upgrade.ts (2)
104-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
upgrade_command_no_updatecannot distinguish "up to date" from "no platform build".This branch emits the same event and the same payload as the genuine up-to-date exit at Lines 93-95. The payload carries only
current_version. Telemetry therefore cannot measure how often a published release has no artifact for a user's platform, which is exactly the failure this PR adds a gate for.Add a discriminating field so the two exits stay separable.
♻️ Proposed telemetry discriminator
if (!isTargetInstallable(source, cache.manifest)) { trackUpgradeEvent(deps.track, 'upgrade_command_no_update', { current_version: currentVersion, + target_version: target.version, + reason: 'no_platform_artifact', });🤖 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 `@apps/pythinker-code/src/cli/sub/upgrade.ts` around lines 104 - 117, Update the isTargetInstallable branch in the upgrade flow so its upgrade_command_no_update telemetry payload includes a discriminator identifying the missing platform artifact case, while leaving the genuine up-to-date event payload unchanged.
184-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe terminal install-state records are built inline at two call sites. The foreground upgrade command and the preflight prompt path each construct the same success record and the same failure record by hand. Both must match
UpdateInstallStateSchemaininstall-state.tsand must match each other, so a change in one file diverges silently from the other. Extract the two builders intoinstall-state.ts, beside the schema they must satisfy.
apps/pythinker-code/src/cli/sub/upgrade.ts#L184-L218: replace the inline success and failure objects with the shared builders, and drop the localnowIsoat Lines 264-266 that duplicates the one inpreflight.ts.apps/pythinker-code/src/cli/update/preflight.ts#L1378-L1402: replace the inline success and failure objects inrunUpdatePreflightwith the same shared builders.🤖 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 `@apps/pythinker-code/src/cli/sub/upgrade.ts` around lines 184 - 218, Extract shared success and failure install-state builders beside UpdateInstallStateSchema in apps/pythinker-code/src/cli/update/install-state.ts, ensuring both builders produce schema-valid records with matching fields and behavior. In apps/pythinker-code/src/cli/sub/upgrade.ts lines 184-218 and apps/pythinker-code/src/cli/update/preflight.ts lines 1378-1402, replace the duplicated inline records with these builders; also remove the local nowIso at lines 264-266 in upgrade.ts and reuse the existing preflight.ts helper.apps/pythinker-web/public/install.sh (1)
654-656: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a portable poll interval in both download loops.
BusyBox fractional
sleepsupport depends onFEATURE_FLOAT_SLEEP. Whensleep 0.12fails, the surrounding conditional call suppresses Basherrexit, so the loop spins until the download ends. Probe fractional support once and fall back to1. Derive thei - last_emit_i >= 9threshold from the selected interval.🤖 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 `@apps/pythinker-web/public/install.sh` around lines 654 - 656, Update both download loops in install.sh to probe fractional sleep support once, select either 0.12 or a 1-second fallback, and use that interval for sleeping so failed fractional sleeps cannot cause a busy loop. Replace the hard-coded i - last_emit_i >= 9 emission threshold with a value derived from the selected interval, keeping progress timing consistent.
🤖 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 `@apps/pythinker-code/src/cli/update/preflight.ts`:
- Around line 918-948: Update recordInstallerProgress and the surrounding
installer lifecycle to track all in-flight progress-write promises, ignore new
progress after the install is settled, and await/drain every pending write
before writing the terminal state. Preserve best-effort error logging for each
write and ensure the terminal active:null/lastSuccess record is written only
after all progress writes complete. Add a regression test that delays
overlapping progress writes and verifies the terminal state cannot be
overwritten by stale progress.
In `@apps/pythinker-code/test/cli/update/cdn.test.ts`:
- Around line 175-218: Update the regex literals in the rejectCases tests and
related assertions to use the Unicode flag, including the existing HTTP,
network, JSON, semver, and timestamp patterns. Replace the broad any-character
expectations for invalid rollout percent and delaySeconds with patterns matching
their respective field names. First verify Zod 4 includes each field path in the
rejection message, then assert those paths.
In `@apps/pythinker-code/test/cli/update/preflight.test.ts`:
- Line 1181: Update the regular expression in the affected assertion of the
preflight test to include the Unicode flag, matching the sibling npm assertions
and satisfying require-unicode-regexp.
- Around line 962-967: Update the test around runUpdatePreflight so execution is
allowed to reach and suspend at the prompt before asserting
tryAcquireUpdateInstallLock has not been called. Use the existing prompt
mock/control flow to wait for the prompt boundary, then retain the pre-prompt
lock assertion, resolve the prompt, and verify the lock call and final result as
before.
In `@apps/pythinker-code/test/tui/pythinker-tui-startup.test.ts`:
- Around line 2024-2026: Update the test setup around PYTHINKER_CODE_HOME to
save the original process.env object, replace process.env with a copied
environment containing the temporary home, and restore the saved object
unconditionally in the finally block. Remove the conditional cleanup branch in
the affected test.
---
Nitpick comments:
In `@apps/pythinker-code/src/cli/sub/upgrade.ts`:
- Around line 104-117: Update the isTargetInstallable branch in the upgrade flow
so its upgrade_command_no_update telemetry payload includes a discriminator
identifying the missing platform artifact case, while leaving the genuine
up-to-date event payload unchanged.
- Around line 184-218: Extract shared success and failure install-state builders
beside UpdateInstallStateSchema in
apps/pythinker-code/src/cli/update/install-state.ts, ensuring both builders
produce schema-valid records with matching fields and behavior. In
apps/pythinker-code/src/cli/sub/upgrade.ts lines 184-218 and
apps/pythinker-code/src/cli/update/preflight.ts lines 1378-1402, replace the
duplicated inline records with these builders; also remove the local nowIso at
lines 264-266 in upgrade.ts and reuse the existing preflight.ts helper.
In `@apps/pythinker-code/src/cli/update/cdn.ts`:
- Around line 50-54: Update the platforms schema in UpdateManifestPlatformSchema
so malformed individual platform entries are discarded while valid entries
remain available, instead of allowing the record-level catch to replace the
entire record with undefined. Preserve the optional behavior for a missing
platforms field and ensure manifestArtifactAvailability can still evaluate
successfully parsed platforms.
In `@apps/pythinker-code/src/cli/update/preflight.ts`:
- Around line 626-639: Bound the unterminated-line buffer in the stream data
handler by truncating `partial` to at most `INSTALLER_STDERR_TAIL_CHARS` after
updating it from `lines.pop()`. Preserve the existing newline splitting and
progress parsing behavior while ensuring `partial` cannot grow beyond the tail
window.
- Line 1252: Remove the unreachable inferred-success branch from
showPendingBackgroundInstallNotice, including the active-record and
!hasFreshActiveInstall(state) path. Keep only the reachable notice behavior
after reconcileAbandonedInstall clears stale active records, and preserve the
existing recorded-failure handling for stale installs.
In `@apps/pythinker-code/src/tui/runtime/footer/footer-model.ts`:
- Around line 472-474: Update the status-line configuration to add a showUpdate
flag, defaulting it to true in DEFAULT_STATUS_LINE_CONFIG, and gate the update
item in selectStatusItems before pushing the formatted value. Preserve existing
update formatting and behavior when the flag is enabled.
In `@apps/pythinker-code/test/cli/upgrade.test.ts`:
- Line 365: Strengthen the upgrade refusal test by replacing the generic
non-empty stderr assertion with an assertion that stderr contains the specific
refusal message emitted by refuseForegroundInstall. Match the sibling test’s
content-based assertion style so unrelated stderr output cannot satisfy the
test.
In `@apps/pythinker-code/test/tui/commands/update-preferences.test.ts`:
- Around line 350-369: Add a separate test case near the existing long-reason
test in the update command tests using a short failure message containing
newline characters and remaining under 160 characters. Mock startManualUpdate
with that message, invoke handleUpdateCommand, and assert showError receives the
reason collapsed to a single line without the truncation suffix.
In `@apps/pythinker-web/public/install.sh`:
- Around line 654-656: Update both download loops in install.sh to probe
fractional sleep support once, select either 0.12 or a 1-second fallback, and
use that interval for sleeping so failed fractional sleeps cannot cause a busy
loop. Replace the hard-coded i - last_emit_i >= 9 emission threshold with a
value derived from the selected interval, keeping progress timing consistent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cdeec1c-fb16-436e-8aa9-6a1610591c0f
📒 Files selected for processing (37)
.changeset/update-flow-reliability.md.changeset/update-minimum-supported-version.md.changeset/update-status-under-the-prompt.mdapps/pythinker-code/src/cli/sub/upgrade.tsapps/pythinker-code/src/cli/update/cdn.tsapps/pythinker-code/src/cli/update/install-lock.tsapps/pythinker-code/src/cli/update/install-state.tsapps/pythinker-code/src/cli/update/lease.tsapps/pythinker-code/src/cli/update/preflight.tsapps/pythinker-code/src/cli/update/refresh.tsapps/pythinker-code/src/cli/update/rollout.tsapps/pythinker-code/src/cli/update/select.tsapps/pythinker-code/src/cli/update/types.tsapps/pythinker-code/src/constant/app.tsapps/pythinker-code/src/tui/commands/info.tsapps/pythinker-code/src/tui/components/chrome/welcome-banner.tsapps/pythinker-code/src/tui/components/chrome/welcome.tsapps/pythinker-code/src/tui/pythinker-tui.tsapps/pythinker-code/src/tui/runtime/footer/footer-model.tsapps/pythinker-code/src/tui/runtime/footer/update-status.tsapps/pythinker-code/test/cli/update/cdn.test.tsapps/pythinker-code/test/cli/update/install-lock.test.tsapps/pythinker-code/test/cli/update/install-state.test.tsapps/pythinker-code/test/cli/update/preflight.test.tsapps/pythinker-code/test/cli/update/refresh.test.tsapps/pythinker-code/test/cli/update/rollout.test.tsapps/pythinker-code/test/cli/update/select.test.tsapps/pythinker-code/test/cli/upgrade.test.tsapps/pythinker-code/test/tui/commands/update-preferences.test.tsapps/pythinker-code/test/tui/pythinker-tui-startup.test.tsapps/pythinker-code/test/tui/runtime/footer-model.test.tsapps/pythinker-code/test/tui/runtime/footer/update-status.test.tsapps/pythinker-web/public/install.ps1apps/pythinker-web/public/install.shapps/site/public/install.ps1apps/site/public/install.shapps/site/scripts/build-cdn.mjs
💤 Files with no reviewable changes (3)
- apps/pythinker-code/src/tui/components/chrome/welcome.ts
- apps/site/public/install.sh
- apps/site/public/install.ps1
The state file is a temp file plus rename, so the last rename wins. Progress writes were fire-and-forget, and the installer's terminal state=done line bypasses the write throttle just as the child exits — so on the ordinary success path that write could land after the outcome, restoring active and dropping lastSuccess. The next launch read that as an abandoned install and recorded a failure for a version that had installed cleanly, which parks the version at two attempts. Progress writes now run on one chain, new ones stop once the outcome is settled, and the finalizer drains the chain before writing. Removing the drain fails the new test. Also from review: the CDN reject cases named the field they expect instead of matching any non-empty message, the lock-ordering test now reaches the prompt before asserting no lock was taken (it previously asserted before the first await, so it held wherever the acquisition sat), and two lint nits this branch introduced.
Merged
elkaix
pushed a commit
that referenced
this pull request
Aug 7, 2026
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @pythoughts/pythinker-code@0.12.0 ### Minor Changes - [#35](#35) [`2ce6b5e`](2ce6b5e) - Show the plan before a Dynamic Workflow runs, and let a good one be saved as a command Manual mode used to approve every `DynamicWorkflow` call outright. That approval only ever fired in manual mode — auto and yolo approve earlier in the chain — so the one mode whose purpose is to ask was the one mode that never saw what it was agreeing to. A `DynamicWorkflow` call in manual mode now asks, and the approval carries the fan-out: how many subagents, the task list, the prompt template, the worker model, and the summed size of the prompts about to be sent. "Approve for this session" is keyed to that workflow's description rather than granting every future `DynamicWorkflow` call. `/workflow save <name>` writes the last run back out as a skill under `.pythinker-code/skills/`, so a fan-out that worked can be re-run by name. - [#38](#38) [`44efbc7`](44efbc7) - Let a release declare a minimum supported version, so a client below it is offered the update without waiting for its staged rollout batch. - [#38](#38) [`44efbc7`](44efbc7) - Show update availability and live download progress in the status row under the prompt, replacing the startup banner chip that was computed once and never refreshed. ### Patch Changes - [#37](#37) [`12069a8`](12069a8) - Stop offering updates to versions that were never published: the update channel now advertises only the release that is actually available for download. - [#38](#38) [`44efbc7`](44efbc7) - Stop offering an update with no build for the running platform, give every installer network call a timeout, expire a stale install lease instead of blocking updates forever, and say which version is installing and why a failed one stopped retrying. ## @pythoughts/pythinker-code-sdk@0.13.0 ### Minor Changes - [#35](#35) [`2ce6b5e`](2ce6b5e) - Show the plan before a Dynamic Workflow runs, and let a good one be saved as a command Manual mode used to approve every `DynamicWorkflow` call outright. That approval only ever fired in manual mode — auto and yolo approve earlier in the chain — so the one mode whose purpose is to ask was the one mode that never saw what it was agreeing to. A `DynamicWorkflow` call in manual mode now asks, and the approval carries the fan-out: how many subagents, the task list, the prompt template, the worker model, and the summed size of the prompts about to be sent. "Approve for this session" is keyed to that workflow's description rather than granting every future `DynamicWorkflow` call. `/workflow save <name>` writes the last run back out as a skill under `.pythinker-code/skills/`, so a fan-out that worked can be re-run by name. ## pythinker-code@0.8.6 ### Patch Changes - Updated dependencies [[`2ce6b5e`](2ce6b5e)]: - @pythoughts/pythinker-code-sdk@0.13.0 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added approval previews for Dynamic Workflow calls in manual mode, including fan-out details. - Added session-scoped workflow approvals. - Added `/workflow save <name>` to save the latest workflow run as a reusable skill. - Added live update availability and download progress indicators. - **Bug Fixes** - Improved update reliability with build checks, network timeouts, stale-install recovery, and clearer retry messages. - **Release Updates** - Released PyThinker Code 0.12.0, VS Code extension 0.8.6, and SDK 0.13.0. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
5 tasks
5 tasks
elkaix
added a commit
that referenced
this pull request
Aug 9, 2026
## Related Issue No issue — reported directly on Windows. The problem is described below. ## Problem A Windows user on v0.12.0 saw `Updating to v0.13.1 / Installing in the background`, then `↑ v0.13.1 restart to apply` under the prompt. Restarting the terminal still gave v0.12.0, and the update log recorded the same install as *succeeded* several times over. `where.exe pythinker` put `%LOCALAPPDATA%\Programs\Pythinker\pythinker.exe` first, and the published 0.13.1 Windows binary is correct (its sha256 matches the channel manifest and it reports 0.13.1), so the executable that ran was the one the installer targets and the advertised version was real. What was wrong is that nothing ever checked. Investigating it found four defects: - **A success is recorded from an exit code alone.** The background finalizer writes `lastSuccess` when the installer exits 0. No step asks whether the binary that runs next is the target version, so an installer that exits 0 without replacing anything advertises "restart to apply" forever, on every launch. - **`doctor` crashes on every native install.** It reports the package root, and a packaged binary ships no `package.json`, so the command died with `Error: Could not locate package.json near …` — exactly when a user needs it most. The same lookup sits on the launch path in install-source detection. - **npm-family auto-update cannot start on Windows.** `npm.cmd`, `pnpm.cmd` and `yarn.cmd` are spawned directly, which Node ≥18.20/20.12 refuses (CVE-2024-27980) with `EINVAL`. The same call fails in the npm-prefix lookup, so those installs also classify as `unsupported`. - **`install.ps1` emits no progress.** `install.sh` writes machine-readable `progress:` lines on stderr and the parent renders them; the PowerShell installer wrote none, so the footer's downloading state was unreachable on Windows and an update in flight looked identical to a wedged one — the defect #38 set out to close, still open on one platform. ## What changed **A success now means the new version runs.** After an installer exits 0, the version is verified against the artifact the installer replaced, and a mismatch is recorded as a failure carrying the reason (`… still reports 0.12.0 (expected 0.13.1)`) instead of a success. Only `native` installs are verified, by probing `process.execPath --version`: an npm global reinstall rewrites the directory this process was loaded from, so nothing readable there proves what the next launch runs, and a wrong answer would park a healthy version. Verification fails **open** — a probe that times out (an antivirus scan on a fresh unsigned exe is the realistic case), cannot run, or prints no version records the success anyway, with a note saying why it is unproven. `doctor` prints that note next to the recorded outcome, so the next report of "it says updated but it did not" is answerable in one command. **Windows package-manager shims run through the command interpreter.** `cmd.exe /d /s /c npm.cmd …`, spelled out as argv rather than `shell: true`, so the exact command line is visible in the source and asserted in tests instead of being assembled by Node's string joining. Same fix in the npm-prefix lookup that classifies the install source. **`install.ps1` speaks the progress protocol**, mirroring `install.sh`: `state=waiting` while release assets are not up yet, `state=downloading` with percent and byte counts (one line per second at most), `state=done`, and a single `state=failed` after the last retry — not between attempts, which would drop the footer out of its downloading state and back into a failure it is about to recover from. **`doctor` survives a native install**, reporting the package root only when there is one, and the launch-path source detection classifies an unresolvable layout as `unsupported` rather than throwing. It also now prints the last recorded update success, which is what would have shown the original problem immediately. One scope decision worth flagging: the interactive `Updated … to X` message still prints unchanged when a native probe could not run. The mismatch case — the actual lie — throws and is reported as a failure on both foreground paths; the unproven case only loses a line in a flow the user is watching, and it is recorded in the install state either way. ## Checklist - [x] I have read the [CONTRIBUTING](https://github.com/Pythoughts-labs/pythinker-code/blob/main/CONTRIBUTING.md) document. - [x] I have linked a related issue, or explained the problem above. - [x] I have added tests that prove my feature works. - [x] Ran `gen-changesets` skill, or this PR needs no changeset. - [x] Ran `gen-docs` skill, or this PR needs no doc update. The user-facing docs describe the commands, not `doctor`'s runtime lines, and the update behaviour is unchanged when an install really works. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Fixed crashes when running diagnostics from native installations. - Improved automatic updates for npm, pnpm, and Yarn installations on Windows. - Updates are no longer reported as successful when the installed version remains unchanged. - **New Features** - Added post-update version verification with clearer failure and unverified status reporting. - Diagnostics now show the most recent successful update and its status. - Windows installer downloads now display progress, waiting, completion, and failure states. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
Related Issue
No issue — reported directly. The problem is described below.
Problem
A user's terminal showed
↑ Update available — v0.11.0on the banner while/updateansweredUpdate to v0.10.0 already in progress, and nothing ever changed. Investigating it found a whole classof defects behind that one screen:
When the guess was wrong it polled for ~6 minutes, on every launch.
close that terminal — so failures went unrecorded, the attempt counter never advanced, and a version
that could not succeed was retried on every launch.
install.shrendered a progress bar only on a TTY; in the background it blocked in a single silentcurland reported nothing at all, which is why an update in flight looked identical to a wedged one.writing the same executable.
/updateread different files, which is how they came to disagree.What changed
Fourteen commits, each with tests, in the order they were verified. Grouped:
The channel tells the truth.
latest.jsonnow carries the resolved per-platform artifact(
url+sha256), copied from the release's own native manifest, and a native client requires an entryfor its platform before it will advertise or install anything. npm-family sources are exempt — the
published version is their artifact — and that exemption is the case the tests protect hardest.
minRequiredVersionlets a release bypass the staged rollout when a client cannot skip it.Progress is visible. The installer emits newline-terminated machine progress on stderr — the stream
the parent already pipes — and the parent records it on the install record. The footer status row under
the prompt renders
↑ v0.11.0,↓ v0.11.0 ▰▰▰▱▱▱▱▱ 42%,↑ v0.11.0 restart to apply, reusing thecontext gauge's own bar glyphs. An unknown download size drops the bar rather than inventing a
percentage.
Nothing wedges. Every installer fetch has a connect bound, a per-attempt ceiling and a stall guard
(
--retryis deliberately absent: it resets--max-time). Onelease.tsstates the lease rule once,with a ceiling on live pids. Startup reconciles an abandoned install into a recorded failure so a doomed
version parks. Both foreground paths hold the lock and write their outcome.
It says what is happening.
/updatereports the installing version and the newer target thatfollows, and reports a parked version's attempt count and recorded reason instead of a bare command.
Deletions rather than additions where the shape allowed: the plain-text
/latestfallback (it carried noplatform data, so it reported an unverifiable target as verified), the duplicate
install.sh/install.ps1under
apps/site/public/, the duplicatedisProcessRunningand four lease constants, the banner's updatechip and its per-frame
readFileSync, and one of the two update decisions per launch.Two scope decisions worth flagging: killing an in-flight installer to switch targets is not
implemented — the lease ceiling and reconciliation make the wait finite, and honest reporting fixes what
the user saw. And a writability precheck was dropped in favour of surfacing the installer's own recorded
error, which covers EACCES, network faults and disk-full alike.
Base is
fix/release-cdn-version-truth(#37) because the manifest generator work stacks on it.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.Summary by CodeRabbit
New Features
Bug Fixes