From c240e6539963251a82ddcf075a16d752ac791aa6 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 4 Sep 2026 07:13:23 -0700 Subject: [PATCH] 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. --- agent/background_review.py | 51 ++++++++++++++----- agent/curator.py | 23 ++++++--- .../test_review_prompt_class_first.py | 25 +++++++++ tests/tools/test_skill_linter.py | 28 ++++++++++ tools/skill_linter.py | 20 ++++++++ tools/skill_manager_tool.py | 15 ++++-- website/docs/user-guide/features/skills.md | 16 ++++++ 7 files changed, 154 insertions(+), 24 deletions(-) diff --git a/agent/background_review.py b/agent/background_review.py index 6342c17579..18c97e8e6d 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -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 " + "'-.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/.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/.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/.` — starter files meant to be copied and modified (boilerplate " "configs, scaffolding, a known-good example the agent can `reproduce with modifications`).\n" " • `scripts/.` — 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/.md` for session-specific detail OR condensed knowledge banks " - "(quoted research, API docs excerpts, domain notes) written concise and task-focused; " + "kinds: `references/.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/.` for starter files meant to be copied and modified; " "`scripts/.` for statically re-runnable actions (verification, fixture generators, " "probes). Add a one-line pointer in SKILL.md so future agents find them.\n" diff --git a/agent/curator.py b/agent/curator.py index 1b5821dbeb..670abfb71d 100644 --- a/agent/curator.py +++ b/agent/curator.py @@ -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/.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/.md` — named by TOPIC, merged into an " + "existing topical file when one covers it (decision tables, recipes, " + "provider quirks, condensed domain notes). Never `.md` " + "copied verbatim; never a per-incident file.\n" " • `templates/.` for starter files meant to be " "copied and modified\n" " • `scripts/.` for statically re-runnable actions " diff --git a/tests/run_agent/test_review_prompt_class_first.py b/tests/run_agent/test_review_prompt_class_first.py index e47b6e1977..1c373e43e9 100644 --- a/tests/run_agent/test_review_prompt_class_first.py +++ b/tests/run_agent/test_review_prompt_class_first.py @@ -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/" diff --git a/tests/tools/test_skill_linter.py b/tests/tools/test_skill_linter.py index 538bcd9a2a..721441766f 100644 --- a/tests/tools/test_skill_linter.py +++ b/tests/tools/test_skill_linter.py @@ -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")) diff --git a/tools/skill_linter.py b/tools/skill_linter.py index e46de3806d..061404d2b4 100644 --- a/tools/skill_linter.py +++ b/tools/skill_linter.py @@ -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"(?= _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]: diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index d090222b6e..81d64b909b 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -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 . .' — skill_view() shows " - "format conventions." + "when . .' 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", diff --git a/website/docs/user-guide/features/skills.md b/website/docs/user-guide/features/skills.md index d9b81d518e..59e1484304 100644 --- a/website/docs/user-guide/features/skills.md +++ b/website/docs/user-guide/features/skills.md @@ -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 |