Repository navigation
Conversation
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 "`<name>` skill" mention without a matching skill. Hermetic tests are T8 in test_quality_check_scripts.sh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…6542) 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 <noreply@anthropic.com>
Scan-Build Report
Bug Summary
Filter Results by FileReports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
CI Test ResultsRun: #42695877308 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-11-09 21:08:16 UTC |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cafd2915e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 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 |
There was a problem hiding this comment.
When a future skill contains malformed YAML such as description: "unterminated or description: Use when: a colon, this line parser still returns a nonempty description and qualityChecks succeeds, while a YAML loader rejects the document and Pi silently skips the skill. This defeats the check's stated purpose and can remove mandatory instructions without a CI signal; reject unsupported scalar syntax or validate the complete frontmatter as YAML, with tests for these malformed cases.
AGENTS.md reference: AGENTS.md:L382-L389
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6a35adcdf. Confirmed first: Pi drops a skill whose description is Use when: ...; of two test skills, it listed only the valid one.
The check now accepts only a closed subset of YAML, so it needs no YAML library in CI:
- Every frontmatter line is blank, a column-0 comment, or
key: valuewith 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 a fixed character set, has no
:,#or trailing:, and is not a null or boolean word; or a quoted string with only\"/\\escapes, or''doubling. - Quoted values are decoded before the length check. Invalid UTF-8 is an error.
T8aa now expects the unquoted : value to fail. New cases cover malformed quotes, unknown escapes, non-string values, every line-grammar rule and the character-range boundaries. 231 test groups pass, and each of 46 hand mutations of the check fails the suite. Over the tested values and blocks, the check never accepts what PyYAML rejects; it is stricter in a few places, and the error then names the fix.
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new validator accepts malformed YAML and reports success, allowing CI to approve a skill that cannot load. An existing test also incorrectly treats an unquoted colon-space value as valid YAML.
🤖 Bits Code Review · Commit 1cafd29
| value = value.strip() | ||
| if len(value) >= 2 and value[0] == value[-1] and value[0] in "'\"": | ||
| value = value[1:-1] | ||
| fields[key.strip()] = value |
There was a problem hiding this comment.
An unterminated value such as description: "Use when writing a PR. is accepted as a nonempty string. Substituting it into pr-writing still produces skills: OK (3 skills) and exit 0, although a YAML parser rejects it. The CI guard therefore approves malformed frontmatter that prevents the skill from loading. Validate YAML syntax and require string fields; also update T8aa, which currently expects an invalid unquoted colon-space value to pass, and add coverage for malformed quotes.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in 6a35adcdf. Confirmed first: Pi drops a skill whose description is Use when: ...; of two test skills, it listed only the valid one.
The check now accepts only a closed subset of YAML, so it needs no YAML library in CI:
- Every frontmatter line is blank, a column-0 comment, or
key: valuewith 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 a fixed character set, has no
:,#or trailing:, and is not a null or boolean word; or a quoted string with only\"/\\escapes, or''doubling. - Quoted values are decoded before the length check. Invalid UTF-8 is an error.
T8aa now expects the unquoted : value to fail. New cases cover malformed quotes, unknown escapes, non-string values, every line-grammar rule and the character-range boundaries. 231 test groups pass, and each of 46 hand mutations of the check fails the suite. Over the tested values and blocks, the check never accepts what PyYAML rejects; it is stricter in a few places, and the error then names the fix.
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 <noreply@anthropic.com>
`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 <noreply@anthropic.com>
…16542) 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 <noreply@anthropic.com>
"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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
What does this PR do?:
Moves the PR, comment, commit and documentation writing rules from
AGENTS.mdinto two agent skills, and adds a CI check that keeps every skill loadable..agents/skills/pr-writing/: PR descriptions, PR and Jira comments, commit messages..agents/skills/doc-writing/: pages underdoc/.utils/check-skills.py, run byqualityChecks, fails on:key: valuewith a one-line string value;.claude/skillssymlink;AGENTS.mdmention of a skill that does not exist.AGENTS.mdkeeps a one-line pointer per moved section and shrinks from 586 to 544 lines.Motivation:
AGENTS.mdloads into every session. These rules apply only while an agent writes a PR, a comment, a commit message or a doc page, and they can be compacted away before then. A skill loads at that moment. This is step 2 of PROF-16466; the design is on PROF-16542.The check exists because Pi silently skips a
SKILL.mdwith malformed frontmatter or without a description. A broken skill would never load, and nothing would report it. A description such asUse when: ...is enough: Pi dropped that skill in a test.The check accepts a strict YAML subset instead of parsing YAML, so CI needs no YAML library. It is stricter than YAML: no nested mappings, lists or multi-line values, and a few plain values must be quoted. The error names the fix.
Additional Notes:
AGENTS.mdheader said writing rules for PRs, comments and commits stay inline. This PR changes it to "rules for writing code and code comments", because those rules now live in skills.pr-writingkeeps only the commit-message rule;doc/too.ticket-writing,work-planning,ci-script-change,verify-profiler-changeand thepre-pr-reviewdepth policy follow in later PRs of PROF-16542./code-review(medium) rounds reviewed it; their findings are fixed.AGENTS.mdandutils/README.md. A test merge with Leave PR commands for other repositories to their own policy #954's head (git merge-tree) has no conflicts.How to test the change?:
bash utils/tests/test_quality_check_scripts.sh(T8 coverscheck-skills.py)node:22-bookwormon a clean clone, Pi cases includedtest_gradle_hook.sh78/78,test_review_marker.sh213/213,test_knowledgebase_guard.sh48/48; the fourth commit changes onlycheck-skills.pyand its testspr-writinganddoc-writingloadSKILL.mdfiles are readmainvs. this branchFor Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.🤖 Generated with Claude Code