Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions .agents/skills/create-pr/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,6 @@ repository.

## Notes

- Do not add any Codex/Anthropic attribution to commits or PRs created via
this skill: omit `Co-Authored-By: Codex ...` and `Codex-Session: ...`
trailers from commit messages, and omit the "Generated with Codex"
line and session link from the PR body.
- Never force-push over another contributor's commits without explicit
confirmation.
- If the change touches `/api/v1/...` response shapes, `README.md`,
Expand Down
7 changes: 0 additions & 7 deletions .agents/skills/fix-issue/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,3 @@ Use this skill to resolve a reported GitHub issue in this repository.
steps even if verification already ran in step 5.
8. If the fix is not fully verifiable without physical Pi hardware, say so
explicitly in the PR description rather than claiming full verification.

## Notes

- As with `create-pr`, do not add Codex/Anthropic attribution to commits or
PRs: no `Co-Authored-By: Codex ...` / `Codex-Session: ...` commit
trailers, and no "Generated with Codex" line or session link in the
PR body.
5 changes: 3 additions & 2 deletions .claude/agents/pimonitor-reviewer.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,9 @@ build, vet and tests, since a fix can break them.
contradicts the code; new or changed logic without a test.
- **NON-BLOCKING** β€” a design or naming choice that is defensible either
way, a documentation improvement, a test that could be stronger. Report it
once with a recommendation and mark it clearly. It does not gate the pull
request and it does not earn another review round.
once with a recommendation and mark it clearly. It does not earn another
review round. What happens to it after that is the calling session's call,
not yours β€” so report it only if it is worth someone's time to resolve.

There is no third category. If a finding feels like a nit, it is
non-blocking, and probably not worth reporting at all.
Expand Down
96 changes: 85 additions & 11 deletions .claude/skills/create-pr/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ repository.
related issue if one exists), Reviewer Notes, and Test Plan, and check off
the checklist items that are actually true (don't check items you haven't
verified) β€” including the REST API/configuration/packaging section when
applicable, not just the General section. In Reviewer Notes, record how
many internal review passes ran and which commits resolved their
findings; put any accepted non-blocking findings under Next Steps.
applicable, not just the General section. Write all of it as a
description of the finished change, never of the review loop that
produced it β€” see "The loop stays invisible" below.
8. Report the PR URL back to the user.

## The internal review loop
Expand All @@ -56,31 +56,105 @@ the repository's full review checklist.
- `APPROVE` β†’ done, go push.
- Blocking findings β†’ fix each one minimally and commit. Do not widen the
change beyond what the finding requires.
- Non-blocking findings β†’ **do not open another round for them**. Fix one
only if it is trivial and already in scope; otherwise carry it into the
PR's Next Steps or propose a follow-up issue, and say so.
- Non-blocking findings β†’ **do not open another round for them**, but do
not park them in the PR either. The branch is still local and the
context that found them is still here, so the cheap outcome is to fix
them now. If one is genuinely out of scope, open an issue for it before
pushing. Either way they are resolved here, not carried into the PR
text as leftovers.
3. **Pass n+1** β€” launch a fresh `pimonitor-reviewer` and give it the round
number, the previous round's findings, and the commits that fixed them.
It reviews the delta only, per its own instructions.
4. **Stop** at the first pass that reports no blocking findings. Cap the loop
at **three passes**: if blocking findings remain after the third, stop and
report the open findings to the user rather than continuing to iterate β€”
at that point the change needs a decision, not another round.
5. **Cover your last fixes.** Fixes you make after the final pass β€” including
fixes for its non-blocking findings β€” are themselves unreviewed. If the
three-pass budget still has a pass left, spend it on them as a delta
review. If it does not, say so when you report, and push anyway rather
than starting a fourth pass.

Two rules keep this loop finite, and they are the point of the whole
arrangement:

- **Later passes review the delta, never the whole diff again.** A fresh full
review of unchanged code always finds something new.
- **Only blocking findings start a new pass.** Non-blocking findings are
recorded, not iterated on.
still resolved β€” fixed now, or filed as an issue before the push β€” they
just do not buy another pass.

The cap is on *passes*, not on findings. A single pass may resolve any
number of them.

## The loop stays invisible

This section is about the **internal loop above** β€” the passes that run in
this session before the push. That loop is working material and does not
travel with the change. Review comments posted on the pull request once it
is open, and the replies to them, are a different thing: they are public
review, governed by the next section, and nothing here forbids them.

The pull request documents the **finished state**: what the change does,
which components it touches, which guarantees it had to preserve, and how to
smoke-test it. It does not document the way there. So nothing you write when
opening the PR β€” the body, and the commit messages on the branch β€” mentions:

- that an internal review ran, or how many passes it took
- its findings, their verdicts or severities, or which commit resolved
which one
- the `pimonitor-reviewer` subagent, or internal review rounds

Commit messages still explain *why* the change is what it is, as always β€”
they just explain it in terms of the change, never in terms of a finding
that prompted it.

A reviewer opening the PR gets the change, not its history β€” the loop's
value was in fixing the code, and that value is already in the diff.

So fill the template like this:

- **Description** β€” the problem and what the change does about it.
- **Reviewer Notes** β€” the components the diff touches (routes, config keys,
collectors, packaging), the guarantees it had to keep intact (`/api/v1`
response shapes, privilege separation, the unprivileged/privileged service
split), where to look first, and the smoke test: the concrete steps to
exercise the change on a Pi.
- **Test Plan** β€” the tests that cover the change and anything that could
only be verified against real hardware.
- **Next Steps** β€” genuine follow-up work, with issue links. Never a parking
lot for review findings; see the next section for where those go.

## Every posted review point gets resolved in this PR

Once a point exists as a review comment on the pull request β€” from a human,
a bot, or the `review-pr` skill, blocking or non-blocking alike β€” it is work
for **this** pull request. Severity decides the order it gets handled in, not
whether it gets handled.

There is no deferring to "the next change that touches this file". That
change is not scheduled, and the session holding the context is gone long
before it happens. A posted point therefore has exactly three outcomes, all
of them reached while this PR is open:

1. **Fixed** β€” implement it, push it, reply `Fixed in <sha>: <what
changed>`, resolve the thread.
2. **Declined** β€” reply with the reason it stays as it is, resolve the
thread. A reason, not a deferral: "this is intentional because …", not
"later".
3. **Split out** β€” only when it is real work that genuinely does not belong
in this PR: create the issue **now**, link it from the reply, resolve the
thread. A promise of a follow-up issue without a created issue is not an
outcome.

The round caps still apply: at most three internal passes here, and at most
two rounds on GitHub per `review-pr`. They limit how often the change is
*re-reviewed* β€” not how many findings get worked off. Arriving at the cap
with open posted points is not "done"; it means fixing, declining, or filing
them and saying so.

## Notes

- Do not add any Claude/Anthropic attribution to commits or PRs created via
this skill: omit `Co-Authored-By: Claude ...` and `Claude-Session: ...`
trailers from commit messages, and omit the "Generated with Claude Code"
line and session link from the PR body.
- Never force-push over another contributor's commits without explicit
confirmation.
- If the change touches `/api/v1/...` response shapes, `README.md`,
Expand Down
55 changes: 44 additions & 11 deletions .claude/skills/fix-issue/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,54 @@ Use this skill to resolve a reported GitHub issue in this repository.
loop from the `create-pr` skill β€” one or more `pimonitor-reviewer`
subagent passes (Opus, fresh context) against the local branch, fixing
blocking findings and re-running the reviewer on the delta until a pass
comes back clean, capped at three passes. Non-blocking findings are
recorded for the PR's Next Steps, not iterated on. This is what keeps the
review out of the pull request comments, so do not skip it and do not
defer it to a separate review session.
comes back clean, capped at three passes. Non-blocking findings do not buy
another pass, but they are still resolved before the push: fixed now while
the branch is local, or filed as an issue if genuinely out of scope. If
fixing them uses up the last pass's clean verdict, spend a remaining pass
on those fixes as a delta review, or say so when you report if the budget
is exhausted. This is what keeps the review out of the pull request
comments, so do not skip it and do not defer it to a separate review
session.
8. **Push** the branch (`git push -u origin <branch-name>`) and **open a PR**
referencing the issue (`Closes #<number>`), following the `create-pr`
skill's verification and template steps β€” do not skip the push/PR-creation
steps even if verification already ran in step 5. Record the internal
review passes and their fix commits in Reviewer Notes.
steps even if verification already ran in step 5. Describe the fixed
state, not the review that got you there (see "The loop stays invisible"
below).
9. If the fix is not fully verifiable without physical Pi hardware, say so
explicitly in the PR description rather than claiming full verification.

## Notes
## The loop stays invisible

- As with `create-pr`, do not add Claude/Anthropic attribution to commits or
PRs: no `Co-Authored-By: Claude ...` / `Claude-Session: ...` commit
trailers, and no "Generated with Claude Code" line or session link in the
PR body.
This is about the **internal loop of step 7** β€” the passes that run in this
session before the push. It is working material and stays here. Review
comments posted on the pull request once it is open, and the replies to
them, are public review and are governed by the next section.

The pull request documents the **finished state** β€” the issue's cause, the
fix, the components it touches, the guarantees it had to preserve, and the
smoke test that shows the reported symptom is gone. It does not document the
way there. So nothing you write when opening the PR β€” the body, and the
commit messages on the branch β€” mentions that an internal review ran, how
many passes it took, what it found, its verdicts or severities, or the
`pimonitor-reviewer` subagent. Commit messages still explain *why* the fix
is what it is, in terms of the bug, never in terms of a finding.

Reviewer Notes say what a reviewer needs in order to review this fix β€” the
affected collector, route or config key, the reported environment it has to
keep working on, where to look first, and how to smoke-test it on a Pi. Next
Steps is for genuine follow-up work with issue links, never a parking lot
for review findings.

## Every posted review point gets resolved in this PR

Once the PR is open, any point posted as a review comment β€” from a human, a
bot, or the `review-pr` skill, blocking or non-blocking β€” is work for this
pull request. There is no deferring to "the next change that touches this
file": it is not scheduled, and the session with the context is gone by
then. Each posted point ends in one of three ways, all inside this PR: fixed
and pushed with a one-line `Fixed in <sha>: …` reply, declined with a
reason, or split into an issue **created now** and linked from the reply β€”
then resolve the thread. The round caps limit re-reviews, not the number of
findings worked off. See `create-pr`'s section of the same name for the full
rule.
33 changes: 25 additions & 8 deletions .claude/skills/review-pr/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,35 @@ it converge:
something β€” including in prose they wrote to fix a documentation finding?
Never re-review untouched code; that is what turns three findings into
four rounds.
- **Only blocking findings justify another round.** Non-blocking findings go
into the PR's Next Steps section or a follow-up issue and are not chased.
- **Only blocking findings justify another round.** Non-blocking findings do
not trigger a re-review β€” but once posted they are still the author's to
resolve in this PR (see below). The cap is on rounds, not on findings.
- **Two consecutive rounds without a blocking finding means done.** Say so
plainly instead of leaving the review open-ended.
- **At most two rounds on GitHub.** If blocking findings survive that, the
change needs a decision from the author, not another review pass β€” say what
is still blocking and stop.
is still blocking and stop. Stopping means no further *review* rounds; the
posted findings themselves still get worked off.

## Answering findings on your own PR

When acting as the author of a PR under review, keep replies to one line:
`Fixed in <sha>: <what changed>`. The reasoning belongs in the commit
message, where it stays with the code; the reviewer verifies the commit, not
the reply. Resolve the thread once it is answered. One summary comment per
round beats one essay per thread.
Every point posted on the PR gets worked off in that same PR β€” whoever wrote
it, whatever its severity. Non-blocking is an ordering signal, not a licence
to skip: "we'll do it with the next change to this file" is not an answer,
because that change is not scheduled and the session that holds the context
will not exist then.

So each posted point ends in exactly one of three ways, before the PR
merges:

1. **Fixed** β€” push the fix, reply in one line, `Fixed in <sha>: <what
changed>`. The reasoning belongs in the commit message, where it stays
with the code; the reviewer verifies the commit, not the reply.
2. **Declined** β€” reply with the reason it stays as it is. A reason, not a
deferral.
3. **Split out** β€” only for real work that genuinely does not belong in this
PR: open the issue **now**, link it from the reply. "Follow-up issue"
without an issue is not an outcome.

Resolve the thread once it is answered, whichever of the three it was. One
summary comment per round beats one essay per thread.
10 changes: 10 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,13 @@ it *before* pushing, so a change is reviewed while it is still local;
review checklist, the integration-surface sweep, the blocking/non-blocking
severity model and the "round 1 is a full review, later rounds review only
the delta" rule live in that one file, so they are identical either way.

Two rules about what reaches the pull request, spelled out in `create-pr`
and `fix-issue`: the *internal* review loop stays invisible β€” a PR documents
the finished change (components, guarantees, smoke test), never the pre-push
passes that produced it β€” and every point posted as a review comment on the
open PR is resolved in that same PR, whoever wrote it and whatever its
severity: fixed, declined with a reason, or split into an issue created
right away and linked from the reply. Public review on an open PR is public
on purpose; only the pre-push loop is the part that stays in the session.
The round caps limit re-reviews, not the number of findings worked off.
Loading