fix(skill-optimizer): address PR #64 review findings

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.

Author: rohitg00

commands/skill-optimize.md

@@ -8,7 +8,7 @@ Run an offline, budget-capped optimization loop over a skill's accumulated `lear
 
 ## Quick Start
 
-```
+```text
 /skill-optimize <slug> [--epochs 3] [--budget-usd 0.50]
 ```
 
@@ -28,10 +28,12 @@ Run an offline, budget-capped optimization loop over a skill's accumulated `lear
 
 ## Examples
 
-```
+```text
 /skill-optimize pro-workflow
 /skill-optimize wiki-research-loop --budget-usd 1.0 --epochs 5
-/skill-optimize wrap-up --optimizer-model claude-opus-4-7 --evaluator-model gpt-4o-mini --json
+/skill-optimize wrap-up --optimizer-model claude-opus-4-7 --evaluator-model gpt-4o-mini
 ```
 
+The third example mixes providers. The CLI infers the provider from the model id (`claude-*` → anthropic, `gpt-*` / `o*` → openai), so you do not need `--evaluator-provider openai` for `gpt-4o-mini`. Pass an explicit `--optimizer-provider` / `--evaluator-provider` to override inference.
+
 See [skills/skill-optimizer/SKILL.md](../skills/skill-optimizer/SKILL.md) for full mechanics, defaults, and the SkillOpt provenance.

scripts/optimize-skill.js

@@ -35,13 +35,33 @@ function parseArgs(argv) {
     if (!a.startsWith('--')) continue;
     const key = a.slice(2).replace(/-([a-z])/g, (_, c) => c.toUpperCase());
     const v = argv[++i];
-    if (intKeys.has(key)) out[key] = parseInt(v, 10);
-    else if (floatKeys.has(key)) out[key] = parseFloat(v);
-    else out[key] = v;
+    if (v === undefined || v === '') {
+      throw new Error(`Missing value for --${key}`);
+    }
+    if (intKeys.has(key)) {
+      const parsed = parseInt(v, 10);
+      if (!Number.isFinite(parsed)) throw new Error(`Invalid integer value for --${key}: ${v}`);
+      out[key] = parsed;
+    } else if (floatKeys.has(key)) {
+      const parsed = parseFloat(v);
+      if (!Number.isFinite(parsed)) throw new Error(`Invalid number value for --${key}: ${v}`);
+      out[key] = parsed;
+    } else {
+      out[key] = v;
+    }
   }
   return out;
 }
 
+function inferProvider(model) {
+  if (!model) return null;
+  if (/^claude-/.test(model)) return 'anthropic';
+  if (/^(gpt-|o\d|chatgpt-)/i.test(model) || /openai/i.test(model)) return 'openai';
+  if (/(?:^|\/)(?:llama-|mistralai\/|accounts\/fireworks)/i.test(model)) return 'fireworks';
+  if (/\//.test(model)) return 'openrouter';
+  return null;
+}
+
 function findSkillPath(slug) {
   const candidates = [
     path.join(ROOT, 'skills', slug, 'SKILL.md'),
@@ -52,11 +72,27 @@ function findSkillPath(slug) {
 }
 
 (async () => {
-  const args = parseArgs(process.argv);
+  let args;
+  try {
+    args = parseArgs(process.argv);
+  } catch (err) {
+    process.stderr.write(`${err.message}\n`);
+    process.exit(1);
+  }
   if (!args.slug) {
     process.stderr.write('Usage: optimize-skill --slug <name> [--skill-path <path>] [options]\n');
     process.exit(1);
   }
+  const userSetOptProvider = process.argv.includes('--optimizer-provider');
+  const userSetEvalProvider = process.argv.includes('--evaluator-provider');
+  if (!userSetOptProvider) {
+    const inferred = inferProvider(args.optimizerModel);
+    if (inferred) args.optimizerProvider = inferred;
+  }
+  if (!userSetEvalProvider) {
+    const inferred = inferProvider(args.evaluatorModel);
+    if (inferred) args.evaluatorProvider = inferred;
+  }
   const skillPath = args.skillPath || findSkillPath(args.slug);
   if (!skillPath || !fs.existsSync(skillPath)) {
     process.stderr.write(`SKILL.md not found for slug "${args.slug}"\n`);

skills/skill-optimizer/SKILL.md

@@ -19,17 +19,17 @@ Do not use when:
 - The user wants real-time edits (this is offline, single-shot)
 - No `ANTHROPIC_API_KEY` (or equivalent provider key) is available
 
-## Architecture (mirrors SkillOpt)
-
-```
-rollout      — pull recent learnings from SQLite (existing learn-rule rows)
-reflect      — optimizer LLM analyzes a minibatch, proposes add/delete/replace patches
-aggregate    — vote-merge patches across minibatches
-select       — clip by LR budget (default: 3 adds, 2 deletes, 3 replaces per step)
-update       — apply selected patches to a candidate skill content
-evaluate     — evaluator LLM scores candidate against held-out validation items
-gate         — accept candidate only if weighted score >= current + acceptThreshold
-slow update  — at epoch boundary, consolidate accepted edits into a coherent rewrite
+## Architecture (mirrors SkillOpt's six-stage loop)
+
+```text
+rollout      pull recent learnings from SQLite (existing learn-rule rows)
+reflect      optimizer LLM analyzes a minibatch, proposes add/delete/replace patches
+aggregate    vote-merge patches across minibatches
+select       clip by LR budget (default: 3 adds, 2 deletes, 3 replaces per step)
+update       apply selected patches to a candidate skill content
+evaluate     evaluator LLM scores candidate against held-out validation items
+gate         accept candidate only if weighted score >= current + acceptThreshold
+slow update  at epoch boundary, consolidate accepted edits into a coherent rewrite
 ```
 
 Failed candidates are stored in a rejection buffer and fed back to the next reflect step so the optimizer doesn't propose the same patch twice.
@@ -84,4 +84,4 @@ sqlite3 ~/.pro-workflow/data.db "SELECT id, skill_slug, initial_score, best_scor
 
 ## Provenance
 
-Inspired by Microsoft SkillOpt (arXiv:2605.23904). The ReflACT 6-stage pipeline, LR budget, rejection buffer, and slow / meta update mechanics are adapted to pro-workflow's existing SQLite + learn-rule data plane. No SkillOpt code is reused.
+Inspired by Microsoft SkillOpt (arXiv:2605.23904). The six-stage rollout/reflect/aggregate/select/update/evaluate pipeline, LR budget, rejection buffer, and slow / meta update mechanics are adapted to pro-workflow's existing SQLite + learn-rule data plane. No SkillOpt code is reused. "ReflACT" is not a SkillOpt term and is not used here; the loop is referred to by stage names only.

src/db/schema.sql

@@ -232,12 +232,14 @@ CREATE TABLE IF NOT EXISTS optimization_validation (
   id INTEGER PRIMARY KEY AUTOINCREMENT,
   skill_slug TEXT NOT NULL,
   learning_id INTEGER REFERENCES learnings(id) ON DELETE SET NULL,
+  prompt_hash TEXT NOT NULL,
   prompt TEXT NOT NULL,
   expected TEXT NOT NULL,
   weight REAL NOT NULL DEFAULT 1.0,
   frozen_at TEXT DEFAULT (datetime('now'))
 );
 CREATE INDEX IF NOT EXISTS idx_opt_val_skill ON optimization_validation(skill_slug);
+CREATE UNIQUE INDEX IF NOT EXISTS idx_opt_val_unique ON optimization_validation(skill_slug, prompt_hash);
 
 CREATE TABLE IF NOT EXISTS optimization_rejections (
   id INTEGER PRIMARY KEY AUTOINCREMENT,

src/optimizer/__tests__/apply.test.ts

@@ -3,6 +3,7 @@ import assert from 'node:assert/strict';
 import { applyPatches } from '../apply';
 import { aggregatePatches } from '../aggregate';
 import { clipByLR } from '../clip';
+import { stripExistingStamp } from '../trainer';
 import type { Patch } from '../types';
 
 describe('applyPatches', () => {
@@ -75,6 +76,38 @@ describe('aggregatePatches', () => {
   });
 });
 
+describe('apply replace handles $-sequences verbatim', () => {
+  it('does not treat $1 in payload as a backreference', () => {
+    const out = applyPatches('# A\nold\n', [
+      { op: 'replace', anchor: 'old', payload: 'cost: $1 per unit, $2 each' },
+    ]);
+    assert.match(out.content, /cost: \$1 per unit, \$2 each/);
+  });
+});
+
+describe('aggregate keeps distinct payloads at same anchor', () => {
+  it('does not collide different add payloads at same anchor', () => {
+    const out = aggregatePatches([
+      [{ op: 'add', anchor: '## X', payload: 'foo' }],
+      [{ op: 'add', anchor: '## X', payload: 'bar' }],
+    ]);
+    assert.equal(out.merged.length, 2);
+    assert.ok(out.conflicts >= 1);
+  });
+});
+
+describe('stripExistingStamp', () => {
+  it('removes the trailing optimizer stamp', () => {
+    const stamped = '# Skill\nbody\n<!-- skill-optimizer: hash=abc slug=foo -->\n';
+    assert.equal(stripExistingStamp(stamped), '# Skill\nbody');
+  });
+
+  it('leaves unstamped content alone', () => {
+    const plain = '# Skill\nbody\n';
+    assert.equal(stripExistingStamp(plain), plain);
+  });
+});
+
 describe('clipByLR', () => {
   it('respects per-op budget', () => {
     const patches: Patch[] = [

src/optimizer/aggregate.ts

@@ -8,25 +8,29 @@ export interface AggregateResult {
 
 export function aggregatePatches(batches: Patch[][]): AggregateResult {
   const seen = new Map<string, { patch: Patch; votes: number }>();
-  let conflicts = 0;
+  const anchorSeen = new Map<string, Set<string>>();
 
   for (const batch of batches) {
     for (const patch of batch) {
       const key = patchKey(patch);
       const prior = seen.get(key);
-      if (!prior) {
-        seen.set(key, { patch, votes: 1 });
-        continue;
-      }
-      if (prior.patch.op === patch.op && prior.patch.payload === patch.payload) {
+      if (prior) {
         prior.votes++;
       } else {
-        conflicts++;
-        if (prior.votes < 2) seen.set(key, { patch, votes: 1 });
+        seen.set(key, { patch, votes: 1 });
       }
+      const anchorKey = anchorKeyFor(patch);
+      const set = anchorSeen.get(anchorKey) ?? new Set<string>();
+      set.add(patch.payload);
+      anchorSeen.set(anchorKey, set);
     }
   }
 
+  let conflicts = 0;
+  for (const variants of anchorSeen.values()) {
+    if (variants.size > 1) conflicts += variants.size - 1;
+  }
+
   const ordered = [...seen.values()].sort((a, b) => {
     if (a.patch.op !== b.patch.op) return opRank(a.patch.op) - opRank(b.patch.op);
     return b.votes - a.votes;
@@ -37,6 +41,10 @@ export function aggregatePatches(batches: Patch[][]): AggregateResult {
 }
 
 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()}`;
 }
 

src/optimizer/apply.ts

@@ -54,7 +54,7 @@ function applyOne(skill: string, patch: Patch): { ok: true; content: string } |
     if (!skill.includes(patch.anchor)) {
       return { ok: false, reason: `anchor not found: ${truncate(patch.anchor)}` };
     }
-    return { ok: true, content: skill.replace(patch.anchor, patch.payload) };
+    return { ok: true, content: skill.replace(patch.anchor, () => patch.payload) };
   }
 
   return { ok: false, reason: `unknown op: ${(patch as Patch).op}` };

src/optimizer/llm.ts

@@ -1,5 +1,7 @@
 import * as https from 'node:https';
 
+const DEFAULT_TIMEOUT_MS = 120_000;
+
 export type Provider = 'anthropic' | 'openai' | 'openrouter' | 'fireworks';
 
 export interface LLMRequest {
@@ -46,11 +48,19 @@ export async function callLLM(req: LLMRequest): Promise<LLMResponse> {
     ? { 'x-api-key': apiKey, 'anthropic-version': '2023-06-01', 'content-type': 'application/json' }
     : { authorization: `Bearer ${apiKey}`, 'content-type': 'application/json' };
 
+  const price = PRICE_PER_M_TOKENS[req.model];
+  if (!price) {
+    const known = Object.keys(PRICE_PER_M_TOKENS).join(', ');
+    throw new Error(
+      `Unknown model "${req.model}". Budget cap cannot be enforced. ` +
+      `Add a price entry to PRICE_PER_M_TOKENS in src/optimizer/llm.ts or pick one of: ${known}.`,
+    );
+  }
+
   const raw = await postJson(cfg.host, cfg.path, headers, body);
   const parsed = JSON.parse(raw);
   const text = extractText(req.provider, parsed);
   const usage = extractUsage(req.provider, parsed);
-  const price = PRICE_PER_M_TOKENS[req.model] ?? { input: 0, output: 0 };
   const costUsd = (usage.input / 1_000_000) * price.input + (usage.output / 1_000_000) * price.output;
 
   return { text, inputTokens: usage.input, outputTokens: usage.output, costUsd };
@@ -99,20 +109,39 @@ function extractUsage(provider: Provider, body: unknown): { input: number; outpu
 }
 
 function postJson(host: string, path: string, headers: Record<string, string>, body: string): Promise<string> {
+  const timeoutMs = parseInt(process.env.SKILL_OPTIMIZER_TIMEOUT_MS ?? '', 10) || DEFAULT_TIMEOUT_MS;
   return new Promise((resolve, reject) => {
+    let timer: NodeJS.Timeout | null = null;
+    let settled = false;
+    const settle = (fn: () => void) => {
+      if (settled) return;
+      settled = true;
+      if (timer) {
+        clearTimeout(timer);
+        timer = null;
+      }
+      fn();
+    };
     const req = https.request(
       { host, path, method: 'POST', headers: { ...headers, 'content-length': Buffer.byteLength(body).toString() } },
       (res) => {
         const chunks: Buffer[] = [];
         res.on('data', (c) => chunks.push(c));
         res.on('end', () => {
           const text = Buffer.concat(chunks).toString('utf8');
-          if ((res.statusCode ?? 500) >= 400) reject(new Error(`HTTP ${res.statusCode}: ${text.slice(0, 500)}`));
-          else resolve(text);
+          if ((res.statusCode ?? 500) >= 400) {
+            settle(() => reject(new Error(`HTTP ${res.statusCode}: ${text.slice(0, 500)}`)));
+          } else {
+            settle(() => resolve(text));
+          }
         });
+        res.on('error', (err) => settle(() => reject(err)));
       },
     );
-    req.on('error', reject);
+    req.on('error', (err) => settle(() => reject(err)));
+    timer = setTimeout(() => {
+      req.destroy(new Error(`Request timed out after ${timeoutMs}ms`));
+    }, timeoutMs);
     req.write(body);
     req.end();
   });

src/optimizer/parse.ts

@@ -0,0 +1,8 @@
+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;
+  }
+}

src/optimizer/reflect.ts

@@ -1,5 +1,6 @@
 import type { LRBudget, Patch, ReflectInput, ReflectOutput, Rejection, Trajectory } from './types';
 import { callLLM, type Provider } from './llm';
+import { stripFencesAndParse } from './parse';
 
 export interface ReflectArgs {
   input: ReflectInput;
@@ -67,15 +68,8 @@ function formatRejection(r: Rejection): Record<string, unknown> {
 }
 
 function parsePatches(text: string): Patch[] {
-  const stripped = stripFences(text).trim();
-  let obj: unknown;
-  try {
-    obj = JSON.parse(stripped);
-  } catch {
-    return [];
-  }
-  const root = obj as { patches?: unknown };
-  if (!Array.isArray(root.patches)) return [];
+  const root = stripFencesAndParse<{ patches?: unknown }>(text);
+  if (!root || !Array.isArray(root.patches)) return [];
   const out: Patch[] = [];
   for (const raw of root.patches) {
     const p = raw as Record<string, unknown>;
@@ -87,17 +81,8 @@ function parsePatches(text: string): Patch[] {
 }
 
 function extractReasoning(text: string): string {
-  const stripped = stripFences(text).trim();
-  try {
-    const obj = JSON.parse(stripped) as { reasoning?: string };
-    return obj.reasoning ?? '';
-  } catch {
-    return '';
-  }
-}
-
-function stripFences(s: string): string {
-  return s.replace(/^```(?:json)?\s*\n?/, '').replace(/\n?```\s*$/, '');
+  const root = stripFencesAndParse<{ reasoning?: string }>(text);
+  return root?.reasoning ?? '';
 }
 
 export const __test = { parsePatches, extractReasoning };