From 4743c4a99510b0850489b860b98fca88a659ad2d Mon Sep 17 00:00:00 2001 From: the_og Date: Tue, 4 Aug 2026 04:03:47 +0000 Subject: [PATCH] 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"