From 8fdbe90b370bbc2a4eb112ce2e02122c681915ce Mon Sep 17 00:00:00 2001 From: the_og Date: Tue, 4 Aug 2026 03:35:21 +0000 Subject: [PATCH 1/3] =?UTF-8?q?feat:=20"Move=20to=20environment=20variable?= =?UTF-8?q?"=20=E2=80=94=20convert=20a=20plaintext=20secret=20to=20${VAR}?= =?UTF-8?q?=20(#83)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #76/#82: BCC warns when a config holds a raw credential and points at ${VAR}, but gave no way to make the change. This adds the one-click conversion, right-click a secret row in the env or headers table. The value is about to leave the file, so the action's real job is handing the secret back before it does: - Core (pure, tested): sanitize_env_var_name (key -> legal upper-case shell name; 'api-key' -> API_KEY, '2fa' -> _2FA, non-ASCII/empty handled), shell_export_lines (the exact export/setx line, POSIX single-quoted safely), move_value_to_env_ref (data in -> new data out, replaces one env/header/args value with ${VAR}, returns the removed secret; never mutates the input; None if the target is missing, non-string, or already a reference), can_move_value_to_env_ref (offer only a real stored secret, not already a ref, AND only on a client that expands references -- offering it on Claude Desktop would author a config that reaches the server as literal ${VAR}, the exact failure #76 exists to prevent), and is_env_var_set (skip the ceremony when the variable already looks set). - GUI: KeyValueTable gains a context menu gated on can_move_value_to_env_ref (so it never appears on a non-secret row or a Claude Desktop profile). MoveToEnvDialog lets the user name the variable (defaulting to the sanitised key), shows the platform-appropriate shell line live, notes when the variable already looks set, and on accept copies the secret to the clipboard before the cell is replaced with the reference. Wired through ServerEditor.set_profile_provider so the tables know which client is loaded. Scope note: env and headers rows for now. The core already handles args by index; wiring the args editor (a free-text widget, not a table) is a small follow-up, deliberately not bundled here. Tests: +15 core (name sanitisation incl. non-ASCII/leading-digit/empty, POSIX quote safety, the gate across secret/non-secret/already-ref/ non-expanding-client, env+headers+args rewrite, input-not-mutated, missing/non-string/already-ref -> None, is_env_var_set). 478 passed, ruff clean. GUI is untestable in CI (no PySide6); the decision logic all lives in bcc_core and is tested there. Closes #83 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01EKwBecy6N83jnqQmw8ezwE --- bcc.py | 137 +++++++++++++++++++++++++++++++++++++++++++++ bcc_core.py | 123 ++++++++++++++++++++++++++++++++++++++++ tests/test_core.py | 89 +++++++++++++++++++++++++++++ 3 files changed, 349 insertions(+) diff --git a/bcc.py b/bcc.py index dc224f7..fc53f11 100644 --- a/bcc.py +++ b/bcc.py @@ -272,6 +272,90 @@ class _SecretMaskDelegate(QStyledItemDelegate): option.text = core.MASK +class MoveToEnvDialog(QDialog): + """Confirm moving a plaintext secret out to a ${VAR} reference (#83). + + The secret is about to leave the config file, so this dialog's whole job is + to hand it back first: it lets the user name the variable, shows the exact + shell line to set it, and (on accept) the caller copies the secret to the + clipboard. If the variable already looks set in this environment, it says so + and drops the urgency. + """ + + def __init__(self, parent, key: str, secret: str): + super().__init__(parent) + self.setWindowTitle("Move to environment variable") + self.setMinimumWidth(460) + self._secret = secret + v = QVBoxLayout(self) + 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." + ) + intro.setWordWrap(True) + v.addWidget(intro) + + 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)) + self._name_edit.textChanged.connect(self._refresh) + grid.addWidget(self._name_edit, 0, 1) + v.addLayout(grid) + + self._already = QLabel("") + self._already.setWordWrap(True) + self._already.setStyleSheet(f"color: {GOOD};") + v.addWidget(self._already) + + set_lbl = QLabel("Set it 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) + + note = QLabel("The secret will be copied to your clipboard when you continue.") + note.setObjectName("muted") + note.setWordWrap(True) + v.addWidget(note) + + btns = QDialogButtonBox( + QDialogButtonBox.StandardButton.Ok | QDialogButtonBox.StandardButton.Cancel + ) + ok = btns.button(QDialogButtonBox.StandardButton.Ok) + ok.setText("Move && copy secret") + ok.setObjectName("primary") + btns.accepted.connect(self.accept) + btns.rejected.connect(self.reject) + v.addWidget(btns) + + self._refresh() + + def var_name(self) -> str: + return core.sanitize_env_var_name(self._name_edit.text()) + + def _refresh(self, *_): + name = self.var_name() + lines = core.shell_export_lines(name, self._secret) + 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']})") + if core.is_env_var_set(name): + self._already.setText(f"{name} already looks set in this environment.") + self._already.show() + else: + self._already.hide() + + class KeyValueTable(QWidget): def __init__(self, key_label="Key", val_label="Value", on_change=None, before_change=None): super().__init__() @@ -292,6 +376,11 @@ class KeyValueTable(QWidget): self.table.setSelectionBehavior(QAbstractItemView.SelectionBehavior.SelectRows) self.table.setMinimumHeight(90) self.table.itemChanged.connect(self._changed) + # Right-click a secret row to move it out to a ${VAR} reference (#83). + # Set by the owner (ServerEditor) so the action can gate on the client. + self.profile_provider = None + self.table.setContextMenuPolicy(Qt.ContextMenuPolicy.CustomContextMenu) + self.table.customContextMenuRequested.connect(self._context_menu) # Secret-looking values (API_KEY, TOKEN, ...) render masked by default. self._mask_delegate = _SecretMaskDelegate(self.table) self.table.setItemDelegateForColumn(1, self._mask_delegate) @@ -315,6 +404,47 @@ class KeyValueTable(QWidget): self.reveal_btn.setText("Hide secrets" if on else "Show secrets") self.table.viewport().update() + def _row_key_value(self, row: int): + key_item = self.table.item(row, 0) + val_item = self.table.item(row, 1) + key = key_item.text().strip() if key_item else "" + # The mask is display-only (a delegate); the model text is the real value. + value = val_item.text() if val_item else "" + return key, value + + def _context_menu(self, pos): + item = self.table.itemAt(pos) + if item is None: + return + row = item.row() + key, value = self._row_key_value(row) + profile = self.profile_provider() if self.profile_provider else None + if not core.can_move_value_to_env_ref(key, value, profile): + return + menu = QMenu(self) + act = QAction("Move to environment variable…", self) + act.triggered.connect(lambda: self._move_row_to_env(row)) + menu.addAction(act) + menu.exec(self.table.viewport().mapToGlobal(pos)) + + def _move_row_to_env(self, row: int): + key, secret = self._row_key_value(row) + if not secret: + return + dlg = MoveToEnvDialog(self.window(), key, secret) + if not dlg.exec(): + return + var_name = dlg.var_name() + # Hand the secret back before it leaves the file: clipboard now holds it, + # and the dialog showed the exact shell line to set it. + QGuiApplication.clipboard().setText(secret) + if self._before_change: + self._before_change() + val_item = self.table.item(row, 1) + if val_item is not None: + # setText fires itemChanged -> _changed -> on_change (dirty + revalidate). + val_item.setText(f"${{{var_name}}}") + def _changed(self, item=None, *_): if item is not None and item.column() == 0: new_key = item.text().strip() @@ -631,6 +761,12 @@ class ServerEditor(QFrame): v.addWidget(self.headers, 1) return w + def set_profile_provider(self, provider): + """Let the env/headers tables gate "move to environment variable" on + which client the loaded profile targets (#83).""" + self.env.profile_provider = provider + self.headers.profile_provider = provider + # --- model <-> form -------------------------------------------------- # def load_entry(self, entry: core.ServerEntry | None): self._loading = True @@ -1649,6 +1785,7 @@ class MainWindow(QMainWindow): split.setHandleWidth(10) split.addWidget(self._build_left()) self.editor = ServerEditor(on_change=self._editor_changed, before_change=self._push_undo) + self.editor.set_profile_provider(lambda: self.current_profile) split.addWidget(self.editor) split.setStretchFactor(0, 3) split.setStretchFactor(1, 4) diff --git a/bcc_core.py b/bcc_core.py index d709b28..8fec31d 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -1837,6 +1837,129 @@ def env_ref_warnings( ] +# --------------------------------------------------------------------------- # +# "Move to environment variable" (issue #83) +# +# Turn a plaintext secret in a config into a ${VAR} reference, so the secret +# stops living in the file. Two things make this more than a string swap: +# * the replaced value is the ONLY copy of the secret the user may have, so +# the caller must hand it back (clipboard + the exact shell line) before it +# leaves the config -- shell_export_lines builds that; +# * only clients that expand ${VAR} (Claude Code, not Desktop) should be +# offered this, or it walks the user straight into a broken config -- that +# gate is can_move_value_to_env_ref, reusing client_expands_env_refs. +# The rewrite itself is pure (data in -> data out) and lives here; the GUI does +# clipboard, the confirm dialog, and the already-set check. +# --------------------------------------------------------------------------- # +@dataclass(frozen=True) +class EnvRefConversion: + """The result of moving one secret out to a ${VAR} reference.""" + + var_name: str # the sanitised shell variable name chosen + reference: str # "${VAR_NAME}" -- what now sits in the config + secret: str # the plaintext value that was removed (hand this back!) + data: dict # a NEW server-definition dict with the value replaced + + +def sanitize_env_var_name(name: str) -> str: + """Coerce an arbitrary key into a legal, conventional shell variable name. + + POSIX names are `[A-Za-z_][A-Za-z0-9_]*`; env vars are conventionally + upper-case. Non-alphanumerics (and any non-ASCII) become `_`, a leading + digit gets an `_` prefix, and an empty/degenerate result falls back to VAR. + 'api-key' -> 'API_KEY'; '2fa' -> '_2FA'; '' -> 'VAR'. + """ + cleaned = "".join( + ch if (ch.isascii() and (ch.isalnum() or ch == "_")) else "_" for ch in (name or "") + ) + if not cleaned.strip("_"): + return "VAR" + if cleaned[0].isdigit(): + cleaned = "_" + cleaned + return cleaned.upper() + + +def _posix_single_quote(s: str) -> str: + """Wrap s in single quotes, safely, for a POSIX shell (handles embedded ').""" + return "'" + s.replace("'", "'\\''") + "'" + + +def shell_export_lines(var_name: str, secret: str) -> dict[str, str]: + """The exact command to set `var_name`=`secret` in the user's shell. + + Returned per-platform so the UI can show the one that fits (or both). This + is what makes moving the secret out safe: the user gets the setter before + the value leaves the file. + """ + return { + "posix": f"export {var_name}={_posix_single_quote(secret)}", + "windows": f'setx {var_name} "{secret}"', + } + + +def can_move_value_to_env_ref(key: str, value, profile: Profile | None = None) -> bool: + """Whether to offer "move to environment variable" for one env/header row. + + True only for a real stored secret (`should_mask_value`) that isn't already + a reference, on a client that expands references. Offering it on Claude + Desktop would produce a config that reaches the server as literal `${VAR}` + text -- the exact failure #76 exists to prevent -- so a non-expanding + profile refuses outright. + """ + if profile is not None and not client_expands_env_refs(profile): + return False + if is_env_ref(value): + return False + return should_mask_value(key, value) + + +def move_value_to_env_ref( + data: dict, *, field: str, key: str | None = None, index: int | None = None, var_name=None +) -> EnvRefConversion | None: + """Replace one secret value in `data` with a `${VAR}` reference. + + `field` is "env" or "headers" (addressed by `key`) or "args" (addressed by + `index`). `var_name` defaults to the row's key (sanitised); args have no + key, so a var name should be supplied there. Returns an EnvRefConversion + carrying a NEW data dict (the input is never mutated) and the removed + secret, or None if the target isn't found or isn't a string to move. + """ + new = copy.deepcopy(data) + + if field in ("env", "headers"): + block = new.get(field) + if not isinstance(block, dict) or key not in block: + return None + current = block[key] + if not isinstance(current, str) or is_env_ref(current): + return None + vn = sanitize_env_var_name(var_name if var_name is not None else key) + block[key] = f"${{{vn}}}" + elif field == "args": + args = new.get("args") + if not isinstance(args, list) or index is None or not (0 <= index < len(args)): + return None + current = args[index] + if not isinstance(current, str) or is_env_ref(current): + return None + vn = sanitize_env_var_name(var_name if var_name is not None else f"ARG_{index}") + args[index] = f"${{{vn}}}" + else: + return None + + return EnvRefConversion(var_name=vn, reference=f"${{{vn}}}", secret=current, data=new) + + +def is_env_var_set(var_name: str, environ: dict | None = None) -> bool: + """Whether `var_name` is already present (non-empty) in the environment. + + Lets the UI skip the copy-the-secret ceremony when the variable is already + set. Best-effort: BCC's environment may differ from the client's. + """ + env = os.environ if environ is None else environ + return bool(env.get(var_name)) + + 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 fd36e80..2f8a7e7 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -3059,3 +3059,92 @@ def test_discover_project_configs_skips_non_object_and_garbage(tmp_path): assert str(array / ".mcp.json") not in paths assert str(garbage / ".mcp.json") not in paths assert str(missing / ".mcp.json") not in paths + + +# --------------------------------------------------------------------------- # +# Move to environment variable (issue #83) +# --------------------------------------------------------------------------- # +@pytest.mark.parametrize( + "raw,expected", + [ + ("API_KEY", "API_KEY"), + ("api-key", "API_KEY"), + ("x.y z", "X_Y_Z"), + ("2fa", "_2FA"), + ("", "VAR"), + ("***", "VAR"), + ("clé", "CL_"), # non-ASCII becomes _ + ], +) +def test_sanitize_env_var_name(raw, expected): + assert c.sanitize_env_var_name(raw) == expected + + +def test_shell_export_lines_quote_safely(): + lines = c.shell_export_lines("TOKEN", "ab'cd") + assert lines["posix"] == "export TOKEN='ab'\\''cd'" + assert lines["windows"] == 'setx TOKEN "ab\'cd"' + + +def test_can_move_gate_requires_secret_and_expanding_client(): + desktop = c.Profile(label="d", path="/x/Claude/claude_desktop_config.json", config_exists=True) + code = c.Profile(label="c", path=Path.home() / ".claude.json", config_exists=True) + # real secret on an expanding client -> offer + assert c.can_move_value_to_env_ref("API_KEY", "ghp_abc", code) is True + assert c.can_move_value_to_env_ref("API_KEY", "ghp_abc", None) is True + # non-secret key -> no + assert c.can_move_value_to_env_ref("REGION", "us-east-1", code) is False + # already a reference -> no + assert c.can_move_value_to_env_ref("API_KEY", "${API_KEY}", code) is False + # non-expanding client (Claude Desktop) -> refuse even a real secret + assert c.can_move_value_to_env_ref("API_KEY", "ghp_abc", desktop) is False + + +def test_move_env_value_replaces_with_reference_and_returns_secret(): + data = {"command": "x", "env": {"API_KEY": "ghp_secret", "REGION": "us"}} + conv = c.move_value_to_env_ref(data, field="env", key="API_KEY") + assert conv is not None + assert conv.var_name == "API_KEY" + assert conv.reference == "${API_KEY}" + assert conv.secret == "ghp_secret" + assert conv.data["env"]["API_KEY"] == "${API_KEY}" + # non-secret row untouched + assert conv.data["env"]["REGION"] == "us" + # input never mutated + assert data["env"]["API_KEY"] == "ghp_secret" + + +def test_move_derives_and_sanitises_var_name_from_key(): + data = {"headers": {"x-api-key": "sekret"}} + conv = c.move_value_to_env_ref(data, field="headers", key="x-api-key") + assert conv.var_name == "X_API_KEY" + assert conv.data["headers"]["x-api-key"] == "${X_API_KEY}" + + +def test_move_honours_explicit_var_name(): + data = {"env": {"tok": "sekret"}} + conv = c.move_value_to_env_ref(data, field="env", key="tok", var_name="GITHUB_TOKEN") + assert conv.reference == "${GITHUB_TOKEN}" + assert conv.data["env"]["tok"] == "${GITHUB_TOKEN}" + + +def test_move_args_by_index(): + data = {"command": "x", "args": ["--token", "ghp_secret"]} + conv = c.move_value_to_env_ref(data, field="args", index=1, var_name="GH_TOKEN") + assert conv.secret == "ghp_secret" + assert conv.data["args"] == ["--token", "${GH_TOKEN}"] + + +def test_move_returns_none_on_missing_or_nonstring_or_already_ref(): + data = {"env": {"API_KEY": "${API_KEY}", "N": 5}} + assert c.move_value_to_env_ref(data, field="env", key="ABSENT") is None + assert c.move_value_to_env_ref(data, field="env", key="N") is None # not a string + assert c.move_value_to_env_ref(data, field="env", key="API_KEY") is None # already a ref + assert c.move_value_to_env_ref({}, field="bogus") is None + assert c.move_value_to_env_ref({"args": ["a"]}, field="args", index=9) is None + + +def test_is_env_var_set(): + assert c.is_env_var_set("FOO", {"FOO": "x"}) is True + assert c.is_env_var_set("FOO", {"FOO": ""}) is False + assert c.is_env_var_set("FOO", {}) is False From 4743c4a99510b0850489b860b98fca88a659ad2d Mon Sep 17 00:00:00 2001 From: the_og Date: Tue, 4 Aug 2026 04:03:47 +0000 Subject: [PATCH 2/3] feat(#83): offer move-to-env on args rows; explain the gate instead of an empty menu MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things surfaced testing the GUI: 1. Args rows showed the secret warning but no move action -- the args editor is a free-text widget, not a table, and was deliberately left out of the first cut. Wired it up: ArgsEdit gains a context menu that offers "Move to environment variable…" on exactly the args that look like a credential. New pure core: secret_arg_indices (which args are secrets, mirroring args_secret_warning per-index) and suggested_env_var_for_arg (default var name from the preceding flag -- `--api-key ` -> API_KEY, else SECRET). The move replaces that one arg line with ${VAR} and copies the secret to the clipboard, same contract as the tables. 2. On a Claude Desktop profile (or any non-expanding client) the menu showed NOTHING, so it read as broken. Now a real stored secret always shows the item -- enabled on a client that expands references, or disabled with the reason ("unavailable for Claude Desktop -- it doesn't expand ${VAR}") so the gate is visible rather than silent. Applies to env, headers and args. Not a change: after converting, env_ref_warnings still notes a variable that isn't set in the environment. That's #82's advisory doing its job -- the user runs the export line the dialog handed them; auto-adding a ':-default' would bake a fallback back into the config and defeat moving the secret out. Tests: +5 core (secret_arg_indices for token/flag-value/embedded-URL/ reference-excluded, suggested_env_var_for_arg with and without a flag). 483 passed, ruff clean. GUI wiring (context menus) remains untestable in CI. Refs #83 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01EKwBecy6N83jnqQmw8ezwE --- PR87-test-checklist.md | 50 ++++++++++++++++++++++++++++++++ bcc.py | 66 ++++++++++++++++++++++++++++++++++++++---- bcc_core.py | 48 ++++++++++++++++++++++++++++++ tests/test_core.py | 32 ++++++++++++++++++++ 4 files changed, 191 insertions(+), 5 deletions(-) create mode 100644 PR87-test-checklist.md diff --git a/PR87-test-checklist.md b/PR87-test-checklist.md new file mode 100644 index 0000000..aa1b231 --- /dev/null +++ b/PR87-test-checklist.md @@ -0,0 +1,50 @@ +# PR #87 — "Move to environment variable" (#83): manual test checklist + +The logic is covered by 15 unit tests in CI; what CI **can't** exercise is the GUI (no PySide6). This checklist is only the parts a human needs to click. Should take ~10 minutes. + +## Setup + +```bash +cd ~/Documents/Claude/Projects/BetterClaudeConfig/better-claude-config +git fetch origin +git checkout feat/83-move-to-env-var +git pull # ensure you're on 8fdbe90 or later +source .venv/bin/activate # or recreate: python3 -m venv .venv && source .venv/bin/activate && pip install -r requirements.txt +python bcc.py +``` + +Pick a **Claude Code** profile (e.g. `~/.claude.json`) that has, or add, a server with an env value that looks like a secret — e.g. `env: { "API_KEY": "ghp_test123" }`. (You can use a throwaway value; nothing is sent anywhere.) + +## The checklist + +### Gating — where the action appears +- [ ] Right-click the **value cell** of a secret env row (`API_KEY`) on a **Claude Code** profile → a **"Move to environment variable…"** item appears. +- [ ] Right-click a **non-secret** row (e.g. `REGION` = `us-east-1`) → the item does **not** appear. +- [ ] Right-click a row whose value is already a reference (`${API_KEY}`) → the item does **not** appear. +- [ ] Switch to a **Claude Desktop** profile (a `claude_desktop_config.json`), right-click the same kind of secret row → the item does **not** appear. (Desktop doesn't expand `${VAR}`, so offering it would break the config — this is the important gate.) + +### The dialog +- [ ] Trigger the action → dialog opens with **Variable** pre-filled from the key, sanitized to a legal shell name (e.g. `api-key` → `API_KEY`). +- [ ] Edit the variable name → the shown **shell line updates live** and matches your platform (`export VAR='…'` on macOS/Linux, `setx VAR "…"` on Windows), with the other platform shown in parentheses. +- [ ] If you type a variable name that **is already set** in your shell environment, the green "already looks set" note appears; if not, it's hidden. +- [ ] **Cancel** → nothing changes (value still the raw secret, no dirty state). + +### The conversion +- [ ] **Move && copy secret** → the cell now shows the reference `${VAR}` (visible, **not** masked to dots), and the window goes dirty (Save enabled). +- [ ] Paste from your clipboard somewhere → it's the **original secret value** (handed back before removal). +- [ ] The reference value is **not** flagged as a secret warning anymore (it's the recommended state). + +### Headers + persistence +- [ ] Repeat on a **remote server's Headers** table (e.g. an `Authorization` header) → same behavior. +- [ ] **Save**, then open the config file on disk in a text editor → the servers block holds `${VAR}`, and the **plaintext secret is gone** from the file. +- [ ] Re-open the profile in BCC → the row still shows `${VAR}` (round-trips). + +### Undo (nice-to-have) +- [ ] After a conversion, **Ctrl+Z / Cmd+Z** restores the previous value. + +## Known scope (not bugs) +- **Args rows** are out of scope for this PR — the core supports them, but the args editor is a free-text widget, so wiring that UI is a deliberate follow-up. Right-clicking args won't offer the action yet. +- The "already set" check reads **BCC's** environment, which may differ from the client's — it's advisory, worded that way. + +## If anything's off +Tell me which checkbox failed and what you saw; I'll fix on the branch and re-push. If everything passes, approve/merge #87 (or tell me to merge it). diff --git a/bcc.py b/bcc.py index fc53f11..57a957c 100644 --- a/bcc.py +++ b/bcc.py @@ -418,12 +418,23 @@ class KeyValueTable(QWidget): return row = item.row() key, value = self._row_key_value(row) - profile = self.profile_provider() if self.profile_provider else None - if not core.can_move_value_to_env_ref(key, value, profile): + # Only a real stored secret is worth moving; nothing to offer otherwise. + if not core.should_mask_value(key, value): return + profile = self.profile_provider() if self.profile_provider else None menu = QMenu(self) act = QAction("Move to environment variable…", self) - act.triggered.connect(lambda: self._move_row_to_env(row)) + 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.setToolTip( + "Claude Desktop doesn't expand ${VAR}, so a reference would reach " + "the server as literal text." + ) menu.addAction(act) menu.exec(self.table.viewport().mapToGlobal(pos)) @@ -762,10 +773,11 @@ class ServerEditor(QFrame): return w def set_profile_provider(self, provider): - """Let the env/headers tables gate "move to environment variable" on - which client the loaded profile targets (#83).""" + """Let the env/headers tables and the args editor gate "move to + environment variable" on which client the loaded profile targets (#83).""" self.env.profile_provider = provider self.headers.profile_provider = provider + self.args.profile_provider = provider # --- model <-> form -------------------------------------------------- # def load_entry(self, entry: core.ServerEntry | None): @@ -1044,6 +1056,50 @@ 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. + self.profile_provider = None + + def contextMenuEvent(self, event): + menu = self.createStandardContextMenu() # keep cut/copy/paste + lines = self.toPlainText().splitlines() + block = self.cursorForPosition(event.pos()).blockNumber() + if 0 <= block < len(lines) and lines[block].strip(): + # This editor is one arg per line; map the clicked block to its + # index among the non-blank args the model actually sees. + cleaned = [ln for ln in lines if ln.strip() != ""] + 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)) + else: + act.setEnabled(False) + act.setText("Move to environment variable — unavailable for Claude Desktop") + 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) + 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()))) diff --git a/bcc_core.py b/bcc_core.py index 8fec31d..ddcf7f0 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -2051,6 +2051,54 @@ def args_secret_warning(data: dict) -> str | None: return None +def secret_arg_indices(args: list) -> list[int]: + """Indices of args that look like a raw credential. + + Same detection as `args_secret_warning`, but per-arg so the "move to + environment variable" action (#83) knows exactly which arg to offer on. + Flags a token-prefixed positional (ghp_..., sk-...), an embedded-credential + URL, or the value following a secret-named flag (`--token abc`). A `${VAR}` + reference is never flagged -- it's the fix, not the problem. + """ + out: list[int] = [] + mask_next = False + for i, a in enumerate(str(x) for x in args): + if is_env_ref(a): + mask_next = False + continue + if mask_next: + mask_next = False + if not a.startswith("-"): + out.append(i) + continue + if a.startswith("-") and "=" in a: + continue + if a.startswith("-") and is_secret_key(a): + mask_next = True + continue + if a.startswith("-"): + continue + if _is_secret_value(a) or _EMBEDDED_CRED_RE.search(a): + out.append(i) + return out + + +def suggested_env_var_for_arg(args: list, index: int) -> str: + """A default variable name for moving `args[index]` to a reference. + + Uses the preceding flag when there is one (`--api-key ` -> + API_KEY), since that names what the value is; otherwise falls back to a + generic SECRET. Always a legal shell name. + """ + if 0 < index <= len(args): + prev = str(args[index - 1]) if index - 1 < len(args) else "" + if prev.startswith("-"): + base = prev.lstrip("-").split("=", 1)[0] + if base: + return sanitize_env_var_name(base) + return "SECRET" + + def split_suspicious_args(args: list[str]) -> tuple[list[str], list[str]]: """ Detect the classic argument-entry mistake: several argv tokens typed on one diff --git a/tests/test_core.py b/tests/test_core.py index 2f8a7e7..16017d4 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -3148,3 +3148,35 @@ def test_is_env_var_set(): assert c.is_env_var_set("FOO", {"FOO": "x"}) is True assert c.is_env_var_set("FOO", {"FOO": ""}) is False assert c.is_env_var_set("FOO", {}) is False + + +# --------------------------------------------------------------------------- # +# Args secret indices + suggested var name (issue #83, args surface) +# --------------------------------------------------------------------------- # +def test_secret_arg_indices_flags_token_and_flag_value(): + args = ["--port", "8080", "ghp_deadbeef", "--token", "sk-abc", "--flag=val"] + idxs = c.secret_arg_indices(args) + assert 2 in idxs # ghp_ token prefix + assert 4 in idxs # value following --token + assert 1 not in idxs # 8080 + assert 5 not in idxs # --flag=val inline pair + + +def test_secret_arg_indices_flags_embedded_url_credentials(): + args = ["postgres://user:pass@host/db"] + assert c.secret_arg_indices(args) == [0] + + +def test_secret_arg_indices_excludes_existing_references(): + args = ["--token", "${GH_TOKEN}"] + assert c.secret_arg_indices(args) == [] + + +def test_suggested_env_var_for_arg_uses_preceding_flag(): + args = ["--api-key", "sk-secret"] + assert c.suggested_env_var_for_arg(args, 1) == "API_KEY" + + +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" From 694439b6f3935c15e05b011dc39bf34c34dc2af9 Mon Sep 17 00:00:00 2001 From: the_og Date: Tue, 4 Aug 2026 04:25:28 +0000 Subject: [PATCH 3/3] 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