From 97e09a43cc4bc39e489e1c3de75ccbc3ef3342f3 Mon Sep 17 00:00:00 2001 From: Abhi Reddy Date: Tue, 22 Sep 2026 23:53:49 -0400 Subject: [PATCH] Add PR-description skill, org-standards review agent, and pre-push check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tooling built while working NWP-101, split onto its own branch since it's not part of that ticket: - .claude/skills/northwind-pr/: writes PR descriptions in the team's exact required shape (title, what changed, how it was verified, acceptance criteria, deliberately not done) — /submit already deferred to a skill by this name if one exists. - .claude/agents/org-standards.md: read-only subagent (Read/Grep/Glob only) that audits a diff against every numbered item in docs/ORG-STANDARDS.md, citing item number, file, and line per finding. - .claude/hooks/pre-push-check.sh + its registration in .claude/settings.json: a PreToolUse Bash hook that runs npm test and npm run build in build-battle/merchant-console before any git push, blocking with the failing command and its log path if either fails. --- .claude/agents/org-standards.md | 45 +++++++++++++++++++++ .claude/hooks/pre-push-check.sh | 59 ++++++++++++++++++++++++++++ .claude/settings.json | 16 ++++++++ .claude/skills/northwind-pr/SKILL.md | 48 ++++++++++++++++++++++ 4 files changed, 168 insertions(+) create mode 100644 .claude/agents/org-standards.md create mode 100755 .claude/hooks/pre-push-check.sh create mode 100644 .claude/settings.json create mode 100644 .claude/skills/northwind-pr/SKILL.md diff --git a/.claude/agents/org-standards.md b/.claude/agents/org-standards.md new file mode 100644 index 00000000..ae042f8d --- /dev/null +++ b/.claude/agents/org-standards.md @@ -0,0 +1,45 @@ +--- +name: org-standards +description: Read-only reviewer that audits code against every numbered item in docs/ORG-STANDARDS.md, the org-wide engineering standards every service is measured against. Use before a PR, or whenever code needs checking against these standards rather than just the app's own CLAUDE.md conventions. Returns findings citing the item number, file, and line — never a fix. +tools: Read, Grep, Glob +--- + +You are an org-standards auditor. You check code against `docs/ORG-STANDARDS.md`. You do not fix. + +You have read-only access on purpose. You cannot edit files, run commands, or change anything, and you should not ask to. Your output is a report someone else acts on. + +## How to audit + +1. **Read `docs/ORG-STANDARDS.md` in full, every time.** It is the ground truth and it can change; do not rely on a memory of its contents from an earlier run. +2. **Confirm scope.** You should be told what to audit — a diff, a PR, a branch, a directory, or a file list. If you were not, say so and ask, rather than guessing at what "the code" means. +3. **Go item by item, not file by file.** Work through all ten numbered items in order. For each one, search the scope for the specific violation shape the standard itself names — the document is written to be checked, so its own wording tells you what to grep for (a stored float, a `toLocaleDateString` near a query, a second filter implementation, an unmasked card number, a stray `console.log`). +4. **Quote the item, don't paraphrase it.** A finding stands on the exact sentence from `ORG-STANDARDS.md` that the code breaks, not on a general impression. +5. **Report what's clean, not just what's broken.** An item you checked and found no violation for is a real result — say so and say where you looked. Silence about an item reads as "not checked," not "passed." + +## Report format + +``` +## Audit: + +### Findings +**# ** — `path/to/file.ts:LINE` +What the code does now, in one or two sentences. +The exact standard it breaks, quoted. +Suggested fix: + +**# ...** + +### Clean +- # — checked, no violation found, and where you looked. + +### Could not verify +- What a read-only, static pass cannot settle (cross-service reconciliation, runtime timezone behavior, data actually in the store) and what would. +``` + +## Rules + +- Every finding carries the item number, the file, and the line. "Violates #6" is a finding; "looks wrong" is not — this is `ORG-STANDARDS.md`'s own closing rule, not just this agent's. +- No code in a suggested fix. One sentence naming the change, not a patch. +- Don't stretch a style preference into a standards violation. If an item's exact wording isn't broken, it's not a finding — note it under Clean instead. +- Read-only. No edits, no commands, no test runs. If something needs a human decision (data that looks wrong but might be intentional seed data, a judgment call the standard doesn't resolve), say that plainly rather than picking a side. +- Keep it scannable. A ten-item audit that takes longer to read than the diff under review has failed at its own job. diff --git a/.claude/hooks/pre-push-check.sh b/.claude/hooks/pre-push-check.sh new file mode 100755 index 00000000..4b164e6e --- /dev/null +++ b/.claude/hooks/pre-push-check.sh @@ -0,0 +1,59 @@ +#!/usr/bin/env bash +# pre-push-check.sh — PreToolUse Bash hook +# +# Gates `git push` on the merchant console's own quality bar: its tests and +# its production build must both pass before a push is allowed through. +# +# - Not a git push -> exit 0, no output, nothing gated. +# - `npm test` or `npm run build` fails -> permissionDecision: deny, naming +# which one failed and the log file with the full output. +# - Both pass -> permissionDecision: allow. + +set -uo pipefail + +INPUT=$(cat) +CMD=$(printf '%s' "$INPUT" | jq -r '.tool_input.command // ""' 2>/dev/null || true) + +if [[ -z "$CMD" ]]; then + exit 0 +fi + +# Flatten to one line and strip quoted strings before matching, so "git push" +# appearing inside quoted data (e.g. a commit message) doesn't trip this gate, +# while a real `git push` anywhere in a compound command (cd x && git push, +# git push && echo done) still matches. +CMD_CLEAN=$(printf '%s' "$CMD" | tr '\n' ' ' | sed 's/"[^"]*"//g; s/'"'"'[^'"'"']*'"'"'//g') + +if ! echo "$CMD_CLEAN" | grep -qE '\bgit[[:space:]]+push\b'; then + exit 0 +fi + +REPO_ROOT=$(git rev-parse --show-toplevel 2>/dev/null || true) +if [[ -z "$REPO_ROOT" ]]; then + # Can't find a repo root from here — don't block on a check we can't run. + exit 0 +fi + +APP_DIR="$REPO_ROOT/build-battle/merchant-console" +if [[ ! -f "$APP_DIR/package.json" ]]; then + exit 0 +fi + +TEST_LOG="/tmp/claude-pre-push-test.log" +BUILD_LOG="/tmp/claude-pre-push-build.log" + +deny() { + jq -n --arg reason "$1" \ + '{"hookSpecificOutput": {"hookEventName": "PreToolUse", "permissionDecision": "deny", "permissionDecisionReason": $reason}}' + exit 0 +} + +if ! (cd "$APP_DIR" && npm test) >"$TEST_LOG" 2>&1; then + deny "Push blocked: \`npm test\` failed in build-battle/merchant-console. See the full output with: cat $TEST_LOG" +fi + +if ! (cd "$APP_DIR" && npm run build) >"$BUILD_LOG" 2>&1; then + deny "Push blocked: \`npm run build\` failed in build-battle/merchant-console. See the full output with: cat $BUILD_LOG" +fi + +jq -n '{"hookSpecificOutput": {"hookEventName": "PreToolUse", "permissionDecision": "allow", "permissionDecisionReason": "npm test and npm run build both passed in build-battle/merchant-console."}}' diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..17e098e0 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,16 @@ +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash \"$CLAUDE_PROJECT_DIR/.claude/hooks/pre-push-check.sh\"", + "timeout": 180 + } + ] + } + ] + } +} diff --git a/.claude/skills/northwind-pr/SKILL.md b/.claude/skills/northwind-pr/SKILL.md new file mode 100644 index 00000000..1dc5a953 --- /dev/null +++ b/.claude/skills/northwind-pr/SKILL.md @@ -0,0 +1,48 @@ +--- +name: northwind-pr +description: Write the pull request description for the current branch in Northwind's required format — title, what changed, how it was verified, acceptance criteria, and what was deliberately left out. Use when the user asks to write or draft a PR description in the team format, or when /submit needs a description and this skill exists. +--- + +# northwind-pr + +Write the pull request description for the work on this branch, in the team's required format. `/submit` calls this skill instead of the generic `.github/pull_request_template.md` fill-in whenever it exists — this is the authoritative shape. + +## What to do + +### 1. Gather the facts. Do not guess at them. + +- `git diff main...HEAD --stat`, then the diff itself — what actually changed +- `git log main..HEAD --oneline` — how it was sequenced +- The ticket in `docs/tickets/` — the acceptance criteria, verbatim + +### 2. Check each acceptance criterion honestly + +Go criterion by criterion. For each one, find the code that satisfies it, or find that nothing does. If it's half done, say half done and say which half — an unmet criterion reported honestly reads better than one left unmentioned, because a reviewer finds it either way. + +### 3. Write the description in this exact shape + +- **Title** — `: `. Ticket ID from the ticket file (or the branch name); the rest a plain-language summary, not a restatement of the ticket title. +- **What changed** — one paragraph, plain language. What can the app do now that it could not do before? Not a file list — the diff already is one. +- **How I verified it** — the actual commands you ran and the actual output. "Tests pass" is weak; `npm test — 12 passing, including the Luhn generator case` is evidence. Say what you clicked and what appeared, if anything was checked by hand. +- **Acceptance criteria** — the ticket's checkboxes, ticked honestly. Where one is partial, tick or don't and add a one-line note on what's missing. +- **Deliberately not done** — anything out of scope, deferred, or left for a follow-up ticket. Distinct from an unmet acceptance criterion: this is what you chose not to build, not what you tried and fell short of. + +### 4. Offer to open it + +Print the finished description. Then offer to open the PR (or update one that's already open): + +```bash +gh pr create --title ": " --body-file +# or, if a PR already exists on this branch: +gh pr edit --body-file +``` + +Ask before running it. Opening or editing a pull request is the user's call, not yours. + +## Rules + +- **Never claim a verification step that was not actually run.** If you did not run it, do not write that you did. This is the fastest way to lose a reviewer's trust, and the grader checks it. +- **No file list dressed up as "what changed."** Describe the capability, not the diff. +- **Say what you did not do.** Stating a limit in "Deliberately not done" is not a weakness; a reviewer discovering it themselves is. +- **Plain language over ceremony.** "Ops can now issue a card and see it in the list" beats "implemented card issuance functionality." +- **No emoji, no filler, no summary of the summary.**