Skip to content

Secrets and config hygiene for VLM driver scripts#2

Open
l2ktech wants to merge 1 commit into
mainfrom
opt/vlm-gemini
Open

Secrets and config hygiene for VLM driver scripts#2
l2ktech wants to merge 1 commit into
mainfrom
opt/vlm-gemini

Conversation

@l2ktech

@l2ktech l2ktech commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Summary

This repository is public, yet two of the top-level VLM driver scripts committed a working API key, and a tracked vlm_config.json contained another credential plus a private LAN endpoint. This PR stops leaking credentials and unifies configuration across the four user-authored driver scripts (22_/23_/25_/26_) using the load_config() precedence pattern 26_vlm_mujoco_grasp.py already established. The upstream Headless/ tree is untouched.

Note: the old key still exists in git history. Removing it from the working tree stops further exposure but does not purge history — please rotate the key with your provider.

Changes

Security / config hygiene

  • Removed the hardcoded API_KEY ("cliproxy-ag-...") from 22_vlm_robot_test.py and 23_vlm_full_test.py. All four scripts now resolve config with one precedence: env vars (VLM_API_BASE / VLM_API_KEY / VLM_MODEL) > vlm_config.json > built-in defaults.
  • Stopped tracking vlm_config.json (it held sk-ai-proxy-key and http://192.168.1.102:8000/v1); added vlm_config.example.json with placeholders.
  • Added a .gitignore (the repo had none): vlm_config.json, .env, __pycache__/, *.pyc, .venv/, caches.

Bugs / portability

  • 23_: replaced the hardcoded absolute sys.path (/l2k/home/wzy/...) with a Path(__file__).parent.parent-derived path, matching 25_/26_.
  • 22_: wired the parsed --model arg through to VLMClient.set_model (previously a no-op), fixed the invalid default model name (flash -> reverse-looked-up DEFAULT_MODEL), corrected the stale flash/thinking/vision help text to the real MODELS keys, and removed a duplicated # ==== 配置 ==== header.
  • Narrowed bare except: clauses: 23_ teardown now except Exception (and logs), 25_/26_ parse_command JSON fallback now except (json.JSONDecodeError, ValueError, IndexError) so KeyboardInterrupt/SystemExit are no longer swallowed.
  • 26_ place(): copy the location_joints list before mutating it, so an unknown-location call no longer alters the shared center pose.

Packaging / docs

  • Declared runtime deps in pyproject.toml: [project.dependencies] = openai; optional [project.optional-dependencies].sim = mujoco, numpy.
  • Documented VLM_API_KEY / vlm_config.json setup in README.md and README_en.md (previously only GEMINI_API_KEY was covered).

Verification

  • python -m py_compile and ast.parse of all four scripts: pass.
  • grep confirms the cliproxy-ag- key, the old 192.168.1.102 endpoint, and the /l2k/home/wzy absolute path are gone from tracked .py/.json.
  • vlm_config.json confirmed untracked and gitignored; vlm_config.example.json added.
  • Runtime check of load_config(): env vars override file/defaults; defaults apply when neither env nor file is present (env > file > default confirmed).
  • No project test runner/CI exists, so verification is limited to compile/syntax/grep/runtime-unit checks; no hardware or live API was exercised.

Automated review

An automated reviewer verified the change end-to-end in the worktree on commit ed5ae35 and approved at minor severity. All four scripts pass py_compile; runtime tests confirmed load_config() precedence (env > file > default), the 22_ DEFAULT_MODEL reverse-lookup, set_model alias resolution, the 26_ place() list-copy fix, and the bare-except narrowing at all three sites. Scope was confined to the author's own files (4 VLM scripts, .gitignore, vlm_config.example.json, removed vlm_config.json, pyproject.toml, 2 READMEs) with no upstream/vendored paths touched; the working tree was clean. Secret scans (cliproxy-ag-, 192.168.1.102, /l2k, sk-ai-proxy, generic sk-*/AKIA/ghp_/api_key=) all came back clean, and vlm_config.json is both untracked and gitignored with the example file using a placeholder.

Concerns raised (all minor, none blocking):

  • Key rotation required (out of band): git history still contains the leaked cliproxy-ag-... key and the old sk-ai-proxy-key / 192.168.1.102 endpoint. This diff only removes them from the working tree — the credential must be rotated at the provider. Already documented by the author; not fixable by this PR.
  • Imprecise verification claim: the summary's mention of a Dict import in 23_ is inaccurate — 23_'s load_config() uses a lowercase dict return annotation and never references Dict from typing. Harmless (no NameError, py_compile passes).
  • Benign behavior change in 25_: it previously read config only from env vars + an old ~/.config/vlm/api_key file; the new load_config() also reads vlm_config.json from cwd and the __file__ dir. Consistent with 22_/23_/26_ and harmless since vlm_config.json is gitignored, but it is a behavior change beyond pure secret removal.
  • Broad except in config loading: load_config() in all four scripts catches config-file parse errors with a broad except Exception (not narrowed like the parse_command sites). Minor inconsistency; acceptable for non-critical config loading.

The only material follow-up is the out-of-band key rotation.

🤖 Generated with Claude Code

Summary by Sourcery

Unify and secure configuration for all VLM driver scripts by removing hardcoded secrets, centralizing config resolution, and documenting runtime and packaging requirements.

New Features:

  • Introduce shared configuration loading for all VLM driver scripts with a consistent precedence of environment variables over vlm_config.json over built-in defaults.
  • Add example VLM configuration file and expand README guidance for configuring VLM endpoints and installing required dependencies.

Bug Fixes:

  • Fix model selection in the 22_vlm_robot_test script so the --model argument and default model alias correctly map to the configured VLM models.
  • Replace a hardcoded absolute import path in the 23_vlm_full_test script with a repository-relative path for better portability.
  • Narrow broad exception handlers in VLM command parsing and teardown logic to avoid swallowing interrupts and improve error visibility.
  • Prevent unintended mutation of shared joint configuration lists in the 26_vlm_mujoco_grasp place operation.

Enhancements:

  • Ensure missing API keys fall back to an explicit placeholder value with warnings instead of silently failing in VLM driver scripts.
  • Align configuration handling in the simulation control script with the other VLM drivers while remaining backward compatible with the legacy key file.

Build:

  • Declare OpenAI as a core dependency and add an optional simulation extras group for MuJoCo and NumPy in pyproject.toml.

Documentation:

  • Document VLM driver configuration, environment variables, and example config usage in both English and Chinese READMEs, including guidance on dependency installation and key rotation.

Chores:

  • Add a .gitignore to exclude local configuration, environment files, virtualenvs, caches, and Python bytecode from version control.
  • Stop tracking the concrete vlm_config.json and replace it with a non-sensitive example configuration file.

…ripts

Stop leaking credentials in this public repo and unify configuration across
the four user-authored VLM driver scripts (22_/23_/25_/26_), without touching
the upstream Headless/ tree.

Security / config hygiene:
- Remove the hardcoded API_KEY ("cliproxy-ag-...") from 22_vlm_robot_test.py
  and 23_vlm_full_test.py; all four scripts now resolve config with a single
  precedence: env vars (VLM_API_BASE/VLM_API_KEY/VLM_MODEL) > vlm_config.json
  > defaults (the load_config() pattern 26_ already used).
- Add vlm_config.example.json (placeholders) and stop tracking vlm_config.json
  (git rm --cached); it is now gitignored. NOTE: the old key still exists in
  git history and must be rotated by the user.
- Add a .gitignore (previously none): vlm_config.json, .env, __pycache__/,
  *.pyc, .venv/, caches.

Bugs / portability:
- 23_: replace hardcoded absolute sys.path ('/l2k/home/wzy/...') with a
  Path(__file__).parent.parent-derived path, matching 25_/26_.
- 22_: wire the parsed --model arg through to VLMClient.set_model, fix the
  invalid default model name ("flash" -> DEFAULT_MODEL), correct the stale
  flash/thinking/vision help text to the real MODELS keys, and remove the
  duplicated '# ==== 配置 ====' header.
- Narrow bare `except:` clauses: 23_ teardown -> except Exception (with log);
  25_/26_ parse_command JSON fallback -> except (JSONDecodeError, ValueError,
  IndexError) so KeyboardInterrupt/SystemExit are no longer swallowed.
- 26_ place(): copy the location_joints list before mutating it so an
  unknown-location call no longer alters the shared 'center' pose.

Packaging / docs:
- Declare runtime deps in pyproject.toml ([project.dependencies] = openai;
  optional [project.optional-dependencies].sim = mujoco, numpy).
- Document VLM_API_KEY / vlm_config.json setup in README.md and README_en.md.

Verification: all four scripts compile (py_compile + ast) and import cleanly;
load_config env-override precedence and the valid --model default confirmed;
grep confirms the cliproxy-ag- key, the old LAN IP, and the absolute path are
gone.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sourcery-ai

sourcery-ai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Reviewer's Guide

Unifies configuration and secret handling across the four VLM driver scripts, removes hardcoded credentials and private endpoints, improves robustness/portability in command parsing and teardown, and adds basic packaging metadata and documentation for configuration and dependencies.

File-Level Changes

Change Details Files
Centralized VLM configuration loading with env > config file > defaults precedence and removal of hardcoded API keys.
  • Replaced hardcoded API_BASE/API_KEY/MODEL constants with a load_config() helper that initializes defaults, optionally merges values from vlm_config.json in several common locations, and then overlays environment variables.
  • Introduced CONFIG/_CONFIG globals in each script to derive API_BASE, API_KEY, and model from the unified configuration result.
  • Added warnings and a non-secret test-key placeholder when no API key is configured instead of relying on hardcoded real keys.
22_vlm_robot_test.py
23_vlm_full_test.py
25_vlm_mujoco_control.py
Improved model alias handling and CLI wiring in the 22_ driver.
  • Expanded the MODELS mapping to act as alias->model ID and introduced DEFAULT_MODEL selection that reverse-looks up the alias for the configured model.
  • Changed the --model argument to default to DEFAULT_MODEL and wired it through to test_vlm_api() and interactive_mode() so the selected model actually affects the VLM client.
  • Updated interactive help text to list valid model aliases based on MODELS and removed a duplicated configuration section header.
22_vlm_robot_test.py
Made the 23_ driver path handling and teardown more portable and observable.
  • Replaced a hardcoded absolute sys.path with a PROJECT_ROOT-based relative path constructed from Path(file).parent.parent to locate the upstream src directory.
  • Narrowed a bare except in teardown() to except Exception and added logging of the disconnect error instead of silently swallowing it.
23_vlm_full_test.py
Adjusted 25_/26_ parsing and Mujoco control behavior for safer error handling and state management.
  • Narrowed bare except clauses in parse_command() to catch specific JSON/Value/Index errors while letting KeyboardInterrupt/SystemExit propagate.
  • Changed 26_vlm_mujoco_grasp.place() to copy the selected joint angle list before mutation so unknown locations no longer corrupt the shared location_joints data.
25_vlm_mujoco_control.py
26_vlm_mujoco_grasp.py
Updated packaging, docs, and repo hygiene around configuration and dependencies.
  • Declared openai as a core runtime dependency and added an optional sim extra with mujoco and numpy in pyproject.toml, along with a more descriptive project metadata block.
  • Documented VLM_API_KEY, VLM_API_BASE, VLM_MODEL, and vlm_config.json usage for the four driver scripts in both README.md and README_en.md, including install commands using the project metadata.
  • Added a .gitignore to exclude vlm_config.json, .env, Python cache artifacts, virtualenvs, and cache directories, and replaced the tracked vlm_config.json with a vlm_config.example.json containing placeholders.
pyproject.toml
README.md
README_en.md
.gitignore
vlm_config.example.json
vlm_config.json

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • The load_config() implementation is duplicated across multiple scripts with only small differences; consider extracting it into a shared helper/module so behavior stays consistent and future changes only need to be made in one place.
  • In all four load_config() functions you still use a broad except Exception when reading/parsing the config file, while other sites now narrow their exceptions; tightening this to the specific JSON/IO errors you expect would make debugging real failures easier and keep behavior consistent.
  • For missing API keys you fall back to a "test-key-placeholder" string even in production-like scripts—consider failing fast (e.g., raising or exiting) unless an explicit test/debug flag is set to avoid accidentally running with an invalid credential.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `load_config()` implementation is duplicated across multiple scripts with only small differences; consider extracting it into a shared helper/module so behavior stays consistent and future changes only need to be made in one place.
- In all four `load_config()` functions you still use a broad `except Exception` when reading/parsing the config file, while other sites now narrow their exceptions; tightening this to the specific JSON/IO errors you expect would make debugging real failures easier and keep behavior consistent.
- For missing API keys you fall back to a `"test-key-placeholder"` string even in production-like scripts—consider failing fast (e.g., raising or exiting) unless an explicit test/debug flag is set to avoid accidentally running with an invalid credential.

## Individual Comments

### Comment 1
<location path="25_vlm_mujoco_control.py" line_range="160" />
<code_context>
                     response = response[4:]
             return json.loads(response)
-        except:
+        except (json.JSONDecodeError, ValueError, IndexError):
             return {"command": "chat", "params": {"response": response}}

</code_context>
<issue_to_address>
**suggestion (bug_risk):** The narrowed exception list for JSON parsing might still miss some realistic failure modes

The previous bare `except` was too broad, but this new list is likely too narrow. For instance, if `response` is not a string (e.g., `None` or a dict), `json.loads` will raise `TypeError`, which now escapes and can break the command loop. Consider adding `TypeError` (and possibly `KeyError` if the upstream structure changes) or using a more generic `Exception`, since all parsing failures should trigger the same conservative fallback here.

```suggestion
        except (json.JSONDecodeError, TypeError, ValueError, IndexError, KeyError):
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread 25_vlm_mujoco_control.py
response = response[4:]
return json.loads(response)
except:
except (json.JSONDecodeError, ValueError, IndexError):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): The narrowed exception list for JSON parsing might still miss some realistic failure modes

The previous bare except was too broad, but this new list is likely too narrow. For instance, if response is not a string (e.g., None or a dict), json.loads will raise TypeError, which now escapes and can break the command loop. Consider adding TypeError (and possibly KeyError if the upstream structure changes) or using a more generic Exception, since all parsing failures should trigger the same conservative fallback here.

Suggested change
except (json.JSONDecodeError, ValueError, IndexError):
except (json.JSONDecodeError, TypeError, ValueError, IndexError, KeyError):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants