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).

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
+        # Only a real stored secret is worth moving; nothing to offer otherwise.
-        if not core.can_move_value_to_env_ref(key, value, profile):
+        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
+        """Let the env/headers tables and the args editor gate "move to
-        which client the loaded profile targets (#83)."""
+        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())))

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 <secret>` ->
+    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

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"