security: warn on model swap; correct docs and CLI credential guidance#172
Open
mchillakuru wants to merge 1 commit into
Open
security: warn on model swap; correct docs and CLI credential guidance#172mchillakuru wants to merge 1 commit into
mchillakuru wants to merge 1 commit into
Conversation
…dential guidance F16: persist last_model_key in sleep state and warn at cycle start when the backend/model changed since the previous night (skill text may not transfer). F12: correct docs to say replay isolation varies by backend. F08: emit a DeprecationWarning when API keys are passed via train.py CLI args, pointing to env vars / managed identity. Adds tests for the state roundtrip, the warning conditions, and the CLI deprecation warning.
6 tasks
There was a problem hiding this comment.
Pull request overview
This PR introduces small security/accuracy hardening updates in SkillOpt-Sleep and the training CLI: it records the prior cycle’s model identity to warn on backend/model swaps, corrects documentation about replay isolation, and discourages passing API keys via CLI flags by emitting a deprecation warning.
Changes:
- Persist
last_model_keyin sleep state and warn at cycle start if the backend/model changed since the prior run. - Update Sleep README to clarify replay isolation depends on the configured backend.
- Emit a
DeprecationWarningwhen API keys are supplied viascripts/train.pyCLI arguments.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
skillopt_sleep/cycle.py |
Adds model-swap key generation and warning hook, and stores the key at the end of a cycle. |
skillopt_sleep/state.py |
Persists last_model_key in state.json and exposes accessors. |
scripts/train.py |
Warns when API keys are provided via CLI args (deprecation guidance). |
docs/sleep/README.md |
Corrects replay isolation wording to be backend-dependent. |
tests/test_model_change_warning.py |
Adds tests for the model-swap warning and CLI credential warning behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+34
to
+39
| def _make_model_key(cfg: SleepConfig) -> str: | ||
| """Stable string identifying the backend+model combination for this cycle.""" | ||
| return "{}::{}".format( | ||
| cfg.get("backend", "mock"), | ||
| cfg.get("model", ""), | ||
| ) |
Comment on lines
+389
to
+402
| # F08: Warn when API keys are supplied on the CLI — prefer env vars or managed identity. | ||
| for _cli_key in ( | ||
| "azure_api_key", "azure_openai_api_key", | ||
| "optimizer_azure_openai_api_key", "target_azure_openai_api_key", | ||
| "minimax_api_key", | ||
| ): | ||
| if getattr(args, _cli_key, None): | ||
| warnings.warn( | ||
| f"--{_cli_key} is deprecated: provide credentials via the " | ||
| f"AZURE_OPENAI_API_KEY environment variable or set " | ||
| f"--azure_openai_auth_mode=managed_identity instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) |
Comment on lines
+17
to
+21
| def test_last_model_key_roundtrips(tmp_path) -> None: | ||
| state = SleepState.load(str(tmp_path / "state.json")) | ||
| assert state.last_model_key == "" | ||
| state.set_last_model_key("anthropic::claude") | ||
| assert state.last_model_key == "anthropic::claude" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three small, related hardening/accuracy changes:
last_model_keyin sleep state and print an advisory warning at cycle start when the backend/model changed since the previous night (learned skill text is backend-specific and may not transfer). Advisory only — the cycle continues.DeprecationWarningwhen API keys are passed viascripts/train.pyCLI args, pointing to theAZURE_OPENAI_API_KEYenv var or--azure_openai_auth_mode=managed_identity.Files:
skillopt_sleep/cycle.py,skillopt_sleep/state.py,docs/sleep/README.md,scripts/train.pyNotes
Tests
tests/test_model_change_warning.py:last_model_keyround-trips through stateDeprecationWarningContext
One of several small PRs replacing bulk PR #166, per reviewer feedback to split and scope. Full suite green except 6 pre-existing Windows-only path-separator failures that also fail on
main.