dsh's ctx.shell is the platform executor: on win32 the bash stack is disabled
and ctx.shell is a PowerShell executor (powershell.exe 5.1 / pwsh 7), on POSIX
it is bash. The plugin emitted POSIX quotes unconditionally, so every tool
command failed on Windows: 'python' 'args' parses as a string-array expression
and errors with 'missing call operator' (bare quoted words are not a command
in PowerShell).
- q(): platform-aware quoting — PowerShell single-quote escaping (doubled
quote for an embedded quote) on win32, POSIX (close/reopen) elsewhere.
- quoteArgv(): prepend the `&` call operator on win32 so the quoted argv runs
as a command; CRLF/lone CR folded to one space, C0 stripped.
- canary/audits: host-shell aware (discover PowerShell 5.1/pwsh 7 like
dsh-pwsh-local, or bash); injection + control-char audits run against the
real host shell.
Verified: canary 40/40 (platform-aware quoting), injection 7/7 + control-char
7/7 under real Windows PowerShell, real dsh 0.1.1-rc.2 skillopt_status runs.
POSIX (bash) path unchanged — existing quoting suite still passes.
Co-authored-by: WODE25500 <WODE25500@users.noreply.github.com>
* feat(claude_code_exec): add claude code optimizer backend with SDK trace support
Register claude_code_exec as a full optimizer/target backend (issue #233).
--backend claude_code_exec now defaults both roles to claude_code_exec so
reflection sees the agent's complete session, and the SDK message stream is
parsed into structured trace steps persisted as claude_trace_steps.txt and
injected into the analyst prompt.
- model/claude_code_backend.py (new): chat_optimizer/chat_optimizer_messages on
run_claude_code_chat, reasoning_effort threaded through, retry loop that
surfaces non-JSON structured replies as RuntimeError, token tracking.
- model/codex_harness.py: parse/format/persist claude trace steps (text,
tool_call, tool_result; drops init/thinking_tokens; 200-char tool_result cap;
total truncation) + effort override on run_claude_code_chat.
- trainer.py/reflect.py: inject Claude Trace Steps gated behind
REFLACT_CLAUDE_TRACE_TO_OPTIMIZER, set by the trainer only for claude_code_exec
targets with model.claude_trace_to_optimizer (mirrors codex gate; default true).
- config.py/default.yaml/docs: model.claude_trace_to_optimizer key + flatten
mapping + config.md rows.
- backend_config.py + model/__init__.py: register backend, route chat dispatch,
token summary, reasoning effort, deployments.
- scripts/train.py, eval_only.py: symmetric default + accurate comments.
- tests: tests/test_claude_code_backend.py (10 tests: parsing, dispatch, effort,
retry, trainer/reflect gating); test_role_backend_resolution.py updated to the
symmetric default.
Verified: 58 unit tests pass; integration smoke on searchqa improved best-on-val
0.7500 -> 0.9375 with 80 claude_trace_steps.txt written; all output files valid
UTF-8 (no GBK mojibake).
* fix(claude_code_exec): address #233 review feedback
Cosmetic only. The block comment and its two argv entries in the task-replay
path were left at the first call site's indentation level (12 spaces) instead
of the 16 the surrounding list uses. Python does not care inside a list
literal -- behavior and tests are unchanged -- but it reads as a mistake.
Two backends were emitting parameters that their target does not accept, so
both silently did nothing (or aborted) instead of what the code intended.
Copilot (skillopt_sleep/backend.py)
-----------------------------------
`--allowed-tools` is not a GitHub Copilot CLI option; the CLI exits with
`error: unknown option '--allowed-tools'` before doing any work, so both
Copilot call paths were dead. Two independent axes were also conflated:
* `--allow-all-tools` waives the interactive approval prompt and the CLI's
own help calls it "required for non-interactive mode" -- removing it breaks
headless runs, so it is not the flag to scope on.
* `--available-tools` is the visibility axis: "Only these tools will be
available to the model". That is where scoping belongs.
Keep the former, scope with the latter. The selector is also case-sensitive:
verified against Copilot CLI 1.0.80 that `--available-tools=bash` lets the
tool run while `--available-tools=Bash` blocks it, so the default is
lowercase `bash` (override: COPILOT_AVAILABLE_TOOLS).
MiniMax (skillopt/model/minimax_backend.py)
-------------------------------------------
`chat_template_kwargs.enable_thinking` is a Qwen/HuggingFace-serving
convention that appears nowhere in MiniMax's OpenAI-compatible reference, so
the endpoint ignored it and thinking stayed at the server default regardless
of the configured flag. Send the documented top-level field instead:
`{"thinking": {"type": "adaptive" | "disabled"}}`.
M2.x accepts `{"type": "disabled"}` but keeps thinking on anyway, so it is
sent `adaptive` rather than a value that misrepresents what the model does.
Unknown deployments default to `adaptive`, matching the documented API
default of thinking-on-when-omitted.
Tests assert the exact wire payload / argv, and each new test fails when its
fix is reverted.
Expose MiniMax-M3 as the default minimax_chat deployment and keep
MiniMax-M2.7 selectable. Resolve thinking per model: MiniMax-M2.7
requires always-on thinking, while MiniMax-M3 supports adaptive or
disabled thinking controlled by the existing flag.
Co-authored-by: octo-patch <266937838+octo-patch@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace --allow-all-tools with --allowed-tools scoped via the COPILOT_ALLOWED_TOOLS env var (default: Bash) in both CopilotCliBackend call paths. Reduces the tool surface granted to the model during sleep-cycle mining. Adds tests that capture the constructed argv and assert the default and env-override scope.
Co-authored-by: Murali Chillakuru <murali.chillakuru@microsoft.com>
* fix(sleep): honor val_fraction and test_fraction in the nightly cycle
config.py documents val_fraction and test_fraction and assign_splits()
implements both, but the nightly path only ever forwarded the legacy
holdout_fraction alias: mine() could not carry the new knobs, so
test_fraction was dead config -- no untouched test split could exist and
no held-out test score was ever recorded.
- mine() now mirrors assign_splits(): val_fraction/test_fraction are the
real controls, holdout_fraction stays a legacy alias with unchanged
override semantics for existing callers.
- run_sleep_cycle() resolves the alias (documented value-based precedence,
since the merged config has no key provenance) and passes both fractions
through; both are now recorded in the evidence config row.
- Nights that produce test-split tasks score the night's FINAL documents
on the untouched test split (same replay_batch + aggregate_scores path
the experiment harness uses) and write a write-only
test/held_out_score row to evidence.jsonl. The gate never reads it.
- Defaults are bit-for-bit unchanged: test_fraction=0.0 yields the legacy
two-way split, no test tasks, no extra calls.
Tests: tests/test_split_wiring.py pins the wiring end to end (mine
forwarding, alias precedence, evidence row present/absent).
* fix(sleep): close PR235 review blockers on split hygiene (#235)
- Track config key provenance; preserve explicit val_fraction=0.0
- Make val/train top-ups hash-stable; drop order-dependent test carve
- Block recall of archived val/test tasks and tonight held-out ids
- Document split knobs; extend test_split_wiring regression suite
* test(sleep): 2x3 hardening for split/recall hygiene (#235)
* Add DeepSeek Harness (dsh) integration
New plugins/dsh/ integration wrapping the shared skillopt_sleep engine
for DeepSeek Harness: a Cordis plugin registering 7 native skillopt_*
tools (status/dry-run/run/adopt/harvest/schedule/unschedule), a bundled
SKILL.md, a bundle patch layer (cordis.patch.yml), and a bootstrap
script. Register the plugin in the plugins/README.md integration table.
* Fix dsh integration per review: safe argv, operator-only auto-adopt, parity tests, English skill
Addresses all review points from the SkillOpt maintainer.
Blocker 1 — shell injection / broken documented example:
- Replace buildCommand() (string join, no quoting) with buildArgv() returning
an argv array; execute() quotes every element with the POSIX-safe '\'' spelling
before shell.resolve(). Model/config-controlled values (project, model,
preferences, source) cannot break out of their argument — verified with a
real-bash injection audit (7 payloads). The documented preferences example
now round-trips as one argument.
- Resolve scripts/sleep.py via an absolute path from the plugin dir so it works
regardless of the dsh cwd.
Blocker 2 — auto-adopt no longer model-callable:
- autoAdopt was a model-facing tool parameter forwarding --auto-adopt. Moved to
operator-only config (default false); the tool parameter is removed. The
canary asserts a model-supplied autoAdopt is ignored.
Should fix — plugin registry parity test:
- Register dsh SKILL.md in tests/test_plugin_sync.py PLUGIN_SKILL_MDS. The
parity tests now cover dsh (backends, schedule/unschedule, memory
consolidation). 13/13 pass.
Minor — English-first skill doc:
- SKILL.md rewritten in English; Chinese README stays as README.zh.md.
Runtime correctness (from the first review round):
- execute() goes through shell.resolve() so workdir/output-cap/sandbox defaults apply.
- Consumes rc.8 CollectedOutput { text, truncated, spillPath }; distinguishes
timeout (exit=timeout) from abort (exit=signal).
- package.json includes cordis.patch.yml in files and declares schemastery.
- scripts/sleep.py mirrors the official runner (repo-root resolution, Python >=
3.10 selection, CLI/installed-package fallback).
- New scripts/canary.mjs: pack + load + invoke checks.
Tested locally: canary 21 checks, real-bash quoting 10 checks, real-DSH (rc.6)
13 checks, repo parity 13/13 — no regressions, nothing touches the shared engine.
* Add LICENSE, portable test scripts; align README.zh.md and pack files with the established plugin pattern
* Security: strip control chars in argv quoting (defense in depth)
Model-controlled values containing \\r, \\r\\n or other control characters
would split a single-quoted word into multiple argv words (broken command,
not RCE — quotes never execute), and corrupt the engine's arg parsing. Strip
C0 control characters to a space so every value arrives as exactly one
argument. Verified: new audit-control-chars.mjs covers \\n, \\r, \\r\\n, tab,
NUL, backtick, quotes — all neutralized (single arg, no file, no execution).
* Fix dsh install command in READMEs: dsh is a global CLI, not a pnpm dependency
The previous form 'pnpm dsh web --patch ...' made pnpm try to fetch a
nonexistent @deepseek-ai/dsh-type-meta package and fail with 404. dsh is
installed as a global CLI; the correct overlay invocation is
'dsh web --patch ./plugins/dsh/cordis.patch.yml' (verified with --dump-config).
* Security: enforce per-tool parameter whitelist (block undeclared arg injection)
dsh's parameter schema accepts undeclared properties by default (no
additionalProperties:false), and buildArgv() forwarded both model-supplied
values and operator config defaults for every known key to the engine. A
model (or prompt-injected transcript) could therefore pass backend/model/
json/editBudget/etc. to tools that do not declare them — including
skillopt_adopt, the live-change boundary.
- buildArgv() now takes an explicit per-tool llowed key set; keys outside
it are neither read from args nor filled from config defaults.
- Each tool's build() passes exactly the keys it declares (whitelist).
- canary.mjs: new 7b step asserts adopt drops undeclared backend/model/
maxTasks/json while keeping declared project; step 4 now drives the
nonzero-exit path via preferences (a declared run parameter).
- audit-*.mjs: BASH_PATH env override for non-Windows portability.
* Security: value-domain guard for path params; unschedule --all is operator-only
The engine re-interpolates model-supplied values into its OWN shell command
strings: scheduler.py splices --project into a crontab line and a Windows
run.cmd executed by schtasks (no escaping), and write_tasks_file() turns an
arbitrary --output into abspath+makedirs+overwrite. argv-level quoting in the
plugin protects the dsh bash -c boundary but cannot protect those secondarysplices. A model-controlled project containing shell metacharacters (quote,
ampersand, semicolon, pipe, dollar, backtick, angle brackets, braces, glob,
control chars) would break out and execute as a separate command under thescheduler shell; an absolute or traversal output would overwrite an arbitrary
file.
- assertSafePath(): rejects shell metacharacters in project and output values.
- assertSafeOutput(): refuses absolute paths and .. traversal for --output.
- execute() runs both guards before buildArgv, so a bad value never reaches
the engine; the rejection is returned to the model as tool output.
- skillopt_unschedule: removed model-callable --all; now operator-only via
config.unscheduleAll (same pattern as autoAdopt).
- canary.mjs: new 7c step asserts injected project / absolute / traversal
output are rejected and legit paths pass (32 checks total).
* Security: clock range guard for schedule; pin dependency versions
- schedule hour/minute were spliced by the engine into a crontab line and a
schtasks start time without validation; out-of-range values (99, -1) would
create broken scheduled entries. execute() now enforces hour in [0,23] and
minute in [0,59] before building argv.
- package.json: replace bare '*' dependency ranges with known-good pinned
versions (@deepseek-ai/schemastery ^3.18.1, cordis ^4.0.1, dsh-tools
^0.1.0-rc.8) so installs are reproducible and not silently broken by a
future upstream release.
- canary.mjs: new 7d step asserts hour=99 / minute=-1 are rejected and legit
clock values pass (35 checks total).
* Align with DSH ecosystem plugin conventions; document both patch-invocation forms
- package.json: add peerDependenciesMeta marking @deepseek-ai/cordis and
@deepseek-ai/dsh-tools optional, matching the official ecosystem practice
(dsh-office-tools et al. declare host-provided peers optional). Without it a
plain 'npm install dsh-skillopt' would hard-fail when the host DSH version
differs from the pinned peer range, instead of warning.
- README.md / plugins/README.md: document BOTH overlay forms - 'pnpm dsh web
--patch' for a DeepSeek Harness source checkout (the official dev workflow)
and 'dsh web --patch' for a globally installed dsh.
* Docs: fix parameter name in SKILL.md (maxTasks, not max_tasks)
The skill's parameter table listed max_tasks (snake_case) but the tools declare
maxTasks (camelCase); a model following the skill doc would send max_tasks and be
rejected by dsh's parameter validation (undeclared property).
* Canary: actually pack + extract and load the packed bundle (review requirement)
The review asked for a clean-package canary that 'loads the packed bundle'.
The previous canary verified the pack file list via --dry-run but then imported
the plugin from the source tree. It now runs 'npm pack --json', extracts the
tarball, and loads src/index.js FROM THE EXTRACTED package/ artifact for every
step (register, status, error paths, quoting, whitelist, value guard, clock),
so the artifact under test is exactly what the 'files' list ships. Tarball and
scratch dir are removed on exit.
* Docs: complete README config keys table (all schema keys, corrected module default)
The config keys table now lists every Config schema key (added engineScript,
scope, autoAdopt, unscheduleAll, timeoutMs) and no longer claims module defaults
to 'skillopt_sleep' (the default path is the scripts/sleep.py bootstrap; module
is an explicit override).
---------
Co-authored-by: WODE25500 <WODE25500@users.noreply.github.com>
_safe_live_path proves a target is absolute, traversal-free and *.md, but
accepts any such path on the machine; containment was never checked at adopt
time. A tampered manifest live_skill_path with self-consistent pins therefore
redirected the write onto an arbitrary existing-directory target.
Record the resolved skills roots in the manifest at stage time and re-check
each live target against them in adopt_skills, after the existing
realpath(live) == live identity check so a symlinked ancestor cannot fake
containment. Manifests without recorded roots fail closed.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* feat(sleep): adopt reviewed skill subsets safely
* fix(sleep): wire cycle staging and adopt-time review checks
Address PR 212 review: run_sleep_cycle stages resolved SkillProposals,
status/adopt list and select a subset, uniqueness is rechecked at adopt,
and a failed adopted_skills.json write rolls live files back.
Refs microsoft/SkillOpt#212
* test(sleep): mega-cover PR 212 review paths
Adversarial CLI, adopt-time, cycle-staging, and auto-adopt cases for
Yifan's five review items. Also tidy isort on the files this slice
touches.
Refs microsoft/SkillOpt#120
* fix(sleep): pin staged skill hashes and confine adopt targets
Harden PR 212 adopt: sha256 pin each staged skill, revalidate the
whole manifest before any live write, refuse symlink/missing-parent
targets, skip notes on the cycle report, and reject empty --skill.
Refs microsoft/SkillOpt#212
* fix(sleep): harden multi-skill fan-out adoption end to end
---------
Co-authored-by: Yif-Yang <yif_yang@qq.com>
CopilotCliBackend._parse_jsonl_response assumed the data field of an
assistant.message event was an object, so a truthy non-dict value raised
AttributeError from the field access. That escaped the per-line try, which
only wraps json.loads, and killed the parse of the entire stream.
Port the isinstance guard already used by parse_copilot_jsonl in
skillopt/model/copilot_backend.py, which was hardened in 5497a31 but did not
reach this vendored copy. The wider except clause is kept, since json.loads
raises RecursionError rather than JSONDecodeError on deeply nested payloads.
Fixes the pre-existing failure in
tests/test_sleep_engine.py::TestCopilotBackend::test_parse_jsonl_ignores_excessively_nested_json
Co-authored-by: Pravit Ampapathini <pravit.amp@gmail.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
chat_template_kwargs is a vLLM/SGLang extension. OpenAI, Azure, and strict
OpenAI-compatible gateways reject the unknown body field with HTTP 400, and
non-Qwen vLLM models served with it can emit <think> output with no <answer>
tag (acc=0.000). c31c50b fixed that by only emitting the field when thinking
was enabled, which closed#28 but left no supported way to send an explicit
enable_thinking: false -- the request in #90/#109.
The protocol has three states, so make the setting three-state:
server_default (default) -> omit chat_template_kwargs
enabled -> send enable_thinking: true
disabled -> send enable_thinking: false
server_default keeps every existing deployment on exactly the bytes it sends
today, so #28 stays fixed, while disabled gives #90 the explicit false it asks
for. The legacy enable_thinking boolean keeps its historical wire meaning
(true -> send true, false -> omit), so no config changes behavior; setting
both keys to conflicting values raises rather than silently picking a winner.
Unknown tokens raise too -- a typo must not silently flip a reproducibility
control.
Because server_default delegates a result-affecting choice to the server's
chat template, the backend warns once per role when it is used, and the
resolved per-role mode is recorded in the run's config.json under
resolved_qwen_thinking_modes.
Also settles the docs contradiction between "local vLLM endpoint" and
"OpenAI-compatible": qwen_chat speaks the OpenAI protocol and reaches both
self-hosted servers and hosted gateways, which is exactly why the wire policy
cannot be inferred and must be explicit.
Closes#90
Two defects that shipped with the merged PRs:
harvest_opencode: the APPDATA branch added by #230 was unreachable.
`LOCALAPPDATA or APPDATA` resolves to the former in virtually every
Windows session, so a database that really lives under Roaming was never
found. Probe the candidate roots in order and pick the one that holds
opencode.db, falling back to the Local root for messaging when no
database exists yet; a relative OPENCODE_DB resolves below the same
selected root.
minimax_backend: configure_minimax_chat applied the region default
unconditionally and wrote it into MINIMAX_BASE_URL, so a proxy or
private gateway configured through the environment was silently
discarded as soon as model.minimax_region was set — trainer and
eval_only pass `cfg.get('minimax_base_url') or None`, i.e. None for the
common case. Track whether the base URL was chosen explicitly and only
fill in the region default when it was not, matching what the docs
already state.
Co-authored-by: Yif-Yang <Yif-Yang@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The minimax_chat backend hardcoded a single global OpenAI-compatible base
URL, so there was no supported way to target the China-region service.
Add a region-to-base-URL table with global_en and cn_zh entries, select the
region from MINIMAX_REGION or the new model.minimax_region setting, and keep
an explicitly configured base URL as the override. Document both regional
base URLs and cover the resolution order with tests.
Co-authored-by: octo-patch <266937838+octo-patch@users.noreply.github.com>
This addresses PR feedback to preserve the fallback resilience contract for LLM optimizer calls while making it observable via warnings.warn, and adds tests for backend failure and malformed outputs.
The warning was a DeprecationWarning, which no normal CLI user would see:
skillopt-train is a console script for scripts.train:main, so the warning is
raised from an imported module rather than from __main__, and Python's default
filters end in ignore::DeprecationWarning. FutureWarning has no such filter.
The CLI flag was also the only path checked, and it is the least dangerous one.
A retired key left in a config file was dropped in silence, since flatten_config
no longer maps it and the trainer no longer reads it, and --cfg-options had the
same hole. All three now warn and name the one that supplied it, for structured
and legacy flat configs alike. An override is reported once rather than twice,
because load_config merges --cfg-options into the config before this check runs.
The check therefore moves below _load: it needs the merged config to see a key
that arrived from a file.
The option was flattened, exposed as --max_analyst_rounds and printed in
the trainer's config banner, but nothing ever read it: the analyst call
count follows from the rollout results, gradient.minibatch_size and
gradient.failure_only. Dropping it also keeps the config.json written
for each run honest about what the run actually used.
The CLI flag is still parsed so existing launch scripts do not fail on
an unrecognised argument, and now warns. It is skipped when CLI
arguments are mapped into the config: an argument with no structured
path would otherwise be filed under env, and env keys are passed
through to the trainer.
WODE25500