Skip to content

fix: preserve saved environment and login during upgrade - #342

Open
Fermionic-Lyu wants to merge 1 commit into
mainfrom
fix/upgrade-preserve-environment
Open

Fermionic-Lyu wants to merge 1 commit into
mainfrom
fix/upgrade-preserve-environment

Conversation

@Fermionic-Lyu

@Fermionic-Lyu Fermionic-Lyu commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

A temporary INSTA_ENV=prod still reaches the binary installer during insta upgrade. With staging saved on disk, the installer runs env 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 upgrade overwriting the saved environment and dropping the session. During a binary upgrade, the installer previously ran env use prod from a temporary INSTA_ENV var, replacing the saved API URL and clearing login fields.

Marks the binary installer invocation as an upgrade (INSTA_UPGRADE=1) so install.sh skips 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.

Review in cubic

@agent-zhang-beihai agent-zhang-beihai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@agent-zhang-beihai agent-zhang-beihai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Fermionic-Lyu

Copy link
Copy Markdown
Member Author

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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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' },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Suggested change
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' },

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

insta upgrade silently switches the environment back to prod

1 participant