diff --git a/agentic-skills/README.md b/agentic-skills/README.md new file mode 100644 index 0000000..e81806b --- /dev/null +++ b/agentic-skills/README.md @@ -0,0 +1,25 @@ +# Agentic skills + +Shared here to pass around, not to standardize. + +These are personal skills. They are here so others can read them, borrow from them, and get ideas - +nothing more. No one owns them, and this repository does not make them official. + +A skill like `human-pr` reflects one person's taste in how a pull request should be shaped. It is not +the house rule for how everyone must open a PR. Read it, take what is useful, drop what is not. + +What is here so far: + +| | | +| --- | --- | +| `human-pr` | Wrap up an in-session change and open its PR, with the author's own words kept separate from anything a model wrote. | +| `trivial-pr` | The same, cut down for a one-or-two-line change made from a fresh branch. | +| `review` | Review the current branch through several independent lenses, then try to disprove each finding before reporting it. | +| `trim-comments` | Push a changeset toward self-documenting code, moving decision history out of comments and into the commit message. | +| `review-principles` | Reference notes behind the `review` lenses. Not a skill. | + +If you use one, make it yours. Fork it, rename it, rewrite it, gut it. Change the voice, the steps, +the rules. A skill that matches how you actually work beats one you follow out of convention. + +So: no canonical version, no approval process, no "correct" way. These are starting points. Make up +your own mind. diff --git a/agentic-skills/human-pr.md b/agentic-skills/human-pr.md new file mode 100644 index 0000000..30a6cb8 --- /dev/null +++ b/agentic-skills/human-pr.md @@ -0,0 +1,230 @@ +--- +allowed-tools: Bash(git:*), Bash(gh:*), Bash(jira-api:*), Bash(jq:*), Read, Edit, Write, Grep, Glob, Skill +description: Human-styled AERIUS PR protocol - wrap up an in-session change and open its PR (the default for all PRs) +argument-hint: [AER-####] and/or your PR body text +--- + +## When to use + +You've developed a change in this conversation - usually on a working branch, changes usually +already committed - and want to finish by opening the PR in the house style. This is the default for +any real PR: the work is whatever you built and reviewed in-session, so there is no size limit and no +minimal-change discipline. Use the branch you're already on - do not start a fresh one or reset - +then commit anything outstanding, push, and run the draft → create flow. + +For a single tiny standalone fix made from scratch (a typo or rename, a one-or-two-line diff), use +`trivial-pr` instead - it adds the fresh-branch + minimal-change discipline this skill deliberately +omits. Same commit/PR conventions, same no-attribution rule, same draft-before-create gate, same +two-zone body either way. + +## Inputs + +`$ARGUMENTS` may be: +- An AERIUS issue key (e.g. `AER-4444`) - fetch context via the jira skill first +- Freeform description of the change - use it directly +- Your own PR body text - the words that go in the **top zone** of the body (see [The two-zone PR + body](#the-two-zone-pr-body)), used near-verbatim and never rewritten. May also arrive from the + conversation rather than the argument. +- Empty - work from the prior conversation context + +If an issue key is present, invoke the jira skill (or run `jira-api` directly) to read the issue +before touching anything. The body usually names the exact strings or files to change. + +## Remotes (assumed) + +- `origin` = your fork (push target) - resolve the owner from `git remote`, don't assume a name +- `upstream` = AERIUS org (PR base, `upstream/main`) - the PR repo (`--repo aerius/`) is the + `upstream` remote's owner/name + +If those names aren't set this way, stop and ask. + +## Roles + +Two roles appear below, normally the same person: +- **the author** - whose words go in the top zone of the PR body +- **the user** - who drives this session and approves the draft before it's created + +## The two-zone PR body + +The body has up to two clearly separated zones, so a reader can always tell the author's words from +the AI's: the author's own text on top - short, direct, sometimes less precise - and anything +machine-written quarantined below the line in an openly labeled block. The aim is that AI prose is +never mistaken for his/her own - that is the thing to avoid. + +**Top zone - the author's text (near-verbatim).** +The author supplies this, as the command argument or from his/her words in the conversation. Place +it almost exactly as given: +- Allowed edits: spelling, grammar, punctuation, and light style/consistency touchups. That is all. +- Not allowed: expanding, padding, restructuring, or rephrasing into different wording. Keep it + short and direct - the terseness is the feature, even at the cost of some precision. +- Preserve his/her voice and his/her hedges ("seems like", "might have been"). Do not sand + uncertainty off into confident claims. +- Plain text only in this zone: no headers, no bullets, no markdown ceremony. +- Fact-check it against the actual change before shipping (see step 4). If it contains a factual + error, handle it per that step - never silently fold a correction into the body. +- A trivial change (typo, rename) may have an empty top zone - and then usually no bottom zone + either. + +**Bottom zone - the AI section (optional, openly AI).** +Below a `---`, inside a collapsed `
` block whose `` marks it as AI-written. GitHub +renders `
`/`` in PR bodies, so it stays collapsed until a reviewer expands it - the +`` line is the label. +- This is machine prose in the AI's own voice. It may be more technical, more verbose, more + elaborate than the top. It may also be minimal when there is little to add - scale it to the + change, never pad it. +- Write it in the AI's own voice. Do **not** write it in the author's voice or borrow his/her hedges + - the whole point is that it reads as clearly not-him/her. +- Include it only when there is technical substance worth recording. It is never mandatory; trivial + changes skip it, and skip it too if the author only wants his/her own text. + +Shape: + +``` + + +--- +
+LLM-generated technical summary + + +
+``` + +Example - the author's terse line on top, the AI's fuller account collapsed below: + +``` +Fixes a flaky select-source test. Most likely just flake from the rollout animation; +nothing nearby changed. + +--- +
+LLM-generated technical summary + +The "select source x" step clicks the source, which deselects it when it is already selected. +When that happens the following assertion can still pass during the ~150ms rollout animation, +while the close button remains clickable - the most likely explanation for the failure in run +660. The preceding runs all passed this test and nothing in the vicinity changed, so test flake +is the leading explanation over a real regression. +
+``` + +## Protocol + +### 1. Branch + +Stay on the feature branch you already developed on - do not create a new branch or reset. It should +be branched off `upstream/main`; if it's stale, that's the user's call, not a `reset`. If you're on a +branch with uncommitted work that belongs to the change, that's expected; if you somehow find +yourself on `main` with nothing to wrap up, stop and ask. + +### 2. Commit + +There are usually already commits from the session - just commit anything still outstanding (`git add +-A` is fine here); never `--amend` what's already there. + +Style (AERIUS): +- Title: `AER-#### - {what}` - dash with spaces, lowercase `{what}`, terse noun phrase +- Body (optional, only if the *why* isn't obvious from the title): one or two short plain-text lines. + No bullets unless genuinely listing distinct items. No headers. No "Changes:" preamble. No test + plans. +- No Claude attribution - no `Co-Authored-By`, no "Generated with" footer (see Hard rules). +- No `--no-verify`, no `--amend`. + +```bash +git add +git commit -m "AER-#### - {what}" +``` + +If a freeform task with no ticket: title is just `{what}` in the same terse style. + +### 3. Push to origin + +```bash +git push -u origin +``` + +### 4. Fact-check the top zone + +Before drafting, read the author's top-zone text against what the change actually does - the diff, +the commit, the code. Two failure modes to catch - both would give a reviewer the wrong picture of +the change: + +1. **Factually wrong** - a claim that is plainly incorrect: names the wrong file or mechanism, + misstates the cause, says it does X when it does Y. +2. **Missing major content** - a distinct, significant part of the change that the text does not + acknowledge at all, such that a reader comes away with the wrong idea of what the PR is. + +What is NOT a failure, and must never be "fixed": +- Terseness, simplification, and honest hedges. The top zone is meant to be short and can trade + precision for brevity. Never sand these off into verbose precision. +- Omitting detail. The AI section carries the fine-grained account; the top zone needs only a fair + gist, not a changelog. Only *major* omissions count - a whole part of the change, not a nuance. + +When you find either: +- Say so in chat, separately, with the specific problem and the smallest correction that fixes it - + do not silently edit his/her text, and do not fold the fix into the body. He/she is the author; + he/she decides whether to adjust the wording, keep it, or let the AI section carry the missing + precision. +- If the top zone and the AI section contradict each other on a fact, that is a signal one of them + is wrong - surface it rather than shipping both. + +**On any mismatch between the author's text and the implementation, reconcile FIRST - never push +anything to force alignment.** A mismatch can mean the text is wrong, the implementation is wrong, +or both. Do not assume one side must yield to the other. Do not push *anything* to line them up - +not a rewrite of the text, and just as importantly not a change to the code / workflow / config to +match the words. Silently editing the implementation to fit the text is exactly as wrong as silently +editing the text to fit the implementation, and it may "fix" the wrong side (the text can be the +mistaken one). Stop, surface the mismatch in chat with both readings, and let him/her decide which +side is correct. Only after he/she decides do you touch either side, and only then push. + +Always state the verdict explicitly - e.g. "read against the diff, the top zone is accurate and +complete enough" - so the check is visible and never silently skipped. + +### 5. Draft the PR - do NOT create it yet + +Show the user: +- Branch name +- Commit subject(s) +- Proposed PR title - same as the commit subject for a single-commit PR; for a multi-commit PR, a + terse noun phrase covering the whole change in the same `AER-#### - {what}` style +- Proposed PR body - show both zones (see [The two-zone PR body](#the-two-zone-pr-body)): the + author's near-verbatim text on top, then the collapsed AI section if there is one. The top zone is + often empty or one line; never test-plan sections. +- The exact `gh pr create` command that would be run + +Format the draft so the user can eyeball it in one screen. Example: + +``` +Branch: aer-4444-warmte-inhoud → upstream/main +Title: AER-4444 - warmteinhoud naar warmte-inhoud +Body: (empty) + +Command: + gh pr create --repo aerius/ --base main --head :aer-4444-warmte-inhoud \ + --title "AER-4444 - warmteinhoud naar warmte-inhoud" --body "" +``` + +For a **non-empty** body - especially one with an AI `
` block - write the full body to a +temp file and pass `--body-file `, not `--body`. Multi-line HTML does not survive inline +`--body` cleanly. + +Then wait for the user's go-ahead. + +### 6. Create the PR - only after explicit approval + +Run the `gh pr create` command shown in step 5. Return the PR URL. + +## Hard rules + +- No Claude/tool attribution anywhere - no `Co-Authored-By`, no "Generated with Claude Code" footer, + in commit messages, PR title, PR body, or branch names. This bans the *promotional* trailer, not + the AI section of the body: that section is a deliberate content label, and its `` stays + generic ("LLM-generated technical summary") - it never names or promotes the tool. +- The author's top-zone text is never rewritten or expanded - light copy-edits only (spelling, + grammar, style). All AI prose stays below the line, in the labeled collapsed block, in the AI's own + voice - never dressed up as the author. +- No emojis anywhere. +- No `--amend` and no force-push. +- Never `reset --hard` over a branch with work on it. +- Never run `gh pr create` until the user has approved the draft in step 5. diff --git a/agentic-skills/review-principles.md b/agentic-skills/review-principles.md new file mode 100644 index 0000000..e4adea0 --- /dev/null +++ b/agentic-skills/review-principles.md @@ -0,0 +1,145 @@ +# Code Review Principles + +Code review principles for Java/Spring Boot backend services, Vue.js frontends, database migrations, OpenAPI definitions, Terraform configs, and shared libraries. This document captures not just what to look for, but how to think about code review in a way that produces lasting quality improvements rather than superficial feedback. + +## Surface Review vs Design-Level Review + +Surface review operates on the code as written. It asks "is this correct?" about the lines in the diff. Design-level review operates on the design underneath. It asks "should this exist?", "what happens next quarter?", and "what does this choice prevent us from doing later?" + +The key differences: + +**Surface review reacts to syntax. Design-level review interrogates design decisions.** At the surface, a new field just needs the right type. Deeper, the questions are why the field exists, whether it duplicates something already derivable, whether it belongs in this object or another, and what happens when the domain evolves. + +**Surface review accepts the PR's framing. Design-level review challenges the premise.** When a PR adds an audit trail tracked per-field-change, the surface check is whether the tracking logic is correct. The deeper question is whether field-by-field tracking is the right approach at all, or whether versioning the entire record and deriving the diff would be more robust and future-proof. When a PR adds a new API endpoint, the surface check is the response shape; the deeper question is whether the endpoint should exist or whether the semantics belong on an existing one. + +**Surface review is local. Design-level review thinks across time and space.** A local read only sees the current PR. A wider read considers: what happens when this queue message gets replayed? What happens when we add support for another region? What if the file gets re-uploaded? Will editing this flyway migration break every other developer's environment? Does this primary key actually allow the cardinality we need, or will it silently cap at 2 rows? + +**Surface review gives answers. Design-level review asks questions.** The phrasing matters. "This is wrong" closes discussion. "Wouldn't it make more sense to...?" opens a conversation that often reveals context the reviewer didn't have. Treat the author as someone who might know something you don't, while still holding him/her to high standards. + +**Surface review is inconsistent. Design-level review is relentless.** Missing newlines get caught only sometimes at the surface; a relentless pass catches them every time, across every file type, in every PR, without exception. Consistent enforcement is what keeps a standard real - enforce it selectively and it quietly becomes optional. + +## Review Against the Feature, Not Against the Diff + +This is the single most important distinction. A diff-only read checks whether the code is internally consistent. Reviewing against the feature starts by understanding what the change is supposed to accomplish, how it fits into the product, and what the user's actual experience will be - then evaluates whether the implementation faithfully serves that intent. + +This means: + +- **Catching the wrong approach, not just wrong code.** When code tracks audit trail changes field-by-field, it might be perfectly correct code - but if the feature needs to retroactively show history for fields added later, the entire approach is wrong. Only someone who understands the feature's trajectory catches this. +- **Knowing what the feature needs to survive.** If a feature needs to keep running while the user navigates between views, putting its state in a Vue component is structurally wrong regardless of how clean the component code is. Understanding the feature's lifecycle requirements is what reveals the architectural issue. +- **Recognizing speculative additions.** When an API endpoint or database field doesn't map to any actual user-facing need, a careful reviewer asks "what is this for?" - not because the code is wrong, but because unused surface area is future maintenance cost. Understanding the feature scope lets you distinguish necessary from speculative. +- **Evaluating naming against domain reality.** When a field is called `reportFile` but the feature treats it as the record's main file, someone who understands the domain catches that the name misrepresents the concept. When an API uses `createdDate` but the value includes a time, someone who understands the data catches that `createdDateTime` or `createdTimestamp` is correct. +- **Anticipating feature evolution.** If the current PR targets one region but others are on the roadmap, a careful reviewer checks whether the approach accommodates that. If the API returns 100,000+ rows in a single list, someone who understands the data scale catches that this won't work. + +This requires the reviewer to have context beyond the PR description. It means understanding the Jira ticket, the product requirements, the roadmap, and the existing architecture well enough to judge whether the implementation is the right shape for the problem. + +## Core Instinct + +Before evaluating how something is implemented, question whether it should exist. Every line of code, documentation, configuration, and abstraction is a future maintenance burden. The default disposition is skeptical: prove that something earns its place. + +This applies at every level: does this endpoint need to exist? Does this field belong on this object? Does this index help any actual query? Does this documentation say anything the code doesn't? Does this test cover real behavior or a mock that will be replaced? + +## Naming & Semantics + +Names are the primary vehicle for understanding code. Treat naming issues as design issues, not cosmetic ones. + +- Names should describe what something *is*, not how it's currently used. If a name will mislead future readers or become wrong when context changes, fix it now. A field called `reportFile` that is really just the main file should be called `mainFile`. A column called `client_specifics` that contains data not specific to any client should be renamed. +- Avoid domain-specific jargon that will date or change. Prefer stable, generic terminology over terms tied to current policy or legislation. A name like `regionProjectData` outlives one tied to a specific law or permit type. +- Boolean names should make `true` the expected/default behavior. If the common case is enabled, name the flag for the uncommon case (e.g. `disableCache` not `enableCache`) so that `false` is the safe default. +- Enum values, constants, and type names should be self-explanatory without reading surrounding code. If a name requires a comment to explain, the name is wrong. +- Prefixes and suffixes should carry consistent meaning across the codebase. If other tables use `code` for identifier columns, follow that convention rather than inventing a different name for the same concept. If stores use `useXYZStore`, match it. + +## Layering & Boundaries + +Code should be organized so that each layer knows only what it needs to. Layer violations are not style issues - they are structural defects that make code harder to change, test, and reason about. + +- Domain objects carry data. They should not contain serialization logic, infrastructure concerns, or business rules. Use the framework's mechanisms (e.g. Spring-managed ObjectMapper) rather than embedding them in domain types. +- Internal identifiers (database IDs, auto-increments) should never leak through APIs. Use stable external identifiers (UUIDs) that are independent of storage. +- If a service or utility already handles a concern, use it. Don't reimplement in another layer because it's locally convenient. +- Logic tied to a UI component's lifecycle but semantically independent of it (polling, tracking, async state) belongs in a store or service, not in the component. Components get destroyed on navigation; stores survive. +- Configuration, security filters, and similar infrastructure should live in dedicated classes, not be inlined into config objects alongside bean definitions. Testability follows from separation. +- Don't introduce package cycles. If class A imports from package B and class B imports from package A, restructure. +- When a class depends on an enum or type from a different layer (e.g. a service importing a resource-layer enum), that type probably belongs in a shared location. + +## API & Schema Design + +APIs and database schemas are the hardest things to change later. Review them with disproportionate scrutiny. A bad field name in a Java class costs minutes to fix; a bad column name in a migration costs a new migration. + +- API responses should not expose more than the consumer needs. Don't return full object trees when IDs suffice. Don't bundle audit trails into general fetch responses. +- Prefer separate endpoints for semantically different operations over overloaded ones. A method that fetches a record and optionally includes its audit trail should be two methods. +- Question whether the frontend actually needs what the API returns. If the frontend could derive something from data it already has, don't add a server-side endpoint for it. +- In OpenAPI definitions: don't repeat descriptions across endpoints, use `$ref` for shared schemas, don't over-document what the schema already expresses. If a description just restates the field name, remove it. +- Database migrations are append-only history. Never edit existing migration files - this violates the migration tool's contract and breaks every other developer's environment. +- Constraints and indices should justify themselves. Don't add indices for columns that will only be filtered in the frontend. Don't add `ON DELETE CASCADE` when records should never be deleted. An index on a primary key component adds nothing over the implicit PK index. +- Question composite primary keys: make sure they actually allow the cardinality you intend. A PK of `(parent_id, is_current)` only allows 2 rows per parent. +- Triggers should operate on the correct scope. A trigger that updates all rows instead of filtering by the relevant ID is a silent data corruption bug. + +## Understand the Surrounding System Before Reviewing Code + +The most impactful review feedback is not "this line has a bug" but "this entire approach is unnecessary." To catch that, you must understand what already exists before evaluating new code. + +- **Read beyond the diff.** Before reviewing feature code, look at the system the change interacts with - schemas, APIs, services, configuration, infrastructure. Understand what capabilities already exist and what design decisions have already been made. +- **Ask: does the existing system already solve this?** Many wrong approaches exist because the author didn't fully understand what was already available. An append-only table already encodes history. A framework already provides the middleware. An existing service already computes the value. A configuration option already controls the behavior. If the system already handles it, adding code to handle it again is the wrong approach regardless of code quality. +- **Work with the existing design, not against it.** If the system uses an append-only pattern, derive from it rather than tracking separately. If the framework provides a hook, use it rather than reimplementing. If an API already returns the data in a different shape, reshape rather than re-fetch. Code that ignores the design of the system it lives in will be fragile and redundant. +- **Complexity is a signal.** If an approach requires significant new machinery for something that feels like it should be simple, that's often because a simpler approach exists that works with the system rather than around it. Trust that instinct and investigate before accepting the complexity. + +## Duplication & Derivability + +Every piece of duplicated knowledge is a future inconsistency. + +- Prefer computing or deriving data from existing state over storing it separately. If you can reconstruct audit trails from versioned records, don't also track individual field changes. This is more robust: when a new field is added, derivation automatically picks it up; explicit tracking silently misses it. +- When adapting code from another class or module, adapt it to its new context. Don't carry over assumptions, imports, naming, or patterns from the source that don't apply. When adapting from an existing service, understand what each line does and whether it still applies rather than copying it wholesale. +- Documentation that restates what code or configuration already expresses is a maintenance liability. Terraform already contains the infrastructure values; don't also list them in a markdown file. +- If the same validation/mapping/transformation logic appears in multiple places, extract it. But only if the duplication is actual (same semantics), not incidental (same characters). + +## Consistency Over Novelty + +- Follow the patterns established in the codebase. If the codebase uses explicit types, don't introduce `var`. If tests are prefixed with `test`, do the same. Consistency across the codebase matters more than local improvement. +- Use normal imports, not star imports, unless there's a genuine conflict. Fully qualified class names in test code signal unfamiliarity with the import conventions. +- When a PR includes unrelated changes (reformats, cherry-picks from main, whitespace changes), flag them. They pollute the diff and make the actual change harder to review. Cherry-picks should be rebases. + +## Testing + +- Tests should assert specific, meaningful things. An assertion that "there is a warning" without checking which warning is a test that can silently pass for the wrong reason. +- Use static test data, not random values. `"MY_LOVELY_KEY"` is debuggable; `UUID.randomUUID()` is not. When a test fails, you should be able to grep for the value. +- Add messages to assertions. When a test fails, the message should tell you what went wrong without reading the test source. +- Integration tests that require external infrastructure (databases, queues) should be named with an `IT` suffix for selective execution in CI. +- Don't add tests for mocked implementations that will be replaced. Tests should cover real behavior. +- Use `assertEquals` unless you specifically need reference identity (`assertSame`). Be deliberate about which assertion you choose. +- Verify collection sizes before comparing elements. If you only check elements but not count, extra items go unnoticed. + +## Formatting & Hygiene + +These are enforced every time, not selectively. The point is not that any individual missing newline matters - it's that inconsistent enforcement teaches the team that standards are optional. + +- Missing newline at end of file. +- Unnecessary blank lines, especially multiple consecutive ones. +- Trailing comments should be above the line, not beside it. +- Comments that restate what code does are noise. Remove them. +- Unused imports, unused variables, unused methods - remove them. +- Operator placement: `&&` and `||` go on the next line, not at the end of the previous one. +- Weird indentation, even when auto-formatter caused it, should be called out. + +## Documentation Skepticism + +- Documentation should only exist if it tells you something the code doesn't. If it can be derived from reading the source, it's redundant. +- Be suspicious of generated documentation. If it reads like generic knowledge rather than project-specific insight, it doesn't belong - remove it. +- Over-documentation of API definitions (repeating type descriptions in endpoint summaries, explaining what each field does when the name is already clear) adds clutter and goes stale. +- If documentation describes environment setup, be precise about which platforms and which steps are required vs optional. Vague instructions waste more time than no instructions. +- Documentation that will go stale when a value changes (listing specific versions, port numbers, or config values that already live in code) is a liability, not an asset. + +## Visibility & Access + +- Question every `public` modifier. If a method or field doesn't need to be public, it shouldn't be. Package-private or private is the default; public is the exception that needs justification. +- The same applies to database exposure: don't return data through an API that the consumer doesn't need, even if it's convenient to include. +- Don't add configuration properties for things that are already the framework default. If Spring's default is what you want, don't specify it explicitly - it just creates a place where someone might change it to something wrong. +- Don't add pom.xml dependencies that are already provided transitively by a parent module. + +## Review Approach + +- **Ask, don't tell.** "Wouldn't it make more sense to...?" invites context. "This is wrong" closes conversation. The author may know something you don't, and framing as a question lets that information surface. +- **Express uncertainty honestly.** "Not sure this is needed" is more useful than silence or false confidence. It signals you've thought about it and want the author's input. +- **Acknowledge good solutions.** A brief note that an approach is clean costs nothing and reinforces good patterns. +- **Follow up on your own comments.** If you find the answer later in the PR, say so - don't leave the author guessing whether the concern still stands. +- **Review correctness, not just style.** Does the trigger update the right rows? Does the primary key allow the intended cardinality? Will this method throw a NullPointerException on unexpected input? Does this validation belong in this layer or should it happen earlier? Can this string parsing fail on unexpected input? +- **Think beyond the current PR.** Will this approach work when we add support for another region? Does this work if the file is re-uploaded? What happens if the queue message is replayed? What happens to this flyway migration on existing environments? +- **Review at multiple altitudes simultaneously.** In a single pass, catch both the architectural issue (this entire approach to audit trails is wrong) and the formatting issue (missing newline). Operating at every level in one pass, rather than one level at a time, is the goal. diff --git a/agentic-skills/review.md b/agentic-skills/review.md new file mode 100644 index 0000000..549b516 --- /dev/null +++ b/agentic-skills/review.md @@ -0,0 +1,228 @@ +--- +allowed-tools: Bash(gh pr view:*), Bash(gh pr diff:*), Bash(gh api:*), Bash(git log:*), Bash(git diff:*), Bash(git branch:*), Bash(git merge-base:*), Bash(git rev-parse:*), Read, Write, Grep, Glob, Agent +description: Review current branch changes using expert code review principles +--- + +## Context + +Branch: !`git branch --show-current` +PR status: !`gh pr view --json title,body,baseRefName,url 2>&1 || echo "No PR found for this branch"` + +## Review Principles + +See [review-principles.md](review-principles.md) for the full rationale behind each lens below. The +lenses are also self-contained - each carries its own mandate and calibration - so the review runs +without it. + +## Task + +You are orchestrating a multi-lens code review. Each lens is reviewed by a dedicated sub-agent that hyperfocuses on that lens only - a single agent trying to hold all lenses in scope invariably skims the ones that don't match its current thread of thought. Your job is scope setup, parallel dispatch, and synthesis. **Do not perform the lens reviews yourself.** + +### 1. Collect scope artifacts ONCE (don't make sub-agents re-do this) + +Run all of these and capture the output: + +- Merge base: `git merge-base upstream/main HEAD` +- If a PR exists, read its title, body, and base branch (use the PR's base if it differs from `upstream/main`). +- Commit log: `git log --oneline ..HEAD` +- Diff stat: `git diff --stat ...HEAD` +- **Full diff**: `git diff ...HEAD > /tmp/review-.diff` - write it to a file once; sub-agents Read the file rather than re-running `git diff`. + +This collected block is the **shared scope context**. Every sub-agent gets the same block and the same diff-file path. + +### 2. Spawn one sub-agent per lens, in parallel + +Send a single message containing one `Agent` tool call per lens (`subagent_type: general-purpose`). Each sub-agent's prompt must include: + +1. The shared scope context (branch, merge base, PR title/body, commit log, diff stat). +2. The path to the pre-collected diff file. Instruct them to Read it first, and only `Read`/`Grep`/`Glob` the working tree for surrounding-system context (existing patterns, related files, conventions). **They should NOT re-run `git diff`.** +3. **Only their lens's mandate** (below). Do not leak other lenses' rubrics into their prompt. +4. The findings format from §3. +5. Explicit stay-in-lens instruction. If they notice something outside their lens, they may note it briefly under "Out of lens" but must not expand. +6. **Prohibited shell commands - pass this verbatim to every sub-agent:** The reason this rule exists is `.gitignore` awareness - plain `grep`/`find`/`ls`/`rg` recurse into `node_modules` / build artifacts / vendored deps and blow up context, and trigger permission prompts that break flow. Permitted (use these): the dedicated **Grep** and **Glob** tools (preferred - they respect `.gitignore`), and as functionally-equivalent fallbacks `git grep` and `git ls-files` (covered by `Bash(git:*)` allow). For file content, use **Read** (not `cat`/`head`/`tail`). Forbidden as Bash commands: standalone `grep`, `rg`, `find`, `ls`, `cat`, `head`, `tail`, `file`. Don't pipe `git grep` output through `head`/`tail`/`grep` either - use its own flags (`-n`, `-m`, `--max-count`) or accept the full output. This applies to every search, listing, and file-read operation the sub-agent performs. +7. **Universal posture (pass this to every sub-agent verbatim):** Returning "no findings" is a valid outcome - many diffs won't trip your lens, and saying so plainly is the right answer when nothing's wrong. Do NOT pad your output with stylistic preferences, alternative phrasings, or speculative concerns just to justify having looked. The bar for surfacing a substantive finding is "I'd want the author to see this." Anything below that bar - pedantic preferences, things that don't actually matter, "I'd have spelled this differently" - goes in the **Picky** bucket (see §3), which the synthesizer drops by default. Contribute Picky items only because you noticed them and they may be optionally available; don't contribute them because you think the user should act on them. + +**Lenses:** + +#### Lens 1 - Approach & Feature Fit +Challenge the premise. Does this change need to exist? Is it building something the system already provides in a different form? Is it working *with* the existing design or bolting on around it? Is it more complex than the problem warrants? Does the approach survive foreseeable evolution (new countries, re-uploads, replayed queue messages, added fields)? A correct implementation of the wrong approach is still wrong - say so even when the code is clean. + +#### Lens 2 - Architecture & Layering +Layer violations, boundaries, placement. Does logic live in the right layer? Do domain objects carry framework/serialization concerns they shouldn't? Does UI-lifecycle-bound state (polling, tracking, async) survive navigation, or is it stranded in a component? Are internal IDs (auto-increments, DB PKs) leaking through APIs instead of stable external identifiers? Package cycles? Types that should move to a shared location because they cross layers? + +#### Lens 3 - API & Schema Design +APIs, schemas, migrations, constraints, indices. Do responses expose more than consumers need? Are overloaded endpoints masking semantically separate operations? Do OpenAPI descriptions add signal or restate field names? Are migrations append-only (never edited)? Do constraints and indices justify themselves against actual queries? Do composite PKs permit the intended cardinality? Do triggers operate on the correct scope? For frontend-only PRs, apply this lens to **component prop surfaces** - overloaded props, mode flags, prop-vs-slot decisions. + +#### Lens 4 - Naming & Semantics + +**High bar.** Only flag a name when it is *actively misleading* - it tells a reader something untrue about what the thing is, or it will mislead a future maintainer once the context shifts. Stylistic preferences, "could be slightly clearer", alternative phrasings, and bikeshedding go in **Picky**, not in substantive findings. If a finding amounts to "I'd have called it something else", it's Picky at most. + +Concrete things that *do* qualify: + +- **Type/semantic mismatch.** A name claims one thing, the value is another: `createdDate` that is actually a timestamp; `reportFile` that is the record's main file, not a report; `clientSpecifics` on a field not specific to clients. +- **Boolean polarity inverted.** `true` should be the expected/default behavior. `disableCache: false` is wrong shape; rename to `enableCache: true`. +- **Name claims a guarantee the impl doesn't keep.** `findActiveId` that returns "the first pending record" - caller will assume the wrong contract. +- **Established convention violated** in a way readers will trip on (codebase uses `IT` suffix for integration tests and this one doesn't; codebase uses `useXyzStore` for stores and this is `xyzStore`). + +Things to skip even if they bug you: a slightly long name; a name you'd have shortened; a name that's accurate but plain; "Collector" vs "Resolver" debates when both are defensible; lambda variable names in short scopes. + +**Self-qualification gate - do this FIRST.** Skim the diff for names. For each candidate, ask: "would a reader form a false belief about this thing because of this name?" If no, skip it. If you cannot point to a specific false belief the name induces, return "no findings" with a one-line note. **Naming concerns, when flagged, default to Quick fix** - they only escalate to Question or Needs-attention when the misleading name is causing actual downstream confusion (wrong consumer assumptions, contract drift, test miscoverage). + +#### Lens 5 - Correctness, Testing & Duplication +Real-input behavior: edge cases, error handling, null/undefined, concurrency, replay, idempotency, boundary conditions. Tests: specific assertions (not "something warned"), static/debuggable test data (not random UUIDs), assertion messages, `IT` suffix for integration tests, no mock-only coverage masquerading as real. Duplication of knowledge: values or state stored separately that could be derived from existing sources of truth. + +#### Lens 6 - Hygiene +Formatting, unused code, visibility, consistency with codebase conventions, documentation necessity. Missing trailing newlines, redundant consecutive blank lines, trailing comments that should be above the line. Unused imports / variables / methods. `public` that could be package-private or private. Comments that restate what code already says. Generated-looking or AI-flavored documentation. Inconsistent import style (star imports, fully-qualified names, `var` vs explicit types, `useXyzStore` naming drift). Unrelated changes polluting the diff. + +#### Lens 7 - Security & Trust Boundaries + +Vulnerabilities at trust boundaries: untrusted input crossing into rendering / persistence / execution sinks, auth/authz changes, secrets and credential handling, network egress with user-controlled targets, deserialization. Think in categories - where is the boundary, what's the threat model for this code path, would a hostile input reach a sink? + +**Self-qualification gate - do this FIRST.** Skim the diff for security-relevant signals in each layer touched (frontend: rendering of user strings, dynamic redirects, token storage; backend: SQL/HQL string building, security filters, file/URL handling, deserialization; infra/migrations: secrets, IAM, sensitive columns). **If no signals appear in the relevant layer, return "no findings" with a one-line note saying which categories you checked.** Do not fabricate concerns to justify the lens - most diffs have nothing for this lens, and that's the right answer. + +When signals do appear, dig in: trace the untrusted input from source to sink; check that the boundary enforces the constraint the threat model requires. + +### 3. Findings format (each sub-agent returns this) + +``` +## Lens: + +### Blockers +- `path/to/file:line` - one-line rationale +(or "None.") + +### Questions +- `path/to/file:line` - ask-don't-tell phrasing ("Wouldn't it make more sense to ...?") +(or "None.") + +### Good +- `path/to/file` - acknowledgment +(or "None.") + +### Picky (synthesizer drops by default - surface only if everything else is empty) +- `path/to/file:line` - pedantic note in ≤10 words (preference, alt phrasing, "I don't actually care") +(or "None.") + +### Out of lens (brief notes only) +- ... +(or "None.") + +### No findings +If the lens had nothing substantive to flag, say so. +``` + +### 4. Synthesize and assign severity + +After all sub-agents return, group findings by topic - the same concern flagged by multiple lenses becomes one item. Assign each item one severity: + +- **Critical** - must fix before merge. Real bugs, correctness defects with a traced crash/breakage path, security issues with confirmed exploitability, regressions of existing behavior. +- **High** - should fix before merge unless explicitly deferred. Latent invariants with no guardrails, serious architectural smells that will be expensive to reverse, missing tests on load-bearing new logic. +- **Medium** - worth deciding before merge. Real tradeoffs where the reviewer can't pick without your context (consumer expectations, product intent, parked-for-next-PR judgment). The item has an actionable choice attached, not just "is this OK?". +- **Low** - polish. Dead code, naming, hygiene, minor cleanups. Always one-liners. + +**Map from sub-agent buckets:** sub-agent "Blockers" usually become Critical or High. Sub-agent "Questions" usually become Medium (or Low if trivial). Sub-agent "Picky" gets dropped by default (see below). + +**Hedging → Medium, not High.** If your synthesis prose is naturally hedging ("I'm not sure if...", "this might be wrong because..."), the item is Medium with a `Decide:` line, not High with an `Action:` line. Forcing a hedged thought into a declarative finding produces false confidence and wastes review attention. If you can't articulate the doubt at all, drop the item. + +**Picky-bucket handling.** Sub-agents return a "Picky" bucket - pedantic preferences they noticed but explicitly don't want acted on. **By default, drop the entire Picky bucket.** Only exception: if the report would otherwise be entirely empty (zero Critical/High/Medium/Low), render `## Picky (you can ignore)` with one-line items and note in Overall that nothing substantive surfaced. + +**Filter pedantry yourself - the synthesizer is a gate, not a passthrough.** Before rendering an item, ask: does this need the user's eye, or is this something I (the synthesizer) can handle, dismiss, or absorb? Examples of things to filter rather than surface: +- A sub-agent flagging a comment that "could be shorter" - drop, it's noise. +- A sub-agent suggesting a method "might be cleaner if split" - drop unless there's a concrete reason. +- Multiple lenses raising the same trivial naming concern - drop entirely, don't even render once. +- A sub-agent asking "wouldn't it be better to X" where X is a defensible alternative but the current code is also defensible - drop. +- A sub-agent surfacing an architectural concern the PR body already explicitly parks for a next PR - drop unless the parking is wrong. +- A sub-agent flagging missing tests where the codebase pattern is light testing in this layer - drop unless the new code is load-bearing. + +The bar for surfacing anything is "I'd want the user to see this." If it doesn't clear the bar, the synthesizer eats it silently. The user does not want a comprehensive enumeration of everything noticed; he/she wants the actionable subset, severity-ordered. A clean report with 3 substantive items is better than a thorough report with 12 where 9 are noise. + +**Calibration:** if an item collapses to "rename this," it's Low - title + file:line + ≤10 words. If a Medium item resolves into "do X," promote to Critical/High (it's a position, not a question). Err toward brevity; short, honest report is the goal. + +### 5. Adversarially verify findings before rendering + +The synthesized list is a set of *candidates*, not the report. Lens agents skim, misread the diff, and occasionally invent a symbol or a code path that isn't there - a plausible-sounding finding is not the same as a real one. Before rendering, stress-test each substantive finding by trying to **refute** it against the actual code, and drop the ones that don't survive. This gate exists to catch false alarms, which cost the reader far more trust than a missed nitpick. + +**Scope.** Verify every Critical, High, and Medium item. Low/Picky items are cheap one-liners the reader can dismiss at a glance - spot-check them only if something looks off; don't spend a full verification pass on them. + +**Who runs it - orchestrator vs sub-agents.** Pick whichever is faster for the count in hand: +- **Few substantive findings (roughly ≤4):** the orchestrator verifies them itself, inline. Spawning agents for a handful costs more latency than it saves. +- **Many findings:** fan out one verifier sub-agent per finding (or small batches) in a single parallel message so they run concurrently - much faster than checking them one after another. Use `subagent_type: general-purpose`, and pass the same prohibited-shell-commands rule from §2 verbatim. + +**Refutation mandate (apply to each finding).** Assume the finding is FALSE and try to prove it. Re-read the real code - the pre-collected diff file and the surrounding source via Read/Grep/Glob - not just the finding's own text; the whole point is to catch claims that don't match what's actually there. Check specifically: +- Does the cited `file:line` actually contain what the finding claims (the symbol exists, the code does what's described)? +- Is the described failure path actually reachable, or is it already guarded/handled somewhere the lens agent didn't look (caller, validation, framework, a later line)? +- Does the finding rest on a misread of the diff - a removed line taken as added, the wrong branch, stale surrounding context? +- For "missing test / missing handling" claims: is it genuinely absent, or present in a location outside the diff? + +Return one verdict per finding: **confirmed** (survives, cite the evidence), **false** (refuted - state why), or **overstated** (real but weaker than claimed - give the corrected severity). + +**Filter.** +- **Drop every `false` finding entirely** - do not render it, do not mention it, not even as a footnote. +- Keep `confirmed` findings; fold the verifier's evidence into the diagnosis where it sharpens the claim. +- For `overstated` findings, render at the corrected lower severity, or drop to Low/Picky if it collapses to a preference. +- **When a finding cannot be confirmed against the code, drop it.** An unverifiable finding is treated as false. Prefer a false negative (a marginal real issue dropped) over a false positive (a wrong finding shipped as Critical). + +State the tally in the output's opening line (see §6): how many candidates were verified and how many were dropped as false or overstated, so the filtering is visible rather than silent. + +### 6. Output structure + +Render only the findings that survived §5 verification. Single readable pass, severity-categorized. No rounds, no pagination, no separate "Questions" section. Every Critical/High/Medium item has the same shape - title, location, brief diagnosis, an Action or Decide line that tells the reader what to do. + +``` +Found items - critical, high, medium, low. ( dropped in verification.) + +## Critical +### - `path/to/file:line` +<1-3 sentences for a cold reader: what the code does, what's wrong, what breaks. Name symbols the first time they appear.> +**Action:** <Concrete fix sketch, ≤2 sentences. Not abstract - "Add null check and route to onFailure," not "handle the null case."> + +(repeat per item, or "None.") + +## High +### <Title> - `path/to/file:line` +<1-3 sentences.> +**Action:** <Fix sketch, or "Options:" with 2-3 paths if more than one defensible fix exists.> + +(repeat, or "None.") + +## Medium +### <Title> - `path/to/file:line` +<1-3 sentences: the context and the choice. State the tradeoff plainly.> +**Decide:** <What to weigh. Name 2-3 options and the trade-off, picking one if you have a defensible preference. Make explicit that the call is the user's.> + +(repeat, or "None.") + +## Low +- `[hygiene]` `path/to/file:line` - <≤10 words: what to change> +- `[naming]` `path/to/file:line` - <≤10 words> +- `[nit]` `path/to/file:line` - <≤10 words> +... + +(or "None.") + +## Overall +<One short paragraph. What this PR does and your candid read - clean and tight, working with the existing design or fighting it, right approach or wrong one. If it's a good change, say so plainly; if it's a slog, say that. Call out genuinely good patterns worth keeping if any stand out.> + +## Lens coverage +| Lens | C | H | M | L | +|---|---|---|---|---| +| Approach & Feature Fit | 0 | 1 | 2 | 0 | +| Architecture & Layering | ... | ... | ... | ... | +... +``` + +**Title:** noun phrase or short imperative - "OrderSummary returned partial", "Drop unused syncLegacyData", "Multi-flag silent overwrite". Not a full sentence, not a question. + +**Location:** `file:line` or `file:line-range`. Forward-slash paths. If an item spans multiple sites, list the primary in the header and mention the others in the diagnosis. + +**Diagnosis (1-3 sentences):** written for a cold reader. Open with what the code is so the reader doesn't need to flip to the file. Name symbols the first time they appear; don't say "the new method" without naming it. State what's wrong directly - no "I think," "maybe," "could be" hedging. If you're hedging, the item is Medium not High. + +**Action line (Critical/High):** concrete fix. Not abstract. Bad: "handle the null case." Good: "Add `if (payload == null) { onFailure(...); return; }` at the top of the success handler." If there are multiple defensible fixes, lead with `Options:` and name 2-3 with the trade-off, then pick one. + +**Decide line (Medium):** name the choice + 2-3 options + trade-offs. The synthesizer can have a preference; the reader makes the call. Bad: "Is this the right approach?" Good: "Either (a) fold into the shared restore util now and parameterize on data source, or (b) keep two paths with a comment marking the duplication as transitional. (a) reduces future drift; (b) is cheaper for this PR. (a) if the next PR will need the shared util anyway." + +**Low items (one-liner each):** `[tag]` `path/to/file:line` - ≤10 words. Tags: `[blocker]`, `[bug]`, `[minor]`, `[hygiene]`, `[naming]`, `[nit]`, `[doc]`, `[test]`. No paragraph rationale - trust the reader to greenlight. + +### 7. Do NOT post comments to GitHub + +Present the synthesized review to the user. He/she decides what gets submitted. diff --git a/agentic-skills/trim-comments.md b/agentic-skills/trim-comments.md new file mode 100644 index 0000000..9ac9598 --- /dev/null +++ b/agentic-skills/trim-comments.md @@ -0,0 +1,103 @@ +--- +allowed-tools: Bash(git diff:*), Bash(git log:*), Bash(git branch:*), Bash(git merge-base:*), Bash(git rev-parse:*), Bash(gh pr view:*), Bash(gh pr diff:*), Read, Edit, Grep, Glob, Agent +description: Trim comments in the current changeset toward self-documenting code; relocate real rationale to a suggested commit message +--- + +## Context + +Branch: !`git branch --show-current` +PR status: !`gh pr view --json title,body,baseRefName,url 2>&1 || echo "No PR found for this branch"` + +## Philosophy + +The goal is self-documenting code: code simple enough that it doesn't need a comment to be +understood. A comment earns its place only when the code genuinely cannot be made clear enough on +its own. + +There are two different kinds of "why," and they go in two different places: + +- **Why the code is shaped this way, permanently** - a hidden constraint, a subtle invariant, a + workaround for a specific external bug, something that would surprise a future reader. This is the + only thing allowed to live in a comment, and only when the code itself can't express it. +- **Why the change was made** - alternatives considered, what was tried first, the reasoning behind + picking this approach, deliberation. This is history, not documentation. It belongs in the commit + message. `git blame` should be what answers "why did this change," not a comment sitting next to + the code. + +Never leave in place: +- Comments that restate what the code already says. +- Comments that narrate the author's thought process ("decided to...", "tried X but switched to Y + because...", "not sure if this is right but..."). +- Comments referencing other, unrelated projects, repos, or code - they rot the moment that context + changes and mean nothing to a reader who wasn't there. +- Commented-out code, changelog-style comments, apologies, hedges, section-banner noise. + +Always defer to a project's own documented comment conventions (its CLAUDE.md / AGENTS.md) when +they set a different or stricter bar than this default - e.g. required Javadoc on public APIs, +license headers, mandatory linter-directive justifications, generated-code markers. Those survive +regardless of this rubric. + +## Task + +### 1. Determine the changeset scope + +- If a PR exists for this branch, use its base ref (`baseRefName` above). +- Otherwise fall back through common bases via `git merge-base`: try `upstream/main`, + `origin/main`, `main`, `master`, whichever exists. +- Full diff: `git diff <merge-base>...HEAD` + +Scope is comments **added or modified by this diff** (they appear on `+` lines). Do not touch +comments elsewhere in a touched file that this diff didn't add or change - editing those is +unrelated-change scope creep. If one is egregiously bad, note it, don't fix it. + +### 2. Extract every comment touched by the diff + +Across whatever comment syntax the changed files use (`//`, `#`, `/* */`, `--`, `"""`, `<!-- -->`, +etc). For a large diff (many files), fan out one `Agent` call per file or small group in parallel; +for a small diff just do it inline. Every judgment needs the actual surrounding code in view - never +classify a comment from the diff hunk alone if the file has more context worth checking. + +### 3. Classify each comment into exactly one bucket + +- **Keep** - explains a genuinely non-obvious, permanent why. The code can't say this on its own. +- **Shorten** - same as Keep, but padded. Compress to the one clause that carries the actual + information; cut the rest. +- **Cut + relocate** - the comment is deliberation or decision history: interesting, but it's about + the *change*, not the code. Delete it from the file, carry its substance into the commit-message + draft in step 5. +- **Cut + discard** - adds nothing anywhere. Restates the code, states the obvious, references + unrelated code/projects, is commented-out code, is boilerplate/apology/banner noise. Delete, keep + nothing. + +### 4. Apply edits + +Edit the comments directly per the classification above. Touch only comments - never change code +logic, formatting, or anything outside the diff's own scope. Keep each edit minimal: a Shorten edit +changes only the comment text, not its surrounding code. + +### 5. Draft commit-message notes from everything Cut + relocated + +Turn the relocated rationale into short bullets, phrased for a commit body - plain text, no +headers, matching how this repo actually writes commit messages (check `git log` for the local +style). This is a draft for the user to use, not something to commit. + +### 6. Do not commit, amend, or push + +This command edits working-tree files only. Never run `git commit`, `git commit --amend`, or any +push - those are for the user to do, and amending an existing commit violates linear-history rules +regardless. + +## Output + +For each file touched, a compact before/after per comment with its bucket and one-line reason. +Then, if anything was Cut + relocated, a clearly separated block: + +``` +## Suggested commit message notes +(paste into your commit message if you want this history kept - not applied automatically) + +- <bullet> +- <bullet> +``` + +If nothing was relocated, omit that block entirely rather than printing it empty. diff --git a/agentic-skills/trivial-pr.md b/agentic-skills/trivial-pr.md new file mode 100644 index 0000000..0f8ebeb --- /dev/null +++ b/agentic-skills/trivial-pr.md @@ -0,0 +1,259 @@ +--- +allowed-tools: Bash(git:*), Bash(gh:*), Bash(jira-api:*), Bash(jq:*), Read, Edit, Write, Grep, Glob, Skill +description: AERIUS PR protocol for one small standalone change - a typo/label/mechanical fix made from a fresh branch, kept minimal +argument-hint: [AER-####] and/or the change to make +--- + +## When to use + +A single small standalone change - typo fixes, label renames, single-paragraph doc updates, +mechanical search-and-replace. The kind of thing where a reviewer sees one or two lines of diff and +immediately knows it's right. The change is made *under* this protocol, from a fresh branch, kept +minimal. + +If it grows beyond a handful of obvious lines - new behavior, restructuring, anything where the right +answer isn't obvious - STOP and hand back. This skill is not for that: develop it properly and finish +with `human-pr` instead. + +Same house style as `human-pr`: same commit/PR conventions, same no-attribution rule, same +draft-before-create gate, same two-zone body. The difference is only the front end - this skill +starts a fresh branch and holds the change to a minimal diff. + +## Inputs + +`$ARGUMENTS` may be: +- An AERIUS issue key (e.g. `AER-4444`) - fetch context via the jira skill first +- Freeform description of the change - use it directly +- Your own PR body text - the words that go in the **top zone** of the body (see [The two-zone PR + body](#the-two-zone-pr-body)), used near-verbatim and never rewritten. May also arrive from the + conversation rather than the argument. +- Empty - work from the prior conversation context + +If an issue key is present, invoke the jira skill (or run `jira-api` directly) to read the issue +before touching anything. The body usually names the exact strings or files to change. + +## Remotes (assumed) + +- `origin` = your fork (push target) - resolve the owner from `git remote`, don't assume a name +- `upstream` = AERIUS org (PR base, `upstream/main`) - the PR repo (`--repo aerius/<repo>`) is the + `upstream` remote's owner/name + +If those names aren't set this way, stop and ask. + +## Roles + +Two roles appear below, normally the same person: +- **the author** - whose words go in the top zone of the PR body +- **the user** - who drives this session and approves the draft before it's created + +## The two-zone PR body + +The body has up to two clearly separated zones, so a reader can always tell the author's words from +the AI's: the author's own text on top - short, direct, sometimes less precise - and anything +machine-written quarantined below the line in an openly labeled block. The aim is that AI prose is +never mistaken for his/her own - that is the thing to avoid. + +**Top zone - the author's text (near-verbatim).** +The author supplies this, as the command argument or from his/her words in the conversation. Place +it almost exactly as given: +- Allowed edits: spelling, grammar, punctuation, and light style/consistency touchups. That is all. +- Not allowed: expanding, padding, restructuring, or rephrasing into different wording. Keep it + short and direct - the terseness is the feature, even at the cost of some precision. +- Preserve his/her voice and his/her hedges ("seems like", "might have been"). Do not sand + uncertainty off into confident claims. +- Plain text only in this zone: no headers, no bullets, no markdown ceremony. +- Fact-check it against the actual change before shipping (see step 5). If it contains a factual + error, handle it per that step - never silently fold a correction into the body. +- A trivial change (typo, rename) may have an empty top zone - and then usually no bottom zone + either. + +**Bottom zone - the AI section (optional, openly AI).** +Below a `---`, inside a collapsed `<details>` block whose `<summary>` marks it as AI-written. GitHub +renders `<details>`/`<summary>` in PR bodies, so it stays collapsed until a reviewer expands it - the +`<summary>` line is the label. +- This is machine prose in the AI's own voice. It may be more technical, more verbose, more + elaborate than the top. It may also be minimal when there is little to add - scale it to the + change, never pad it. +- Write it in the AI's own voice. Do **not** write it in the author's voice or borrow his/her hedges + - the whole point is that it reads as clearly not-him/her. +- Include it only when there is technical substance worth recording. It is never mandatory. Because + this skill is for trivial changes by definition, the AI section is usually absent - or, when a + small change still hides a detail worth a note, a single line such as "nothing special to add" or a + one-liner on that detail. Scale it to the triviality; skip it too if the author only wants his/her + own text. + +Shape: + +``` +<author's own text, lightly cleaned up> + +--- +<details> +<summary>LLM-generated technical summary</summary> + +<technical prose in the AI's voice - as detailed as the change warrants, +or minimal when there is little to say> +</details> +``` + +Example - the author's terse line on top, the AI's fuller account collapsed below: + +``` +Fixes a flaky select-source test. Most likely just flake from the rollout animation; +nothing nearby changed. + +--- +<details> +<summary>LLM-generated technical summary</summary> + +The "select source x" step clicks the source, which deselects it when it is already selected. +When that happens the following assertion can still pass during the ~150ms rollout animation, +while the close button remains clickable - the most likely explanation for the failure in run +660. The preceding runs all passed this test and nothing in the vicinity changed, so test flake +is the leading explanation over a real regression. +</details> +``` + +## Protocol + +### 1. Branch + +Fresh branch off upstream/main. + +```bash +git fetch upstream +git checkout -b <branch-name> upstream/main +``` + +Branch name: short kebab-case derived from the task. For an AER ticket, lowercase the key as a prefix +(e.g. `aer-4444-warmte-inhoud`). If currently on a branch with uncommitted work, STOP - don't reset +over it. + +If a branch with this name already exists locally, pick a different name or ask. Never `reset --hard` +over an existing branch without confirmation. + +### 2. Make the minimal change + +Rules - these are the whole point of this skill: +- Only touch what the task literally asks for. No drive-by fixes, no neighboring cleanup. +- No new files unless the task requires it. No README or doc additions. +- No comments explaining the change. The diff and commit message are enough. If a comment genuinely + must exist (e.g. a one-line doc on a new shared constant), keep it short - one line, no backstory, + no ticket references. +- No reformatting of untouched lines. +- No "while I'm here" refactors. + +If the task is "fix this typo in these three places", the diff should be three lines. If it grows +beyond that, STOP and tell the user - the task probably isn't a fit for this skill; develop it +properly and use `human-pr`. + +### 3. Commit + +A single focused commit for the change you just made. + +Style (AERIUS): +- Title: `AER-#### - {what}` - dash with spaces, lowercase `{what}`, terse noun phrase +- Body (optional, only if the *why* isn't obvious from the title): one or two short plain-text lines. + No bullets unless genuinely listing distinct items. No headers. No "Changes:" preamble. No test + plans. +- No Claude attribution - no `Co-Authored-By`, no "Generated with" footer (see Hard rules). +- No `--no-verify`, no `--amend`. + +```bash +git add <specific-files> +git commit -m "AER-#### - {what}" +``` + +If a freeform task with no ticket: title is just `{what}` in the same terse style. + +### 4. Push to origin + +```bash +git push -u origin <branch-name> +``` + +### 5. Fact-check the top zone + +Before drafting, read the author's top-zone text against what the change actually does - the diff, +the commit, the code. Two failure modes to catch - both would give a reviewer the wrong picture of +the change: + +1. **Factually wrong** - a claim that is plainly incorrect: names the wrong file or mechanism, + misstates the cause, says it does X when it does Y. +2. **Missing major content** - a distinct, significant part of the change that the text does not + acknowledge at all, such that a reader comes away with the wrong idea of what the PR is. + +What is NOT a failure, and must never be "fixed": +- Terseness, simplification, and honest hedges. The top zone is meant to be short and can trade + precision for brevity. Never sand these off into verbose precision. +- Omitting detail. The AI section carries the fine-grained account; the top zone needs only a fair + gist, not a changelog. Only *major* omissions count - a whole part of the change, not a nuance. + +When you find either: +- Say so in chat, separately, with the specific problem and the smallest correction that fixes it - + do not silently edit his/her text, and do not fold the fix into the body. He/she is the author; + he/she decides whether to adjust the wording, keep it, or let the AI section carry the missing + precision. +- If the top zone and the AI section contradict each other on a fact, that is a signal one of them + is wrong - surface it rather than shipping both. + +**On any mismatch between the author's text and the implementation, reconcile FIRST - never push +anything to force alignment.** A mismatch can mean the text is wrong, the implementation is wrong, +or both. Do not assume one side must yield to the other. Do not push *anything* to line them up - +not a rewrite of the text, and just as importantly not a change to the code / workflow / config to +match the words. Silently editing the implementation to fit the text is exactly as wrong as silently +editing the text to fit the implementation, and it may "fix" the wrong side (the text can be the +mistaken one). Stop, surface the mismatch in chat with both readings, and let him/her decide which +side is correct. Only after he/she decides do you touch either side, and only then push. + +Always state the verdict explicitly - e.g. "read against the diff, the top zone is accurate and +complete enough" - so the check is visible and never silently skipped. + +### 6. Draft the PR - do NOT create it yet + +Show the user: +- Branch name +- Commit subject(s) +- Proposed PR title - same as the commit subject (a trivial change is a single focused commit) +- Proposed PR body - show both zones (see [The two-zone PR body](#the-two-zone-pr-body)): the + author's near-verbatim text on top, then the collapsed AI section if there is one. The top zone is + often empty or one line; never test-plan sections. +- The exact `gh pr create` command that would be run + +Format the draft so the user can eyeball it in one screen. Example: + +``` +Branch: aer-4444-warmte-inhoud → upstream/main +Title: AER-4444 - warmteinhoud naar warmte-inhoud +Body: (empty) + +Command: + gh pr create --repo aerius/<repo> --base main --head <fork>:aer-4444-warmte-inhoud \ + --title "AER-4444 - warmteinhoud naar warmte-inhoud" --body "" +``` + +For a **non-empty** body - especially one with an AI `<details>` block - write the full body to a +temp file and pass `--body-file <file>`, not `--body`. Multi-line HTML does not survive inline +`--body` cleanly. + +Then wait for the user's go-ahead. + +### 7. Create the PR - only after explicit approval + +Run the `gh pr create` command shown in step 6. Return the PR URL. + +## Hard rules + +- No Claude/tool attribution anywhere - no `Co-Authored-By`, no "Generated with Claude Code" footer, + in commit messages, PR title, PR body, or branch names. This bans the *promotional* trailer, not + the AI section of the body: that section is a deliberate content label, and its `<summary>` stays + generic ("LLM-generated technical summary") - it never names or promotes the tool. +- The author's top-zone text is never rewritten or expanded - light copy-edits only (spelling, + grammar, style). All AI prose stays below the line, in the labeled collapsed block, in the AI's own + voice - never dressed up as the author. +- No emojis anywhere. +- No `--amend` and no force-push. +- Never `reset --hard` over a branch with work on it. +- Never run `gh pr create` until the user has approved the draft in step 6. +- If the change grows beyond "small and uncontroversial," stop and hand back to the user - develop it + properly and use `human-pr` instead.