-
-
Notifications
You must be signed in to change notification settings - Fork 7
Add PR self-merge rubric #2471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Add PR self-merge rubric #2471
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # Domain Docs | ||
|
|
||
| How the engineering skills should consume this repo's domain documentation when exploring the codebase. | ||
|
|
||
| This repo uses the **multi-context** layout: a root `CONTEXT-MAP.md` points at one `CONTEXT.md` per context (e.g. `backend/FwLite/CONTEXT.md`). | ||
|
|
||
| ## Before exploring, read these | ||
|
|
||
| - **`CONTEXT-MAP.md`** at the repo root — it points at one `CONTEXT.md` per context. Read each one relevant to the topic. | ||
| - **`docs/adr/`** — read ADRs that touch the area you're about to work in. Also check `<context>/docs/adr/` for context-scoped decisions. | ||
|
|
||
| If any of these files don't exist, **proceed silently**. Don't flag their absence; don't suggest creating them upfront. The `/domain-modeling` skill (reached via `/grill-with-docs` and `/improve-codebase-architecture`) creates them lazily when terms or decisions actually get resolved. | ||
|
|
||
| ## File structure | ||
|
|
||
| Multi-context repo (presence of `CONTEXT-MAP.md` at the root): | ||
|
|
||
| ``` | ||
| / | ||
| ├── CONTEXT-MAP.md | ||
| ├── docs/adr/ ← system-wide decisions | ||
| └── backend/ | ||
| └── FwLite/ | ||
| ├── CONTEXT.md | ||
| └── docs/adr/ ← context-specific decisions | ||
| ``` | ||
|
|
||
| ## Use the glossary's vocabulary | ||
|
|
||
| When your output names a domain concept (in an issue title, a refactor proposal, a hypothesis, a test name), use the term as defined in the relevant `CONTEXT.md`. Don't drift to synonyms the glossary explicitly avoids. | ||
|
|
||
| If the concept you need isn't in the glossary yet, that's a signal — either you're inventing language the project doesn't use (reconsider) or there's a real gap (note it for `/domain-modeling`). | ||
|
|
||
| ## Flag ADR conflicts | ||
|
|
||
| If your output contradicts an existing ADR, surface it explicitly rather than silently overriding: | ||
|
|
||
| > _Contradicts ADR-0007 (event-sourced orders) — but worth reopening because…_ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # Issue tracker: Local Markdown | ||
|
|
||
| Issues and specs (you may know a spec as a PRD) for this repo live as markdown files in `.scratch/`. | ||
|
|
||
| > **Note:** `.scratch/` is git-ignored — issues are local to your working copy and are not shared through the repo. Don't expect them on other clones or in CI. | ||
|
|
||
| ## Conventions | ||
|
|
||
| - One feature per directory: `.scratch/<feature-slug>/` | ||
| - The spec is `.scratch/<feature-slug>/spec.md` | ||
| - Implementation issues are one file per ticket at `.scratch/<feature-slug>/issues/<NN>-<slug>.md`, numbered from `01` — never a single combined tickets file | ||
| - Triage state is recorded as a `Status:` line near the top of each issue file (see `triage-labels.md` for the role strings) | ||
| - Comments and conversation history append to the bottom of the file under a `## Comments` heading | ||
|
|
||
| ## When a skill says "publish to the issue tracker" | ||
|
|
||
| Create a new file under `.scratch/<feature-slug>/` (creating the directory if needed). | ||
|
|
||
| ## When a skill says "fetch the relevant ticket" | ||
|
|
||
| Read the file at the referenced path. The user will normally pass the path or the issue number directly. | ||
|
|
||
| ## Wayfinding operations | ||
|
|
||
| Used by `/wayfinder`. The **map** is a file with one **child** file per ticket. | ||
|
|
||
| - **Map**: `.scratch/<effort>/map.md` — the Notes / Decisions-so-far / Fog body. | ||
| - **Child ticket**: `.scratch/<effort>/issues/NN-<slug>.md`, numbered from `01`, with the question in the body. A `Type:` line records the ticket type (`research`/`prototype`/`grilling`/`task`); a `Status:` line records `claimed`/`resolved`. | ||
| - **Blocking**: a `Blocked by: NN, NN` line near the top. A ticket is unblocked when every file it lists is `resolved`. | ||
| - **Frontier**: scan `.scratch/<effort>/issues/` for files that are open, unblocked, and unclaimed; first by number wins. | ||
| - **Claim**: set `Status: claimed` and save before any work. | ||
| - **Resolve**: append the answer under an `## Answer` heading, set `Status: resolved`, then append a context pointer (gist + link) to the map's Decisions-so-far in `map.md`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # Triage Labels | ||
|
|
||
| The skills speak in terms of five canonical triage roles. This file maps those roles to the actual strings used in this repo's issue tracker. | ||
|
|
||
| Because issues live as local markdown (see `issue-tracker.md`), a "label" is just the value of the `Status:` line near the top of an issue file — there is no external label system to reconcile with. | ||
|
|
||
| | Label in mattpocock/skills | Status string in our tracker | Meaning | | ||
| | -------------------------- | ---------------------------- | ---------------------------------------- | | ||
| | `needs-triage` | `needs-triage` | Maintainer needs to evaluate this issue | | ||
| | `needs-info` | `needs-info` | Waiting on reporter for more information | | ||
| | `ready-for-agent` | `ready-for-agent` | Fully specified, ready for an AFK agent | | ||
| | `ready-for-human` | `ready-for-human` | Requires human implementation | | ||
| | `wontfix` | `wontfix` | Will not be actioned | | ||
|
|
||
| When a skill mentions a role (e.g. "apply the AFK-ready triage label"), write the corresponding string on the issue file's `Status:` line. | ||
|
|
||
| Edit the right-hand column to match whatever vocabulary you actually use. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,144 @@ | ||||||||||||||||||
| # PR Self-Merge Rubric (languageforge-lexbox) | ||||||||||||||||||
|
|
||||||||||||||||||
| > **Interactive version:** an interactive, tick-through copy of this rubric is | ||||||||||||||||||
| > published as an artifact: | ||||||||||||||||||
| > <https://claude.ai/code/artifact/7792cc42-991f-4d2e-8990-7a582b29162c> | ||||||||||||||||||
| > (viewable once shared from the artifact's share menu). | ||||||||||||||||||
|
|
||||||||||||||||||
| Decide whether **your own** PR can be merged **without a second human review**. | ||||||||||||||||||
| Internal authors only (not outside contributors). Default-closed: **if anything | ||||||||||||||||||
| is uncertain, get a human.** You are not saving time by merging something that | ||||||||||||||||||
| needed review — you are moving the cost downstream and making it bigger. | ||||||||||||||||||
|
|
||||||||||||||||||
| --- | ||||||||||||||||||
|
|
||||||||||||||||||
| ## 0. Preconditions — both true, or stop here | ||||||||||||||||||
|
|
||||||||||||||||||
| - [ ] **CI is green.** | ||||||||||||||||||
| - [ ] **An AI review has run** (Devin / CodeRabbit / your own agent) and **you | ||||||||||||||||||
| read its output** and addressed anything real. | ||||||||||||||||||
|
|
||||||||||||||||||
| These are assumed by everything below. This rubric only decides whether a | ||||||||||||||||||
| *second human* is still required on top of them. | ||||||||||||||||||
|
|
||||||||||||||||||
| --- | ||||||||||||||||||
|
|
||||||||||||||||||
| ## The rule | ||||||||||||||||||
|
|
||||||||||||||||||
| > **A PR is self-mergeable if — and only if — *every* changed file is GREEN.** | ||||||||||||||||||
|
|
||||||||||||||||||
| Judge each changed file by its **(path, change-type)** pair. Generated files | ||||||||||||||||||
| don't count as their own files — they inherit the classification of the source | ||||||||||||||||||
| edit that produced them (see reference). | ||||||||||||||||||
|
|
||||||||||||||||||
| A changed file is **GREEN** when either: | ||||||||||||||||||
|
|
||||||||||||||||||
| 1. **Its change-type is universal-green** — *regardless of path*: docs / | ||||||||||||||||||
| comment-only, formatting-or-whitespace-only, a **new** test (not a test | ||||||||||||||||||
| edit), or i18n / new UI string additions. No further gates. | ||||||||||||||||||
| 2. **Its path is not 🔴** *and* its change-type is **conditional-green** | ||||||||||||||||||
| (everything else) *and* it clears the gates in §2 that apply to it. | ||||||||||||||||||
|
Comment on lines
+39
to
+40
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "(everything else)" means the current wording is saying redundant i.e.:
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| If a file is neither → **not self-mergeable, get a human.** A **mixed PR** is | ||||||||||||||||||
| judged file-by-file: one non-green file sinks it — split the PR or request | ||||||||||||||||||
| review. | ||||||||||||||||||
|
|
||||||||||||||||||
| --- | ||||||||||||||||||
|
|
||||||||||||||||||
| ## 1. 🔴 Always-human — the hard stops | ||||||||||||||||||
|
|
||||||||||||||||||
| If the PR changes any of these, **get a human** — *unless every such change is a | ||||||||||||||||||
| universal-green type* (docs/comment, formatting, new test, i18n), which is | ||||||||||||||||||
| allowed anywhere. | ||||||||||||||||||
|
|
||||||||||||||||||
| - `backend/FwLite/FwLiteProjectSync/**` — CRDT↔FwData sync orchestration | ||||||||||||||||||
| - `backend/FwLite/LcmCrdt/**` — CRDT implementation | ||||||||||||||||||
| - `backend/FwLite/FwDataMiniLcmBridge/**` — FwData↔MiniLcm conversion | ||||||||||||||||||
| - `backend/FwLite/MiniLcm/**` model / `IMiniLcmApi` files — model-field fanout | ||||||||||||||||||
| - `backend/FwHeadless/**` · `backend/FixFwData/**` · `backend/LexCore/Sync/**` · | ||||||||||||||||||
| `backend/SyncReverseProxy/**` · `hgweb/**` | ||||||||||||||||||
| - `**/Migrations/**` — EF migrations | ||||||||||||||||||
| - Harmony package version bumps / MSBuild reference changes | ||||||||||||||||||
| - **Auth / JWT / secrets anywhere** (incl. `*/Auth/`, connection strings) | ||||||||||||||||||
| - **Breaking GraphQL schema changes** — removes/renames a field/type/enum-value, | ||||||||||||||||||
| tightens nullability, adds a required arg, or changes an auth attribute on an | ||||||||||||||||||
| existing resolver | ||||||||||||||||||
| - `deployment/**` — k8s / Kustomize / PVCs / secrets | ||||||||||||||||||
|
|
||||||||||||||||||
| *Why data loss / security / deployed-contract risk — see the repo's 🔴 CRITICAL | ||||||||||||||||||
| AGENTS.md flags and the sentinel review agents.* | ||||||||||||||||||
|
|
||||||||||||||||||
| --- | ||||||||||||||||||
|
|
||||||||||||||||||
| ## 2. Gates — apply to every *conditional-green* file | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| - **Size (always).** ≤ **150 net changed lines** of *reviewable, non-green* code | ||||||||||||||||||
| — excluding generated files, universal-green files, lockfiles, and snapshots — | ||||||||||||||||||
| **AND** it's *one coherent change you can hold in your head* (not 40 lines | ||||||||||||||||||
| smeared across 15 unrelated files). | ||||||||||||||||||
|
Comment on lines
+75
to
+78
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| - **Test (only if the change alters behavior** — new behavior / bug fix / | ||||||||||||||||||
| feature**).** New behavior → a new test; bug fix → a **regression test**. *If | ||||||||||||||||||
| it can't reasonably be tested, get a human.* Non-behavior changes (refactor, | ||||||||||||||||||
| config, dependency bump, test edit) need no new test — green CI covers them. | ||||||||||||||||||
| Run the cheap targeted tests locally (per AGENTS.md tiers); rely on CI for | ||||||||||||||||||
| stack-dependent suites. | ||||||||||||||||||
| - *(Verification is folded into the attestation below.)* | ||||||||||||||||||
|
|
||||||||||||||||||
| --- | ||||||||||||||||||
|
|
||||||||||||||||||
| ## 3. Final attestation — you are the reviewer of record | ||||||||||||||||||
|
|
||||||||||||||||||
| Applies to **every** PR. Vouch for this diff to **the same standard you'd hold a | ||||||||||||||||||
| teammate's PR to** (the repo's "Integrity is non-negotiable" ethos). Honest | ||||||||||||||||||
| **YES** to all four, or get a human: | ||||||||||||||||||
|
|
||||||||||||||||||
| - [ ] **Read** — I read every non-generated changed line, not skimmed. | ||||||||||||||||||
| - [ ] **Explain** — I can explain why each change exists, *without re-reading* — | ||||||||||||||||||
| as if I wrote it. | ||||||||||||||||||
| - [ ] **Blast radius & ran it** — I checked what this could break (callers / | ||||||||||||||||||
| consumers of any changed contract), and where it has a runtime surface a | ||||||||||||||||||
| test can't fully capture, I exercised it and saw the intended behavior. | ||||||||||||||||||
| - [ ] **No mystery code** — there is nothing here I don't fully understand | ||||||||||||||||||
| ("the agent probably knows what it's doing" is a NO). | ||||||||||||||||||
|
|
||||||||||||||||||
| --- | ||||||||||||||||||
|
|
||||||||||||||||||
| ## Tie-breaker | ||||||||||||||||||
|
|
||||||||||||||||||
| **Any "I'm not sure" on any line above → get a human.** That is the correct, | ||||||||||||||||||
| low-cost outcome, not a failure. | ||||||||||||||||||
|
|
||||||||||||||||||
| --- | ||||||||||||||||||
|
|
||||||||||||||||||
| ## Reference | ||||||||||||||||||
|
|
||||||||||||||||||
| ### Change-type reach | ||||||||||||||||||
|
|
||||||||||||||||||
| *Reach* = how far a change-type green-lights across paths. **Universal** overrides | ||||||||||||||||||
| even a 🔴 path. **Conditional** is green only outside 🔴, and is subject to the §2 | ||||||||||||||||||
| gates. | ||||||||||||||||||
|
|
||||||||||||||||||
| | Change-type | Reach | Test needed? / Notes | | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Test needed" should be its own column for readability/scanability. |
||||||||||||||||||
| |---|---|---| | ||||||||||||||||||
| | Docs / comment-only | 🟢 Universal | No runtime effect | | ||||||||||||||||||
| | Formatting / whitespace | 🟢 Universal | Auto-format, import reorder | | ||||||||||||||||||
| | **New** test (addition, not edit) | 🟢 Universal | Adds coverage, can't weaken a guard | | ||||||||||||||||||
| | i18n / new UI string additions | 🟢 Universal | Low-risk content | | ||||||||||||||||||
| | Test *modification* | 🟡 Conditional | No test; a weakened assertion in a 🔴 test → 🔴 = human | | ||||||||||||||||||
| | Dependency bump | 🟡 Conditional | No test; Harmony bump is 🔴 by path | | ||||||||||||||||||
| | Config change | 🟡 Conditional | No test; secret/auth/connection-string config is 🔴 by path | | ||||||||||||||||||
| | Pure refactor (no behavior change) | 🟡 Conditional | No test (no behavior change); leans on the §3 attestation | | ||||||||||||||||||
| | New behavior / bug fix / feature | 🟡 Conditional | **Needs a test** (§2); additive GraphQL lives here | | ||||||||||||||||||
|
|
||||||||||||||||||
| ### Other | ||||||||||||||||||
|
|
||||||||||||||||||
| - **Generated files** (`frontend/viewer/src/lib/dotnet-types/generated-types/**`, | ||||||||||||||||||
| `.po` catalogs) are *transparent*: classify the PR by the source edit; they're | ||||||||||||||||||
| excluded from the size count. | ||||||||||||||||||
|
Comment on lines
+135
to
+137
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Already mostly stated higher up (except for .po, which is only partially generated, because you can add translator comments, but I'd still classify it as generated)
Suggested change
|
||||||||||||||||||
| - **Tool-ability.** The path lists are globs and the size rule is a computable | ||||||||||||||||||
| line count on purpose — a future helper tool can implement this rubric | ||||||||||||||||||
| verbatim. | ||||||||||||||||||
| - **On the two tiers.** An earlier draft split conditional-green into | ||||||||||||||||||
| "zone-limited" and "gated." They had identical *reach* (green only outside 🔴, | ||||||||||||||||||
| same size + attestation) and differed only by the test requirement — so | ||||||||||||||||||
| they're merged, and "behavior change → test" is just a rule in §2. | ||||||||||||||||||
|
Comment on lines
+141
to
+144
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think recording this is helpful.
Suggested change
|
||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to add these 3 agents files? (and the matching gitignore and agents.md changes)