fix(cli): address Codex review round 2

fengmk2 · fengmk2 · commit be1826f4ff38 · 2026-07-06T02:59:26.000+08:00
- known value-taking flags (--port 4000, --mode production) keep an
  invocation bare so root elicitation still applies; unknown/optional-value
  flags keep the conservative fallback
- refresh POSIX PWD wherever -C or an elicited target applies (global main,
  local bin, binding env for retargeted tools) so process.env.PWD readers
  match the cd form
- a runnable workspace root stays selectable as a '.' row (picker, listing,
  auto-select), with a PTY regression fixture
- open field maps now record explicit NonStatic declarations, so a spread
  followed by defaultPackage: process.env.X errors instead of being
  silently ignored
- apply -C before shell completion so cwd-sensitive completers suggest
  from the target directory
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/index.html b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/index.html
@@ -0,0 +1,6 @@
+<!doctype html>
+<html>
+  <body>
+    <h1>root app</h1>
+  </body>
+</html>
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/package.json b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/package.json
@@ -0,0 +1 @@
+{ "name": "runnable-root", "private": true, "workspaces": ["packages/*"] }
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/packages/ui/package.json b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/packages/ui/package.json
@@ -0,0 +1 @@
+{ "name": "ui", "private": true, "type": "module", "main": "src/index.ts" }
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/packages/ui/src/index.ts b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/packages/ui/src/index.ts
@@ -0,0 +1 @@
+export const ui = true;
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/snapshots.toml b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/snapshots.toml
@@ -0,0 +1,15 @@
+[[case]]
+name = "auto_select_root"
+vp = ["local", "global"]
+comment = """
+A workspace root that is itself the only runnable app stays selectable: bare
+vp build auto-selects the root (shown as `.`) and runs in place, matching
+today's behavior for root apps (rfcs/cwd-flag.md, picker contents).
+"""
+steps = [["vp", "build"]]
+
+[[case]]
+name = "listing_includes_root"
+vp = ["local", "global"]
+comment = "The non-interactive listing offers the runnable root as a `.` row."
+steps = [{ argv = ["vp", "build"], tty = false }]
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/snapshots/auto_select_root.global.md b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/snapshots/auto_select_root.global.md
@@ -0,0 +1,20 @@
+# auto_select_root
+
+A workspace root that is itself the only runnable app stays selectable: bare
+vp build auto-selects the root (shown as `.`) and runs in place, matching
+today's behavior for root apps (rfcs/cwd-flag.md, picker contents).
+
+## `vp build`
+
+```
+VITE+ - The Unified Toolchain for the Web
+
+Selected package: runnable-root (.)
+Tip: run this directly with `vp -C . build`
+vite <version> building client environment for production...
+✓ 2 modules transformed.
+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_runnable_root/snapshots/auto_select_root.local.md b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/snapshots/auto_select_root.local.md
@@ -0,0 +1,18 @@
+# auto_select_root
+
+A workspace root that is itself the only runnable app stays selectable: bare
+vp build auto-selects the root (shown as `.`) and runs in place, matching
+today's behavior for root apps (rfcs/cwd-flag.md, picker contents).
+
+## `vp build`
+
+```
+Selected package: runnable-root (.)
+Tip: run this directly with `vp -C . build`
+vite <version> building client environment for production...
+✓ 2 modules transformed.
+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_runnable_root/snapshots/listing_includes_root.global.md b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/snapshots/listing_includes_root.global.md
@@ -0,0 +1,18 @@
+# listing_includes_root
+
+The non-interactive listing offers the runnable root as a `.` row.
+
+## `vp build`
+
+**Exit code:** 1
+
+```
+[1m[31merror:[39m[0m `vp build` at the workspace root needs a target package.
+
+  Packages in this workspace:
+    runnable-root  .
+    ui             packages/ui
+
+  Pass a directory:  vp -C . build
+  Or run every package's build script:  vp run -r build
+```
diff --git a/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/snapshots/listing_includes_root.local.md b/crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/app_root_runnable_root/snapshots/listing_includes_root.local.md
@@ -0,0 +1,18 @@
+# listing_includes_root
+
+The non-interactive listing offers the runnable root as a `.` row.
+
+## `vp build`
+
+**Exit code:** 1
+
+```
+[1m[31merror:[39m[0m `vp build` at the workspace root needs a target package.
+
+  Packages in this workspace:
+    runnable-root  .
+    ui             packages/ui
+
+  Pass a directory:  vp -C . build
+  Or run every package's build script:  vp run -r build
+```
diff --git a/crates/vite_global_cli/src/main.rs b/crates/vite_global_cli/src/main.rs
@@ -325,6 +325,18 @@ async fn main() -> ExitCode {
     }
 
     // Handle shell completion
+    // `complete()` exits before the normal `-C` handling below runs, so apply
+    // the target directory here too: cwd-sensitive completers (run tasks)
+    // should suggest from <dir>, where the completed command will run.
+    if env::var_os("VP_COMPLETE").is_some()
+        && let Some((dir, _)) = parse_leading_chdir(&args)
+        && let Ok(current) = vite_path::current_dir()
+    {
+        let target = current.join(&dir).clean();
+        if target.as_path().is_dir() {
+            let _ = std::env::set_current_dir(target.as_path());
+        }
+    }
     CompleteEnv::with_factory(command_with_help).var("VP_COMPLETE").complete();
 
     // Check for shim mode (invoked as node, npm, or npx)
@@ -363,6 +375,13 @@ async fn main() -> ExitCode {
             output::error(&format!("Failed to change directory to {dir}: {e}"));
             return ExitCode::FAILURE;
         }
+        // Keep the POSIX PWD in sync, like a real `cd`: Node tools commonly
+        // read process.env.PWD.
+        #[cfg(unix)]
+        // SAFETY: single-threaded startup, before any command logic runs.
+        unsafe {
+            std::env::set_var("PWD", cwd.as_path());
+        }
         args.drain(1..=consumed);
     }
 
diff --git a/crates/vite_static_config/src/lib.rs b/crates/vite_static_config/src/lib.rs
@@ -29,12 +29,13 @@ pub enum FieldValue {
 ///   The map is exhaustive: every field is accounted for, absent keys do not exist.
 ///
 /// - [`FieldMapInner::Open`] — the object had at least one spread or computed-key
-///   property. The map contains only [`serde_json::Value`] entries for keys
-///   explicitly declared **after** the last such entry. Absent keys may exist via
-///   the spread and are treated as [`FieldValue::NonStatic`] by [`FieldMap::get`].
+///   property. The map contains entries only for keys explicitly declared
+///   **after** the last such entry (both static and `NonStatic` values). Absent
+///   keys may exist via the spread and are treated as [`FieldValue::NonStatic`]
+///   by [`FieldMap::get`], but not by [`FieldMap::get_declared`].
 enum FieldMapInner {
     Closed(FxHashMap<Box<str>, FieldValue>),
-    Open(FxHashMap<Box<str>, serde_json::Value>),
+    Open(FxHashMap<Box<str>, FieldValue>),
 }
 
 /// Extracted fields from a vite config object.
@@ -57,15 +58,15 @@ impl FieldMap {
     ///
     /// - [`Closed`](FieldMapInner::Closed): returns the stored value, or `None`
     ///   if the field is definitively absent.
-    /// - [`Open`](FieldMapInner::Open): returns the stored `Json` value if
-    ///   explicitly declared after the last spread/computed key, or
-    ///   `Some(NonStatic)` for any other key (it may exist in the spread).
+    /// - [`Open`](FieldMapInner::Open): returns the stored value if explicitly
+    ///   declared after the last spread/computed key, or `Some(NonStatic)` for
+    ///   any other key (it may exist in the spread).
     #[must_use]
     pub fn get(&self, key: &str) -> Option<FieldValue> {
         match &self.0 {
             FieldMapInner::Closed(map) => map.get(key).cloned(),
             FieldMapInner::Open(map) => {
-                Some(map.get(key).map_or(FieldValue::NonStatic, |v| FieldValue::Json(v.clone())))
+                Some(map.get(key).cloned().unwrap_or(FieldValue::NonStatic))
             }
         }
     }
@@ -91,7 +92,7 @@ impl FieldMap {
     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())),
+            FieldMapInner::Open(map) => map.get(key).cloned(),
         }
     }
 }
@@ -364,11 +365,12 @@ fn extract_object_fields(obj: &oxc_ast::ast::ObjectExpression<'_>) -> FieldMap {
                 map.insert(Box::from(key.as_ref()), value);
             }
             FieldMapInner::Open(map) => {
-                // Only Json values are meaningful in Open — NonStatic is already implied
-                // for any absent key, so there's no need to record it explicitly.
-                if let Some(json) = expr_to_json(&prop.value) {
-                    map.insert(Box::from(key.as_ref()), json);
-                }
+                // Record explicit declarations, including NonStatic ones:
+                // `get_declared` must distinguish a written `key: expr` from a
+                // key that merely might exist behind the spread.
+                let value =
+                    expr_to_json(&prop.value).map_or(FieldValue::NonStatic, FieldValue::Json);
+                map.insert(Box::from(key.as_ref()), value);
             }
         }
     }
@@ -485,6 +487,12 @@ mod tests {
         let map = parse("export default { defaultPackage: process.env.DIR };");
         assert_eq!(map.get_declared("defaultPackage"), Some(FieldValue::NonStatic));
 
+        // Open map: an explicit non-static declaration after a spread is
+        // still a declaration, not an unknown.
+        let map =
+            parse("const base = {};\nexport default { ...base, 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);
diff --git a/packages/cli/binding/src/cli/app_target.rs b/packages/cli/binding/src/cli/app_target.rs
@@ -44,13 +44,46 @@ fn app_command_parts(subcommand: &SynthesizableSubcommand) -> Option<(&'static s
     }
 }
 
-/// Bare = no positional target and no help-like flag. A non-flag token may be
-/// a flag value (`--port 3000`), so any non-flag argument conservatively
-/// disables elicitation; help/version requests are answered by the underlying
-/// tool and must never be redirected.
+/// Flags of the app tools (Vite dev/build/preview and tsdown) that always
+/// consume the next argv token as their value. Space-separated values of
+/// these flags are not positional targets. Flags with optional values
+/// (`--host`, `--sourcemap`, `--minify`, ...) are deliberately absent: a
+/// token after them stays ambiguous and keeps the conservative fallback.
+const VALUE_TAKING_FLAGS: &[&str] = &[
+    "-c",
+    "--config",
+    "-m",
+    "--mode",
+    "-l",
+    "--logLevel",
+    "--port",
+    "--base",
+    "--outDir",
+    "-d",
+    "--out-dir",
+    "--target",
+    "-f",
+    "--format",
+    "--platform",
+];
+
+/// Bare = no positional target and no help-like flag. Values of known
+/// value-taking flags (`--port 3000`) are skipped; any other non-flag token
+/// may be a positional target and conservatively disables elicitation.
+/// Help/version requests are answered by the underlying tool and must never
+/// be redirected.
 fn is_bare(args: &[String]) -> bool {
-    args.iter()
-        .all(|arg| arg.starts_with('-') && !super::help::is_app_tool_help_or_version_flag(arg))
+    let mut iter = args.iter();
+    while let Some(arg) = iter.next() {
+        if !arg.starts_with('-') || super::help::is_app_tool_help_or_version_flag(arg) {
+            return false;
+        }
+        if VALUE_TAKING_FLAGS.contains(&arg.as_str()) {
+            // Consume the flag's value; a missing value is the tool's error.
+            iter.next();
+        }
+    }
+    true
 }
 
 /// Heuristic ranking signal: does `dir` look runnable for `command`?
@@ -182,15 +215,22 @@ pub(super) fn resolve_app_target(
         vite_workspace::load_package_graph(&workspace_root).map_err(|e| Error::Anyhow(e.into()))?;
     let mut rows: Vec<PackageRow> = graph
         .node_weights()
-        .filter(|info| !info.path.as_str().is_empty())
-        .map(|info| {
+        .filter_map(|info| {
             let absolute = info.absolute_path.to_absolute_path_buf();
-            PackageRow {
+            let runnable = looks_runnable(&absolute, command);
+            let is_root = info.path.as_str().is_empty();
+            // The root itself is a valid target only when it looks runnable;
+            // `.` keeps the -C hint and the selection working there.
+            if is_root && !runnable {
+                return None;
+            }
+            let path = if is_root { "." } else { info.path.as_str() };
+            Some(PackageRow {
                 name: info.package_json.name.clone(),
-                path: vite_str::Str::from(info.path.as_str()),
-                runnable: looks_runnable(&absolute, command),
+                path: vite_str::Str::from(path),
+                runnable,
                 absolute,
-            }
+            })
         })
         .collect();
     if rows.is_empty() {
@@ -241,9 +281,14 @@ mod tests {
         assert!(is_bare(&to_args(&["-w", "--minify"])));
         // A positional target disables elicitation.
         assert!(!is_bare(&to_args(&["apps/web"])));
-        // A flag value is indistinguishable from a positional without knowing
-        // the tool's flag arity, so it conservatively counts as non-bare.
-        assert!(!is_bare(&to_args(&["--port", "3000"])));
+        // Known value-taking flags consume their value.
+        assert!(is_bare(&to_args(&["--port", "3000"])));
+        assert!(is_bare(&to_args(&["--mode", "production", "--minify"])));
+        assert!(is_bare(&to_args(&["--port=3000"])));
+        // A token after an unknown or optional-value flag is ambiguous with a
+        // positional target, so it conservatively counts as non-bare.
+        assert!(!is_bare(&to_args(&["--watch", "apps/web"])));
+        assert!(!is_bare(&to_args(&["--host", "0.0.0.0"])));
         // Help/version requests go to the underlying tool, never elicitation.
         assert!(!is_bare(&to_args(&["--help"])));
         assert!(!is_bare(&to_args(&["-h"])));
diff --git a/packages/cli/binding/src/cli/mod.rs b/packages/cli/binding/src/cli/mod.rs
@@ -66,11 +66,18 @@ async fn execute_direct_subcommand(
         SubcommandResolver::new(Arc::clone(&workspace_path))
     };
 
-    let envs: Arc<FxHashMap<Arc<OsStr>, Arc<OsStr>>> = Arc::new(
-        std::env::vars_os()
+    let envs: Arc<FxHashMap<Arc<OsStr>, Arc<OsStr>>> = Arc::new({
+        let mut envs: FxHashMap<Arc<OsStr>, Arc<OsStr>> = std::env::vars_os()
             .map(|(k, v)| (Arc::from(k.as_os_str()), Arc::from(v.as_os_str())))
-            .collect(),
-    );
+            .collect();
+        // The tool runs with `cwd` as its working directory; keep the POSIX
+        // PWD consistent with it, like a real `cd` (matters when elicitation
+        // retargeted the command to another package).
+        if cfg!(unix) {
+            envs.insert(Arc::from(OsStr::new("PWD")), Arc::from(cwd.as_path().as_os_str()));
+        }
+        envs
+    });
     let envs = envs_with_explicit_package_manager_path(cwd, envs).await?;
 
     let status = match subcommand {
diff --git a/packages/cli/src/bin.ts b/packages/cli/src/bin.ts
@@ -57,6 +57,10 @@ if (args[0]?.startsWith('-C')) {
     process.exit(1);
   }
   process.chdir(target);
+  if (process.platform !== 'win32') {
+    // Keep the POSIX PWD in sync, like a real `cd`.
+    process.env.PWD = target;
+  }
   args = args.slice(inline ? 1 : 2);
   process.argv = process.argv.slice(0, 2).concat(args);
 }
diff --git a/rfcs/cwd-flag.md b/rfcs/cwd-flag.md
@@ -246,7 +246,7 @@ Rejected alternatives: repurposing the app-command positional to mean "run there
 
 ### Target directory resolution
 
-An app command invocation is **bare** when it has no `-C` and no positional target (no Vite `[root]`, no pack entries); flags alone keep it bare. For `vp dev` / `build` / `preview` / `pack`, the target directory is resolved in this order:
+An app command invocation is **bare** when it has no `-C` and no positional target (no Vite `[root]`, no pack entries). Flags keep it bare, including known value-taking flags together with their values (`--port 3000`, `--mode production`); a token following an unknown or optional-value flag is ambiguous with a positional target and conservatively counts as one. For `vp dev` / `build` / `preview` / `pack`, the target directory is resolved in this order:
 
 1. **`-C <dir>`**: run there. Never triggers the picker.
 2. **Positional target present**: forward as today, upstream semantics, vp does not interfere.
@@ -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. 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 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 POSIX `PWD` environment variable is refreshed too (by both entry points and for elicitation-retargeted tools), so `process.env.PWD` readers match the `cd` form. 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).
 

-Original file line number
+Diff line change
@@ @@ -0,0 +1,6 @@ @@
 +<!doctype html>
 +<html>
 +  <body>
 +    <h1>root app</h1>
 +  </body>
 +</html>
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+{ "name": "runnable-root", "private": true, "workspaces": ["packages/*"] }`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+{ "name": "ui", "private": true, "type": "module", "main": "src/index.ts" }`