Skip to content

Commit 68adbc0

Browse files
derobinsclaude
andauthored
Add a section to AGENTS.md about code review (#272)
Ask agents to review their whole change before handing it back, with a checklist drawn from what Copilot has asked for on past hipObject PRs: changelog entries, missing files, incomplete fixes, tests that pass without the fix, cleanup and exception paths, strict parsing, script failure handling, the rules in AGENTS.md, and accurate wording. Co-authored-by: Claude <noreply@anthropic.com>
1 parent a59f360 commit 68adbc0

1 file changed

Lines changed: 70 additions & 0 deletions

File tree

‎AGENTS.md‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -795,6 +795,76 @@ Add the entry to the topmost (unreleased) version, under the matching heading:
795795
existing entries: say what changed, and what users have to do differently, if
796796
anything.
797797

798+
## Review your change before you finish
799+
800+
**When you have modified the repository, step back and review the whole
801+
change as a reviewer would before you call it done.** Every PR gets a
802+
Copilot review, and most of what it asks for could have been caught
803+
first. Read all of `git diff`, and every new file `git status` lists, from
804+
start to finish, not just the lines you meant to change, and check it
805+
against the list below, which comes from what Copilot has asked for on past
806+
PRs. Fix what you find, then run the
807+
[checks](#linters-warnings-and-sanitizers-mandatory) again.
808+
809+
- **Is there a changelog entry, and is it right?** The request Copilot
810+
makes most often. See [Changelog](#changelog): a new compiler warning, a
811+
change to how CMake finds a tool, a fix to a code path few callers use,
812+
and a reworded error message all count. An entry must not claim a change
813+
the diff doesn't make, or cover only part of one it does.
814+
- **Is every new file in the change?** Copilot has flagged PRs that
815+
include a header, add a test source to CMake, or point to a notes file
816+
that was never added. Name each new file when you hand the change back,
817+
so it gets committed.
818+
- **Is the fix complete?** Search for every other place with the same
819+
pattern or the same state, and fix them too or say why not. Copilot
820+
regularly finds the bug left in a sibling: a shellcheck cleanup that
821+
missed two of the hardware-test workflows, a warning suppression added
822+
to every Google Test file but one, a stricter status-line parser in one
823+
test client but not the other.
824+
- **Does each new test fail without the fix?** Revert the fix in a
825+
scratch copy and check. Copilot reports tests that pass either way: a
826+
move-assignment test that only moves into empty objects, an environment
827+
variable set before the program starts for a fix about when it's read, a
828+
fake that never returns the edge case (HIP filling the whole buffer with
829+
no NUL), a test that skips when the library it needs won't load. Have
830+
`FakePlatform` fail the call or return the edge case, and cover new error
831+
paths too, for the [coverage floor](#tests).
832+
- **Is cleanup right on every path, exceptions included?** Follow each
833+
failure branch, and ask what happens if an allocation or a container
834+
insertion along the way throws. A class's members are destroyed after its
835+
destructor's body runs, in the reverse of the order they're declared in,
836+
so the declaration order is the teardown order: a queue pair has to go
837+
before the memory region its work uses, and a memory region before the
838+
memory it registers. Copilot has found buffers freed while still
839+
registered with the NIC, a rollback dismissed before the last step that
840+
could throw, and a pending GPU copy that lost its tracking when
841+
`push_back()` threw.
842+
- **Is untrusted input parsed strictly?** See [Security](#security).
843+
Copilot has found parsers that accepted a sign, a fourth digit in an
844+
HTTP status, junk after the last field, an empty header that was
845+
present, a value too wide for its 24-bit wire field, a reserved zero, and
846+
a peer token after a non-200 status. Check each field's exact width,
847+
characters, and range, and test the malformed forms.
848+
- **Does a failure make the script fail?** In scripts and workflow steps,
849+
a retry loop whose last `sleep` succeeds, a pipeline without `pipefail`,
850+
or a command whose exit code means both "absent" and "lookup failed"
851+
turns an error into success. Exit nonzero, and tell the exit codes apart.
852+
- **Does it follow the rules above?** Copilot checks the code against
853+
this file: `parseNumber()`, not `strtoull()`; no rkeys or remote
854+
addresses in logs, even debug ones; doc8's 80-column limit; cache
855+
variables named `HIPOBJ_…`, and never forced on a parent project;
856+
targets made with the `hipobj_add_*()` helpers, which give them the
857+
standard, warnings, hardening, and clang-tidy; `Threads::Threads` kept on
858+
what uses threads.
859+
- **Do the words match the code?** Comments, documentation (the public
860+
header's included), changelog entries, and the PR description must claim
861+
only what the diff does. Copilot has found a guarantee that holds only on
862+
success stated for every return (a staging copy that times out is still
863+
running after `hipObjBufSync()` returns), a response header called a
864+
request header, optional build dependencies listed as required, steps
865+
described in the wrong order, and good advice given for the wrong reason.
866+
Check each claim against the code.
867+
798868
## Lab provisioning
799869

800870
For GPU hosts, ROCm, RDMA, and MinIO AIStor installs, use

0 commit comments

Comments
 (0)