Secrets and config hygiene for VLM driver scripts#2
Conversation
…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>
Reviewer's GuideUnifies 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
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
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 broadexcept Exceptionwhen 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| response = response[4:] | ||
| return json.loads(response) | ||
| except: | ||
| except (json.JSONDecodeError, ValueError, IndexError): |
There was a problem hiding this comment.
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.
| except (json.JSONDecodeError, ValueError, IndexError): | |
| except (json.JSONDecodeError, TypeError, ValueError, IndexError, KeyError): |
Summary
This repository is public, yet two of the top-level VLM driver scripts committed a working API key, and a tracked
vlm_config.jsoncontained 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 theload_config()precedence pattern26_vlm_mujoco_grasp.pyalready established. The upstreamHeadless/tree is untouched.Changes
Security / config hygiene
API_KEY("cliproxy-ag-...") from22_vlm_robot_test.pyand23_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.vlm_config.json(it heldsk-ai-proxy-keyandhttp://192.168.1.102:8000/v1); addedvlm_config.example.jsonwith placeholders..gitignore(the repo had none):vlm_config.json,.env,__pycache__/,*.pyc,.venv/, caches.Bugs / portability
23_: replaced the hardcoded absolutesys.path(/l2k/home/wzy/...) with aPath(__file__).parent.parent-derived path, matching25_/26_.22_: wired the parsed--modelarg through toVLMClient.set_model(previously a no-op), fixed the invalid default model name (flash-> reverse-looked-upDEFAULT_MODEL), corrected the staleflash/thinking/visionhelp text to the realMODELSkeys, and removed a duplicated# ==== 配置 ====header.except:clauses:23_teardown nowexcept Exception(and logs),25_/26_parse_commandJSON fallback nowexcept (json.JSONDecodeError, ValueError, IndexError)soKeyboardInterrupt/SystemExitare no longer swallowed.26_place(): copy thelocation_jointslist before mutating it, so an unknown-location call no longer alters the sharedcenterpose.Packaging / docs
pyproject.toml:[project.dependencies] = openai; optional[project.optional-dependencies].sim = mujoco, numpy.VLM_API_KEY/vlm_config.jsonsetup inREADME.mdandREADME_en.md(previously onlyGEMINI_API_KEYwas covered).Verification
python -m py_compileandast.parseof all four scripts: pass.grepconfirms thecliproxy-ag-key, the old192.168.1.102endpoint, and the/l2k/home/wzyabsolute path are gone from tracked.py/.json.vlm_config.jsonconfirmed untracked and gitignored;vlm_config.example.jsonadded.load_config(): env vars override file/defaults; defaults apply when neither env nor file is present (env > file > default confirmed).Automated review
An automated reviewer verified the change end-to-end in the worktree on commit
ed5ae35and approved at minor severity. All four scripts passpy_compile; runtime tests confirmedload_config()precedence (env > file > default), the22_DEFAULT_MODELreverse-lookup,set_modelalias resolution, the26_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, removedvlm_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, genericsk-*/AKIA/ghp_/api_key=) all came back clean, andvlm_config.jsonis both untracked and gitignored with the example file using a placeholder.Concerns raised (all minor, none blocking):
cliproxy-ag-...key and the oldsk-ai-proxy-key/192.168.1.102endpoint. 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.Dictimport in23_is inaccurate —23_'sload_config()uses a lowercasedictreturn annotation and never referencesDictfrom typing. Harmless (noNameError,py_compilepasses).25_: it previously read config only from env vars + an old~/.config/vlm/api_keyfile; the newload_config()also readsvlm_config.jsonfrom cwd and the__file__dir. Consistent with22_/23_/26_and harmless sincevlm_config.jsonis gitignored, but it is a behavior change beyond pure secret removal.load_config()in all four scripts catches config-file parse errors with a broadexcept Exception(not narrowed like theparse_commandsites). 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:
Bug Fixes:
Enhancements:
Build:
Documentation:
Chores: