diff --git a/CLI/programa.swift b/CLI/programa.swift index 078a56d5..ae21aa26 100644 --- a/CLI/programa.swift +++ b/CLI/programa.swift @@ -2112,7 +2112,7 @@ struct ProgramaCLI { let (panelArg, rem1) = self.parseOption(rem0, name: "--panel") let (sfArg, rem2) = self.parseOption(rem1, name: "--surface") let workspaceArg = wsArg ?? (ctx.windowId == nil ? ProcessInfo.processInfo.environment["PROGRAMA_WORKSPACE_ID"] : nil) - let surfaceRaw = sfArg ?? panelArg ?? (wsArg == nil && ctx.windowId == nil ? ProcessInfo.processInfo.environment["PROGRAMA_SURFACE_ID"] : nil) + let surfaceRaw = sfArg ?? panelArg ?? (workspaceArg == nil && ctx.windowId == nil ? ProcessInfo.processInfo.environment["PROGRAMA_SURFACE_ID"] : nil) guard let direction = rem2.first else { throw CLIError(message: "new-split requires a direction") } @@ -2336,7 +2336,7 @@ struct ProgramaCLI { execute: { ctx in let csWsFlag = self.optionValue(ctx.commandArgs, name: "--workspace") let workspaceArg = csWsFlag ?? (ctx.windowId == nil ? ProcessInfo.processInfo.environment["PROGRAMA_WORKSPACE_ID"] : nil) - let surfaceRaw = self.optionValue(ctx.commandArgs, name: "--surface") ?? self.optionValue(ctx.commandArgs, name: "--panel") ?? (csWsFlag == nil && ctx.windowId == nil ? ProcessInfo.processInfo.environment["PROGRAMA_SURFACE_ID"] : nil) + let surfaceRaw = self.optionValue(ctx.commandArgs, name: "--surface") ?? self.optionValue(ctx.commandArgs, name: "--panel") ?? (workspaceArg == nil && ctx.windowId == nil ? ProcessInfo.processInfo.environment["PROGRAMA_SURFACE_ID"] : nil) var params: [String: Any] = [:] let wsId = try self.normalizeWorkspaceHandle(workspaceArg, client: ctx.client) if let wsId { params["workspace_id"] = wsId } @@ -3088,7 +3088,7 @@ struct ProgramaCLI { : (ctx.windowId == nil ? ProcessInfo.processInfo.environment["PROGRAMA_WORKSPACE_ID"] : nil) let workspaceArg = explicitWorkspaceArg ?? callerWorkspaceArg let explicitSurfaceArg = self.optionValue(ctx.commandArgs, name: "--surface") - let callerSurfaceArg = explicitSurfaceArg == nil && preferTTYFallback == false && ctx.windowId == nil + let callerSurfaceArg = explicitSurfaceArg == nil && workspaceArg == nil && preferTTYFallback == false && ctx.windowId == nil ? ProcessInfo.processInfo.environment["PROGRAMA_SURFACE_ID"] : nil let surfaceArg = explicitSurfaceArg ?? callerSurfaceArg diff --git a/tests_v2/ci_subset.txt b/tests_v2/ci_subset.txt index 0e419c5f..fe3f26ab 100644 --- a/tests_v2/ci_subset.txt +++ b/tests_v2/ci_subset.txt @@ -19,3 +19,4 @@ test_cli_browser_console_errors_text.py test_jsonrpc_strict_param_validation.py test_workspace_relative.py test_surface_send_unknown_id.py +test_close_surface_env_precedence.py diff --git a/tests_v2/test_close_surface_env_precedence.py b/tests_v2/test_close_surface_env_precedence.py new file mode 100644 index 00000000..9c79e18c --- /dev/null +++ b/tests_v2/test_close_surface_env_precedence.py @@ -0,0 +1,135 @@ +#!/usr/bin/env python3 +"""Regression: PROGRAMA_SURFACE_ID env fallback must not override a workspace +target that itself resolved from PROGRAMA_WORKSPACE_ID env (issue #162). + +Issue #148 fixed this precedence bug for send/send-key/read-screen: the +PROGRAMA_SURFACE_ID env fallback is suppressed whenever a workspace target +resolved (explicit --workspace flag OR PROGRAMA_WORKSPACE_ID env). #162 found +that close-surface (and new-split, and notify) still only suppressed the +surface fallback when an *explicit --workspace flag* was passed +(`csWsFlag == nil`), not when the workspace came from env instead +(`workspaceArg == nil`) -- so a stale surface id inherited from another app +instance's shell could still be sent as surface_id even though +PROGRAMA_WORKSPACE_ID already resolved an explicit target. + +This is observable end-to-end because the app enforces workspace membership +for the surface id: `guard ws.panels[surfaceId] != nil else { ... not_found }` +in v2SurfaceClose (Sources/TerminalController+Surface.swift). So: + + - Pre-fix: close-surface with only PROGRAMA_WORKSPACE_ID= and + PROGRAMA_SURFACE_ID= set + (no CLI flags) sends that foreign surface_id straight through, and the + app rejects it with not_found -- even though a perfectly good surface + exists in the target workspace and should have been used instead. + - Post-fix: the stale env surface id is suppressed because a workspace + target already resolved (via env), so close-surface falls back to the + target workspace's own focused surface and succeeds. +""" + +import glob +import os +import subprocess +import sys +from pathlib import Path +from typing import Dict, List, Optional + +sys.path.insert(0, str(Path(__file__).parent)) +from cmux import cmux, cmuxError + + +SOCKET_PATH = os.environ.get("PROGRAMA_SOCKET", "/tmp/programa-debug.sock") + + +def _must(cond: bool, msg: str) -> None: + if not cond: + raise cmuxError(msg) + + +def _find_cli_binary() -> str: + env_cli = os.environ.get("CMUXTERM_CLI") + if env_cli and os.path.isfile(env_cli) and os.access(env_cli, os.X_OK): + return env_cli + + fixed = os.path.expanduser("~/Library/Developer/Xcode/DerivedData/cmux-tests-v2/Build/Products/Debug/cmux") + if os.path.isfile(fixed) and os.access(fixed, os.X_OK): + return fixed + + candidates = glob.glob(os.path.expanduser("~/Library/Developer/Xcode/DerivedData/**/Build/Products/Debug/cmux"), recursive=True) + candidates += glob.glob("/tmp/programa-*/Build/Products/Debug/programa") + candidates = [p for p in candidates if os.path.isfile(p) and os.access(p, os.X_OK)] + if not candidates: + raise cmuxError("Could not locate cmux CLI binary; set CMUXTERM_CLI") + candidates.sort(key=lambda p: os.path.getmtime(p), reverse=True) + return candidates[0] + + +def _run_cli(cli: str, args: List[str], env_overrides: Optional[Dict[str, str]] = None) -> str: + env = dict(os.environ) + # Hermetic: strip caller-context env vars so this test's behavior does not + # depend on whatever instance/workspace/surface the invoking shell happens + # to be scoped to (see tests_v2/test_workspace_relative.py:52-57). Tests + # opt back in via env_overrides. + for var in ("PROGRAMA_WORKSPACE_ID", "PROGRAMA_SURFACE_ID", "PROGRAMA_PANEL_ID", "PROGRAMA_TAB_ID"): + env.pop(var, None) + if env_overrides: + env.update(env_overrides) + cmd = [cli, "--socket", SOCKET_PATH] + args + proc = subprocess.run(cmd, capture_output=True, text=True, check=False, env=env) + if proc.returncode != 0: + merged = f"{proc.stdout}\n{proc.stderr}".strip() + raise cmuxError(f"CLI failed ({' '.join(cmd)}): {merged}") + return proc.stdout.strip() + + +def main() -> int: + cli = _find_cli_binary() + + with cmux(SOCKET_PATH) as c: + target_ws = c.new_workspace() + stale_ws = c.new_workspace() + try: + stale_surfaces = c.list_surfaces(stale_ws) + _must(len(stale_surfaces) == 1, f"expected exactly one surface in a fresh workspace, got {stale_surfaces!r}") + stale_surface_id = stale_surfaces[0][1] + + # Give target_ws a second surface so close-surface has something + # to close without tripping the "cannot close the last surface" + # guard, regardless of which of the two ends up focused. + c.select_workspace(target_ws) + c.new_surface() + before = c.list_surfaces(target_ws) + _must(len(before) == 2, f"expected two surfaces in target workspace, got {before!r}") + before_ids = {row[1] for row in before} + + # Repro: only PROGRAMA_WORKSPACE_ID + PROGRAMA_SURFACE_ID env vars + # set, no --workspace/--surface flags -- this is the exact shape + # that bypassed the old `csWsFlag == nil` check. + _run_cli( + cli, + ["close-surface"], + env_overrides={ + "PROGRAMA_WORKSPACE_ID": target_ws, + "PROGRAMA_SURFACE_ID": stale_surface_id, + }, + ) + + after = c.list_surfaces(target_ws) + after_ids = {row[1] for row in after} + _must(len(after) == 1, f"expected one surface left in target workspace, got {after!r}") + _must(after_ids.issubset(before_ids), "close-surface closed a surface that was never in target_ws") + + stale_after = c.list_surfaces(stale_ws) + _must( + len(stale_after) == 1 and stale_after[0][1] == stale_surface_id, + f"stale workspace's surface must be untouched, got {stale_after!r}", + ) + finally: + c.close_workspace(target_ws) + c.close_workspace(stale_ws) + + print("PASS: close-surface ignores stale PROGRAMA_SURFACE_ID once PROGRAMA_WORKSPACE_ID resolved a target") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main())