feat(skills): outcome-driven skill repair, with a human in the loop

[Nivesh353]

Problem

Skills were learned once and trusted forever.

The reinforcement machinery already existed on main — confidence rose on
success, fell twice as fast on failure, and anything under 0.4 was "flagged".
Three things were missing:

1. The flag was computed and then ignored. task_tracker begin told the
   model "⚡ SKILL MATCH FOUND — YOU MUST USE IT … do NOT proceed with a manual
   approach" regardless of confidence. A skill that had failed 5 of 7 times got
   the same mandatory language as a perfect one. isSkillFlagged was only read
   by status/review — commands a human had to go and type.
2. Failure lessons were write-only. Each failure appended the model's own
   one-line excuse to negative_examples. Nothing ever read that list to change
   anything, so ## Steps stayed wrong forever and the same mistake repeated.
3. A broken skill stayed broken. No repair path existed: hand-edit it,
   delete it, or keep following it — and per (1), the agent was being told to
   keep following it.

Net effect was a ratchet: a bad skill is mandated → fails → confidence drops →
still mandated → fails again, each failure writing a shallow lesson that nothing
consumes.

What's in this PR

1. Skills learn from failure (a5f1ba6)
On any non-success outcome, task_tracker end replaces the model's one-line
excuse with a grounded root-cause + next-strategy reflection derived from the
steps actually recorded. Fails soft — a reflection error keeps the raw reason.

2. Skills can repair themselves (a5f1ba6)
New skill_learner repair reads a flagged skill's accumulated lessons and
rewrites its steps to avoid each one. Capped at 3 repairs per skill, after which
it requires update/delete. Confidence resets to 0.6 — enough to clear the
flag, not enough to be treated as proven.

3. A human decides when it matters (396f3f3)

• Flagged match → prompt proceed / repair / skip, with the skill's
  confidence, success/failure ratio, and recent failures shown.
• Proposed repair → diff of current vs proposed steps, then
  accept / edit / cancel. edit opens $VISUAL/$EDITOR and re-shows the
  diff of what you saved. Nothing is written or committed before accept.
• The user's answer reaches the model as an explicit USER DECISION:
  instruction, so it can't quietly do the opposite.
• Headless/SDK: one autoRepair boolean instead of prompts.

4. Closed a hole this exposed (fix commit)
crystallize could overwrite an existing skill, resetting confidence to 1.0 and
erasing its failure history with no approval. Found when a live voice session
did exactly that, unprompted, after being refused a repair.

Behavior changes

• Unattended repair now requires opt-in. autoRepair defaults off, so voice
  sessions and scheduled jobs no longer auto-repair; they get a "flagged,
  repair is disabled" report instead. Rationale: an agent rewriting instructions
  it will then follow shouldn't happen unobserved. Flip the default in
  sdk-types.ts if we'd rather preserve the old behavior.
• crystallize refuses an existing name and points at update.

[shreyas-lyzr]

This is a well-structured PR that addresses three real gaps in the skill reinforcement loop. The architecture is sound, the approval gate is correctly placed, and the test suite is thorough. A few things worth addressing before or shortly after merge:

Correctness — repair/update/delete accept un-validated skill_name

crystallize validates skill_name with /^[a-z0-9]+(-[a-z0-9]+)*$/ (lines 242-243 of skill-learner.ts), but the repair, update, and delete actions skip that check. In practice, all extant skill names come from crystallize so they are already kebab-case — but an externally-crafted call (prompt injection, direct SDK use) could pass skill_name = "../../../some/path". The file-existence check (readFile → "Skill not found") limits the damage to files that already exist, but defense-in-depth suggests adding the same regex guard to the other three actions. Not blocking given the internal nature of this tool, but worth a follow-up.

Minor correctness — repair reads the SKILL.md twice

The repair case reads the file once directly (line 440: readFile(skillFile)) to parse frontmatter and body, then calls loadSkillStats(skillDir) (line 453) which reads it a second time for stats.negative_examples. These two reads are sequential in a single-threaded async context, so there is no practical race, but a single parse call would be cleaner and remove the theoretical staleness window entirely.

Subtle edge case — empty editor result is silently accepted

If the user chooses edit and clears the entire file (saves an empty buffer), edited.trim() === initial.trim() is false, so the code sets finalSteps = "" and userEdited = true. The repair then writes a SKILL.md with an empty Steps section, which would silently break the skill. A one-line guard (if (edited !== null && edited.trim().length > 0)) would prevent this.

Suggestion — the reflected failure reason might be verbose as a negative_example

The reflection prompt caps output at 500 chars (2-4 sentences). That text is then stored verbatim as a negative_example entry, which in turn gets fed as input to the repair prompt (up to 10 examples × 500 chars = 5000 chars of injected context). The repair prompt already has its own cap at 3000 chars output — this won't cause errors — but a terse lesson (one sentence) would be more useful than a paragraph at retrieval time. Consider trimming the lesson before appending to negative_examples, or splitting the reflection output into a "lesson" field separate from the verbose analysis.

Security pass

No new dependencies added in this PR. No hardcoded secrets or keys found in any of the new files. No injection/authz gaps reachable from untrusted input (skill_name values are not user-supplied in normal flows). The autoRepair opt-in default-off is the right call — an agent rewriting instructions it will then follow unobserved is the correct thing to prevent by default.

Tests

The hermetic suite in learning.test.ts is solid: reinforcement math, the full task lifecycle, all guard rails. The Proxy-based exploding model is a good technique for proving the fail-soft paths were actually reached (the touched flag closes the gap). The live test file is gated correctly behind GITAGENT_LIVE_TESTS. No coverage gaps on the new paths that can be exercised without a real model.