fix(env): hold env keys to the identifier rule env.sh already assumes (#740)

`generateEnvFile` interpolated the key raw into `export <key>=<quoted value>`.
The value had `shellQuoteValue`; the key had nothing, so a key from the team
repo's env/env.yaml could produce a line that is not valid shell
(`export bad key='x'`) or one that runs code (`export FOO;cmd='x'`), in every
member's shell — env is pushable, so any member who can push can put such a
key in front of everyone else.

`parseEnvFile` already refused to read back any key outside
`[A-Za-z_][A-Za-z0-9_]*`, which made the asymmetry worse: the variable was
written into env.sh and then invisible to the CLI, so nothing reported it as
missing. That regex is now a module-level `ENV_KEY_RE` shared by both sides,
so write and read agree by construction rather than by two copies drifting.

The generator drops a non-matching key instead of failing: one member's bad
key must not take env.sh down for everyone, and the remaining variables are
still correct. `env add` rejects such a key up front with the offending name,
because the local command is where the mistake is still visible — accepting
it there would report success for a variable that never reaches a shell.

Refs #738

Co-authored-by: ydflow <314143294+ydflow@users.noreply.github.com>

src/__tests__/env-commands.test.ts

@@ -192,6 +192,20 @@ scope: 'user',
   // ─── envAdd ──────────────────────────────────────────────
 
   describe('envAdd', () => {
+    it('refuses a key that would not survive the round trip into env.sh', async () => {
+      // `generateEnvFile` drops any key that is not a shell identifier, so
+      // accepting one here would write a variable that never reaches the
+      // member's shell — and `FOO;cmd` would run `cmd` there if it did. Better
+      // to reject it at the point the user can still see the mistake.
+      await envAdd('bad key', 'v', {});
+
+      expect(log.error).toHaveBeenCalledWith(expect.stringContaining('bad key'));
+      // Nothing written, and no env.yaml is created just to hold nothing.
+      const envYamlPath = path.join(repoPath, 'env', 'env.yaml');
+      expect(await fse.pathExists(envYamlPath)).toBe(false);
+      expect(log.success).not.toHaveBeenCalled();
+    });
+
     it('should add a new variable locally and show push hint', async () => {
       await envAdd('NEW_VAR', 'new_value', {});
 

src/__tests__/env-handler.test.ts

@@ -396,6 +396,35 @@ scope: 'user',
       const content = handler.generateEnvFile([]);
       expect(content).toBe('\n');
     });
+
+    it('should drop keys that are not valid shell identifiers', () => {
+      // A key is interpolated raw into `export <key>=...`, so anything that is
+      // not an identifier either breaks the line or runs as shell code. The
+      // whole variable is dropped, not the line rewritten: `parseEnvFile` skips
+      // such a line anyway, so emitting it would put a variable in env.sh that
+      // the CLI can never read back.
+      const content = handler.generateEnvFile([
+        { key: 'GOOD_KEY', value: 'ok' },
+        { key: 'bad key', value: 'oops' },
+        { key: 'FOO;touch /tmp/pwned', value: 'y' },
+        { key: '$(whoami)', value: 'w' },
+        { key: 'A=B', value: 'z' },
+        { key: '9LEADING', value: 'n' },
+      ]);
+
+      expect(content).toBe("export GOOD_KEY='ok'\n");
+    });
+
+    it('should keep keys that are valid shell identifiers', () => {
+      // The guard must not narrow what a legitimate team repo can express:
+      // digits and underscores after the first character are all valid.
+      const content = handler.generateEnvFile([
+        { key: '_PRIVATE', value: 'a' },
+        { key: 'A1_b2', value: 'b' },
+      ]);
+
+      expect(content).toBe("export _PRIVATE='a'\nexport A1_b2='b'\n");
+    });
   });
 
   // ─── pullItem ────────────────────────────────────────────

src/env-commands.ts

@@ -4,7 +4,7 @@ import { requireInit, detectProjectConfig } from './config.js';
 import { pullRepo } from './utils/git.js';
 import { ensureDir, readFileSafe, writeFile, pathExists } from './utils/fs.js';
 import { log, spinner } from './utils/logger.js';
-import { EnvHandler, maskEnvValue } from './resources/env.js';
+import { EnvHandler, maskEnvValue, ENV_KEY_RE } from './resources/env.js';
 import type { GlobalOptions } from './types.js';
 import { isSelfMode } from './types.js';
 
@@ -59,6 +59,18 @@ export async function envAdd(
   value: string,
   options: GlobalOptions & { description?: string },
 ): Promise<void> {
+  // env.sh is generated as `export <key>=...` and sourced by every member, so a
+  // key that is not a shell identifier either breaks that line or runs as code.
+  // `generateEnvFile` drops such keys, which would make this command report
+  // success for a variable that never reaches anyone's shell — reject it here,
+  // where the user still sees what they typed.
+  if (!ENV_KEY_RE.test(key)) {
+    log.error(
+      `Invalid env variable name "${key}": use letters, digits and underscores, starting with a letter or underscore.`,
+    );
+    return;
+  }
+
   const projectConfig = await detectProjectConfig();
   const localConfig = projectConfig ?? (await requireInit()).localConfig;
   const repoPath = localConfig.repo.localPath;

src/resources/env.ts

@@ -107,6 +107,15 @@ export function maskEnvValue(value: string): string {
   return `${value.slice(0, 2)}****`;
 }
 
+/**
+ * A key this module will write into env.sh, and the only shape it reads back.
+ *
+ * Shared by `parseEnvFile` and `generateEnvFile` on purpose: the write side has
+ * to reject exactly what the read side skips, or a variable can exist in env.sh
+ * that the CLI can never see again.
+ */
+export const ENV_KEY_RE = /^[A-Za-z_][A-Za-z0-9_]*$/;
+
 /**
  * Read back the assignments `generateEnvFile` writes, as key → value.
  *
@@ -118,14 +127,13 @@ export function maskEnvValue(value: string): string {
  */
 export function parseEnvFile(content: string): Map<string, string> {
   const PREFIX = 'export ';
-  const KEY = /^[A-Za-z_][A-Za-z0-9_]*$/;
   const assignments = new Map<string, string>();
 
   let i = 0;
   while (i < content.length) {
     const eq = content.startsWith(PREFIX, i) ? content.indexOf('=', i + PREFIX.length) : -1;
     const key = eq === -1 ? '' : content.slice(i + PREFIX.length, eq);
-    if (eq === -1 || !KEY.test(key) || content[eq + 1] !== "'") {
+    if (eq === -1 || !ENV_KEY_RE.test(key) || content[eq + 1] !== "'") {
       const nl = content.indexOf('\n', i);
       if (nl === -1) break;
       i = nl + 1;
@@ -437,9 +445,19 @@ export class EnvHandler extends ResourceHandler {
    * the sourced script. An embedded single quote is encoded with the standard
    * `'\''` sequence. env.sh is sourced from every team member's shell profile,
    * so values (which originate from the team repo's env/env.yaml) must be safe.
+   *
+   * Keys are interpolated raw into the export statement, so they are held to
+   * the same identifier rule `parseEnvFile` applies when reading env.sh back. A
+   * key that fails it is dropped rather than emitted: `export bad key='x'` is
+   * not valid shell, and `export FOO;cmd='x'` would run `cmd` in every member's
+   * shell. Dropping keeps the write and read sides in agreement — a line
+   * `parseEnvFile` must skip anyway is better left unwritten. One member's bad
+   * key must not take the whole file down with it, so the rest still ship.
    */
   generateEnvFile(variables: EnvVariable[]): string {
-    const lines = variables.map(v => `export ${v.key}=${shellQuoteValue(v.value)}`);
+    const lines = variables
+      .filter(v => ENV_KEY_RE.test(v.key))
+      .map(v => `export ${v.key}=${shellQuoteValue(v.value)}`);
     return lines.join('\n') + '\n';
   }