From 694439b6f3935c15e05b011dc39bf34c34dc2af9 Mon Sep 17 00:00:00 2001 From: the_og Date: Tue, 4 Aug 2026 04:25:28 +0000 Subject: [PATCH] feat(#83): variables readout, two clear move actions, args->env relocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Field testing showed the feature was doing the right thing but describing it wrong, and left the moved variable invisible. Reworked per that feedback: Naming. "Move to environment variable" read like "move into the Environment variables table"; it actually creates a ${VAR} reference to the shell/OS environment. Renamed the action to "Replace with a ${VAR} reference (out of file)…" everywhere, and the dialog now says plainly the secret goes to your shell environment -- not this file, not the table below. Visibility (the real gap). A lone ${SECRET_KEY} with nothing saying whether it's wired up isn't much better than a mystery. New Variables… button opens ReferencedVarsDialog: every ${VAR} the loaded server references, each with ✓ set / ✓ default / ✗ not set and the exact export/setx line to set it. Backed by pure core.referenced_env_vars (dedupes across fields, resolves against a given environment or a default). Two actions on an args secret, because the user may want either: * "Replace with a ${VAR} reference (out of file)…" -- secret leaves the file (needs a client that expands refs; disabled with reason on Desktop). * "Move into Environment variables (kept in this config)…" -- relocates the arg into the env block where it's visible and editable. Works on any client (no ${VAR} needed). core.move_arg_to_env_block drops the flag+value and sets env[VAR]; the dialog warns it changes how the server launches. Both args actions run through ServerEditor (moving into env touches args AND the env table), which reloads the form from the transformed data. Tests: +10 core (referenced_env_vars dedupe/status/default; move_arg_to_env_block flag+value removal, bare positional, None on bad target). 488 passed, ruff clean. New dialogs/menus are GUI, untestable in CI as before. Refs #83 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01EKwBecy6N83jnqQmw8ezwE --- bcc.py | 252 +++++++++++++++++++++++++++++++++++++++------ bcc_core.py | 82 +++++++++++++++ tests/test_core.py | 51 +++++++++ 3 files changed, 354 insertions(+), 31 deletions(-) diff --git a/bcc.py b/bcc.py index 57a957c..a37eaeb 100644 --- a/bcc.py +++ b/bcc.py @@ -291,9 +291,10 @@ class MoveToEnvDialog(QDialog): v.setSpacing(10) intro = QLabel( - "The value will be removed from the config and replaced with a " - "reference. Set the variable in your environment first, or the " - "server won't authenticate." + "This replaces the value in place with a ${VAR} reference. The secret " + "moves to your shell/OS environment — not this config file, and not the " + "Environment variables table below. Run the line below to set it there, " + "or the server won't authenticate." ) intro.setWordWrap(True) v.addWidget(intro) @@ -356,6 +357,141 @@ class MoveToEnvDialog(QDialog): self._already.hide() +class MoveArgToEnvDialog(QDialog): + """Confirm relocating a secret arg into the env block (#83, kept in file). + + Unlike the reference move, this keeps the value in the config -- it just + moves it out of the argument list (visible in process listings) and into + the Environment variables table, where the user can see and edit it. It + changes how the server is launched, so it says so plainly. + """ + + def __init__(self, parent, key: str, value: str): + super().__init__(parent) + self.setWindowTitle("Move into environment variables") + self.setMinimumWidth(460) + v = QVBoxLayout(self) + v.setSpacing(10) + + intro = QLabel( + "This moves the secret out of the arguments and into the Environment " + "variables table below, where you can see and edit its value. The value " + "stays in this config file." + ) + intro.setWordWrap(True) + v.addWidget(intro) + + warn = QLabel( + "⚠ This changes how the server is launched: the flag is dropped and the " + "value is set as an environment variable instead. It only works if the " + "server reads this secret from that variable." + ) + warn.setWordWrap(True) + warn.setStyleSheet(f"color: {WARN};") + v.addWidget(warn) + + grid = QGridLayout() + grid.setSpacing(8) + lbl = QLabel("Variable:") + lbl.setObjectName("muted") + grid.addWidget(lbl, 0, 0) + self._name_edit = QLineEdit(core.sanitize_env_var_name(key)) + grid.addWidget(self._name_edit, 0, 1) + v.addLayout(grid) + + btns = QDialogButtonBox( + QDialogButtonBox.StandardButton.Ok | QDialogButtonBox.StandardButton.Cancel + ) + ok = btns.button(QDialogButtonBox.StandardButton.Ok) + ok.setText("Move into env") + ok.setObjectName("primary") + btns.accepted.connect(self.accept) + btns.rejected.connect(self.reject) + v.addWidget(btns) + + def var_name(self) -> str: + return core.sanitize_env_var_name(self._name_edit.text()) + + +class ReferencedVarsDialog(QDialog): + """Show every ${VAR} the loaded server references and whether it's set (#83). + + After a secret becomes a reference, the variable lives in the user's + environment, not the config -- so this is where they confirm it exists and + get the command to set it. Read-only; BCC can't (and shouldn't) store the + value. + """ + + def __init__(self, parent, data: dict): + super().__init__(parent) + self.setWindowTitle("Referenced variables") + self.setMinimumWidth(560) + v = QVBoxLayout(self) + v.setSpacing(10) + + self._usages = core.referenced_env_vars(data) + if not self._usages: + v.addWidget(QLabel("This server references no ${VAR} variables.")) + btns = QDialogButtonBox(QDialogButtonBox.StandardButton.Close) + btns.rejected.connect(self.reject) + btns.accepted.connect(self.accept) + v.addWidget(btns) + return + + intro = QLabel( + "These references are read from your shell/OS environment when the client " + "runs. ✓ means it's set in BCC's environment (which may differ from the " + "client's) or has a default; ✗ means nothing would fill it." + ) + intro.setWordWrap(True) + v.addWidget(intro) + + self._table = QTableWidget(len(self._usages), 3) + self._table.setHorizontalHeaderLabels(["Variable", "Status", "Used in"]) + self._table.horizontalHeader().setSectionResizeMode(0, QHeaderView.ResizeMode.Stretch) + self._table.horizontalHeader().setSectionResizeMode(2, QHeaderView.ResizeMode.Stretch) + self._table.verticalHeader().setVisible(False) + self._table.setSelectionBehavior(QAbstractItemView.SelectionBehavior.SelectRows) + self._table.setEditTriggers(QAbstractItemView.EditTrigger.NoEditTriggers) + for r, u in enumerate(self._usages): + is_set = core.is_env_var_set(u.name) + status = "✓ set" if is_set else ("✓ default" if u.has_default else "✗ not set") + self._table.setItem(r, 0, QTableWidgetItem(u.name)) + self._table.setItem(r, 1, QTableWidgetItem(status)) + self._table.setItem(r, 2, QTableWidgetItem(", ".join(u.fields))) + self._table.selectionModel().selectionChanged.connect(self._refresh_cmd) + v.addWidget(self._table, 1) + + set_lbl = QLabel("Set the selected variable with:") + set_lbl.setObjectName("muted") + v.addWidget(set_lbl) + self._cmd = QLabel("") + self._cmd.setWordWrap(True) + self._cmd.setTextInteractionFlags(Qt.TextInteractionFlag.TextSelectableByMouse) + self._cmd.setStyleSheet("font-family: monospace;") + v.addWidget(self._cmd) + + btns = QDialogButtonBox(QDialogButtonBox.StandardButton.Close) + btns.rejected.connect(self.reject) + btns.accepted.connect(self.accept) + v.addWidget(btns) + + self._table.selectRow(0) + + def _refresh_cmd(self, *_): + rows = self._table.selectionModel().selectedRows() + if not rows: + self._cmd.setText("") + return + name = self._usages[rows[0].row()].name + # A placeholder value -- BCC doesn't hold the secret, this shows the shape. + lines = core.shell_export_lines(name, "") + if sys.platform == "win32": + self._cmd.setText(f"{lines['windows']}\n\n(macOS/Linux: {lines['posix']})") + else: + self._cmd.setText(f"{lines['posix']}\n\n(Windows: {lines['windows']})") + + class KeyValueTable(QWidget): def __init__(self, key_label="Key", val_label="Value", on_change=None, before_change=None): super().__init__() @@ -423,14 +559,14 @@ class KeyValueTable(QWidget): return profile = self.profile_provider() if self.profile_provider else None menu = QMenu(self) - act = QAction("Move to environment variable…", self) + act = QAction("Replace with a ${VAR} reference (out of file)…", self) if core.can_move_value_to_env_ref(key, value, profile): act.triggered.connect(lambda: self._move_row_to_env(row)) else: # Show it disabled with the reason rather than an empty menu, so the # feature is discoverable and Claude Desktop's gating is explained. act.setEnabled(False) - act.setText("Move to environment variable — unavailable for Claude Desktop") + act.setText("Replace with ${VAR} reference — unavailable for Claude Desktop") act.setToolTip( "Claude Desktop doesn't expand ${VAR}, so a reference would reach " "the server as literal text." @@ -661,6 +797,12 @@ class ServerEditor(QFrame): self.logs_btn = QPushButton("View logs") self.logs_btn.setToolTip("Open this server's MCP log in a read-only, auto-tailing viewer") self.logs_btn.clicked.connect(self._view_logs) + self.vars_btn = QPushButton("Variables…") + self.vars_btn.setToolTip( + "Show the ${VAR} references this server uses and whether each is set in " + "your environment" + ) + self.vars_btn.clicked.connect(self._show_referenced_vars) self.details_btn = QPushButton("Details ▸") self.details_btn.setCheckable(True) self.details_btn.toggled.connect(self._toggle_diag) @@ -672,6 +814,7 @@ class ServerEditor(QFrame): dep.addWidget(self.test_btn) dep.addWidget(self.spawn_btn) dep.addWidget(self.logs_btn) + dep.addWidget(self.vars_btn) dep.addWidget(self.details_btn) dep.addWidget(recheck) outer.addLayout(dep) @@ -773,11 +916,56 @@ class ServerEditor(QFrame): return w def set_profile_provider(self, provider): - """Let the env/headers tables and the args editor gate "move to - environment variable" on which client the loaded profile targets (#83).""" + """Let the env/headers tables and the args editor gate the secret-move + actions on which client the loaded profile targets (#83), and wire the + args editor's two move actions back to this editor (which owns the whole + form, since moving an arg into env touches both fields).""" self.env.profile_provider = provider self.headers.profile_provider = provider self.args.profile_provider = provider + self.args.on_move_to_ref = self.move_arg_to_reference + self.args.on_move_to_env = self.move_arg_into_env + + # --- secret moves from args (#83) ------------------------------------ # + def _reload_from_data(self, new_data: dict): + """Repopulate the form from a transformed data dict and mark dirty.""" + self.load_entry(core.ServerEntry(self.current_name(), new_data, True)) + self._emit() + + def move_arg_to_reference(self, index: int): + """Args secret -> ${VAR} reference in place (secret leaves the file).""" + data = self.dump_data() + args = data.get("args") or [] + if not (0 <= index < len(args)): + return + dlg = MoveToEnvDialog( + self.window(), core.suggested_env_var_for_arg(args, index), args[index] + ) + if not dlg.exec(): + return + conv = core.move_value_to_env_ref(data, field="args", index=index, var_name=dlg.var_name()) + if conv is None: + return + QGuiApplication.clipboard().setText(conv.secret) + self._reload_from_data(conv.data) + + def move_arg_into_env(self, index: int): + """Args secret -> env block, kept in this config (visible/editable).""" + data = self.dump_data() + args = data.get("args") or [] + if not (0 <= index < len(args)): + return + default_name = core.suggested_env_var_for_arg(args, index) + dlg = MoveArgToEnvDialog(self.window(), default_name, args[index]) + if not dlg.exec(): + return + new = core.move_arg_to_env_block(data, index, var_name=dlg.var_name()) + if new is None: + return + self._reload_from_data(new) + + def _show_referenced_vars(self): + ReferencedVarsDialog(self.window(), self.dump_data()).exec() # --- model <-> form -------------------------------------------------- # def load_entry(self, entry: core.ServerEntry | None): @@ -1056,9 +1244,13 @@ class ArgsEdit(QPlainTextEdit): self.blockCountChanged.connect(self._update_gutter_width) self.updateRequest.connect(self._on_update_request) self._update_gutter_width() - # Set by ServerEditor so "move to environment variable" (#83) can gate on - # the loaded client, same as the env/headers tables. + # Wired by ServerEditor: gate on the loaded client, and the two move + # actions (which the editor performs, since moving an arg into env + # touches both the args and the env table). Indices are into the + # non-blank arg list, matching dump_data()'s args. self.profile_provider = None + self.on_move_to_ref = None + self.on_move_to_env = None def contextMenuEvent(self, event): menu = self.createStandardContextMenu() # keep cut/copy/paste @@ -1071,36 +1263,34 @@ class ArgsEdit(QPlainTextEdit): idx = sum(1 for ln in lines[:block] if ln.strip() != "") if idx in set(core.secret_arg_indices(cleaned)): profile = self.profile_provider() if self.profile_provider else None - act = QAction("Move to environment variable…", self) - if profile is None or core.client_expands_env_refs(profile): - act.triggered.connect(lambda: self._move_arg_to_env(block, idx, cleaned)) + expands = profile is None or core.client_expands_env_refs(profile) + first = menu.actions()[0] if menu.actions() else None + + # Reference (secret leaves the file) -- needs an expanding client. + ref_act = QAction("Replace with a ${VAR} reference (out of file)…", self) + if expands and self.on_move_to_ref: + ref_act.triggered.connect(lambda: self.on_move_to_ref(idx)) else: - act.setEnabled(False) - act.setText("Move to environment variable — unavailable for Claude Desktop") - act.setToolTip( + ref_act.setEnabled(False) + ref_act.setText( + "Replace with ${VAR} reference — unavailable for Claude Desktop" + ) + ref_act.setToolTip( "Claude Desktop doesn't expand ${VAR}, so a reference would " "reach the server as literal text." ) - first = menu.actions()[0] if menu.actions() else None - menu.insertAction(first, act) + + # Move into the env block (kept in file) -- works on any client. + env_act = QAction("Move into Environment variables (kept in this config)…", self) + if self.on_move_to_env: + env_act.triggered.connect(lambda: self.on_move_to_env(idx)) + + menu.insertAction(first, ref_act) + menu.insertAction(first, env_act) if first is not None: menu.insertSeparator(first) menu.exec(event.globalPos()) - def _move_arg_to_env(self, block: int, idx: int, cleaned: list): - value = cleaned[idx] - default_name = core.suggested_env_var_for_arg(cleaned, idx) - dlg = MoveToEnvDialog(self.window(), default_name, value) - if not dlg.exec(): - return - var_name = dlg.var_name() - QGuiApplication.clipboard().setText(value) - lines = self.toPlainText().splitlines() - if 0 <= block < len(lines): - lines[block] = f"${{{var_name}}}" - # setPlainText fires textChanged -> _emit (dirty + arg recheck). - self.setPlainText("\n".join(lines)) - def gutter_width(self) -> int: digits = max(1, len(str(self.blockCount()))) return 14 + self.fontMetrics().horizontalAdvance("9") * digits diff --git a/bcc_core.py b/bcc_core.py index ddcf7f0..f77441c 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -1960,6 +1960,88 @@ def is_env_var_set(var_name: str, environ: dict | None = None) -> bool: return bool(env.get(var_name)) +class EnvVarUsage(NamedTuple): + """One ${VAR} referenced by a server definition, and whether it resolves. + + `resolved` is best-effort: a variable is considered resolvable if it's set + in the checked environment OR any occurrence carries a `:-default`. Since + BCC's environment isn't necessarily the client's, this is advisory (the UI + says so). + """ + + name: str + fields: tuple[str, ...] # which config fields it appears in (sorted) + has_default: bool + resolved: bool + + +def referenced_env_vars(data: dict, environ: dict | None = None) -> list[EnvVarUsage]: + """Every distinct ${VAR} a server definition references, with its status. + + This is the "where did my secret go / is it wired up?" readout for #83: + after a value becomes `${VAR}`, the variable lives in the user's + environment, not the config, so BCC surfaces the list and whether each one + currently resolves. Sorted by name; deduped across fields. + """ + env = os.environ if environ is None else environ + by_name: dict[str, dict] = {} + for ref in server_env_refs(data): + slot = by_name.setdefault(ref.name, {"fields": set(), "has_default": False}) + slot["fields"].add(ref.field) + slot["has_default"] = slot["has_default"] or ref.has_default + out: list[EnvVarUsage] = [] + for name in sorted(by_name): + slot = by_name[name] + has_default = slot["has_default"] + out.append( + EnvVarUsage( + name=name, + fields=tuple(sorted(slot["fields"])), + has_default=has_default, + resolved=has_default or name in env, + ) + ) + return out + + +def move_arg_to_env_block(data: dict, index: int, var_name=None) -> dict | None: + """Relocate one secret arg into the `env` block, keeping the value in-file. + + The "managed in-file" alternative to a ${VAR} reference: the secret leaves + the args (where it's visible in process listings) and becomes an env entry + the user can see and edit in BCC's table. Returns a NEW data dict, or None + if the target isn't a movable string. + + When the arg follows a `--flag`, the flag is removed too, since a server + that reads the secret from an env var no longer needs the switch. This + changes how the server is launched -- the GUI warns before doing it. + """ + new = copy.deepcopy(data) + args = new.get("args") + if not isinstance(args, list) or not (0 <= index < len(args)): + return None + value = args[index] + if not isinstance(value, str) or is_env_ref(value): + return None + + vn = sanitize_env_var_name( + var_name if var_name is not None else suggested_env_var_for_arg(args, index) + ) + + # Drop the value, and the preceding flag if there is one (--api-key SECRET). + remove_from = index + if index > 0 and isinstance(args[index - 1], str) and args[index - 1].startswith("-"): + remove_from = index - 1 + del args[remove_from : index + 1] + + env = new.get("env") + if not isinstance(env, dict): + env = {} + new["env"] = env + env[vn] = value + return new + + def is_secret_key(name: str) -> bool: """Does this env-var / header / flag name look like it holds a secret?""" return bool(_SECRET_KEY_RE.search(name or "")) diff --git a/tests/test_core.py b/tests/test_core.py index 16017d4..7c8c0ec 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -3180,3 +3180,54 @@ def test_suggested_env_var_for_arg_uses_preceding_flag(): def test_suggested_env_var_for_arg_falls_back_when_no_flag(): args = ["ghp_secret"] assert c.suggested_env_var_for_arg(args, 0) == "SECRET" + + +# --------------------------------------------------------------------------- # +# Referenced-variables readout + args->env relocation (issue #83, "both") +# --------------------------------------------------------------------------- # +def test_referenced_env_vars_dedupes_and_reports_status(): + data = { + "command": "npx", + "args": ["--token", "${GH_TOKEN}", "${GH_TOKEN}"], + "env": {"API_KEY": "${API_KEY}", "REGION": "${REGION:-us-east-1}"}, + } + usages = c.referenced_env_vars(data, environ={"GH_TOKEN": "x"}) + by = {u.name: u for u in usages} + assert set(by) == {"GH_TOKEN", "API_KEY", "REGION"} + # GH_TOKEN appears only in args, deduped to one entry, set in env -> resolved + assert by["GH_TOKEN"].fields == ("args",) + assert by["GH_TOKEN"].resolved is True + # API_KEY not set, no default -> unresolved + assert by["API_KEY"].resolved is False + # REGION has a default -> resolved regardless of environment + assert by["REGION"].has_default is True + assert by["REGION"].resolved is True + # sorted by name + assert [u.name for u in usages] == sorted(u.name for u in usages) + + +def test_referenced_env_vars_empty_when_no_refs(): + assert c.referenced_env_vars({"command": "npx", "args": ["-y", "pkg"]}) == [] + + +def test_move_arg_to_env_block_removes_flag_and_value(): + data = {"command": "x", "args": ["--api-key", "sk-secret", "run"], "env": {"KEEP": "1"}} + out = c.move_arg_to_env_block(data, 1) + assert out["args"] == ["run"] # flag + value both gone + assert out["env"]["API_KEY"] == "sk-secret" + assert out["env"]["KEEP"] == "1" # existing env preserved + # input not mutated + assert data["args"] == ["--api-key", "sk-secret", "run"] + + +def test_move_arg_to_env_block_bare_positional_keeps_no_flag(): + data = {"command": "x", "args": ["ghp_secret", "serve"]} + out = c.move_arg_to_env_block(data, 0, var_name="GITHUB_TOKEN") + assert out["args"] == ["serve"] + assert out["env"] == {"GITHUB_TOKEN": "ghp_secret"} + + +def test_move_arg_to_env_block_none_on_bad_target(): + assert c.move_arg_to_env_block({"args": ["a"]}, 5) is None + assert c.move_arg_to_env_block({"args": ["${REF}"]}, 0) is None # already a ref + assert c.move_arg_to_env_block({}, 0) is None