Repository navigation
fix: preserve saved environment and login during upgrade - #342
Fermionic-Lyu wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Reviewed by Wang Miao
With INSTA_ENV set in their shell, users who ran insta upgrade or got a background auto-update on the binary channel used to hit env use in the installer. When INSTA_ENV differed from the saved environment, that switched the saved environment and logged them out. This change has upgrade() mark the installer run with INSTA_UPGRADE=1, and install.sh then skips the environment step. Both the explicit upgrade and the background upgrade call upgrade(), and upgrade() sets the marker on both its pinned and unpinned installer runs, so the fix covers every path. I found no defects, and I would merge it.
No findings.
There was a problem hiding this comment.
Reviewed by Yang Dong
This adds an upgrade marker so inherited INSTA_ENV no longer rewrites the saved environment and login during binary upgrades. However, the mechanism cannot protect existing installations on the upgrade that first delivers this fix, so I would request changes.
The first upgrade still switches the environment and drops the saved login
important · defect · correctness · install.sh:207
Existing users initiate the upgrade with the old upgrade() implementation, which fetches the live installer but does not set INSTA_UPGRADE. Consequently, the updated installer still runs env use "$ENV_NAME" on that first upgrade; when the inherited environment differs from the persisted one, this switches the saved environment and deletes the session. Only subsequent upgrades, initiated by a binary already containing the new marker, are protected.
The installer needs to recognize and safely handle upgrades from existing binaries without depending exclusively on a marker that those binaries cannot supply.
Evidence
read-the-code — 3618e8a6:src/commands/upgrade.ts:294-310, src/commands/upgrade.ts:294-310, install.sh:203-220, src/commands/env.ts:37-76
|
Pausing PR #342 after the first review round under the cleanup rule: record disagreements here and move on. At f379323, Wang approves; Yang requests changes because an older binary initiating its first upgrade does not supply the new INSTA_UPGRADE marker. I verified the old caller at 3618e8a:src/commands/upgrade.ts: it passes INSTA_INSTALL_DIR but no marker. The live installer therefore still applies inherited INSTA_ENV on that transition, potentially changing the saved environment and clearing login. New callers are covered, including the pinned-install fallback. The unresolved boundary is whether a forward-only fix is acceptable or the installer must also distinguish old upgrade callers from direct installations, while retaining intentional environment selection for direct installs. No such distinction is established by this patch. The PR stays open and unmerged; this issue stays open. No release was published. Validation for the current patch: local typecheck and 1,999 tests passed; Linux and Windows CI passed. First/final diff is unchanged: 3 files, +59/-2, one push. Passing tests do not cover away the first-upgrade limitation. |
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/upgrade-environment.test.ts">
<violation number="1" location="test/upgrade-environment.test.ts:30">
P2: The spawned real CLI (`src/index.ts` via the `home/insta` wrapper) runs the full startup path: `trackCommand` at src/index.ts posts PostHog telemetry for every command and is awaited, and `maybeUpdate` can respawn a detached `__update-check` that hits the npm registry. `runEnvironment` disables auto-update but not telemetry, so the regression suite sends real network requests (and pollutes prod Telemetry with test events), and offline/slow CI can break the 10 s `spawnSync` timeout. Add `INSTA_NO_TELEMETRY: '1'` to the spawned env.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const result = spawnSync('sh', ['-c', `set -eu\n${environmentStep}`], { | ||
| cwd: repo, encoding: 'utf8', timeout: 10_000, | ||
| env: { ...env, HOME: home, INSTALL_DIR: home, BIN: 'insta', ENV_NAME: env.INSTA_ENV ?? '', | ||
| TEST_NODE: process.execPath, TEST_CLI: join(repo, 'src/index.ts'), INSTA_NO_AUTOUPDATE: '1' }, |
There was a problem hiding this comment.
P2: The spawned real CLI (src/index.ts via the home/insta wrapper) runs the full startup path: trackCommand at src/index.ts posts PostHog telemetry for every command and is awaited, and maybeUpdate can respawn a detached __update-check that hits the npm registry. runEnvironment disables auto-update but not telemetry, so the regression suite sends real network requests (and pollutes prod Telemetry with test events), and offline/slow CI can break the 10 s spawnSync timeout. Add INSTA_NO_TELEMETRY: '1' to the spawned env.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At test/upgrade-environment.test.ts, line 30:
<comment>The spawned real CLI (`src/index.ts` via the `home/insta` wrapper) runs the full startup path: `trackCommand` at src/index.ts posts PostHog telemetry for every command and is awaited, and `maybeUpdate` can respawn a detached `__update-check` that hits the npm registry. `runEnvironment` disables auto-update but not telemetry, so the regression suite sends real network requests (and pollutes prod Telemetry with test events), and offline/slow CI can break the 10 s `spawnSync` timeout. Add `INSTA_NO_TELEMETRY: '1'` to the spawned env.</comment>
<file context>
@@ -0,0 +1,57 @@
+ const result = spawnSync('sh', ['-c', `set -eu\n${environmentStep}`], {
+ cwd: repo, encoding: 'utf8', timeout: 10_000,
+ env: { ...env, HOME: home, INSTALL_DIR: home, BIN: 'insta', ENV_NAME: env.INSTA_ENV ?? '',
+ TEST_NODE: process.execPath, TEST_CLI: join(repo, 'src/index.ts'), INSTA_NO_AUTOUPDATE: '1' },
+ })
+ expect(result.status, result.stderr).toBe(0)
</file context>
| TEST_NODE: process.execPath, TEST_CLI: join(repo, 'src/index.ts'), INSTA_NO_AUTOUPDATE: '1' }, | |
| TEST_NODE: process.execPath, TEST_CLI: join(repo, 'src/index.ts'), INSTA_NO_AUTOUPDATE: '1', INSTA_NO_TELEMETRY: '1' }, |
A temporary
INSTA_ENV=prodstill reaches the binary installer duringinsta upgrade. With staging saved on disk, the installer runsenv use prod, overwrites the saved API URL, and drops the session.Mark the binary installer invocation as an upgrade and skip its environment-persistence step in that mode. Keep the inherited environment available for release-channel selection. Both the pinned install and unpinned fallback preserve the saved config; ordinary explicit installer environment selection retains its existing behavior.
Closes #145.
Validation: reproduced the staging-to-prod switch and loss of access/refresh/user fields before the fix. The regression runs the checked-in installer's environment step with the real CLI in an isolated home, checks the saved config byte-for-byte for absent/prod/staging overrides and retry, and verifies ordinary installation can still switch environments. Typecheck, shell syntax, whitespace checks and all 1,999 tests pass. Independent review and targeted mutation checks pass; Linux/Windows CI is required before merge. No separate lint/format scripts are configured.
First push: 3 files, +59/-2. No public command or flag changes. The binary-caller change requires a CLI release; no release is included in this PR.
Summary by cubic
Fixes
insta upgradeoverwriting the saved environment and dropping the session. During a binary upgrade, the installer previously ranenv use prodfrom a temporaryINSTA_ENVvar, replacing the saved API URL and clearing login fields.Marks the binary installer invocation as an upgrade (
INSTA_UPGRADE=1) soinstall.shskips its environment-persistence step. The inherited environment is still available for release-channel selection, and both the pinned install and unpinned fallback preserve the saved config. Ordinary explicit installer environment selection behaves as before.Adds a regression test that reproduces the staging-to-prod switch and verifies the saved config stays byte-for-byte intact during upgrades while still allowing explicit environment changes.
Closes #145. No public command or flag changes; the binary-caller change requires a CLI release, which is not included here.
Written for commit f379323. Summary will update on new commits.