From 08d92e84dbe0114dc3e4792862dcaff39a169319 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Mon, 9 Nov 2026 14:43:07 +0000 Subject: [PATCH 1/8] Check agent skills in qualityChecks (PROF-16542) Add utils/check-skills.py to qualityChecks. Pi silently skips a SKILL.md with malformed frontmatter or without a description, so a broken skill never loads and nothing reports it. The check fails on: - a skill directory without SKILL.md, or without a `---` frontmatter block; - a missing name or description, a name that differs from its directory or breaks the naming rule, a description over 1024 characters, or a YAML block scalar the line parser cannot read; - a .claude/skills entry that is not the symlink to ../.agents/skills, through which Claude Code reads the skills; - an AGENTS.md "`` skill" mention without a matching skill. Hermetic tests are T8 in test_quality_check_scripts.sh. Co-Authored-By: Claude Opus 5.5 --- ddprof-lib/build.gradle.kts | 1 + utils/README.md | 1 + utils/check-skills.py | 142 ++++++++++++++++++++++ utils/tests/test_quality_check_scripts.sh | 81 +++++++++++- 4 files changed, 224 insertions(+), 1 deletion(-) create mode 100755 utils/check-skills.py diff --git a/ddprof-lib/build.gradle.kts b/ddprof-lib/build.gradle.kts index adfe9b2708..c9ae5c0e8e 100644 --- a/ddprof-lib/build.gradle.kts +++ b/ddprof-lib/build.gradle.kts @@ -518,6 +518,7 @@ tasks.register("qualityChecks") { "check-copyright.py", "check-claude-md.py", "check-knowledgebase.py", + "check-skills.py", ).map { File(root, "utils/$it") } doLast { diff --git a/utils/README.md b/utils/README.md index 4d80313e04..c5e4ad0817 100644 --- a/utils/README.md +++ b/utils/README.md @@ -232,6 +232,7 @@ Standalone, stdlib-only checks over the git-tracked files. Each prints one `.gh-merge-base` for a branch stacked on another feature branch. diff --git a/utils/check-skills.py b/utils/check-skills.py new file mode 100755 index 0000000000..251ff3d111 --- /dev/null +++ b/utils/check-skills.py @@ -0,0 +1,142 @@ +#!/usr/bin/env python3 +# Copyright 2026, Datadog, Inc. +"""Check that every agent skill under .agents/skills/ loads, and that AGENTS.md names only +existing skills. + +Pi silently skips a SKILL.md with malformed frontmatter or without a description, so a +broken skill would otherwise go unnoticed. Rules, per tracked `.agents/skills//`: +a SKILL.md with a leading `---` block; a single-line `name` equal to , made of +lowercase letters, digits and single hyphens, at most 64 characters; a non-empty +single-line `description` of at most 1024 characters. Block scalars (`>`, `|`) are +rejected because the line parser here cannot read them. `.claude/skills` must be the +symlink to `../.agents/skills` that Claude Code reads them through. Every "`` skill" +mention in AGENTS.md must name a skill directory. + +Exit codes: 0 clean, 1 violations, 2 environment error. +""" + +import argparse +import os +import re +import subprocess +import sys + +SKILLS = ".agents/skills/" +LINK, LINK_TARGET = ".claude/skills", "../.agents/skills" +NAME = re.compile(r"[a-z0-9]+(-[a-z0-9]+)*") +MAX_NAME, MAX_DESCRIPTION = 64, 1024 +POINTER = re.compile(r"`([^`\s]+)` skill\b") + + +class EnvError(Exception): + pass + + +def git(root, *args): + res = subprocess.run(["git", "-C", root, *args], capture_output=True, text=True, + errors="surrogateescape") + if res.returncode != 0: + raise EnvError("git %s failed: %s" % (args[0], res.stderr.strip())) + return res.stdout + + +def skill_dirs(root): + """{dir name: tracked file names directly in it} for each tracked skill directory.""" + dirs = {} + for path in git(root, "ls-files", "-z", "--", SKILLS).split("\0"): + parts = path[len(SKILLS):].split("/") if path else [] + if len(parts) >= 2: + dirs.setdefault(parts[0], set()).add(parts[1]) + return dirs + + +def frontmatter(text): + """The leading `---` block's `key: value` lines, or None without a closed block.""" + lines = [l.rstrip() for l in text.splitlines()] + if not lines or lines[0] != "---" or "---" not in lines[1:]: + return None + fields = {} + for line in lines[1:lines.index("---", 1)]: + if line and not line[0].isspace() and not line.startswith("#") and ":" in line: + key, _, value = line.partition(":") + value = value.strip() + if len(value) >= 2 and value[0] == value[-1] and value[0] in "'\"": + value = value[1:-1] + fields[key.strip()] = value + return fields + + +def skill_problems(root, name, files): + path = SKILLS + name + "/SKILL.md" + if "SKILL.md" not in files: + return ["%s: missing" % path] + try: + with open(os.path.join(root, path), encoding="utf-8", errors="replace") as f: + fm = frontmatter(f.read()) + except OSError as e: + return ["%s: cannot read: %s" % (path, e.strerror)] + if fm is None: + return ["%s: no `---` frontmatter block" % path] + found = [] + declared, description = fm.get("name", ""), fm.get("description", "") + if declared in (">", "|", ">-", "|-") or description[:1] in (">", "|"): + found.append("%s: use single-line values, not YAML block scalars" % path) + elif not declared: + found.append("%s: frontmatter 'name' is required" % path) + elif declared != name: + found.append("%s: name '%s' differs from its directory '%s'" % (path, declared, name)) + elif not NAME.fullmatch(declared) or len(declared) > MAX_NAME: + found.append("%s: name '%s' must be lowercase letters, digits and single hyphens, " + "at most %d characters" % (path, declared, MAX_NAME)) + if not description: + found.append("%s: frontmatter 'description' is required; without one the skill " + "never loads" % path) + elif len(description) > MAX_DESCRIPTION: + found.append("%s: description has %d characters, at most %d load" + % (path, len(description), MAX_DESCRIPTION)) + return found + + +def link_problems(root): + entry = git(root, "ls-files", "-s", "--", LINK).split() + if not entry or entry[0] != "120000": + return ["%s: must be a tracked symlink to %s" % (LINK, LINK_TARGET)] + target = git(root, "cat-file", "-p", entry[1]) + if target != LINK_TARGET: + return ["%s: points to '%s', expected %s" % (LINK, target, LINK_TARGET)] + return [] + + +def pointer_problems(root, names): + try: + with open(os.path.join(root, "AGENTS.md"), encoding="utf-8", errors="replace") as f: + text = f.read() + except OSError as e: + raise EnvError("cannot read AGENTS.md: %s" % e.strerror) + missing = sorted({m for m in POINTER.findall(text) if m not in names}) + return ["AGENTS.md: names the `%s` skill, but %s%s/ does not exist" % (m, SKILLS, m) + for m in missing] + + +def main(argv): + ap = argparse.ArgumentParser(description=__doc__.split("\n\n")[0]) + ap.add_argument("--root", default=None, + help="repository root (default: the checkout containing this script)") + args = ap.parse_args(argv) + root = args.root or os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + try: + dirs = skill_dirs(root) + errors = [e for name in sorted(dirs) for e in skill_problems(root, name, dirs[name])] + errors += link_problems(root) + pointer_problems(root, dirs) + except EnvError as e: + print("error: %s" % e, file=sys.stderr) + return 2 + for e in errors: + print(e) + print("skills: %s" % ("%d problem(s)" % len(errors) if errors else + "OK (%d skills)" % len(dirs))) + return 1 if errors else 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/utils/tests/test_quality_check_scripts.sh b/utils/tests/test_quality_check_scripts.sh index 70d8e5448b..a666a08c73 100755 --- a/utils/tests/test_quality_check_scripts.sh +++ b/utils/tests/test_quality_check_scripts.sh @@ -363,13 +363,16 @@ if [ "$n" -lt 2 ]; then fail "T4 RefCountGuard.md mentions REFCOUNT_NESTED_OVERF rc=0; out=$(python3 "$ROOT/utils/check-claude-md.py" 2>&1) || rc=$? expect "T4 tracked CLAUDE.md is canonical" 0 "$rc" "$out" "CLAUDE.md: OK" -for s in check-copyright.py check-claude-md.py check-knowledgebase.py; do +for s in check-copyright.py check-claude-md.py check-knowledgebase.py check-skills.py; do if grep -q "\"$s\"" "$GRADLE"; then pass; else fail "T4 qualityChecks does not run $s"; fi done rc=0; out=$(python3 "$ROOT/utils/check-knowledgebase.py" 2>&1) || rc=$? expect "T4 tracked memories carry cairn frontmatter" 0 "$rc" "$out" "knowledgebase: OK" +rc=0; out=$(python3 "$ROOT/utils/check-skills.py" 2>&1) || rc=$? +expect "T4 tracked skills load and AGENTS.md names only existing skills" 0 "$rc" "$out" "skills: OK" + # --- T5: check-copyright.py over a branch's added and modified files --------- CR="$ROOT/utils/check-copyright.py" @@ -607,6 +610,82 @@ for opener in '---garbage' '----'; do kb; expect "T7o delimiter '$opener' is not a frontmatter line" 1 "$rc" "$out" "no \`---\` frontmatter block" done +# --- T8: check-skills.py -------------------------------------------------------- + +SK="$ROOT/utils/check-skills.py" +SK_ROOT="$TEMP_DIR/skills" +skill() { # skill : writes and tracks one SKILL.md + mkdir -p "$SK_ROOT/.agents/skills/$1" + local f="$SK_ROOT/.agents/skills/$1/SKILL.md"; shift + { echo ---; printf '%s\n' "$@"; echo ---; echo; echo body; } > "$f" + git -C "$SK_ROOT" add -f "$f" +} +sk() { rc=0; out=$(python3 "$SK" --root "$SK_ROOT" 2>&1) || rc=$?; } +sk_reset() { # a valid tree: one skill, the .claude/skills symlink, an AGENTS.md pointer + rm -rf "$SK_ROOT" && mkdir -p "$SK_ROOT/.agents/skills" "$SK_ROOT/.claude" + git -C "$SK_ROOT" init -q + ln -s ../.agents/skills "$SK_ROOT/.claude/skills" + echo 'Run the `good-one` skill.' > "$SK_ROOT/AGENTS.md" + git -C "$SK_ROOT" add -f .claude/skills AGENTS.md + skill good-one "name: good-one" "description: Does a thing. Use when a thing is needed." +} +unlink_skills() { git -C "$SK_ROOT" rm -q --cached .claude/skills && rm "$SK_ROOT/.claude/skills"; } +sk_reset +sk; expect "T8a valid skill, symlink and pointer" 0 "$rc" "$out" "skills: OK (1 skills)" +sk_reset; skill quoted "name: 'quoted'" 'description: "Quoted values load too."' +sk; expect "T8b quoted values" 0 "$rc" "$out" "skills: OK (2 skills)" +sk_reset; printf 'no frontmatter\n' > "$SK_ROOT/.agents/skills/good-one/SKILL.md" +sk; expect "T8c SKILL.md without frontmatter" 1 "$rc" "$out" "good-one/SKILL.md: no \`---\` frontmatter block" +sk_reset; printf -- '---\nname: good-one\ndescription: x\n' > "$SK_ROOT/.agents/skills/good-one/SKILL.md" +sk; expect "T8d unclosed frontmatter" 1 "$rc" "$out" "no \`---\` frontmatter block" +sk_reset; skill good-one "name: good-one" +sk; expect "T8e missing description" 1 "$rc" "$out" "frontmatter 'description' is required" +sk_reset; skill good-one "name: good-one" 'description: ""' +sk; expect "T8f empty quoted description" 1 "$rc" "$out" "frontmatter 'description' is required" +sk_reset; skill good-one "description: x" +sk; expect "T8g missing name" 1 "$rc" "$out" "frontmatter 'name' is required" +sk_reset; skill good-one "name: other-one" "description: x" +sk; expect "T8h name differs from directory" 1 "$rc" "$out" "name 'other-one' differs from its directory 'good-one'" +for bad in Bad_Name bad--name -bad bad-; do + sk_reset; skill "$bad" "name: $bad" "description: x" + sk; expect "T8i invalid name '$bad'" 1 "$rc" "$out" "name '$bad' must be lowercase letters" +done +long=$(printf 'a%.0s' $(seq 65)) +sk_reset; skill "$long" "name: $long" "description: x" +sk; expect "T8j name of 65 characters" 1 "$rc" "$out" "at most 64 characters" +sk_reset; skill "${long:1}" "name: ${long:1}" "description: x" +sk; expect "T8j name of 64 characters" 0 "$rc" "$out" "skills: OK (2 skills)" +sk_reset; skill good-one "name: good-one" "description: $(printf 'd%.0s' $(seq 1025))" +sk; expect "T8k description of 1025 characters" 1 "$rc" "$out" "description has 1025 characters, at most 1024" +sk_reset; skill good-one "name: good-one" "description: $(printf 'd%.0s' $(seq 1024))" +sk; expect "T8k description of 1024 characters" 0 "$rc" "$out" "skills: OK (1 skills)" +for scalar in '>' '|-'; do + sk_reset; skill good-one "name: good-one" "description: $scalar" " Folded text." + sk; expect "T8l block-scalar description '$scalar'" 1 "$rc" "$out" "use single-line values" +done +sk_reset; mkdir -p "$SK_ROOT/.agents/skills/no-skill" && echo x > "$SK_ROOT/.agents/skills/no-skill/notes.md" +git -C "$SK_ROOT" add -f .agents/skills/no-skill/notes.md +sk; expect "T8m skill directory without SKILL.md" 1 "$rc" "$out" "no-skill/SKILL.md: missing" +sk_reset; mkdir -p "$SK_ROOT/.agents/skills/draft" && echo x > "$SK_ROOT/.agents/skills/draft/SKILL.md" +sk; expect "T8n untracked skill directories are ignored" 0 "$rc" "$out" "skills: OK (1 skills)" +sk_reset; rm "$SK_ROOT/.agents/skills/good-one/SKILL.md" +sk; expect "T8o tracked SKILL.md missing from the working tree" 1 "$rc" "$out" "good-one/SKILL.md: cannot read" +sk_reset; echo 'Run the `good-one` skill, then the `gone-one` skill.' > "$SK_ROOT/AGENTS.md" +sk; expect "T8p AGENTS.md names a missing skill" 1 "$rc" "$out" "names the \`gone-one\` skill, but .agents/skills/gone-one/ does not exist" +sk_reset; echo 'The cairn-memory skill and `code` spans are not pointers.' > "$SK_ROOT/AGENTS.md" +sk; expect "T8q mentions without a backticked skill name" 0 "$rc" "$out" "skills: OK" +sk_reset; unlink_skills +sk; expect "T8r .claude/skills symlink missing" 1 "$rc" "$out" ".claude/skills: must be a tracked symlink" +sk_reset; unlink_skills; ln -s ../skills "$SK_ROOT/.claude/skills" && git -C "$SK_ROOT" add -f .claude/skills +sk; expect "T8s .claude/skills symlink points elsewhere" 1 "$rc" "$out" "points to '../skills', expected ../.agents/skills" +sk_reset; unlink_skills; mkdir "$SK_ROOT/.claude/skills" && echo x > "$SK_ROOT/.claude/skills/f" +git -C "$SK_ROOT" add -f .claude/skills +sk; expect "T8t .claude/skills is a directory, not a symlink" 1 "$rc" "$out" ".claude/skills: must be a tracked symlink" +sk_reset; rm "$SK_ROOT/AGENTS.md" +sk; expect "T8u AGENTS.md missing" 2 "$rc" "$out" "error: cannot read AGENTS.md" +rc=0; out=$(python3 "$SK" --root "$TEMP_DIR/nonexistent" 2>&1) || rc=$? +expect "T8v environment error outside a checkout" 2 "$rc" "$out" "error: git ls-files failed" + # --- Summary ----------------------------------------------------------------- if [ "$FAILED" -gt 0 ]; then From 3836d72a41f54119178a500a2c6ece5782904868 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Mon, 9 Nov 2026 14:43:16 +0000 Subject: [PATCH 2/8] Move PR, comment, commit and doc writing rules into skills (PROF-16542) AGENTS.md loads into every session, but these rules apply only while an agent writes a PR description, a comment, a commit message or a doc page. The pr-writing and doc-writing skills now carry them and load at that moment. Each AGENTS.md section keeps a one-line pointer that names the moment and the skill. What stays in AGENTS.md: - the header now lists code and code-comment rules as inline, instead of all writing rules for PRs, comments and commits; - "never commit design artifacts" and "no scratch files": they apply at every commit, whether or not a skill is loaded; - the ban on citing plans, phases and review reports moves into the Comments bullet and covers doc/ too, replacing its copy in the documentation rules; - "update this file from repeated failures" moves to the header, because it maintains AGENTS.md itself. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/doc-writing/SKILL.md | 24 +++++++++ .agents/skills/pr-writing/SKILL.md | 45 +++++++++++++++++ AGENTS.md | 78 +++++++---------------------- 3 files changed, 87 insertions(+), 60 deletions(-) create mode 100644 .agents/skills/doc-writing/SKILL.md create mode 100644 .agents/skills/pr-writing/SKILL.md diff --git a/.agents/skills/doc-writing/SKILL.md b/.agents/skills/doc-writing/SKILL.md new file mode 100644 index 0000000000..0568aadb08 --- /dev/null +++ b/.agents/skills/doc-writing/SKILL.md @@ -0,0 +1,24 @@ +--- +name: doc-writing +description: Rules for java-profiler documentation under doc/ (naming, index, accuracy, stale sections). Use before creating, editing, renaming or deleting a file under doc/. +--- + +# Documentation writing + +- **Naming.** Files in `doc/` use PascalCase (e.g. `BuildSystemGuide.md`); + `doc/README.md` is the only lowercase exception. New subsystem docs go in + `doc/architecture/` or `doc/reference/`; build docs in `doc/building/`. +- **Index.** Every doc page is indexed in `doc/README.md`. CI (`qualityChecks`) + enforces naming and indexing; design artifacts under `docs/sphinx/`, `.sphinx/` and + similar scratch paths must never be tracked. +- **Current state only.** Documentation never carries transient or historical notes, + and never claims what is not true of the code. Verify numbers before writing them + down. +- **Remove finished work.** Preserved historical sections of landed design or handover + docs ("Suggested next steps", unverified hypotheses, open items) are rewritten or + deleted once the work they instruct is done: never point a reader at finished work. +- **Link, do not copy.** Link to deeper sources instead of copying large documents. +- The AGENTS.md **Comment budget** and **Comments** rules apply to doc paragraphs too: + no references to plans, phases, review reports or tickets. +- When a page changes a subsystem contract, update its row in the AGENTS.md subsystem + index in the same change. diff --git a/.agents/skills/pr-writing/SKILL.md b/.agents/skills/pr-writing/SKILL.md new file mode 100644 index 0000000000..8a3e85ba53 --- /dev/null +++ b/.agents/skills/pr-writing/SKILL.md @@ -0,0 +1,45 @@ +--- +name: pr-writing +description: Writing rules for java-profiler PR titles and descriptions, PR comments and review replies, Jira ticket comments, and git commit messages. Use before writing or editing any of these. +--- + +# PR, comment and commit writing + +These rules apply to PR descriptions and to every comment a session writes: PR +comments, review replies, Jira work-item comments. The template rule applies to +descriptions only. + +## PR descriptions and comments + +- **Follow the PR template strictly** (`.github/PULL_REQUEST_TEMPLATE.md`, descriptions + only): fill in every section it defines or mark it N/A explicitly; do not invent a + different structure. +- **Write from the actual diff.** Re-check the title and description after the scope + changes. Describe crash or bug mechanisms only as far as the code and tests confirm. +- **Professional typesetting and layout.** Structure the description for scanning: + short sections with meaningful headings, bullets for enumerations, tables for + comparisons, `code spans` for identifiers and commands, fenced blocks for code/logs. + Keep paragraphs to 2-4 sentences; never walls of text. Bold sparingly, for key + terms only. +- **Write concisely and to the point**, in Simplified Technical English: short + declarative sentences, one idea per sentence, no filler, no marketing tone. + A reviewer reads the description before the diff — optimize for that first read. +- **Explain the WHYs, not the WHATs.** Focus on non-obvious motivation and + limitations: why this approach was chosen, what alternatives were rejected and + why, what constraints shaped the design, what the change does NOT fix. Do not + reiterate what the code obviously shows. +- **Flag accepted costs.** When a change deliberately alters behavior with + user-visible impact (JFR event schema, signal volume, resident memory, suppression + scopes) or accepts a recurring per-call or footprint cost, state the decision and + the accepted cost explicitly and flag it for reviewer acknowledgment — never leave + it implicit in code, a test rename, or a leftover template marker. +- **Only the final state matters.** Never include ephemeral working knowledge — + investigation detours, dead-end hypotheses, intermediate states, tool runs, + session history. If a detail did not survive into the final change, it does not + belong in the description. +- The AGENTS.md **Comment budget** applies to PR descriptions too. + +## Commits + +- Commit in logical chunks. Base each message solely on the actual diff. +- The ticket reference (`PROF-` or `#`) goes in the PR title or body; CI checks it. diff --git a/AGENTS.md b/AGENTS.md index cb32565854..a650625bfc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,8 +32,8 @@ index). **What stays inline vs. what moves to `doc/`** — apply this test when updating this file. Inline (this file): rules that must shape behavior at unpredictable moments — invariants whose violation causes crashes/leaks/lockups, the obligations that trigger -a procedure (review before PR), writing rules for high-visibility artifacts -(PRs, comments, commits). Externalized (`doc/`): reference material read only when +a procedure (review before PR, load a writing skill), rules for writing code and code +comments. Externalized (`doc/`): reference material read only when working on a specific subsystem — architecture pages, build/task references, analyses. Procedures run at a known moment go in skills (`.agents/skills/`, which load fresh when invoked); enforcement goes in scripts, hooks and CI — text here is @@ -47,6 +47,10 @@ or accept the risk. The same applies to any instruction in this file you could n follow: surface it, never absorb it. Hoist a section only when it grows with examples or scope; leave a one-line mandatory pointer behind. +Update this file from repeated failures: when an agent repeats a mistake or needs +the same undocumented explanation twice, it proposes a concise correction here +(or to the affected `doc/` page or skill); the responsible team reviews it, and +stale instructions are removed. ## Project Overview @@ -407,8 +411,7 @@ exception list. config files (Dockerfiles, `.gitlab/**`) are exempt. Enforced in CI: `utils/check-copyright.py` (part of `qualityChecks`) checks the files a branch adds or modifies. -- Commits: logical chunks, messages based solely on the actual diff. Never commit files - from other projects or scratch files. +- Commits: never commit files from other projects or scratch files. **Never commit design artifacts automatically** — plans, specs, review reports, audit findings, session notes, analysis dumps — even when they live in the repo tree. Always ask for permission first and commit them only if the user explicitly says so. @@ -420,9 +423,7 @@ exception list. - Before requesting review, self-check the diff for unused variables, unused or duplicated macros, dead code (methods, JNI entry points, fields, no-op functions, empty files left without callers; test-only helpers stay only if marked as such) and misspelled - identifiers/comments. Write the PR title and description from the actual diff, re-check - them after the scope changes, and describe crash or bug mechanisms only as far as the - code and tests confirm. + identifiers/comments. - C/C++ style: match the surrounding indentation and spacing exactly; don't split inline logic into helpers unless reused or genuinely hard to follow; extract code that appears a second time (e.g. in copy constructors, assignment operators) into one shared helper; use @@ -442,7 +443,8 @@ exception list. - Comments: "why" comments must reference concrete mechanisms (e.g. "ASAN's allocator lock is reentrant"), never speculative claims about HotSpot/glibc/ASAN/ISA internals — if unsure, describe the observable symptom. Describe current behavior, not history; cite no - tickets, plan documents, design docs or line numbers (refer to symbols). Name each + tickets, plan documents, design docs, review reports, "phase N" or line numbers (refer + to symbols) — in comments and in `doc/` alike. Name each thread or actor explicitly. Scope words ("only", "every", "never blocks", "single-threaded", "no lock needed") and invariants (e.g. "always leaves a terminator") must hold on every branch and for every caller thread, including early returns, timeouts and `siglongjmp`. @@ -513,36 +515,11 @@ a branch with an open PR (install with `utils/review-marker.py install-hook`, ne via `core.hooksPath`), and in Claude Code and Pi (`.pi/extensions/`) the `require-fresh-review` hook blocks `gh pr create`/`gh pr ready`. -## Pull Request Descriptions and Comments - -These rules apply to PR descriptions and to every comment a session writes — PR -comments, review replies, Jira work-item comments. Only the PR-template rule is -description-specific: - -- **Follow the PR template strictly** (descriptions only) — every section it - defines is filled in or explicitly marked N/A; do not invent a different - structure. -- **Professional typesetting and layout.** Structure the description for scanning: - short sections with meaningful headings, bullets for enumerations, tables for - comparisons, `code spans` for identifiers and commands, fenced blocks for code/logs. - Keep paragraphs to 2-4 sentences; never walls of text. Bold sparingly, for key - terms only. -- **Write concisely and to the point**, in Simplified Technical English: short - declarative sentences, one idea per sentence, no filler, no marketing tone. - A reviewer reads the description before the diff — optimize for that first read. -- **Explain the WHYs, not the WHATs.** Focus on non-obvious motivation and - limitations: why this approach was chosen, what alternatives were rejected and - why, what constraints shaped the design, what the change does NOT fix. Do not - reiterate what the code obviously shows. When a change deliberately alters - behavior with user-visible impact (JFR event schema, signal volume, resident - memory, suppression scopes) or accepts a recurring per-call or footprint cost, - state the decision and the accepted cost explicitly and flag it for reviewer - acknowledgment — never leave it implicit in code, a test rename, or a leftover - template marker in the description. -- **Only the final state matters.** Never include ephemeral working knowledge — - investigation detours, dead-end hypotheses, intermediate states, tool runs, - session history. If a detail did not survive into the final change, it does not - belong in the description. +## Pull Request Descriptions, Comments and Commits + +Before writing or editing a PR title or description, a PR comment or review reply, a +Jira comment or a commit message, load the `pr-writing` skill +([.agents/skills/pr-writing/SKILL.md](.agents/skills/pr-writing/SKILL.md)). ## CI / Automation Script Changes @@ -562,25 +539,6 @@ description-specific: ## Documentation Rules -- All documentation files in `doc/` use PascalCase naming (e.g. `BuildSystemGuide.md`); - `doc/README.md` is the only lowercase exception. New subsystem docs go in - `doc/architecture/` or `doc/reference/`; build docs in `doc/building/`. -- Every doc page must be indexed in [doc/README.md](doc/README.md). Both rules are - enforced in CI by the `qualityChecks` task; design artifacts under `docs/sphinx/`, - `.sphinx/`, and similar scratch paths must never be tracked. -- Documentation reflects the current state — never transient/historical notes, and never - claims that aren't true of the code. Verify numbers before writing them down. Preserved - historical sections of landed design/handover docs ("Suggested next steps", unverified - hypotheses, open items) must be rewritten or deleted once the work they instruct is done: - a reader must never be pointed at already-finished work. -- Update this file from repeated failures: when an agent repeats a mistake or needs - the same undocumented explanation twice, it proposes a concise correction here - (or to the affected `doc/` page); the responsible team reviews it, and stale - instructions are removed. Link to deeper sources instead of copying large - documents into this file. -- Never mention internal plan/design/implementation phases or artifacts in docs or code - comments — no references to plan files, task lists, review/audit reports, "phase N", - "as designed in…", "this used to…". They are meaningless outside the session that - produced them. State the current behavior, the concrete mechanism, and (where - warranted) the incident or constraint that motivates it — nothing about how the - change came to be. +Before creating, editing, renaming or deleting a file under `doc/`, load the +`doc-writing` skill ([.agents/skills/doc-writing/SKILL.md](.agents/skills/doc-writing/SKILL.md)). +CI (`qualityChecks`) checks doc naming and the `doc/README.md` index. From 1cafd2915e369fade0dc33c069453cda6cf0c77b Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Mon, 9 Nov 2026 14:58:42 +0000 Subject: [PATCH 3/8] Cover the frontmatter parser edge cases in check-skills tests (PROF-16542) Add T8w-T8ad. Each case fails when its parser rule changes: - double quotes are stripped before the description length check; - frontmatter must start on the first line, and ends at its closing `---`; - indented lines are not top-level fields; - the first colon splits key and value; - a block-scalar name is rejected; - files directly under .agents/skills/ are not skills; - names may contain digits. Co-Authored-By: Claude Opus 5.5 --- utils/tests/test_quality_check_scripts.sh | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/utils/tests/test_quality_check_scripts.sh b/utils/tests/test_quality_check_scripts.sh index a666a08c73..5d3038ca57 100755 --- a/utils/tests/test_quality_check_scripts.sh +++ b/utils/tests/test_quality_check_scripts.sh @@ -685,6 +685,22 @@ sk_reset; rm "$SK_ROOT/AGENTS.md" sk; expect "T8u AGENTS.md missing" 2 "$rc" "$out" "error: cannot read AGENTS.md" rc=0; out=$(python3 "$SK" --root "$TEMP_DIR/nonexistent" 2>&1) || rc=$? expect "T8v environment error outside a checkout" 2 "$rc" "$out" "error: git ls-files failed" +sk_reset; skill good-one "name: good-one" "description: \"$(printf 'd%.0s' $(seq 1024))\"" +sk; expect "T8w double quotes do not count toward the description limit" 0 "$rc" "$out" "skills: OK (1 skills)" +sk_reset; printf 'intro\n---\nname: good-one\ndescription: x\n---\n' > "$SK_ROOT/.agents/skills/good-one/SKILL.md" +sk; expect "T8x frontmatter must start on the first line" 1 "$rc" "$out" "no \`---\` frontmatter block" +sk_reset; printf -- '---\nname: good-one\n---\ndescription: in the body\n' > "$SK_ROOT/.agents/skills/good-one/SKILL.md" +sk; expect "T8y body lines are not frontmatter" 1 "$rc" "$out" "frontmatter 'description' is required" +sk_reset; skill good-one "name: good-one" "metadata:" " description: nested" +sk; expect "T8z indented lines are not top-level fields" 1 "$rc" "$out" "frontmatter 'description' is required" +sk_reset; skill good-one "name: good-one" "description: Use when: a colon is in the value" +sk; expect "T8aa a colon inside the value" 0 "$rc" "$out" "skills: OK (1 skills)" +sk_reset; skill good-one "name: >" " good-one" "description: x" +sk; expect "T8ab block-scalar name" 1 "$rc" "$out" "use single-line values" +sk_reset; echo x > "$SK_ROOT/.agents/skills/README.md" && git -C "$SK_ROOT" add -f .agents/skills/README.md +sk; expect "T8ac files directly under .agents/skills/ are not skills" 0 "$rc" "$out" "skills: OK (1 skills)" +sk_reset; skill skill2 "name: skill2" "description: x" +sk; expect "T8ad digits in a name" 0 "$rc" "$out" "skills: OK (2 skills)" # --- Summary ----------------------------------------------------------------- From 6a35adcdf2eea09e0ed1d96aefbf99be630bbfb5 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Mon, 9 Nov 2026 16:00:35 +0000 Subject: [PATCH 4/8] Accept only a strict YAML subset in skill frontmatter (PROF-16542) check-skills.py read `key: value` lines without YAML rules and skipped lines it did not understand. It passed frontmatter that a YAML loader rejects, such as `description: Use when: x`, an unterminated quote, an unknown escape, `name:value` or a stray line. Pi silently drops such a skill: of two test skills, it listed only the one without `Use when: ...` in its description. The frontmatter must now follow a closed line grammar that needs no YAML library in CI: - every line is blank, a column-0 `#` comment, or `key: value` with a unique key; indented, list and continuation lines are errors; - every value is empty or a one-line string: a plain value that starts with a letter, uses only letters, digits, spaces, non-ASCII text and common punctuation, has no `: `, ` #` or trailing `:`, and is not a YAML null or boolean word; or a double-quoted string with only the \" and \\ escapes, or a single-quoted string with '' doubling; - non-ASCII text is limited to YAML's printable range, without U+2028, U+2029, the byte-order mark and U+FFFD; a file that is not valid UTF-8 is an error, and a tab after the colon gets its own message. Quoted values are decoded before the length check. Over the test values and blocks, the check never accepts what PyYAML rejects. It is stricter for nested mappings, multi-line values, a byte-order mark, and a few plain values YAML reads as strings, such as `1.2.3`; the error names the fix. Co-Authored-By: Claude Opus 5.5 --- utils/check-skills.py | 150 ++++++++++++++++------ utils/tests/test_quality_check_scripts.sh | 67 ++++++++-- 2 files changed, 168 insertions(+), 49 deletions(-) diff --git a/utils/check-skills.py b/utils/check-skills.py index 251ff3d111..76017dd4b1 100755 --- a/utils/check-skills.py +++ b/utils/check-skills.py @@ -3,14 +3,18 @@ """Check that every agent skill under .agents/skills/ loads, and that AGENTS.md names only existing skills. -Pi silently skips a SKILL.md with malformed frontmatter or without a description, so a -broken skill would otherwise go unnoticed. Rules, per tracked `.agents/skills//`: -a SKILL.md with a leading `---` block; a single-line `name` equal to , made of -lowercase letters, digits and single hyphens, at most 64 characters; a non-empty -single-line `description` of at most 1024 characters. Block scalars (`>`, `|`) are -rejected because the line parser here cannot read them. `.claude/skills` must be the -symlink to `../.agents/skills` that Claude Code reads them through. Every "`` skill" -mention in AGENTS.md must name a skill directory. +Pi silently skips a SKILL.md whose frontmatter does not parse as YAML or has no +description, so a broken skill would otherwise go unnoticed. Rules, per tracked +`.agents/skills//`: +- SKILL.md starts with a `---` block; +- every line in the block is blank, a column-0 `#` comment, or `key: value` with a unique + key, and every value is empty or a one-line string accepted by scalar(). This subset + needs no YAML library and is stricter than YAML: it has no nested mappings, lists or + multi-line values; +- `name` equals and uses lowercase letters, digits and single hyphens, at most 64 + characters; `description` is non-empty, at most 1024 characters. +`.claude/skills` must be the symlink to `../.agents/skills` that Claude Code reads the +skills through. Every "`` skill" mention in AGENTS.md must name a skill directory. Exit codes: 0 clean, 1 violations, 2 environment error. """ @@ -26,6 +30,17 @@ NAME = re.compile(r"[a-z0-9]+(-[a-z0-9]+)*") MAX_NAME, MAX_DESCRIPTION = 64, 1024 POINTER = re.compile(r"`([^`\s]+)` skill\b") +KEY_LINE = re.compile(r"([A-Za-z][A-Za-z0-9_-]*):(?: (.*))?") +# YAML's printable non-ASCII range, minus U+2028/U+2029 (line breaks to some tools), the +# byte-order mark and U+FFFD (the decode-error replacement character). +TEXT = "\u00a0-\u2027\u202a-\ud7ff\ue000-\ufefe\uff00-\ufffc\U00010000-\U0010ffff" +PLAIN_PUNCTUATION = ".,;:()'\"/_+=?!#*&%@<>~^$`-" +PLAIN = re.compile("[A-Za-z%s][A-Za-z0-9 %s%s]*" % (TEXT, TEXT, re.escape(PLAIN_PUNCTUATION))) +PRINTABLE = re.compile("[\t\x20-\x7e%s]*" % TEXT) +TAB_AFTER_KEY = re.compile(r"[A-Za-z][A-Za-z0-9_-]*:\t") +DOUBLE_QUOTED = re.compile(r'"((?:[^"\\]|\\["\\])*)"(?:\s+#.*)?') +SINGLE_QUOTED = re.compile(r"'((?:[^']|'')*)'(?:\s+#.*)?") +NON_STRING_WORDS = {"null", "true", "false", "yes", "no", "on", "off", "y", "n"} class EnvError(Exception): @@ -50,20 +65,84 @@ def skill_dirs(root): return dirs +def scalar(raw): + """(value, None) for a non-empty raw value in the accepted one-line string subset, else + (None, reason). Accepted: a plain value that starts with a letter, uses only letters, + digits, spaces, non-ASCII text and PLAIN_PUNCTUATION, has no `: `, ` #` or trailing + `:` and is not a YAML null or boolean word; or a double-quoted string whose only + escapes are \\" and \\\\, or a single-quoted string with '' doubling, either optionally + followed by a comment.""" + if raw[:1] in ("|", ">"): + return None, "block scalars are not supported; put the value on one line" + double, single = DOUBLE_QUOTED.fullmatch(raw), SINGLE_QUOTED.fullmatch(raw) + if double or single: + value = (re.sub(r'\\(["\\])', r"\1", double.group(1)) if double + else single.group(1).replace("''", "'")) + if not PRINTABLE.fullmatch(value): + return None, "it contains a control or non-printable character" + return value, None + if raw[:1] in ("'", '"'): + return None, ("the quoted string is unterminated, has an unescaped quote, or uses " + "an escape other than \\\" and \\\\") + if not PLAIN.fullmatch(raw): + return None, ("a plain value must start with a letter and contain only letters, " + "digits, spaces, non-ASCII text and %s; quote it" % PLAIN_PUNCTUATION) + if ": " in raw or " #" in raw or raw.endswith(":"): + return None, "it contains ': ' or ' #' or ends with ':', which YAML parses differently; quote it" + if raw.lower() in NON_STRING_WORDS: + return None, "YAML reads it as a null or boolean, not a string; quote it" + return raw, None + + def frontmatter(text): - """The leading `---` block's `key: value` lines, or None without a closed block.""" - lines = [l.rstrip() for l in text.splitlines()] + """(fields, problems) for the leading `---` block, or None without a closed block. + + fields maps each key to its decoded value, "" when empty, or None when the value was + rejected; problems lists every line outside the accepted subset.""" + lines = [l.rstrip() for l in text.split("\n")] if not lines or lines[0] != "---" or "---" not in lines[1:]: return None - fields = {} - for line in lines[1:lines.index("---", 1)]: - if line and not line[0].isspace() and not line.startswith("#") and ":" in line: - key, _, value = line.partition(":") - value = value.strip() - if len(value) >= 2 and value[0] == value[-1] and value[0] in "'\"": - value = value[1:-1] - fields[key.strip()] = value - return fields + fields, problems = {}, [] + for number, line in enumerate(lines[1:lines.index("---", 1)], start=2): + if not line or line.startswith("#"): + continue + match = KEY_LINE.fullmatch(line) + if not match and TAB_AFTER_KEY.match(line): + problems.append("line %d: use a space, not a tab, after the colon" % number) + continue + if not match: + problems.append("line %d: expected `key: value` at column 0; indented, list and " + "continuation lines are not supported" % number) + continue + key, raw = match.group(1), (match.group(2) or "").strip() + if key in fields: + problems.append("line %d: duplicate key '%s'" % (number, key)) + value, why = scalar(raw) if raw else ("", None) + if why: + problems.append("line %d: '%s' must be a one-line string: %s" % (number, key, why)) + fields[key] = value + return fields, problems + + +def name_problems(path, directory, declared): + if not declared: + return ["%s: frontmatter 'name' is required" % path] + if declared != directory: + return ["%s: name '%s' differs from its directory '%s'" % (path, declared, directory)] + if not NAME.fullmatch(declared) or len(declared) > MAX_NAME: + return ["%s: name '%s' must be lowercase letters, digits and single hyphens, " + "at most %d characters" % (path, declared, MAX_NAME)] + return [] + + +def description_problems(path, description): + if not description: + return ["%s: frontmatter 'description' is required; without one the skill never " + "loads" % path] + if len(description) > MAX_DESCRIPTION: + return ["%s: description has %d characters, at most %d load" + % (path, len(description), MAX_DESCRIPTION)] + return [] def skill_problems(root, name, files): @@ -71,29 +150,22 @@ def skill_problems(root, name, files): if "SKILL.md" not in files: return ["%s: missing" % path] try: - with open(os.path.join(root, path), encoding="utf-8", errors="replace") as f: - fm = frontmatter(f.read()) + with open(os.path.join(root, path), encoding="utf-8") as f: + parsed = frontmatter(f.read()) except OSError as e: return ["%s: cannot read: %s" % (path, e.strerror)] - if fm is None: + except UnicodeDecodeError as e: + return ["%s: not valid UTF-8 at byte %d" % (path, e.start)] + if parsed is None: return ["%s: no `---` frontmatter block" % path] - found = [] - declared, description = fm.get("name", ""), fm.get("description", "") - if declared in (">", "|", ">-", "|-") or description[:1] in (">", "|"): - found.append("%s: use single-line values, not YAML block scalars" % path) - elif not declared: - found.append("%s: frontmatter 'name' is required" % path) - elif declared != name: - found.append("%s: name '%s' differs from its directory '%s'" % (path, declared, name)) - elif not NAME.fullmatch(declared) or len(declared) > MAX_NAME: - found.append("%s: name '%s' must be lowercase letters, digits and single hyphens, " - "at most %d characters" % (path, declared, MAX_NAME)) - if not description: - found.append("%s: frontmatter 'description' is required; without one the skill " - "never loads" % path) - elif len(description) > MAX_DESCRIPTION: - found.append("%s: description has %d characters, at most %d load" - % (path, len(description), MAX_DESCRIPTION)) + fields, problems = parsed + found = ["%s: %s" % (path, p) for p in problems] + # A None value was rejected by scalar() and is already in problems. + declared, description = fields.get("name", ""), fields.get("description", "") + if declared is not None: + found += name_problems(path, name, declared) + if description is not None: + found += description_problems(path, description) return found diff --git a/utils/tests/test_quality_check_scripts.sh b/utils/tests/test_quality_check_scripts.sh index 5d3038ca57..9370077d5b 100755 --- a/utils/tests/test_quality_check_scripts.sh +++ b/utils/tests/test_quality_check_scripts.sh @@ -647,7 +647,7 @@ sk; expect "T8g missing name" 1 "$rc" "$out" "frontmatter 'name' is required" sk_reset; skill good-one "name: other-one" "description: x" sk; expect "T8h name differs from directory" 1 "$rc" "$out" "name 'other-one' differs from its directory 'good-one'" for bad in Bad_Name bad--name -bad bad-; do - sk_reset; skill "$bad" "name: $bad" "description: x" + sk_reset; skill "$bad" "name: \"$bad\"" "description: x" # quoted, so only the name rule applies sk; expect "T8i invalid name '$bad'" 1 "$rc" "$out" "name '$bad' must be lowercase letters" done long=$(printf 'a%.0s' $(seq 65)) @@ -661,7 +661,7 @@ sk_reset; skill good-one "name: good-one" "description: $(printf 'd%.0s' $(seq 1 sk; expect "T8k description of 1024 characters" 0 "$rc" "$out" "skills: OK (1 skills)" for scalar in '>' '|-'; do sk_reset; skill good-one "name: good-one" "description: $scalar" " Folded text." - sk; expect "T8l block-scalar description '$scalar'" 1 "$rc" "$out" "use single-line values" + sk; expect "T8l block-scalar description '$scalar'" 1 "$rc" "$out" "block scalars are not supported" done sk_reset; mkdir -p "$SK_ROOT/.agents/skills/no-skill" && echo x > "$SK_ROOT/.agents/skills/no-skill/notes.md" git -C "$SK_ROOT" add -f .agents/skills/no-skill/notes.md @@ -691,16 +691,63 @@ sk_reset; printf 'intro\n---\nname: good-one\ndescription: x\n---\n' > "$SK_ROOT sk; expect "T8x frontmatter must start on the first line" 1 "$rc" "$out" "no \`---\` frontmatter block" sk_reset; printf -- '---\nname: good-one\n---\ndescription: in the body\n' > "$SK_ROOT/.agents/skills/good-one/SKILL.md" sk; expect "T8y body lines are not frontmatter" 1 "$rc" "$out" "frontmatter 'description' is required" -sk_reset; skill good-one "name: good-one" "metadata:" " description: nested" -sk; expect "T8z indented lines are not top-level fields" 1 "$rc" "$out" "frontmatter 'description' is required" -sk_reset; skill good-one "name: good-one" "description: Use when: a colon is in the value" -sk; expect "T8aa a colon inside the value" 0 "$rc" "$out" "skills: OK (1 skills)" -sk_reset; skill good-one "name: >" " good-one" "description: x" -sk; expect "T8ab block-scalar name" 1 "$rc" "$out" "use single-line values" +sk_reset; skill good-one "name: good-one" "description: x" "metadata:" " nested: value" +sk; expect "T8z nested mapping" 1 "$rc" "$out" "line 5: expected \`key: value\` at column 0" sk_reset; echo x > "$SK_ROOT/.agents/skills/README.md" && git -C "$SK_ROOT" add -f .agents/skills/README.md -sk; expect "T8ac files directly under .agents/skills/ are not skills" 0 "$rc" "$out" "skills: OK (1 skills)" +sk; expect "T8aa files directly under .agents/skills/ are not skills" 0 "$rc" "$out" "skills: OK (1 skills)" sk_reset; skill skill2 "name: skill2" "description: x" -sk; expect "T8ad digits in a name" 0 "$rc" "$out" "skills: OK (2 skills)" +sk; expect "T8ab digits in a name" 0 "$rc" "$out" "skills: OK (2 skills)" +sk_reset; skill good-one "name: >" " good-one" "description: x" +sk; expect "T8ac block-scalar name" 1 "$rc" "$out" "'name' must be a one-line string: block scalars are not supported" +sk_reset; skill good-one "name: good-one: x" "description: x" +sk; expect "T8ad name outside the string subset" 1 "$rc" "$out" "'name' must be a one-line string" +sk_reset; skill good-one "name: good-one" "description: '''$(printf 'd%.0s' $(seq 1023))'" +sk; expect "T8ae a doubled single quote counts as one character" 0 "$rc" "$out" "skills: OK (1 skills)" +sk_reset; skill good-one "name: good-one" "description: \"\\\"$(printf 'd%.0s' $(seq 1023))\"" +sk; expect "T8af an escaped double quote counts as one character" 0 "$rc" "$out" "skills: OK (1 skills)" +sk_reset; skill good-one "name: good-one" "description: short" " more text" +sk; expect "T8ag value continued on an indented line" 1 "$rc" "$out" "line 4: expected \`key: value\` at column 0" +sk_reset; skill good-one "name: good-one" "description: short" "more text" +sk; expect "T8ah stray unindented line" 1 "$rc" "$out" "line 4: expected \`key: value\` at column 0" +sk_reset; skill good-one "name: good-one" "description: x" "- item" +sk; expect "T8ai list item" 1 "$rc" "$out" "line 4: expected \`key: value\` at column 0" +sk_reset; skill good-one "name:good-one" "description: x" +sk; expect "T8aj key without a space after the colon" 1 "$rc" "$out" "line 2: expected \`key: value\` at column 0" +sk_reset; skill good-one "name: good-one" "description: x" " # indented comment" +sk; expect "T8ak indented comment" 1 "$rc" "$out" "line 4: expected \`key: value\` at column 0" +sk_reset; skill good-one "name: good-one" "description: x" "allowed-tools: [Read" +sk; expect "T8al other keys are checked too" 1 "$rc" "$out" "line 4: 'allowed-tools' must be a one-line string" +sk_reset; skill good-one "name: wrong" "name: good-one" "description: x" +sk; expect "T8am duplicate key" 1 "$rc" "$out" "line 3: duplicate key 'name'" +sk_reset; skill good-one "# a comment" "name: good-one" "" "description: x" "metadata:" +sk; expect "T8an comments, blank lines and empty values of other keys" 0 "$rc" "$out" "skills: OK (1 skills)" +sk_reset; skill good-one "name: good-one" 'description: "bad \p escape"' +sk; expect "T8ao malformed quoted value names the quoting problem" 1 "$rc" "$out" "the quoted string is unterminated" +sk_reset; skill good-one "name: good-one" "description: Café notes" +sk; expect "T8ap non-ASCII text in a plain value" 0 "$rc" "$out" "skills: OK (1 skills)" +# Values YAML rejects or reads as non-strings, and values outside the accepted subset. +for bad in 'Use when: a colon' 'x # comment' 'ends with:' '[list]' '{map}' '*alias' '&anchor' '!tag' \ + '%pct' '@at' '`tick' '|pipe' '- dash' '-' '? q' ': x' '0x1F' '123' '.Inf' '1:30' 'back\slash' \ + "$(printf 'a\tb')" "$(printf '\xef\xbb\xbfbom')" '"unterminated' '"inner " quote"' \ + "'unterminated" "'it's'" true No null '~' y OFF \ + "$(printf 'c1\xc2\x85char')" "$(printf 'non\xef\xbf\xbechar')" "$(printf 'line\xe2\x80\xa8sep')" \ + "$(printf '"ctrl\x01char"')" "$(printf '"c1\xc2\x85char"')"; do + sk_reset; skill good-one "name: good-one" "description: $bad" + sk; expect "T8aq rejected description <$bad>" 1 "$rc" "$out" "'description' must be a one-line string" +done +for good in '"Use when: quoted"' '"escaped \" quote"' '"escaped \\ backslash"' "'doubled '' quote'" \ + 'a#hash without a space before it' 'time 10:30 is fine' '"true"' 'trueish' 'v1.2 notes' \ + '"quoted" # trailing comment' "'single' # trailing comment" 'Plain words, with (parens) and a comma.' \ + 'Use when — writing' 'Über alles' "$(printf 'nbsp\xc2\xa0ok')" "$(printf '"tab\tin quotes"')"; do + sk_reset; skill good-one "name: good-one" "description: $good" + sk; expect "T8ar accepted description <$good>" 0 "$rc" "$out" "skills: OK (1 skills)" +done +sk_reset; skill good-one "name: good-one" "$(printf 'description:\tx')" +sk; expect "T8as tab after the colon" 1 "$rc" "$out" "line 3: use a space, not a tab, after the colon" +sk_reset; skill good-one "name: good-one" "$(printf 'description: caf\xe9')" +sk; expect "T8at SKILL.md that is not valid UTF-8" 1 "$rc" "$out" "good-one/SKILL.md: not valid UTF-8 at byte" +sk_reset; skill good-one "name: good-one" "$(printf '"description": x')" +sk; expect "T8au quoted key" 1 "$rc" "$out" "line 3: expected \`key: value\` at column 0" # --- Summary ----------------------------------------------------------------- From f9b841a2987f7eb0211818ab534a6cf4d44b7082 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Mon, 9 Nov 2026 18:53:44 +0000 Subject: [PATCH 5/8] Fix the doc-name exception wording in doc-writing (PROF-16542) `doc/README.md` is upper case, so calling it "the only lowercase exception" was wrong. It is the only doc name that is not PascalCase, which is the rule check-doc-index.py enforces. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/doc-writing/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.agents/skills/doc-writing/SKILL.md b/.agents/skills/doc-writing/SKILL.md index 0568aadb08..4d358510fc 100644 --- a/.agents/skills/doc-writing/SKILL.md +++ b/.agents/skills/doc-writing/SKILL.md @@ -6,7 +6,7 @@ description: Rules for java-profiler documentation under doc/ (naming, index, ac # Documentation writing - **Naming.** Files in `doc/` use PascalCase (e.g. `BuildSystemGuide.md`); - `doc/README.md` is the only lowercase exception. New subsystem docs go in + `doc/README.md` is the only name that is not PascalCase. New subsystem docs go in `doc/architecture/` or `doc/reference/`; build docs in `doc/building/`. - **Index.** Every doc page is indexed in `doc/README.md`. CI (`qualityChecks`) enforces naming and indexing; design artifacts under `docs/sphinx/`, `.sphinx/` and From f70d63bd2de8f58ce1e14ac7528a1f385cf1776d Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Mon, 9 Nov 2026 18:59:21 +0000 Subject: [PATCH 6/8] Point doc-writing at the subsystem index rows it must maintain (PROF-16542) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old sentence said a page "changes a subsystem contract", which a doc page does not do; the AGENTS.md "keep this file current" rule already covers code changes that alter a contract. The doc-side duty is different: adding, renaming or removing a subsystem page means updating its row in the AGENTS.md "Architecture — Subsystem Index" table, which no CI check covers. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/doc-writing/SKILL.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.agents/skills/doc-writing/SKILL.md b/.agents/skills/doc-writing/SKILL.md index 4d358510fc..6b192e1d0f 100644 --- a/.agents/skills/doc-writing/SKILL.md +++ b/.agents/skills/doc-writing/SKILL.md @@ -20,5 +20,6 @@ description: Rules for java-profiler documentation under doc/ (naming, index, ac - **Link, do not copy.** Link to deeper sources instead of copying large documents. - The AGENTS.md **Comment budget** and **Comments** rules apply to doc paragraphs too: no references to plans, phases, review reports or tickets. -- When a page changes a subsystem contract, update its row in the AGENTS.md subsystem - index in the same change. +- When you add, rename or remove a subsystem page under `doc/architecture/` or + `doc/reference/`, update its row in the AGENTS.md "Architecture — Subsystem Index" + table. CI checks the `doc/README.md` index, not that table. From 36bd025ce2bedd6d13a5545e98fd661e6a0f1115 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Mon, 9 Nov 2026 19:15:26 +0000 Subject: [PATCH 7/8] Keep the commit-structure rule in AGENTS.md (PROF-16542) "Commit in logical chunks" decides how work is split, before any message is written, so it belongs with the other always-loaded commit rules in AGENTS.md, not in pr-writing. pr-writing keeps only the commit-message rule, and drops its copy of the ticket-reference rule, which AGENTS.md Work Tracking already states. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/pr-writing/SKILL.md | 3 +-- AGENTS.md | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.agents/skills/pr-writing/SKILL.md b/.agents/skills/pr-writing/SKILL.md index 8a3e85ba53..19a1816324 100644 --- a/.agents/skills/pr-writing/SKILL.md +++ b/.agents/skills/pr-writing/SKILL.md @@ -41,5 +41,4 @@ descriptions only. ## Commits -- Commit in logical chunks. Base each message solely on the actual diff. -- The ticket reference (`PROF-` or `#`) goes in the PR title or body; CI checks it. +- Base each commit message solely on the actual diff. diff --git a/AGENTS.md b/AGENTS.md index a650625bfc..cb9ceb465e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -411,7 +411,7 @@ exception list. config files (Dockerfiles, `.gitlab/**`) are exempt. Enforced in CI: `utils/check-copyright.py` (part of `qualityChecks`) checks the files a branch adds or modifies. -- Commits: never commit files from other projects or scratch files. +- Commits: logical chunks. Never commit files from other projects or scratch files. **Never commit design artifacts automatically** — plans, specs, review reports, audit findings, session notes, analysis dumps — even when they live in the repo tree. Always ask for permission first and commit them only if the user explicitly says so. From 231d2901ba939b8131e48212d990202e6bed5326 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Mon, 9 Nov 2026 20:38:17 +0000 Subject: [PATCH 8/8] Add style and number rules to the writing skills (PROF-16542) Two gaps surfaced while PROF-16542 was run: - doc-writing had no sentence-style rule. Test drives of the pre-PR review's repeated-failure step found it in both Claude Code and Pi: after two corrections on sentence style in doc text, each proposed a rule for doc-writing. doc-writing now asks for Simplified Technical English in the text being added or rewritten, and leaves other text alone. - Three ticket comments carried wrong counts that came from memory. pr-writing gets a "Measured numbers" section: take them from output, a saved log or a CI result for the current candidate, name that source, and update them when the candidate changes. Its scope statement now names PR titles and commit messages. doc-writing's "verify numbers" rule becomes its own bullet: verify each added or changed number against the code, a reproducible measurement or a named observation, and fix numbers the change makes stale. The rules are stated in each skill, not by reference, so an agent that loads only one skill still has them. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/doc-writing/SKILL.md | 11 +++++++---- .agents/skills/pr-writing/SKILL.md | 13 ++++++++++--- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/.agents/skills/doc-writing/SKILL.md b/.agents/skills/doc-writing/SKILL.md index 6b192e1d0f..077c4e3e9b 100644 --- a/.agents/skills/doc-writing/SKILL.md +++ b/.agents/skills/doc-writing/SKILL.md @@ -1,6 +1,6 @@ --- name: doc-writing -description: Rules for java-profiler documentation under doc/ (naming, index, accuracy, stale sections). Use before creating, editing, renaming or deleting a file under doc/. +description: Rules for java-profiler documentation under doc/ (naming, index, style, accuracy, stale sections). Use before creating, editing, renaming or deleting a file under doc/. --- # Documentation writing @@ -11,9 +11,12 @@ description: Rules for java-profiler documentation under doc/ (naming, index, ac - **Index.** Every doc page is indexed in `doc/README.md`. CI (`qualityChecks`) enforces naming and indexing; design artifacts under `docs/sphinx/`, `.sphinx/` and similar scratch paths must never be tracked. -- **Current state only.** Documentation never carries transient or historical notes, - and never claims what is not true of the code. Verify numbers before writing them - down. +- **Style.** Write the text you add or rewrite in Simplified Technical English: short + declarative sentences, one idea per sentence. Leave the style of other text alone. +- **Current state only.** Documentation never carries transient or historical notes. + It never claims what is not true of the code. +- **Numbers.** Verify each number you add or change against its source: the code, a + reproducible measurement, or a named observation. Fix numbers your change makes stale. - **Remove finished work.** Preserved historical sections of landed design or handover docs ("Suggested next steps", unverified hypotheses, open items) are rewritten or deleted once the work they instruct is done: never point a reader at finished work. diff --git a/.agents/skills/pr-writing/SKILL.md b/.agents/skills/pr-writing/SKILL.md index 19a1816324..9d27ee1795 100644 --- a/.agents/skills/pr-writing/SKILL.md +++ b/.agents/skills/pr-writing/SKILL.md @@ -5,9 +5,16 @@ description: Writing rules for java-profiler PR titles and descriptions, PR comm # PR, comment and commit writing -These rules apply to PR descriptions and to every comment a session writes: PR -comments, review replies, Jira work-item comments. The template rule applies to -descriptions only. +These rules apply to PR titles and descriptions, commit messages, and every comment a +session writes: PR comments, review replies, Jira work-item comments. The template rule +applies to descriptions only. + +## Measured numbers + +Take counts, sizes, test totals and timings from a command's output, a saved log or a +CI result for the current candidate, never from memory, and name that source. Naming +the source is not a tool-run narrative. When the candidate changes, update the numbers +or say they are from an earlier commit. ## PR descriptions and comments