feat(skill-optimizer): SkillOpt-flavored offline training loop#64
Merged
Conversation
Adds a new skill, slash command, and Node trainer that treats accumulated learn-rule rows as training trajectories and runs a budget-capped offline optimization over an existing SKILL.md. Mirrors Microsoft SkillOpt's ReflACT pipeline adapted to pro-workflow's SQLite plane: rollout pulls recent learnings, reflect calls an optimizer LLM for bounded add/delete/replace patches, aggregate vote-merges across minibatches, clip enforces an LR budget, apply patches deterministically, evaluator LLM scores the candidate against a frozen held-out validation set built from the same trajectories, and the gate accepts only when delta >= accept threshold. Rejected candidates feed a buffer that biases the next reflect step; epoch boundaries trigger a slow-update consolidation pass that must itself pass the gate. Kill switch at ~/.pro-workflow/STOP halts between steps. Provider dispatch supports anthropic / openai / openrouter / fireworks, matching the existing llm-council pattern. Schema: 5 new tables (optimization_runs, optimization_candidates, optimization_patches, optimization_validation, optimization_rejections). CLI: scripts/optimize-skill.js with --json mode. Skill: skills/skill-optimizer/SKILL.md. Command: /skill-optimize. Pure layer (apply / aggregate / clip / hash) covered by 9 node:test cases. LLM-driven layer (reflect / validate / slow / trainer) wires the dispatch but is tested live against a real provider key on invocation. Version bump 3.3.0 -> 3.4.0 for the new top-level skill + command + schema surface.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR introduces a complete offline skill optimizer that generates, aggregates, clips, applies, and evaluates LLM-proposed patches against SKILL.md; it persists runs, candidates, patches, validation items, and rejections in SQLite, enforces LR/USD budgets and a kill-switch, and provides a CLI, tests, and documentation. ChangesOffline Skill Optimizer System
Sequence Diagram(s)sequenceDiagram
participant User
participant CLIScript
participant Trainer
participant Store
participant Reflect
participant Provider
participant Validate
User->>CLIScript: /skill-optimize skill-slug --epochs 3 ...
CLIScript->>CLIScript: parse flags, resolve SKILL.md
CLIScript->>Trainer: train(skillSlug, skillPath, config)
Trainer->>Store: createRun, collectTrajectories
loop each epoch
Trainer->>Trainer: shuffle minibatches
loop each minibatch
Trainer->>Reflect: reflect(skill, trajectories, rejections)
Reflect->>Provider: callLLM(system, user)
Provider-->>Reflect: { text, tokens, cost }
Reflect-->>Trainer: { patches, reasoning, costUsd }
Trainer->>Trainer: aggregatePatches, clipByLR
Trainer->>Trainer: applyPatches → candidate
Trainer->>Validate: validateSkill(candidate, items)
Validate->>Provider: callLLM(scoring prompt)
Provider-->>Validate: outcomes JSON
Validate-->>Trainer: { score, passed, costUsd }
alt score improves
Trainer->>Store: saveCandidate, recordPatches
Trainer->>Trainer: update bestSkill, bestScore
else score worsens
Trainer->>Store: rejectPatches, saveRejection
end
end
alt slow-update interval
Trainer->>Provider: runSlowUpdate(bestSkill, diffs)
Provider-->>Trainer: { skill, changed, cost }
Trainer->>Validate: validateSkill(slowSkill)
alt improves
Trainer->>Trainer: accept slow-updated skill
end
end
end
Trainer->>Trainer: writeSkillFile + hash stamp
Trainer->>Store: endRun
Trainer-->>CLIScript: TrainerOutcome
CLIScript-->>User: JSON or summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (4)
src/optimizer/reflect.ts (1)
99-101: ⚡ Quick winDuplicated fence-strip + safe-JSON-parse logic across modules.
The
^```(?:json)?/ trailing-fence strip plus try/catchJSON.parsepattern is repeated here, invalidate.ts(Line 55) andslow.ts(Line 60). Extracting a small sharedstripFencesAndParsehelper would keep parsing behavior consistent if the model output format changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/optimizer/reflect.ts` around lines 99 - 101, Duplicate fence-stripping and JSON-parse logic exists in stripFences (function name) and similar code in validate.ts and slow.ts; extract a single helper (e.g., stripFencesAndParse) that 1) strips leading ``` or ```json and trailing ``` using the existing regex, 2) safely parses JSON in a try/catch and returns either the parsed object or a controlled error/undefined, and 3) replace the inline logic in stripFences, validate.ts, and slow.ts to call this new helper so all modules share identical parsing behavior.skills/skill-optimizer/SKILL.md (1)
24-24: 💤 Low valueAdd a language to the fenced code block (MD040).
markdownlint flags this fence; use
```textto satisfy the linter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/skill-optimizer/SKILL.md` at line 24, The fenced code block in SKILL.md is missing a language specifier which triggers markdownlint MD040; update the triple-backtick fence on the block at the shown location to include a language (for example use ```text) so the fence becomes a labeled code block and satisfies the linter.commands/skill-optimize.md (1)
11-11: 💤 Low valueAdd a language to fenced code blocks (MD040).
markdownlint flags these fences as missing a language. Use
```text(or```bash) to satisfy the linter.Also applies to: 31-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@commands/skill-optimize.md` at line 11, The markdown file contains fenced code blocks that lack a specified language (MD040); update each triple-backtick fence in commands/skill-optimize.md to include an explicit language token (e.g., change ``` to ```text or ```bash) for the blocks referenced (the fences around the snippets at the reported locations), ensuring all code fences across the file include a language to satisfy markdownlint.scripts/optimize-skill.js (1)
38-39: 💤 Low valueNumeric flags aren't validated; bad input becomes
NaN.
parseInt/parseFloaton a missing or non-numeric value yieldsNaN, which silently propagates intoconfig(e.g.epochs,budgetUsd) and produces confusing downstream behavior rather than a clear error. A small guard would fail fast.♻️ Optional guard
- if (intKeys.has(key)) out[key] = parseInt(v, 10); - else if (floatKeys.has(key)) out[key] = parseFloat(v); - else out[key] = v; + if (intKeys.has(key)) { + const n = parseInt(v, 10); + if (Number.isNaN(n)) { process.stderr.write(`Invalid integer for --${a.slice(2)}: ${v}\n`); process.exit(1); } + out[key] = n; + } else if (floatKeys.has(key)) { + const n = parseFloat(v); + if (Number.isNaN(n)) { process.stderr.write(`Invalid number for --${a.slice(2)}: ${v}\n`); process.exit(1); } + out[key] = n; + } else out[key] = v;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/optimize-skill.js` around lines 38 - 39, The current branch that sets numeric config values (using intKeys, floatKeys, out, key, v) can insert NaN when v is missing or non-numeric; add a guard before parsing: if v is null/empty or Number(v) is NaN or not finite, throw or surface a clear error indicating the invalid flag and key (e.g., `Invalid numeric value for ${key}: ${v}`), otherwise parse (parseInt for intKeys, parseFloat for floatKeys) and assign to out[key]; apply the same validation to both intKeys and floatKeys branches so invalid inputs fail fast instead of producing NaN in config.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@commands/skill-optimize.md`:
- Line 34: The example CLI call shows --evaluator-model gpt-4o-mini but omits
--evaluator-provider, which causes the default Anthropic provider to be used;
either update the example to include --evaluator-provider openai (so the example
becomes explicit) or change the CLI logic in scripts/optimize-skill.js that
handles the evaluator-model/evaluator-provider flags to infer provider from the
model name (e.g., map gpt-* to openai) and set evaluator-provider when missing;
touch the evaluator-model/evaluator-provider parsing logic in
scripts/optimize-skill.js to implement the inference if you choose that route.
In `@scripts/optimize-skill.js`:
- Around line 81-90: The optimizer/evaluator provider flags default to
'anthropic', so passing only --evaluator-model (e.g., gpt-4o-mini) can route
OpenAI IDs to Anthropic; update the logic that builds the options object (fields
optimizerProvider, evaluatorProvider sourced from
args.optimizerProvider/evaluatorProvider and
args.optimizerModel/args.evaluatorModel) to either (A) infer provider from the
model id when the corresponding provider flag is absent (e.g., treat model id
starting with "gpt-" or containing "openai" as OpenAI) or (B) validate
compatibility before calling train() (the train() callsite) and throw a clear
error if model/provider mismatch is detected; locate and change the code that
sets optimizerProvider/evaluatorProvider and the train() invocation to perform
this inference/validation and surface a helpful error message.
In `@skills/skill-optimizer/SKILL.md`:
- Line 87: Replace the current phrasing that calls “ReflACT” a SkillOpt named
6-stage pipeline: edit SKILL.md to either (a) directly describe Microsoft
SkillOpt’s six-stage loop by name (rollout → reflect → aggregate → select →
update → evaluate) and keep the arXiv:2605.23904 reference, or (b) explicitly
mark “ReflACT” as a local label in this repo (e.g., “locally labeled ‘ReflACT’
for our adaptation of SkillOpt’s six-stage loop”) so it’s clear the term is not
used in the original SkillOpt paper; update the sentence that mentions “ReflACT”
to follow one of these two options.
In `@src/db/schema.sql`:
- Around line 231-240: The optimization_validation table is currently
append-only and upsertValidation in src/optimizer/store.ts always INSERTs rows
with learning_id=null, causing duplicates; fix by adding a uniqueness constraint
that treats NULL learning_id consistently and update upsertValidation to use an
UPSERT/ON CONFLICT target. Specifically, add a unique index on (skill_slug,
COALESCE(learning_id, -1)) for the optimization_validation table (or equivalent
expression) so NULLs collide, then change upsertValidation to perform INSERT ...
ON CONFLICT(skill_slug, COALESCE(learning_id, -1)) DO UPDATE ... (or use INSERT
OR REPLACE) and ensure it sets learning_id explicitly (or uses COALESCE) rather
than leaving it NULL; this will prevent duplicate rows returned by
listValidation.
In `@src/optimizer/aggregate.ts`:
- Around line 39-41: The patchKey currently collapses distinct patches by
returning `${p.op}::${p.anchor.trim().toLowerCase()}`, causing different `add`
patches (or any patches with identical op+anchor but different `payload`) to
collide in the `seen` map; update patchKey (function patchKey) to include
payload identity for cases where payload matters (e.g., for op === 'add' or when
p.payload !== undefined) — for example by appending a stable representation of
p.payload (JSON.stringify or a content hash) to the key so different payloads
produce distinct keys — then ensure the existing `seen` map and conflict/votes
logic continue to operate against the new key so unique patches are preserved
instead of overwritten.
In `@src/optimizer/apply.ts`:
- Around line 52-58: The current replace branch uses skill.replace(patch.anchor,
patch.payload) which treats $-sequences in patch.payload as special replacement
patterns and can mangle LLM-authored content; change the replacement to use a
function replacer so the payload is inserted verbatim (i.e., call replace with a
function that returns patch.payload) while keeping the existing checks for
patch.op and patch.anchor and the truncate(anchor) error path; update the
expression where skill.replace is invoked to use the function-returning-payload
form to ensure literal insertion.
In `@src/optimizer/llm.ts`:
- Around line 28-34: PRICE_PER_M_TOKENS currently returns {input:0, output:0}
for unknown models which makes costUsd zero and bypasses the budget cap; change
the lookup logic that uses PRICE_PER_M_TOKENS so that if a model key is missing
it does not default to zero but instead logs a clear warning/error (e.g.,
processLogger.warn/error) and fails closed by throwing an Error (or otherwise
rejecting the run) so unlisted models cannot silently accrue $0 cost; update the
code paths that compute costUsd (the place referencing PRICE_PER_M_TOKENS and
producing costUsd) to perform an explicit existence check and handle missing
prices accordingly, and add/update tests to cover the missing-model case.
- Around line 101-119: The postJson function lacks a network timeout, so add a
timer that aborts the HTTPS request and destroys its socket if no response
within a reasonable interval (e.g., configurable constant); attach the timeout
to the req (and handle the 'socket' event to set socket.setTimeout or destroy on
timer), on timeout call req.destroy(new Error('Request timed out')) and reject
the Promise with that error, and ensure you clear the timeout on 'response' end
and on 'error' so you don't leak timers; update uses of req, res, and the
Promise resolution/rejection in postJson accordingly.
In `@src/optimizer/store.ts`:
- Around line 269-278: trajectoriesToValidation currently drops the originating
learningId and upsertValidation inserts every seed with learning_id = null,
causing duplicates on repeated train() runs; restore and persist a stable key
(the original learningId or a deterministic hash of the prompt/expected) in
trajectoriesToValidation so upsertValidation can use it, then change
upsertValidation to perform a real upsert/skip against storage (e.g. enforce a
unique constraint on (skill_slug, learning_id) or (skill_slug, prompt_hash) and
use INSERT ... ON CONFLICT DO NOTHING / UPDATE or a SELECT-then-INSERT guard)
instead of always inserting nulls; apply the same fix to the related insert path
referenced at lines 291-305 so both insertion sites use the stable key and
idempotent upsert behavior (refer to functions trajectoriesToValidation,
upsertValidation, train, and listValidation).
In `@src/optimizer/trainer.ts`:
- Around line 80-86: The current flow lets paid LLM calls (e.g., validateSkill
invoked for initialEval and later candidate/reflect/slow-update evaluations)
start before verifying remaining budget, so a single call can overshoot
args.config.budgetUsd; before every paid call that would create an evaluation
(all validateSkill calls and any reflect/candidate/slow-update evaluator
invocations referenced around the other blocks), add a preflight guard that
computes remaining = args.config.budgetUsd - spent and refuses/short-circuits
the call if remaining <= estimatedCallCost (or <= 0) and return an appropriate
error/result; update spent only after successful calls and centralize the check
so every place that calls validateSkill or the reflect/candidate/slow-update
evaluators performs the same preflight guard.
- Around line 80-90: The run can exit via multiple branches without persisting
updated counters; before calling endRun() ensure you flush the authoritative
metrics by calling store.updateRun(runId, { spentUsd: spent, rejectedSteps:
rejected, epochsCompleted: epochs, bestScore, bestHash, bestSkill, initialScore:
/* keep existing */ }); Add this final updateRun() immediately prior to any
endRun() invocation (and in the early-continue/exit paths that currently skip
it) so spent, rejected and epochs are always written; reference the variables
spent, rejected, epochs, bestScore, bestHash, bestSkill and the functions
store.updateRun and endRun to locate where to insert the call.
- Around line 215-241: The slow-update branch currently calls runSlowUpdate({
recentlyAcceptedDiffs: [] }) and uses a raw >= bestScore acceptance check, which
prevents consolidation from seeing recent diffs and weakens the acceptance gate;
update this flow so runSlowUpdate is invoked with the actual recently accepted
diffs (pass the variable that tracks recentlyAcceptedDiffs instead of an empty
array) and apply the same acceptance criterion used elsewhere (compute delta =
slowEval.result.weightedScore - bestScore and compare against acceptThreshold)
before promoting bestSkill/bestHash and calling store.updateRun so the
consolidation step receives the accepted diffs and the slow update uses the
identical acceptance logic as regular candidates.
- Around line 281-284: The writeBestSkill function currently appends a new
optimizer stamp each run; change it to replace any existing optimizer stamp(s)
instead: in writeBestSkill, if the target file exists read its current contents
(or operate on the incoming content if you prefer), remove any lines matching
the optimizer stamp pattern (e.g. /^<!-- skill-optimizer: .* -->\s*$/ or a
global regex that finds all <!-- skill-optimizer: ... --> entries), then append
a single fresh stamp variable (stamp) to the cleaned content and write that with
writeFileSync; keep the stamp string and function name (writeBestSkill) intact
but ensure you dedupe/remove stale stamps before writing.
---
Nitpick comments:
In `@commands/skill-optimize.md`:
- Line 11: The markdown file contains fenced code blocks that lack a specified
language (MD040); update each triple-backtick fence in
commands/skill-optimize.md to include an explicit language token (e.g., change
``` to ```text or ```bash) for the blocks referenced (the fences around the
snippets at the reported locations), ensuring all code fences across the file
include a language to satisfy markdownlint.
In `@scripts/optimize-skill.js`:
- Around line 38-39: The current branch that sets numeric config values (using
intKeys, floatKeys, out, key, v) can insert NaN when v is missing or
non-numeric; add a guard before parsing: if v is null/empty or Number(v) is NaN
or not finite, throw or surface a clear error indicating the invalid flag and
key (e.g., `Invalid numeric value for ${key}: ${v}`), otherwise parse (parseInt
for intKeys, parseFloat for floatKeys) and assign to out[key]; apply the same
validation to both intKeys and floatKeys branches so invalid inputs fail fast
instead of producing NaN in config.
In `@skills/skill-optimizer/SKILL.md`:
- Line 24: The fenced code block in SKILL.md is missing a language specifier
which triggers markdownlint MD040; update the triple-backtick fence on the block
at the shown location to include a language (for example use ```text) so the
fence becomes a labeled code block and satisfies the linter.
In `@src/optimizer/reflect.ts`:
- Around line 99-101: Duplicate fence-stripping and JSON-parse logic exists in
stripFences (function name) and similar code in validate.ts and slow.ts; extract
a single helper (e.g., stripFencesAndParse) that 1) strips leading ``` or
```json and trailing ``` using the existing regex, 2) safely parses JSON in a
try/catch and returns either the parsed object or a controlled error/undefined,
and 3) replace the inline logic in stripFences, validate.ts, and slow.ts to call
this new helper so all modules share identical parsing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 455854cb-5588-42c4-8d32-195b125bcd33
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
commands/skill-optimize.mdpackage.jsonscripts/optimize-skill.jsskills/skill-optimizer/SKILL.mdsrc/db/schema.sqlsrc/optimizer/__tests__/apply.test.tssrc/optimizer/aggregate.tssrc/optimizer/apply.tssrc/optimizer/clip.tssrc/optimizer/hash.tssrc/optimizer/llm.tssrc/optimizer/reflect.tssrc/optimizer/slow.tssrc/optimizer/store.tssrc/optimizer/trainer.tssrc/optimizer/types.tssrc/optimizer/validate.ts
13 review findings, all valid, all fixed:
1. CLI infers provider from model id (claude-* -> anthropic, gpt-*/o* -> openai). Mixed-provider example in commands/skill-optimize.md no longer requires --evaluator-provider for gpt-4o-mini.
2. NaN guard on numeric flag parsing: --epochs notanumber now errors fast instead of producing NaN in config.
3. SKILL.md drops the 'ReflACT' label since it is not a SkillOpt term; loop is referred to by stage names only.
4. optimization_validation gains prompt_hash + unique(skill_slug, prompt_hash); upsertValidation now uses ON CONFLICT DO UPDATE and persists learning_id from trajectoriesToValidation. Eliminates duplicate validation rows on repeated train() runs.
5. aggregate.patchKey now includes payload, so two distinct add proposals at the same anchor are preserved. Conflict counter moved to a separate anchor-level set so 'same anchor, different payloads' is still surfaced as a conflict.
6. apply.ts replace branch uses skill.replace(anchor, () => payload) function form. Payloads containing $1 / $& / $` are no longer mangled as backreferences.
7. llm.ts fails closed on unknown models: PRICE_PER_M_TOKENS lookup throws with the known-model list instead of silently returning {0,0} and bypassing budget cap.
8. llm.ts postJson gains an abortable timeout (default 120s, override via SKILL_OPTIMIZER_TIMEOUT_MS). Timer cleared on response end and on error to avoid leaks.
9. trainer.ts adds a preflight guardBudget() check before every paid LLM call (baseline eval, reflect, evaluate, slow update, slow update eval). Loop short-circuits when remaining budget cannot cover the call.
10. trainer.ts flushes spent / accepted / rejected / epochs / bestSkillHash / bestScore via flushAndEnd() before every endRun() call (completed / failed / stopped). Counters now persist even on early-exit branches.
11. trainer.ts slow update now passes recentlyAcceptedDiffs (last 8) and uses delta >= acceptThreshold against the gate, identical to the per-step gate. Previously consolidation saw an empty diff list and used raw >= bestScore.
12. trainer.ts writeBestSkill strips any prior <!-- skill-optimizer: ... --> stamp before writing the new one, so re-runs do not accumulate stamps. stripExistingStamp exported for test coverage.
13. Shared parse.ts (stripFencesAndParse) extracted from reflect.ts / validate.ts / slow.ts. All three modules now share identical fence-strip + try/catch JSON parse behavior.
Tests: 13/13 pass (4 new cases for the new behavior). Build + typecheck clean.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/optimizer/aggregate.ts`:
- Around line 43-49: The keys used for aggregation lowercasing and trimming
anchors causing distinct anchors to be collapsed; update patchKey and
anchorKeyFor to use the anchor exactly as-used by applyPatches (no .trim() and
no .toLowerCase()) so identity is consistent with applyPatches' exact matching;
ensure the functions patchKey and anchorKeyFor concatenate p.anchor verbatim
(still include p.op and p.payload as before) to avoid discarding patches that
would actually apply.
In `@src/optimizer/parse.ts`:
- Around line 1-7: The stripFencesAndParse function fails to remove fenced JSON
when the input has leading whitespace/newlines; update stripFencesAndParse to
first trim or otherwise ignore leading/trailing whitespace, then apply regexes
that match the opening fence even after whitespace (e.g., use text.trim() or
text.replace(/^\s*/, '') before replacing /^```(?:json)?/i and /\s*```$/ to
remove fences robustly), and then JSON.parse the resulting stripped string,
keeping the function signature and error handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e11f568-2aef-423c-9b9d-73e315c38304
📒 Files selected for processing (14)
commands/skill-optimize.mdscripts/optimize-skill.jsskills/skill-optimizer/SKILL.mdsrc/db/schema.sqlsrc/optimizer/__tests__/apply.test.tssrc/optimizer/aggregate.tssrc/optimizer/apply.tssrc/optimizer/llm.tssrc/optimizer/parse.tssrc/optimizer/reflect.tssrc/optimizer/slow.tssrc/optimizer/store.tssrc/optimizer/trainer.tssrc/optimizer/validate.ts
✅ Files skipped from review due to trivial changes (2)
- commands/skill-optimize.md
- skills/skill-optimizer/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (9)
- src/db/schema.sql
- src/optimizer/validate.ts
- src/optimizer/reflect.ts
- src/optimizer/slow.ts
- src/optimizer/llm.ts
- src/optimizer/apply.ts
- scripts/optimize-skill.js
- src/optimizer/trainer.ts
- src/optimizer/store.ts
Comment on lines
+43
to
+49
| function patchKey(p: Patch): string { | ||
| return `${p.op}::${p.anchor.trim().toLowerCase()}::${p.payload}`; | ||
| } | ||
|
|
||
| function anchorKeyFor(p: Patch): string { | ||
| return `${p.op}::${p.anchor.trim().toLowerCase()}`; | ||
| } |
Contributor
There was a problem hiding this comment.
Keep anchor identity consistent with applyPatches.
src/optimizer/apply.ts matches anchors exactly, but patchKey/anchorKeyFor lowercase and trim them here. That lets distinct patches like ## Rules vs ## rules or anchors that intentionally include boundary whitespace collapse into one vote/conflict bucket, so aggregation can keep a patch that will never apply and discard the one that would.
Suggested fix
function patchKey(p: Patch): string {
- return `${p.op}::${p.anchor.trim().toLowerCase()}::${p.payload}`;
+ return `${p.op}::${p.anchor}::${p.payload}`;
}
function anchorKeyFor(p: Patch): string {
- return `${p.op}::${p.anchor.trim().toLowerCase()}`;
+ return `${p.op}::${p.anchor}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function patchKey(p: Patch): string { | |
| return `${p.op}::${p.anchor.trim().toLowerCase()}::${p.payload}`; | |
| } | |
| function anchorKeyFor(p: Patch): string { | |
| return `${p.op}::${p.anchor.trim().toLowerCase()}`; | |
| } | |
| function patchKey(p: Patch): string { | |
| return `${p.op}::${p.anchor}::${p.payload}`; | |
| } | |
| function anchorKeyFor(p: Patch): string { | |
| return `${p.op}::${p.anchor}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/optimizer/aggregate.ts` around lines 43 - 49, The keys used for
aggregation lowercasing and trimming anchors causing distinct anchors to be
collapsed; update patchKey and anchorKeyFor to use the anchor exactly as-used by
applyPatches (no .trim() and no .toLowerCase()) so identity is consistent with
applyPatches' exact matching; ensure the functions patchKey and anchorKeyFor
concatenate p.anchor verbatim (still include p.op and p.payload as before) to
avoid discarding patches that would actually apply.
Comment on lines
+1
to
+7
| export function stripFencesAndParse<T = unknown>(text: string): T | null { | ||
| const stripped = text.replace(/^```(?:json)?\s*\n?/, '').replace(/\n?```\s*$/, '').trim(); | ||
| try { | ||
| return JSON.parse(stripped) as T; | ||
| } catch { | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
Handle leading whitespace before fenced JSON.
This only strips fences when the very first character is ```, so a response that starts with a blank line or spaces before the fence falls through to JSON.parse, returns `null`, and `src/optimizer/reflect.ts` / `src/optimizer/validate.ts` silently treat that as no patches/outcomes.
Suggested fix
export function stripFencesAndParse<T = unknown>(text: string): T | null {
- const stripped = text.replace(/^```(?:json)?\s*\n?/, '').replace(/\n?```\s*$/, '').trim();
+ const stripped = text
+ .trim()
+ .replace(/^```(?:json)?\s*/i, '')
+ .replace(/\s*```\s*$/, '');
try {
return JSON.parse(stripped) as T;
} catch {
return null;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/optimizer/parse.ts` around lines 1 - 7, The stripFencesAndParse function
fails to remove fenced JSON when the input has leading whitespace/newlines;
update stripFencesAndParse to first trim or otherwise ignore leading/trailing
whitespace, then apply regexes that match the opening fence even after
whitespace (e.g., use text.trim() or text.replace(/^\s*/, '') before replacing
/^```(?:json)?/i and /\s*```$/ to remove fences robustly), and then JSON.parse
the resulting stripped string, keeping the function signature and error handling
unchanged.
Five issues caught reading the prior commit cold. 1. trainer.ts writeBestSkill: skip the write entirely when bestHash === initialHash. Previously every run rewrote SKILL.md with a fresh stamp even when no candidate beat the baseline, polluting the file with optimizer metadata on no-op runs. 2. trainer.ts makeMinibatches: replaced biased Array.sort(() => Math.random() - 0.5) with Fisher-Yates shuffle. The sort-based shuffle is non-uniform because the comparator violates transitivity; Fisher-Yates is the standard fix. 3. trainer.ts step loop: moved the budget guard above store.saveCandidate / store.recordPatches. Previously a budget exhaustion right before the evaluator left orphaned 'pending' candidates and their patches persisted in the DB with no eval to follow. 4. trainer.ts validation guard: throws early if the validation set has fewer than 2 items (e.g. --holdout 0 or pathological trajectories). Empty validation made every candidate score 0, delta 0 >= 0 threshold, trivially accepting garbage edits. 5. trainer.ts STAMP_RE: dropped the trailing $ anchor so a global replace strips ALL accumulated <!-- skill-optimizer: ... --> stamps, not just the last one. Pre-existing risk only on files that already had multiple stamps from older runs. Tests: 14/14 pass (1 new case for multi-stamp strip).
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
1 participant
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.
Implements the bridge sketched in the SkillOpt analysis — treat learn-rule outputs as training trajectories, run a budget-capped offline optimization over each skill's SKILL.md, gate by validation set of past corrections.
What it does
A new
/skill-optimize <slug>command runs a 6-stage pipeline borrowed from Microsoft SkillOpt's ReflACT loop, adapted to pro-workflow's existing SQLite + learnings tables:Rejected candidates feed a rejection buffer that biases the next reflect step. At epoch boundaries a slow-update consolidation rewrites the best skill; the rewrite itself must pass the gate.
Files
Configuration
--epochs N--batch-size N--minibatches N--holdout N--budget-usd X--optimizer-model Mclaude-sonnet-4-6--evaluator-model Mclaude-haiku-4-5-20251001--max-adds / --max-deletes / --max-replaces--accept-threshold X--max-skill-tokens N--slow-every NKill switch:
touch ~/.pro-workflow/STOP.Provenance
Inspired by Microsoft SkillOpt (arXiv:2605.23904). The 6-stage pipeline, LR budget, rejection buffer, and slow / meta update mechanics are adapted to pro-workflow's data plane. No SkillOpt code reused; this is an independent port of the concepts in TypeScript on top of
better-sqlite3.Test plan
npm testnpm run buildcleanonly N trajectories; need >= 8ANTHROPIC_API_KEYagainst a real skill (requires API spend; defaults cap at $0.50)~/.pro-workflow/data.dbOut of scope
Summary by CodeRabbit
New Features
Chores
Tests