doc: add a code review checklist for reviewers and coding agents - #15
Open
ppannuto-claude wants to merge 2 commits into
Open
doc: add a code review checklist for reviewers and coding agents#15ppannuto-claude wants to merge 2 commits into
ppannuto-claude wants to merge 2 commits into
Conversation
Adds `.claude/skills/tock-code-review/`, a repository-scoped skill that
performs a heavy-weight review of a change set against Tock's conventions
and its recurring defect classes.
The rules Tock cares about are largely invisible to rustc and clippy: they
are properties of the execution model (no allocation, no unwinding,
callbacks only from interrupts or deferred calls, static resource
ownership) rather than of the type system. Code that violates them compiles
cleanly and boots. This skill collects those rules, along with the
per-subsystem review criteria already written down in doc/CodeReview.md,
into a procedure an assistant can follow at a milestone or before a
submission.
SKILL.md scope, procedure, and the requirement to
verify a finding before reporting it
references/mechanical-checks.md the make targets, which boards a given
change has to be built into, license
headers
references/subsystem-criteria.md kernel/HIL/capsules/chips/boards/arch/
libraries criteria, plus the ripple
checks a change fans out along
references/bug-patterns.md twenty recurring defect classes, each
with how to spot it, how to confirm it,
and the fix
The skill is deliberately scoped to run at milestones and before
submission, not on every edit.
Two points are documented from the tree rather than from the docs, since
they disagree: new code uses `/// # Safety` and `// SAFETY:` (the prose in
AGENTS.md and doc/CodeReview.md says `### Safety`, which appears nowhere in
the tree), and arch/x86 is the standing exception to "static_init! only in
board crates", where call-once is a documented safety precondition instead.
Per AGENTS.md, the skill instructs assistants to deliver findings as
technical points and not to draft PR or review prose for the contributor.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two corrections to the checklist added in the previous commit. **It was Claude-specific.** It lived in `.claude/skills/` and led with the YAML frontmatter that Claude Code uses to discover and auto-trigger a skill. Tock has an AGENTS.md and deliberately no CLAUDE.md, and nothing in the checklist's content is tied to one assistant. Moved to `doc/code-review/`, alongside the other subdirectories under `doc/`, with README.md as the entry point and the frontmatter dropped. Discovery now runs through AGENTS.md, which every agent reads, and through doc/README.md for human reviewers -- who are as much the audience as any tool. **The safety-comment rule was stated as a style preference.** It described `/// # Safety` and `// SAFETY:` as "the in-tree convention" and noted that AGENTS.md and doc/CodeReview.md say `### Safety` instead, advising readers to match the tree. That framing is wrong in both directions: - `### Safety` is not the error the previous commit implied. Verified against clippy: `missing_safety_doc` accepts a `Safety` heading at any level, and `undocumented_unsafe_blocks` matches the `SAFETY:` prefix case-insensitively, so `// safety:` passes as well. The prose in AGENTS.md is not describing a lint violation. - More importantly, "match the tree" is the wrong instruction. Clippy's lints define the convention, and new code should be written as if they were denied tree-wide. That they are currently `allow` -- and that `undocumented_unsafe_blocks` sits in the `restriction` group -- is a transitional state rather than permission. tock#5003 makes `missing_safety_doc` a hard error for the kernel crate, headed tree-wide. Both files now say that, and note the consequence for a reviewer: until those lints land, nothing in CI checks that unsafe code is documented at all, so review is the only thing that does. Also corrected the AI-policy paragraph. It asserted that Tock's policy forbids AI-written prose addressed to humans; the policy in .github/CONTRIBUTING.md requires disclosure and contributor review, and says nothing of the sort. It now cites what the policy actually says. Added a step to the procedure: unrelated convention violations turned up during a review belong in their own pull request, not folded into the change under review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Staging branch for review comments. Adds
doc/code-review/— a review checklist for a Tock change set, aimed at both human reviewers and coding agents.doc/CodeReview.mdcovers the process (who reviews, upkeep vs. significant, votes). This covers the content of a review, which was not written down anywhere.doc/code-review/README.mdmechanical-checks.mdmaketargets, picking which board to build, license headerssubsystem-criteria.mdbug-patterns.mdHooked into
AGENTS.md(new "Reviewing Your Own Work" section) and indexed indoc/README.md.Two commits on purpose: the first adds it under
.claude/skills/, the second moves it todoc/and de-Claude-ifies it. Squash on merge.Things worth commenting on
Placement.
doc/code-review/was chosen to matchdoc/wg,doc/reference,doc/syscalls. Easy to move.Safety comments. Written as "act as if
missing_safety_doc/undocumented_unsafe_blockswere denied tree-wide," citing kernel: Denyclippy::missing_safety_docfor the kernel crate tock/tock#5003 for the kernel crate. Verified against clippy while writing:### Safetydoes satisfymissing_safety_doc(any heading level), and// safety:satisfiesundocumented_unsafe_blocks(case-insensitive). So the ~34 lowercase occurrences and the### Safetywording in AGENTS.md /doc/CodeReview.mdare style deviations, not lint failures. Separate PRs, per the note below.clippy.tomlcomment appears stale. It saysmissing_safety_docis "allowed in Tock, except kernel", but the rootCargo.tomlsets it toallowandkernel/Cargo.tomlisworkspace = truewith no override — so it is allowed in the kernel too until kernel: Denyclippy::missing_safety_docfor the kernel crate tock/tock#5003 lands. Not touched here.static_init!exception. "Board crates only" has one standing exception in-tree:arch/x86/src/segmentation.rsandinterrupts/idt.rscall it fromunsafe fn init()where call-once is a documented precondition. Documented as the bar a non-board use must meet, rather than as a violation — worth confirming that is the intended reading.AI-policy paragraph in
README.mdcites.github/CONTRIBUTING.mdas-is: disclosure + contributor review. It does not assume the stricter "no AI-written prose" wording, which is not currently in master.Scope. The
bug-patterns.mdlist is 20 items drawn from the docs plus recurring failure modes. Additions and deletions both welcome — particularly whether anything there is too speculative to keep.Checks run:
make licensecheck,make format-check,tools/ci/check-for-readmes.sh. Markdown only, so clippy/syntax are unaffected. No<!-- toc -->markers, soci-job-markdown-tocskips these files.