From 646f2ebbe631911c3494d42bc595759678373c73 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 28 Jul 2026 17:37:09 -0500 Subject: [PATCH 1/2] Add CLAUDE.md rules to improve Claude Code Review effectiveness The code-review plugin's compliance agents only enforce what CLAUDE.md states explicitly and quotably, scoped to CLAUDE.md files sharing a path with the changed files. It has no other repo-local config file. Adds concrete, quotable rules targeting the classes of bugs a recent CodeRabbit pass caught that the automated review missed in a SKILL.md rewrite: self-contradicting instructions after a late-stage section rewrite, skill steps that silently contradict this repo's own CLAUDE.md policies, undefined yes/no branches, bad markdown fences, and bats tests deriving new paths instead of reusing existing helpers like PGXNREPO. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 2331ac0..20a62e2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,6 +63,18 @@ The `/commit` skill lives in `.claude/skills/commit/` with a preprocessing scrip The `/test` skill lives in `.claude/skills/test/` with a TAP-parsing test runner. Other commands (worktree, pr, pgxntool-update) remain in `.claude/commands/`. +## Skill and Process Doc Quality + +These rules apply to `SKILL.md` files, command markdown, and any other process doc under `.claude/`. + +**CRITICAL**: After rewriting or restructuring part of a skill or process doc, re-read the entire file straight through, as if you were the one executing it, before pushing. A step's instructions must never contradict another section of the same file (e.g., one section pushing repo A before repo B, another section saying B must go first). Large, late-stage rewrites of a single section are the most common place for this kind of self-contradiction to hide, because editing section-by-section doesn't surface whether the doc still agrees with itself end-to-end. + +**CRITICAL**: A skill or process doc must never instruct an action that contradicts this repo's own `CLAUDE.md` policies (e.g., committing or pushing without the required confirmation gate, pushing without triggering the required CI-monitoring step, or skipping a required audit step before a branch is deleted). Check new or rewritten skill steps against `CLAUDE.md` directly, not from memory. + +**RULE**: Any point where a skill or process doc asks the user a yes/no or multiple-choice question must define the follow-up for every answer, not just the common-path one. A branch that just says "continue" without defining what continuing does is incomplete. + +**RULE**: Markdown fenced code blocks must have a blank line before and after the fence, and every fence must carry a language tag (` ```bash `, not a bare ` ``` `). + ## What This Repo Is **pgxntool-test** is the test harness for validating **../pgxntool/** (a PostgreSQL extension build framework). @@ -162,6 +174,10 @@ Note: "state modifications" here means changes to an already-initialized test en 3. **Never use `skip` for "already done" state modifications.** Make them idempotent with conditionals that simply don't re-run when unnecessary (no skip, no `@test`). Rebuilding state every time is too expensive, so reuse is important - but that logic belongs in non-test code. 4. **Abort early on environment setup failures.** Since we never commit with failing tests, it's better to abort the suite immediately when environment setup fails rather than continuing and collecting potentially many false failures. A failed state modification can invalidate all downstream tests, and the false failures just obscure the real problem. +### Reuse Canonical Test Helpers + +**RULE**: Before deriving a new path or piece of state in a `.bats` test (e.g., the pgxntool checkout location), grep sibling `.bats` files for how they already solve it, and reuse the existing helper/variable (e.g. `PGXNREPO` from `load_test_env`) instead of deriving it independently (e.g. via `$TOPDIR/../pgxntool`). A configured non-default checkout should not silently cause the wrong thing to be tested. + ### Template Design Principles Tests should generally avoid making changes to template environments. Writing test code to modify the test environment is more complex than having the correct files in the template to begin with. Tests that depend on running `make test` inside a template should strongly consider having the template itself contain the necessary test SQL and expected output files. From 1baa98c7b292304d19136086d13d7dcda5e114c4 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 29 Jul 2026 13:23:55 -0500 Subject: [PATCH 2/2] Rephrase CLAUDE.md doc-quality rules as artifact properties, not author steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code-review plugin's compliance agents only see the diff — they can't verify that a process step (re-reading a file, running a linter) happened during authoring. Lead each rule with a declarative "must/must not" statement checkable directly from the doc's content, keeping the authoring-workflow advice as secondary framing. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 20a62e2..ac60ea1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -65,13 +65,13 @@ Other commands (worktree, pr, pgxntool-update) remain in `.claude/commands/`. ## Skill and Process Doc Quality -These rules apply to `SKILL.md` files, command markdown, and any other process doc under `.claude/`. +These rules apply to `SKILL.md` files, command markdown, and any other process doc under `.claude/`. They describe required properties of the doc itself — check them by reading the resulting file/diff, not by trusting that a rewrite process was followed. -**CRITICAL**: After rewriting or restructuring part of a skill or process doc, re-read the entire file straight through, as if you were the one executing it, before pushing. A step's instructions must never contradict another section of the same file (e.g., one section pushing repo A before repo B, another section saying B must go first). Large, late-stage rewrites of a single section are the most common place for this kind of self-contradiction to hide, because editing section-by-section doesn't surface whether the doc still agrees with itself end-to-end. +**CRITICAL**: A step's instructions must never contradict another section of the same file (e.g., one section pushing repo A before repo B, another section saying B must go first). When reviewing a diff that rewrites or restructures part of a skill or process doc, read the whole file, not just the changed hunk — large, late-stage rewrites of a single section are the most common place for this kind of self-contradiction to hide, because editing section-by-section doesn't surface whether the doc still agrees with itself end-to-end. -**CRITICAL**: A skill or process doc must never instruct an action that contradicts this repo's own `CLAUDE.md` policies (e.g., committing or pushing without the required confirmation gate, pushing without triggering the required CI-monitoring step, or skipping a required audit step before a branch is deleted). Check new or rewritten skill steps against `CLAUDE.md` directly, not from memory. +**CRITICAL**: A skill or process doc must never instruct an action that contradicts this repo's own `CLAUDE.md` policies (e.g., committing or pushing without the required confirmation gate, pushing without triggering the required CI-monitoring step, or skipping a required audit step before a branch is deleted). A diff to a skill/process doc must be checked against `CLAUDE.md`'s actual current text, not against a paraphrase of it. -**RULE**: Any point where a skill or process doc asks the user a yes/no or multiple-choice question must define the follow-up for every answer, not just the common-path one. A branch that just says "continue" without defining what continuing does is incomplete. +**RULE**: Every point where a skill or process doc asks the user a yes/no or multiple-choice question must state the follow-up for every answer, not just the common-path one. A branch that just says "continue" without defining what continuing does is incomplete. **RULE**: Markdown fenced code blocks must have a blank line before and after the fence, and every fence must carry a language tag (` ```bash `, not a bare ` ``` `). @@ -176,7 +176,7 @@ Note: "state modifications" here means changes to an already-initialized test en ### Reuse Canonical Test Helpers -**RULE**: Before deriving a new path or piece of state in a `.bats` test (e.g., the pgxntool checkout location), grep sibling `.bats` files for how they already solve it, and reuse the existing helper/variable (e.g. `PGXNREPO` from `load_test_env`) instead of deriving it independently (e.g. via `$TOPDIR/../pgxntool`). A configured non-default checkout should not silently cause the wrong thing to be tested. +**RULE**: A `.bats` test must resolve a path or piece of state (e.g., the pgxntool checkout location) via the existing canonical helper/variable (e.g. `PGXNREPO` from `load_test_env`), not by deriving it independently (e.g. via `$TOPDIR/../pgxntool`). A configured non-default checkout should not silently cause the wrong thing to be tested. Before introducing new path-resolution logic, grep sibling `.bats` files for how they already solve it. ### Template Design Principles