fix(skills): self-improvement writes lessons, not incident logs
The skill review fork, the combined memory+skill review, and the curator's consolidation pass all described references/ as the place for "session-specific detail", and the curator's demote step said to move a sibling's file under the umbrella. Followed literally over months that produced one dev skill with a 100k SKILL.md dense in PR numbers and 443 one-per-session reference files, plus five sibling skills restating the same rules and the repo's AGENTS.md. The three prompts now share one shape contract: an entry is an imperative rule plus one clause of why, stated once; no PR/issue numbers, dates, or quoted chat as content; references/ is a small topical set extended in place, never a per-session file; skills do not restate always-loaded context. Consolidation is defined as distilling, and copying a sibling verbatim under references/ is named as the failure. skill_manage's schema carries the one-sentence version. Two advisory linter rules make the shape visible in the tool result the moment it starts to drift: incident-log-shape (PR/issue-number density in prose) and references-sprawl (>60 reference files), the latter also run on references/ writes. On the real before/after: the old skill trips both, the consolidated one trips neither.
This commit is contained in:
@@ -307,6 +307,32 @@ _MEMORY_REVIEW_PROMPT = (
|
||||
"'Nothing to save.' and stop."
|
||||
)
|
||||
|
||||
# Shared shape contract for anything written into a skill. The failure mode this prevents is the
|
||||
# hoarding library: one references/ file per session, incident narration instead of rules, PR numbers
|
||||
# and quotes as content, and duplicating what the repo's AGENTS.md / the tool schemas already teach.
|
||||
_LESSON_LAYER_BLOCK = (
|
||||
"What a skill entry IS (the lesson layer):\n"
|
||||
" • A generalizable rule + one clause of WHY (the mechanism), imperative, task-ordered. 'Grep the "
|
||||
"test tree for the SYMBOL before widening a helper signature — hand-rolled mocks reimplement the "
|
||||
"old shape and fail on a shard you did not run.' Not a narrative of what happened this session.\n"
|
||||
" • No PR/issue numbers, dates, ticket IDs, or quoted user text as content — the rule must stand "
|
||||
"without the incident behind it. Keep a short quote ONLY when the quote itself is the clearest "
|
||||
"statement of the rule.\n"
|
||||
" • The same lesson learned twice is ONE rule. Before adding, search the skill (and its "
|
||||
"references/) for the rule already stated; strengthen or clarify it rather than appending a "
|
||||
"second copy.\n"
|
||||
" • Not a duplicate of what the environment already teaches: repo AGENTS.md files, tool schema "
|
||||
"descriptions, and other always-loaded context. A skill carries the WORKFLOW and the pitfalls; "
|
||||
"it does not restate the codebase map or a tool's parameter list.\n"
|
||||
" • Always-on rules (standing user preferences, gates that apply to every instance of the "
|
||||
"task) live in SKILL.md itself, whole. references/ is for depth that is only needed sometimes: "
|
||||
"a decision table, a recipe, a domain note — each file topical and reusable, never "
|
||||
"'<date>-<incident>.md'. Prefer extending an existing references/ file over creating one; "
|
||||
"a skill with dozens of one-off references is the failure shape, not the goal.\n"
|
||||
" • Fix the skill in place when it is wrong: edit the sentence that misled, do not append "
|
||||
"'UPDATE: actually...' underneath it.\n\n"
|
||||
)
|
||||
|
||||
# Shared tail of the skill and combined prompts: what NOT to persist as a skill.
|
||||
_DO_NOT_CAPTURE_BLOCK = (
|
||||
" (these become persistent self-imposed constraints that bite you later when the environment "
|
||||
@@ -337,9 +363,10 @@ _SKILL_REVIEW_PROMPT = (
|
||||
"Review the conversation above and update the skill library. Be ACTIVE — most sessions produce "
|
||||
"at least one skill update, even if small. A pass that does nothing is a missed learning "
|
||||
"opportunity, not a neutral outcome.\n\n"
|
||||
"Target shape of the library: CLASS-LEVEL skills, each with a rich SKILL.md and a "
|
||||
"`references/` directory for session-specific detail. Not a long flat list of narrow "
|
||||
"one-session-one-skill entries. This shapes HOW you update, not WHETHER you update.\n\n"
|
||||
"Target shape of the library: CLASS-LEVEL skills, each with a SKILL.md of always-on rules and a "
|
||||
"small `references/` set of topical depth. Not a flat list of narrow one-session skills, and "
|
||||
"not an umbrella hoarding a references/ file per session. This shapes HOW you update, not "
|
||||
"WHETHER you update.\n\n" + _LESSON_LAYER_BLOCK +
|
||||
"Signals to look for (any one of these warrants action):\n"
|
||||
" • User corrected your style, tone, format, legibility, or verbosity. Frustration signals "
|
||||
"like 'stop doing X', 'this is too verbose', 'don't format like this', 'why are you "
|
||||
@@ -366,10 +393,10 @@ _SKILL_REVIEW_PROMPT = (
|
||||
"trigger.\n"
|
||||
" 3. ADD A SUPPORT FILE under an existing umbrella. Skills can be packaged with three kinds "
|
||||
"of support files — use the right directory per kind:\n"
|
||||
" • `references/<topic>.md` — session-specific detail (error transcripts, reproduction "
|
||||
"recipes, provider quirks) AND condensed knowledge banks: quoted research, API docs, external "
|
||||
"authoritative excerpts, or domain notes you found while working on the problem. Write it "
|
||||
"concise and for the value of the task, not as a full mirror of upstream docs.\n"
|
||||
" • `references/<topic>.md` — topical depth needed only sometimes: a decision table, a "
|
||||
"reproduction recipe, provider quirks, condensed domain notes or API excerpts. Name it by "
|
||||
"TOPIC and extend an existing file when one covers the topic; do not create a per-session or "
|
||||
"per-incident file, and do not paste error transcripts — distill them to the rule.\n"
|
||||
" • `templates/<name>.<ext>` — starter files meant to be copied and modified (boilerplate "
|
||||
"configs, scaffolding, a known-good example the agent can `reproduce with modifications`).\n"
|
||||
" • `scripts/<name>.<ext>` — statically re-runnable actions the skill can invoke directly "
|
||||
@@ -426,9 +453,9 @@ _COMBINED_REVIEW_PROMPT = (
|
||||
"**Skills**: how to do this class of task. Be ACTIVE — most sessions produce at least one "
|
||||
"skill update. A pass that does nothing is a missed learning opportunity, not a neutral "
|
||||
"outcome.\n\n"
|
||||
"Target shape of the skill library: CLASS-LEVEL skills with a rich SKILL.md and a "
|
||||
"`references/` directory for session-specific detail. Not a long flat list of narrow "
|
||||
"one-session-one-skill entries.\n\n"
|
||||
"Target shape of the skill library: CLASS-LEVEL skills with a SKILL.md of always-on rules and a "
|
||||
"small `references/` set of topical depth — not narrow one-session skills, and not an umbrella "
|
||||
"hoarding a references/ file per session.\n\n" + _LESSON_LAYER_BLOCK +
|
||||
"Signals that warrant a skill update (any one is enough):\n"
|
||||
" • User corrected your style, tone, format, legibility, verbosity, or approach. Frustration "
|
||||
"is a FIRST-CLASS skill signal, not just a memory signal. 'stop doing X', 'don't format like "
|
||||
@@ -445,8 +472,8 @@ _COMBINED_REVIEW_PROMPT = (
|
||||
"off-limits however relevant; fall through when one of those is the best fit.\n"
|
||||
" 2. UPDATE AN EXISTING UMBRELLA (skills_list + skill_view to find the right one). Patch it.\n"
|
||||
" 3. ADD A SUPPORT FILE under an existing umbrella via skill_manage action=write_file. Three "
|
||||
"kinds: `references/<topic>.md` for session-specific detail OR condensed knowledge banks "
|
||||
"(quoted research, API docs excerpts, domain notes) written concise and task-focused; "
|
||||
"kinds: `references/<topic>.md` for topical depth (decision tables, recipes, quirks, condensed "
|
||||
"domain notes) — extend an existing topical file before creating one, never a per-session file; "
|
||||
"`templates/<name>.<ext>` for starter files meant to be copied and modified; "
|
||||
"`scripts/<name>.<ext>` for statically re-runnable actions (verification, fixture generators, "
|
||||
"probes). Add a one-line pointer in SKILL.md so future agents find them.\n"
|
||||
|
||||
@@ -276,9 +276,15 @@ CURATOR_REVIEW_PROMPT = (
|
||||
"trigger class in that window). One broad umbrella "
|
||||
"skill with labeled subsections beats five narrow siblings for "
|
||||
"discoverability, not the other way around.\n\n"
|
||||
"The right target shape is CLASS-LEVEL skills with rich SKILL.md "
|
||||
"bodies + `references/`, `templates/`, and `scripts/` subfiles for "
|
||||
"session-specific detail — not one-session-one-skill micro-entries.\n\n"
|
||||
"The right target shape is CLASS-LEVEL skills whose SKILL.md carries the "
|
||||
"always-on rules and whose `references/`, `templates/`, and `scripts/` hold a "
|
||||
"SMALL set of topical depth — not one-session-one-skill micro-entries, and "
|
||||
"not an umbrella that hoards one references/ file per absorbed sibling. "
|
||||
"Consolidation means DISTILLING: the absorbed content becomes rules "
|
||||
"(imperative + one clause of why), the same lesson stated twice becomes "
|
||||
"one rule, and incident narration, PR/issue numbers, dates and quoted "
|
||||
"chatter are dropped — the rule must stand without the story. Moving a "
|
||||
"file unchanged under references/ is filing, not consolidating.\n\n"
|
||||
"Hard rules — do not violate:\n"
|
||||
"1. DO NOT touch bundled, hub-installed, or external-dir skills "
|
||||
"(`skills.external_dirs`). The candidate list below is already filtered "
|
||||
@@ -333,11 +339,12 @@ CURATOR_REVIEW_PROMPT = (
|
||||
"skill whose SKILL.md covers the shared workflow and has short "
|
||||
"labeled subsections. Archive the now-absorbed narrow siblings.\n"
|
||||
" c. DEMOTE TO REFERENCES/TEMPLATES/SCRIPTS — a sibling has "
|
||||
"narrow-but-valuable session-specific content. Move it into the "
|
||||
"umbrella's appropriate support directory:\n"
|
||||
" • `references/<topic>.md` for session-specific detail OR "
|
||||
"condensed knowledge banks (quoted research, API docs excerpts, "
|
||||
"domain notes, provider quirks, reproduction recipes)\n"
|
||||
"narrow-but-valuable depth that is only needed sometimes. Distill it "
|
||||
"into the umbrella's appropriate support directory:\n"
|
||||
" • `references/<topic>.md` — named by TOPIC, merged into an "
|
||||
"existing topical file when one covers it (decision tables, recipes, "
|
||||
"provider quirks, condensed domain notes). Never `<sibling-name>.md` "
|
||||
"copied verbatim; never a per-incident file.\n"
|
||||
" • `templates/<name>.<ext>` for starter files meant to be "
|
||||
"copied and modified\n"
|
||||
" • `scripts/<name>.<ext>` for statically re-runnable actions "
|
||||
|
||||
@@ -189,3 +189,28 @@ def test_combined_review_prompt_teaches_read_before_write():
|
||||
# ---------------------------------------------------------------------------
|
||||
# _MEMORY_REVIEW_PROMPT — unchanged, still memory-focused
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _assert_lesson_layer_guidance(prompt: str, label: str) -> None:
|
||||
"""Skill writes must be lessons (rule + why), not incident logs or per-session reference files."""
|
||||
lower = prompt.lower()
|
||||
assert "why" in lower and "rule" in lower, f"{label}: must ask for rule + why"
|
||||
assert "pr/issue numbers" in lower or "pr numbers" in lower, f"{label}: must ban PR/issue numbers as content"
|
||||
assert "one rule" in lower, f"{label}: must collapse repeated lessons into one rule"
|
||||
assert "agents.md" in lower, f"{label}: must forbid duplicating always-loaded context"
|
||||
assert "per-session" in lower or "per-incident" in lower, f"{label}: must forbid per-session reference files"
|
||||
|
||||
|
||||
def test_skill_review_prompt_teaches_lesson_layer():
|
||||
_assert_lesson_layer_guidance(AIAgent._SKILL_REVIEW_PROMPT, "_SKILL_REVIEW_PROMPT")
|
||||
|
||||
|
||||
def test_combined_review_prompt_teaches_lesson_layer():
|
||||
_assert_lesson_layer_guidance(AIAgent._COMBINED_REVIEW_PROMPT, "_COMBINED_REVIEW_PROMPT")
|
||||
|
||||
|
||||
def test_curator_prompt_consolidates_by_distilling():
|
||||
from agent.curator import CURATOR_REVIEW_PROMPT
|
||||
lower = CURATOR_REVIEW_PROMPT.lower()
|
||||
assert "distill" in lower, "curator must distill absorbed content, not file it"
|
||||
assert "verbatim" in lower and "per-incident" in lower, "curator must not copy siblings verbatim into references/"
|
||||
|
||||
@@ -184,3 +184,31 @@ def test_author_caps_warned():
|
||||
def test_findings_carry_rule_and_severity():
|
||||
findings = lint_content(CLEAN.replace("name: my-skill", "name: BAD"))
|
||||
assert any(f.rule == "name-format" and f.severity == ERROR for f in findings)
|
||||
|
||||
|
||||
def test_incident_log_shape_flagged_and_rule_shape_not():
|
||||
# A body narrating incidents by PR number is a log, not a lesson; the same lesson stated as a
|
||||
# rule + why with no numbers passes. Density-gated so one citation in a long body is fine.
|
||||
log = CLEAN.replace(
|
||||
"1. Use `read_file` to load it.",
|
||||
"In #12345 the watcher died; #23456 was the same; see PR #34567 and issue #45678 for the fix.",
|
||||
)
|
||||
rule = CLEAN.replace(
|
||||
"1. Use `read_file` to load it.",
|
||||
"Launch the watcher from a directory that outlives the watch; a deleted cwd reads as a stall.",
|
||||
)
|
||||
assert "incident-log-shape" in _rules(lint_content(log))
|
||||
assert "incident-log-shape" not in _rules(lint_content(rule))
|
||||
|
||||
|
||||
def test_references_sprawl_flagged_above_cap(tmp_path):
|
||||
from tools.skill_linter import _MAX_REFERENCE_FILES
|
||||
skill_dir = tmp_path / "my-skill"
|
||||
refs = skill_dir / "references"
|
||||
refs.mkdir(parents=True)
|
||||
for i in range(_MAX_REFERENCE_FILES + 1):
|
||||
(refs / f"note-{i}.md").write_text("x")
|
||||
(skill_dir / "SKILL.md").write_text(CLEAN)
|
||||
assert "references-sprawl" in _rules(lint_skill(skill_dir / "SKILL.md"))
|
||||
(refs / f"note-{_MAX_REFERENCE_FILES}.md").unlink()
|
||||
assert "references-sprawl" not in _rules(lint_skill(skill_dir / "SKILL.md"))
|
||||
|
||||
@@ -42,6 +42,13 @@ _FORBIDDEN_FILES = ("README.md", "CHANGELOG.md", "install.sh", ".env", ".env.exa
|
||||
# Presence of the load-bearing section is checked, not exact ordering, so the
|
||||
# linter is not a change-detector.
|
||||
_EXPECTED_SECTIONS = ("When to Use", "When to use")
|
||||
# incident-log-shape: at least this many PR/issue refs AND this density (per 1k chars of prose).
|
||||
_INCIDENT_REF_MIN = 4
|
||||
_INCIDENT_REF_PER_KCHAR = 0.5 # the 100k incident-log SKILL.md this targets sat at ~0.7
|
||||
# references-sprawl: a skill carrying more reference files than this is hoarding per-session notes.
|
||||
# Calibration: a deliberately curated large workflow skill sits near 50 topical files; the hoarding
|
||||
# shape this catches was 443 one-per-session files.
|
||||
_MAX_REFERENCE_FILES = 60
|
||||
|
||||
ERROR = "error"
|
||||
WARNING = "warning"
|
||||
@@ -123,6 +130,12 @@ def _check_body(body: str, skill_dir: Optional[Path]) -> Iterator[LintFinding]:
|
||||
if not any(re.search(rf"^#+\s+{re.escape(s)}", body, re.M) for s in _EXPECTED_SECTIONS):
|
||||
yield _warn("missing-section", "no '## When to Use' section found; skills need explicit "
|
||||
"trigger conditions near the top.")
|
||||
# Incident-log shape: a skill body dense in PR/issue numbers is narrating history instead of
|
||||
# stating rules. Threshold is per 1k chars so a long body with one citation is fine.
|
||||
refs = len(re.findall(r"(?<![\w/])#\d{3,6}\b|\b(?:PR|issue)\s*#?\d{3,6}\b", prose))
|
||||
if refs >= _INCIDENT_REF_MIN and refs / max(len(prose), 1) * 1000 >= _INCIDENT_REF_PER_KCHAR:
|
||||
yield _warn("incident-log-shape", f"{refs} PR/issue references in prose; write the generalizable "
|
||||
"rule + why and drop the incident numbers — the rule must stand without the story.")
|
||||
if skill_dir is None:
|
||||
return
|
||||
# Dangling links. Only references/, templates/, assets/ are reliably skill-owned;
|
||||
@@ -162,6 +175,13 @@ def _check_files(frontmatter: Dict[str, Any], skill_dir: Path) -> Iterator[LintF
|
||||
if (skill_dir / fname).exists():
|
||||
yield _warn("forbidden-file",
|
||||
f"skill ships '{fname}'; skills should not include scaffolding/config files.")
|
||||
refs_dir = skill_dir / "references"
|
||||
if refs_dir.is_dir():
|
||||
n_refs = sum(1 for p in refs_dir.rglob("*.md") if not any(part.startswith("_") for part in p.parts))
|
||||
if n_refs > _MAX_REFERENCE_FILES:
|
||||
yield _warn("references-sprawl",
|
||||
f"{n_refs} files under references/; that is a per-session log, not topical depth. "
|
||||
"Merge same-topic files into one rule set and drop incident narration.")
|
||||
|
||||
|
||||
def lint_content(content: str, *, skill_dir: Optional[Path] = None) -> List[LintFinding]:
|
||||
|
||||
@@ -541,8 +541,13 @@ def _write_file(name: str, file_path: str, file_content: str) -> Dict[str, Any]:
|
||||
target, err = _resolve_supporting_file(skill_dir, file_path)
|
||||
if guard := err or _guarded_write(name, skill_dir, target, "write_file", file_path, file_content):
|
||||
return guard
|
||||
return _attach_org_note({"success": True, "message": f"File '{file_path}' written to skill '{name}'.",
|
||||
"path": str(target)}, name, skill_dir)
|
||||
result = _attach_org_note({"success": True, "message": f"File '{file_path}' written to skill '{name}'.",
|
||||
"path": str(target)}, name, skill_dir)
|
||||
# references/ is where per-session hoarding shows up; surface the sprawl finding on the write
|
||||
# that crosses the line so the review fork sees it in the same turn.
|
||||
if file_path.startswith("references/") and (skill_dir / "SKILL.md").exists():
|
||||
_attach_lint_findings(result, skill_dir / "SKILL.md")
|
||||
return result
|
||||
|
||||
|
||||
def _remove_file(name: str, file_path: str) -> Dict[str, Any]:
|
||||
@@ -790,8 +795,10 @@ SKILL_MANAGE_SCHEMA = {
|
||||
"first), write_file/remove_file (supporting files), delete (sole "
|
||||
"op only). Existing skills are modified wherever they live. Keep "
|
||||
"the description's first 57 chars a self-contained trigger: 'Use "
|
||||
"when <trigger>. <one-line behavior>.' — skill_view() shows "
|
||||
"format conventions."
|
||||
"when <trigger>. <one-line behavior>.' Write lessons, not logs: "
|
||||
"imperative rule + why, no PR numbers/dates/incident narration, one "
|
||||
"rule per lesson, references/ named by topic (extend before adding). "
|
||||
"skill_view() shows format conventions."
|
||||
),
|
||||
"parameters": {
|
||||
"type": "object",
|
||||
|
||||
@@ -578,6 +578,22 @@ future reuse. In practice that covers:
|
||||
- When it hit errors or dead ends and found the working path
|
||||
- When the user corrected its approach
|
||||
|
||||
### What a skill entry looks like
|
||||
|
||||
Skills capture **lessons, not logs**. Whether written in a foreground turn, by the
|
||||
background review, or by the curator's consolidation pass, an entry is a generalizable
|
||||
rule plus one clause of *why* (the mechanism), stated once. Incident narration, PR or
|
||||
issue numbers, dates, and quoted chat are not skill content; the rule has to stand
|
||||
without the story behind it. Always-on rules live in `SKILL.md` itself; `references/`
|
||||
holds a small set of files named by topic (a decision table, a recipe, provider quirks),
|
||||
extended in place rather than accumulated one file per session. Skills also do not
|
||||
restate what is already loaded every turn (the repo's `AGENTS.md`, tool schemas).
|
||||
|
||||
`skill_manage` runs an advisory linter on `create` and on `references/` writes and
|
||||
returns its findings in the tool result. Two rules exist specifically for this shape:
|
||||
`incident-log-shape` (a body dense in PR/issue numbers) and `references-sprawl` (more
|
||||
than 60 reference files). They warn; they never block a write.
|
||||
|
||||
### Actions
|
||||
|
||||
| Action | Use for | Key params |
|
||||
|
||||
Reference in New Issue
Block a user