fix(cli): address Codex review round 1

fengmk2 · fengmk2 · commit e1209f264b5f · 2026-07-05T23:44:49.000+08:00
- defaultPackage: only an explicitly declared key changes behavior. New
  FieldMap::get_declared keeps unanalyzable/spread configs from failing
  every bare app command (they fall through to picker/current-dir); a
  declared non-static value still errors
- consume -C first thing in main, before alias normalization, the
  no-command picker, and clap: vp -C dir node --version now normalizes
  like the cd form, bare vp -C dir opens the command picker for dir, and
  setting the process cwd makes in-process helpers (global add -g file:
  specs, node version inference) resolve from dir
- PTY regression case for the spread-config fall-through; unit tests for
  parse_leading_chdir and get_declared
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/snapshots.toml b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/snapshots.toml
@@ -16,3 +16,13 @@ name = "default_package_missing"
 vp = ["local", "global"]
 comment = "defaultPackage pointing at a missing directory errors before any workspace lookup."
 steps = [{ argv = ["vp", "build"], cwd = "missing" }]
+
+[[case]]
+name = "unanalyzable_config_ignored"
+vp = ["local", "global"]
+comment = """
+Regression guard for spread/unanalyzable configs: a config that only parses
+as an open map might hide defaultPackage behind the spread, but that must
+not fail the command. Bare vp build falls through and runs in place.
+"""
+steps = [{ argv = ["vp", "build"], cwd = "spread", tty = false }]
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/snapshots/unanalyzable_config_ignored.global.md b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/snapshots/unanalyzable_config_ignored.global.md
@@ -0,0 +1,17 @@
+# unanalyzable_config_ignored
+
+Regression guard for spread/unanalyzable configs: a config that only parses
+as an open map might hide defaultPackage behind the spread, but that must
+not fail the command. Bare vp build falls through and runs in place.
+
+## `cd spread && vp build`
+
+```
+vite <version> building client environment for production...
+[2Ktransforming...✓ 2 modules transformed.
+rendering chunks...
+computing gzip size...
+dist/index.html  <size> kB │ gzip: <size> kB
+
+✓ built in <duration>
+```
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/snapshots/unanalyzable_config_ignored.local.md b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/snapshots/unanalyzable_config_ignored.local.md
@@ -0,0 +1,17 @@
+# unanalyzable_config_ignored
+
+Regression guard for spread/unanalyzable configs: a config that only parses
+as an open map might hide defaultPackage behind the spread, but that must
+not fail the command. Bare vp build falls through and runs in place.
+
+## `cd spread && vp build`
+
+```
+vite <version> building client environment for production...
+[2Ktransforming...✓ 2 modules transformed.
+rendering chunks...
+computing gzip size...
+dist/index.html  <size> kB │ gzip: <size> kB
+
+✓ built in <duration>
+```
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/spread/index.html b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/spread/index.html
@@ -0,0 +1,6 @@
+<!doctype html>
+<html>
+  <body>
+    <h1>spread</h1>
+  </body>
+</html>
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/spread/package.json b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/spread/package.json
@@ -0,0 +1 @@
+{ "name": "spread-config", "private": true }
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/spread/vite.config.ts b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_default_package/spread/vite.config.ts
@@ -0,0 +1,5 @@
+const base = {};
+
+export default {
+  ...base,
+};
diff --git a/crates/vite_global_cli/src/main.rs b/crates/vite_global_cli/src/main.rs
@@ -39,6 +39,23 @@ use crate::cli::{
     try_parse_args_from_with_options,
 };
 
+/// Parse a leading `-C <dir>` / `-C=<dir>` / `-C<dir>` as the first user
+/// argument, mirroring clap's short-option grammar (and bin.ts on the local
+/// path). Returns the directory and how many argv tokens it consumed.
+fn parse_leading_chdir(args: &[String]) -> Option<(String, usize)> {
+    let first = args.get(1)?;
+    if first == "-C" {
+        return args.get(2).map(|dir| (dir.clone(), 2));
+    }
+    if let Some(rest) = first.strip_prefix("-C") {
+        let dir = rest.strip_prefix('=').unwrap_or(rest);
+        if !dir.is_empty() {
+            return Some((dir.to_string(), 1));
+        }
+    }
+    None
+}
+
 /// Normalize CLI arguments:
 /// - `vp list ...` / `vp ls ...` → `vp pm list ...`
 /// - `vp rebuild ...` → `vp pm rebuild ...`
@@ -323,14 +340,32 @@ async fn main() -> ExitCode {
     }
 
     // Normal CLI mode - get current working directory
-    let cwd = match vite_path::current_dir() {
+    let mut cwd = match vite_path::current_dir() {
         Ok(path) => path,
         Err(e) => {
             output::error(&format!("Failed to get current directory: {e}"));
             return ExitCode::FAILURE;
         }
     };
 
+    // Consume a leading `-C <dir>` here, before anything inspects args or cwd,
+    // so alias normalization, the command picker, and in-process helpers all
+    // behave exactly as if vp had been started in <dir>. Setting the process
+    // cwd (before any command logic runs) keeps `vite_path::current_dir()`
+    // callers deep inside command implementations equivalent to the cd form.
+    if let Some((dir, consumed)) = parse_leading_chdir(&args) {
+        cwd = cwd.join(&dir).clean();
+        if !cwd.as_path().is_dir() {
+            output::raw_stderr(&format!("directory not found: {dir}"));
+            return ExitCode::FAILURE;
+        }
+        if let Err(e) = std::env::set_current_dir(cwd.as_path()) {
+            output::error(&format!("Failed to change directory to {dir}: {e}"));
+            return ExitCode::FAILURE;
+        }
+        args.drain(1..=consumed);
+    }
+
     if args.len() == 1 {
         match command_picker::pick_top_level_command_if_interactive(&cwd) {
             Ok(command_picker::TopLevelCommandPick::Selected(selection)) => {
@@ -469,6 +504,23 @@ mod tests {
         assert_eq!(normalized, s(&["vp", "env", "exec", "node", "script.js", "foo", "--flag"]));
     }
 
+    #[test]
+    fn parse_leading_chdir_accepts_all_clap_short_forms() {
+        // `main` consumes these before alias normalization and the picker, so
+        // `vp -C dir node --version` normalizes like `cd dir && vp node ...`.
+        assert_eq!(parse_leading_chdir(&s(&["vp", "-C", "apps/web", "dev"])), some_dir(2));
+        assert_eq!(parse_leading_chdir(&s(&["vp", "-Capps/web", "dev"])), some_dir(1));
+        assert_eq!(parse_leading_chdir(&s(&["vp", "-C=apps/web", "dev"])), some_dir(1));
+        // Missing or empty value falls through to clap's own error.
+        assert_eq!(parse_leading_chdir(&s(&["vp", "-C"])), None);
+        assert_eq!(parse_leading_chdir(&s(&["vp", "-C="])), None);
+        assert_eq!(parse_leading_chdir(&s(&["vp", "dev"])), None);
+    }
+
+    fn some_dir(consumed: usize) -> Option<(String, usize)> {
+        Some(("apps/web".to_string(), consumed))
+    }
+
     #[test]
     fn normalize_args_rewrites_bare_vp_node() {
         let input = s(&["vp", "node"]);
diff --git a/crates/vite_static_config/src/lib.rs b/crates/vite_static_config/src/lib.rs
@@ -80,6 +80,20 @@ impl FieldMap {
             FieldMapInner::Open(_) => true,
         }
     }
+
+    /// Like [`get`](Self::get), but only for fields the config explicitly
+    /// declares. Unlike `get`, an open map (spread, computed keys, or an
+    /// unanalyzable config) does NOT report undeclared keys as `NonStatic`:
+    /// they return `None`. Use this when a merely-possible field must not
+    /// change behavior (e.g. `defaultPackage`, where erroring on every
+    /// unanalyzable config would break unrelated commands).
+    #[must_use]
+    pub fn get_declared(&self, key: &str) -> Option<FieldValue> {
+        match &self.0 {
+            FieldMapInner::Closed(map) => map.get(key).cloned(),
+            FieldMapInner::Open(map) => map.get(key).map(|v| FieldValue::Json(v.clone())),
+        }
+    }
 }
 
 /// Config file names to try, in priority order.
@@ -458,6 +472,24 @@ mod tests {
         assert_eq!(map.get(key), Some(FieldValue::Json(expected)));
     }
 
+    #[test]
+    fn get_declared_ignores_undeclared_keys_in_open_maps() {
+        // A spread makes the map open: `get` reports any key as possibly
+        // present, but `get_declared` only reports explicitly written ones.
+        let map = parse("const base = {};\nexport default { ...base, run: { tasks: {} } };");
+        assert_eq!(map.get("defaultPackage"), Some(FieldValue::NonStatic));
+        assert_eq!(map.get_declared("defaultPackage"), None);
+        assert!(map.get_declared("run").is_some());
+
+        // Closed map: explicitly declared non-static values still surface.
+        let map = parse("export default { defaultPackage: process.env.DIR };");
+        assert_eq!(map.get_declared("defaultPackage"), Some(FieldValue::NonStatic));
+
+        // Closed map without the key: definitively absent either way.
+        let map = parse("export default { run: {} };");
+        assert_eq!(map.get_declared("defaultPackage"), None);
+    }
+
     /// Shorthand for asserting a field is `NonStatic`.
     fn assert_non_static(map: &FieldMap, key: &str) {
         assert_eq!(
diff --git a/packages/cli/binding/src/cli/app_target.rs b/packages/cli/binding/src/cli/app_target.rs
@@ -75,12 +75,16 @@ fn looks_runnable(dir: &AbsolutePathBuf, command: &str) -> bool {
 /// `defaultPackage` from the `vite.config.*` in `cwd`, read via static
 /// extraction so it works at roots without a vite-plus install (non-workspace
 /// framework repos). The value must be a static string literal.
+///
+/// `get_declared` keeps this to explicitly written fields: a config that is
+/// unanalyzable or hides fields behind a spread simply falls through to the
+/// picker/current-dir resolution instead of failing every bare app command.
 fn resolve_default_package(command: &str, cwd: &AbsolutePathBuf) -> Option<AppTarget> {
     let fail = |msg: &str| {
         output::error(msg);
         Some(AppTarget::Exit(ExitStatus(1)))
     };
-    match vite_static_config::resolve_static_config(cwd).get("defaultPackage") {
+    match vite_static_config::resolve_static_config(cwd).get_declared("defaultPackage") {
         Some(vite_static_config::FieldValue::Json(serde_json::Value::String(dir))) => {
             let target = cwd.join(&dir).clean();
             if !target.as_path().is_dir() {
diff --git a/rfcs/cwd-flag.md b/rfcs/cwd-flag.md
@@ -265,7 +265,7 @@ For every vp command:
 vp -C <dir> <cmd> [args...]  ===  cd <dir> && vp <cmd> [args...]
 ```
 
-The child's spawn cwd is `<dir>`, so config lookup, `.env` loading, `process.cwd()` reads in configs and plugins, and relative CLI args all behave as if the user had `cd`'d. The Rust layers never mutate their own cwd; the one exception is the local `vp` bin, which applies `-C` by changing its process cwd at startup before any dispatch, indistinguishable from having been started in `<dir>`.
+The child's spawn cwd is `<dir>`, so config lookup, `.env` loading, `process.cwd()` reads in configs and plugins, and relative CLI args all behave as if the user had `cd`'d. Both entry points apply `-C` by changing their own process cwd at startup, before any argument normalization, picker, or command logic runs, which is indistinguishable from having been started in `<dir>`: the global binary consumes the flag first thing in `main` (so command aliases, the no-command picker, and in-process helpers that read the process cwd are all covered), and the local `vp` bin does the same before dispatch.
 
 The global binary also resolves the local `vite-plus` install from `<dir>`, matching `cd` exactly; through the package's own `vp` bin the executing CLI is already chosen, so there the invariant assumes a single Vite+ version per workspace (the supported monorepo model).
 
@@ -312,7 +312,8 @@ export default defineConfig({
 - Type: `string`, a single directory. A per-command map can come later if real demand appears.
 - Consulted when a bare app command runs in the directory containing the root config: a workspace root, or a non-workspace repo root. The non-workspace shape has no package list, so `defaultPackage` is the only mechanism that covers it. An explicit `-C` always wins.
 - A missing directory errors: `defaultPackage points to a missing directory: ./frontend`.
-- Read via static extraction (`vite_static_config` + the loader in `packages/cli/binding/src/cli/handler.rs`), like `run` config. At a non-workspace root there is no install to execute the config, so the file must work unexecuted: a plain default-export object with a static string value. If extraction fails and no local install can execute the config, vp errors and names the offending construct.
+- Read via static extraction (`vite_static_config` + the loader in `packages/cli/binding/src/cli/handler.rs`), like `run` config. At a non-workspace root there is no install to execute the config, so the file must work unexecuted: a plain default-export object with a static string value.
+- Only an explicitly declared `defaultPackage` changes behavior. A declared but non-static value (e.g. `process.env.DIR`) errors; a config that is unanalyzable or hides fields behind a spread is treated as not declaring the key and falls through to the picker or current-dir resolution, so an exotic config can never break unrelated bare commands.
 
 ## Decisions
 

-Original file line number
+Diff line change
@@ @@ -0,0 +1,6 @@ @@
 +<!doctype html>
 +<html>
 +  <body>
 +    <h1>spread</h1>
 +  </body>
 +</html>
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+{ "name": "spread-config", "private": true }`