diff --git a/.claude/agents/batch-worker.md b/.claude/agents/batch-worker.md index 5af73d1531..8b09b44449 100644 --- a/.claude/agents/batch-worker.md +++ b/.claude/agents/batch-worker.md @@ -22,6 +22,7 @@ For each issue in the work order (sequentially, never parallel): 1. Read the issue spec carefully. Identify every acceptance criterion and behavioral requirement. 2. Explore the codebase to understand existing patterns and relevant files. 3. Implement the feature/fix. Follow all project rules — scan `memory/INDEX.md` and read any atom whose description matches the change you're making. The architecture story is in `docs/architecture/design-rules.md`. + Before adding durable surface (new file, public type, interface method, service/repository method, DTO/view model, helper, endpoint, dependency, or DI registration), read `memory/process/reuse-first-change-discipline.md` and audit the existing owner/surface. 3.5. **Escape valve — privilege OR broader spec change.** STOP, do NOT commit, do NOT push, and report back to the orchestrator if EITHER of these fires during exploration or implementation: a. **Privilege change.** The change crosses into privilege territory — bumping a default permission tier, modifying `[Authorize]` requirements, expanding a role grant, adding to a CORS allowlist beyond clearly-internal dev origins, adding an admin flag, lowering an auth bar, granting move/delete/admin on shared resources. Per `memory/process/privilege-changes-need-explicit-approval.md`. @@ -52,7 +53,17 @@ For each acceptance criterion: **Critical: no scope reduction.** When the review finds N failures, fix N failures. Do not fix some and report others as "too big" or "needs human review." The authorization to fix was given when the batch was launched. If a fix is genuinely blocked (missing data, needs a DB migration you can't create, external dependency), explain the specific blocker — "this is hard" is not a blocker. -**If all criteria PASS:** Move to the next issue. +**If all criteria PASS:** Proceed to reuse review. + +### Phase 2.5: Reuse Review + +Before moving to the next issue, review the issue's diff against `docs/architecture/reuse-review-rules.md`. + +1. Inventory new durable surface: files, public types, interface methods, service/repository methods, DTOs/view models, helpers, endpoints, dependencies, and DI registrations. +2. For each item, identify the existing owner/surface considered for reuse. +3. If any added surface should reuse existing code instead, fix it now. +4. If public/interface surface is genuinely necessary and Peter has not approved it, STOP and report the needed approval instead of committing it. +5. Include the reuse-review result in the issue handoff: `PASS - no unnecessary durable surface` or `WARN - fixed reuse issues`. ### Phase 3: Code Review (after all issues in batch) @@ -74,6 +85,7 @@ After all issues are implemented and spec-reviewed: - Type safety in views - CSP compliance - Dead code +4. Review the full batch diff against `docs/architecture/reuse-review-rules.md`. Confirm no unnecessary durable surface remains. **If any CRITICAL issues found:** - Fix ALL of them, not just the easy ones. @@ -117,6 +129,7 @@ Self-reviewed against CODE_REVIEW_RULES.md. No critical issues. ## Important Rules - **Never skip a review gate.** Every issue gets spec-reviewed. Every batch gets code-reviewed. +- **Never skip reuse review.** New surface is treated as debt until the existing owner/surface has been audited. - **Never move past a failing issue.** If issue #2 of 4 won't pass spec review, the batch stops at #2. Don't implement #3 and #4 on top of broken work. - **Read the issue spec, not your own summary of it.** The failure this agent prevents is building something that sounds right but doesn't match the spec. Re-read the original spec every time you review. - **One commit per issue, plus fix commits.** Keep the history clean so individual issues can be traced. diff --git a/.claude/skills/reuse-review/SKILL.md b/.claude/skills/reuse-review/SKILL.md new file mode 100644 index 0000000000..52ad9c3684 --- /dev/null +++ b/.claude/skills/reuse-review/SKILL.md @@ -0,0 +1,59 @@ +--- +name: reuse-review +description: "Review a local diff or PR for unnecessary durable surface: new files, public types, interface methods, DTOs/view models, helpers, routes, DI registrations, services, repositories, and dependencies that should reuse existing owners instead." +argument-hint: "PR 64 | (no args = current branch)" +--- + +# Reuse Review + +This is not a correctness review. It checks whether the change added durable surface where reuse, caller-side composition, or an existing owner would have been better. + +## Inputs + +`$ARGUMENTS` can be: +- `PR ` - review an existing PR on `peterdrier/Humans` +- Empty - review the current branch against `origin/main` + +## Steps + +1. Read: + - `CLAUDE.md` reuse-first section + - `memory/process/reuse-first-change-discipline.md` + - `memory/architecture/interface-method-additions-are-debt.md` + - `docs/architecture/reuse-review-rules.md` +2. Fetch the diff: + - PR: `gh pr diff --repo peterdrier/Humans` + - Local: `git diff --find-renames origin/main...HEAD` +3. Build a surface inventory: + - new files + - new public types + - interface method changes + - new/changed service and repository methods + - new DTOs/view models/helpers + - new controller actions/routes/pages + - new DI registrations and package references +4. For each item, search for the existing owner/surface before flagging. +5. Report only concrete findings. A finding must name both the new surface and the existing surface that should absorb or replace it. + +## Output + +Use this format: + +```markdown +## Reuse Review: PASS|WARN + +### Findings +- REUSE - should reuse . + Why: . + Fix: . + +### Surface Inventory +- New files: +- Public/interface surface: +- Services/repositories: +- DTOs/view models/helpers: +- Routes/pages: +- DI/dependencies: +``` + +If there are no findings, say `PASS - no unnecessary durable surface found` and still include the surface inventory. diff --git a/.claude/tech-debt-prompt.md b/.claude/tech-debt-prompt.md index 73d8321b79..cb064f8aef 100644 --- a/.claude/tech-debt-prompt.md +++ b/.claude/tech-debt-prompt.md @@ -2,11 +2,11 @@ ## Mission -You are autonomously improving a ~70k-line ASP.NET Core 10 Clean Architecture codebase (membership management for a Spanish nonprofit). It grew quickly and patterns have diverged. Your job is to **find and fix the most impactful tech debt** — duplicated logic, inconsistent patterns, misplaced responsibilities, and unnecessary complexity. +You are autonomously improving a ~70k-line ASP.NET Core 10 Clean Architecture codebase (membership management for a Spanish nonprofit). It grew quickly and patterns have diverged. Your job is to **find and fix the most impactful tech debt** — duplicated logic, inconsistent patterns, misplaced responsibilities, unnecessary complexity, and unnecessary durable surface. -**You decide what to work on.** Scan the codebase, identify the highest-value improvements, fix them, verify the build passes, commit, and move on. Use your judgement about what matters most. +**You decide what to work on.** Scan the codebase, identify the highest-value improvements, fix them, verify the build passes, commit, and move on. Use your judgement about what matters most. Prefer work that deletes, consolidates, or ratchets existing behavior over work that adds new abstractions. -**Work autonomously.** Do not stop for milestone reports. Find the next opportunity, fix it, verify the build passes, commit, push the branch to `origin`, and move to the next one. +**Work autonomously.** Do not stop for milestone reports. Find the next opportunity, fix it, verify the build passes, commit, push the branch to `origin`, and move to the next one. If a cleanup/refactor would add more durable surface than it removes, stop unless the value is concrete and documented in the commit/PR. --- @@ -102,7 +102,8 @@ dotnet build Humans.slnx -v q && dotnet test Humans.slnx -v q --filter "FullyQua - **Controllers are thin.** A controller action should: validate input, call a service, map the result, return a view. Business logic (LINQ transforms, multi-step mutations, domain decisions) belongs in services. - **Services own their domain.** A TeamService should only contain team logic. If it has methods that query role assignments, budget data, or shift schedules, those methods belong elsewhere. - **Large files are fine if cohesive.** A 2000-line service is not a problem if every method relates to the same domain. Do NOT split files just to reduce line count. -- **Shared patterns use shared code.** If the same 5-line pattern appears in 10 controllers, extract it. If a ViewComponent exists but views use inline HTML instead, fix the views. +- **Shared patterns use shared code carefully.** Extract only when there are multiple live call sites, one obvious owner, and the extraction makes the net system simpler. If a ViewComponent exists but views use inline HTML instead, fix the views. +- **Reuse beats surface growth.** Before adding any new file, public type, interface method, service/repository method, DTO/view model, helper, endpoint, dependency, or DI registration, audit the existing owner/surface first. Follow `memory/process/reuse-first-change-discipline.md`. - **Constants over magic strings.** Role names, cache keys, action names — all should reference constants or use `nameof()`. ## What to look for @@ -118,13 +119,13 @@ When the same thing is done multiple ways across the codebase. Examples: caching Methods or logic in the wrong layer or the wrong service. A controller doing business logic. A service containing methods that belong to a different domain. Code that queries across domain boundaries when it should delegate. ### Duplicated logic -The same LINQ query, the same validation check, the same mapping logic appearing in multiple places. Extract to a shared method in the appropriate service or helper. +The same LINQ query, validation check, or mapping logic appearing in multiple places. Extract only when there are multiple live call sites, a clear owner, and the extraction is simpler than local composition. Do not add a one-off helper or interface method just to make a single call site look cleaner. ### Inconsistent conventions Naming (`GetXAsync` vs `FetchXAsync` vs `LoadXAsync`), method signatures, parameter ordering, return types. When you find inconsistency, standardize on the dominant convention. ### Unused abstraction or missing abstraction -A ViewComponent that exists but isn't used (views use inline HTML instead). A pattern that's repeated 10 times but has no shared helper. An interface with a single implementation that adds no value. +A ViewComponent that exists but isn't used (views use inline HTML instead). A repeated pattern with no clear shared owner. An interface with a single implementation that adds no value. When in doubt, remove or reuse existing surface before adding another abstraction. --- @@ -136,9 +137,10 @@ Before every change, verify: 2. **Am I renaming a property on a class that might be serialized to JSON?** → STOP, skip this change. 3. **Am I removing a property that looks unused?** → STOP, it's likely used via reflection. 4. **Am I changing a method signature on an interface in `Application/`?** → Check all implementations AND all callers. -5. **Am I changing authorization?** → Verify the attribute/policy matches the original access level exactly. -6. **Am I removing a lazy `IServiceProvider` resolution?** → Check for a comment explaining why. If it exists to break a ctor cycle (common in §15 migrations), leave it alone. -7. **Am I splitting a large "god class" service?** → STOP. Large cohesive services are preferred over fragmented ones here; splitting breaks caching and cross-method state. Do NOT split services to reduce line count. -8. **Am I about to half-migrate a §15 section?** → STOP. If you extract a repository you must also move the service to `Humans.Application.Services.X`, stitch cross-section reads through owning-service interfaces, and commit everything together. -9. **Does `dotnet build Humans.slnx` still pass?** → Must pass after every change. -10. **Does `dotnet test Humans.slnx` still pass?** → Must pass after every change. +5. **Am I adding durable surface?** → Audit the existing owner/surface first. Public/interface surface requires Peter approval. +6. **Am I changing authorization?** → Verify the attribute/policy matches the original access level exactly. +7. **Am I removing a lazy `IServiceProvider` resolution?** → Check for a comment explaining why. If it exists to break a ctor cycle (common in §15 migrations), leave it alone. +8. **Am I splitting a large "god class" service?** → STOP. Large cohesive services are preferred over fragmented ones here; splitting breaks caching and cross-method state. Do NOT split services to reduce line count. +9. **Am I about to half-migrate a §15 section?** → STOP. If you extract a repository you must also move the service to `Humans.Application.Services.X`, stitch cross-section reads through owning-service interfaces, and commit everything together. +10. **Does `dotnet build Humans.slnx` still pass?** → Must pass after every change. +11. **Does `dotnet test Humans.slnx` still pass?** → Must pass after every change. diff --git a/.claude/tech-debt-tasks-prompt.md b/.claude/tech-debt-tasks-prompt.md index 0feba83e11..84b81f81a5 100644 --- a/.claude/tech-debt-tasks-prompt.md +++ b/.claude/tech-debt-tasks-prompt.md @@ -2,9 +2,9 @@ ## Mission -You are executing specific, pre-identified tech debt tasks from the backlog below. Work through them in order of priority. After completing each task, mark it done in this section of your handoff notes. +You are executing specific, pre-identified tech debt tasks from the backlog below. Work through them in order of priority. After completing each task, mark it done in this section of your handoff notes. Prefer changes that delete, consolidate, or reuse existing surface; do not create a new abstraction unless it clearly reduces net complexity. -**Work autonomously.** Pick the next uncompleted task, implement it, verify the build passes, commit, push, move on. +**Work autonomously.** Pick the next uncompleted task, implement it, verify the build passes, commit, push, move on. If a task would add new durable surface instead of consolidating existing surface, document why in the commit/PR before proceeding. --- @@ -48,6 +48,7 @@ Also do not modify: 7. **No concurrency tokens.** 8. **Every new page needs a nav link.** 9. **Admin pages don't need localization.** +10. **Reuse-first discipline.** Before adding new files, public types, interface methods, service/repository methods, DTOs/view models, helpers, endpoints, dependencies, or DI registrations, audit the existing owner/surface first. See `memory/process/reuse-first-change-discipline.md`. ## Build Command @@ -137,6 +138,7 @@ Before every change, verify: 2. **Am I renaming a serialized property?** → STOP. 3. **Am I removing something that looks unused?** → STOP, likely used via reflection. 4. **Am I changing an interface in `Application/`?** → Check all implementations AND callers. -5. **Am I changing authorization?** → Verify exact match. -6. **Does build pass?** → Must pass after every change. -7. **Do tests pass?** → Must pass after every change. +5. **Am I adding durable surface?** → Audit existing owners first; public/interface surface requires Peter approval. +6. **Am I changing authorization?** → Verify exact match. +7. **Does build pass?** → Must pass after every change. +8. **Do tests pass?** → Must pass after every change. diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml index e7b7fd87ad..255f8dd4f7 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.yml +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -57,6 +57,25 @@ body: validations: required: true + - type: textarea + id: existing-surface + attributes: + label: Existing surface to check + description: Existing services, views/components, DTOs/view models, helpers, routes, or tests that likely own the fix. Optional, but useful when known. + placeholder: | + - Services: + - Views/components: + - Tests/helpers: + + - type: textarea + id: out-of-scope + attributes: + label: Out of scope / non-goals + description: Anything intentionally NOT being changed by this bug fix. + placeholder: | + - No new interface methods unless explicitly approved. + - No new service/repository unless the existing owner cannot carry the fix. + - type: input id: environment attributes: diff --git a/.github/ISSUE_TEMPLATE/feature_request.yml b/.github/ISSUE_TEMPLATE/feature_request.yml index b6efc70cc1..24674c78d8 100644 --- a/.github/ISSUE_TEMPLATE/feature_request.yml +++ b/.github/ISSUE_TEMPLATE/feature_request.yml @@ -45,6 +45,17 @@ body: label: Proposed change description: The shape of the change. Leave open ends for the implementer to decide where genuine. + - type: textarea + id: existing-surface + attributes: + label: Existing surface to reuse + description: Existing services, views/components, DTOs/view models, helpers, routes, or tests the implementer should check before adding new surface. + placeholder: | + - Services: + - Views/components: + - DTOs/view models: + - Tests/helpers: + - type: textarea id: acceptance attributes: @@ -61,3 +72,7 @@ body: attributes: label: Out of scope / non-goals description: Anything intentionally NOT being done here. Helps prevent scope creep. + placeholder: | + - No new interface methods unless explicitly approved. + - No new service/repository unless listed above. + - No new route/page unless listed in acceptance criteria. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index d2077f5474..ba2414a787 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -11,6 +11,10 @@ Tick items as you go, or strike through (`~~item~~`) with a reason if a row genu +## Existing surface checked + + + ## UI changes / screenshots @@ -24,6 +28,7 @@ Tick items as you go, or strike through (`~~item~~`) with a reason if a row genu - [ ] **EF migrations** (if any) auto-generated, not hand-edited — `memory/architecture/no-hand-edited-migrations.md` and `memory/architecture/migration-regen-after-rebase.md`. - [ ] **NuGet packages updated?** If yes, `Views/About/Index.cshtml` updated with new versions + licenses — `memory/process/about-page-license-attribution.md`. - [ ] **New project rule?** Captured as a `memory//.md` atom **in this PR**, with a one-line entry added to `memory/INDEX.md`. See `memory/META.md`. +- [ ] **Reuse-first checked** — no unnecessary new files, public types, interface methods, service/repository methods, DTOs/view models, helpers, endpoints, dependencies, or DI registrations. See `memory/process/reuse-first-change-discipline.md`. - [ ] **Build + test pass locally**: `dotnet build Humans.slnx -v quiet` and `dotnet test Humans.slnx -v quiet`. - [ ] **Nav coverage** — any new page is reachable from navigation (no orphan pages). - [ ] **No magic strings** — `nameof()` / constants used where applicable. diff --git a/.github/workflows/pr-surface-report.yml b/.github/workflows/pr-surface-report.yml new file mode 100644 index 0000000000..db4d21837d --- /dev/null +++ b/.github/workflows/pr-surface-report.yml @@ -0,0 +1,82 @@ +name: PR Surface Report + +on: + pull_request_target: + types: [opened, reopened, synchronize, ready_for_review] + +permissions: + contents: read + pull-requests: read + issues: write + +concurrency: + group: pr-surface-report-${{ github.repository }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + report: + if: github.event.pull_request.draft == false + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v6 + with: + ref: ${{ github.event.pull_request.base.ref }} + fetch-depth: 0 + + - name: Fetch PR head + run: | + set -euo pipefail + git fetch --no-tags origin "pull/${{ github.event.pull_request.number }}/head:pr-surface-head" + + - name: Generate surface report + run: | + python3 scripts/pr-surface-report.py \ + --base "${{ github.event.pull_request.base.sha }}" \ + --head "pr-surface-head" \ + --output pr-surface-report.md \ + --json-output pr-surface-report.json + + - name: Post or update PR comment + uses: actions/github-script@v8 + with: + script: | + const fs = require('fs'); + const marker = ''; + const body = fs.readFileSync('pr-surface-report.md', 'utf8'); + const {owner, repo} = context.repo; + const issue_number = context.payload.pull_request.number; + + const comments = await github.paginate(github.rest.issues.listComments, { + owner, + repo, + issue_number, + per_page: 100, + }); + const existing = comments.find(comment => + comment.user.type === 'Bot' && comment.body.includes(marker)); + + if (existing) { + await github.rest.issues.updateComment({ + owner, + repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ + owner, + repo, + issue_number, + body, + }); + } + + - name: Enforce migration count + if: always() + run: | + count="$(jq -r '.migration_count' pr-surface-report.json)" + if [ "$count" -gt 1 ]; then + echo "::error::PR changes $count real EF migration files. Keep at most 1 migration per PR." + exit 1 + fi diff --git a/CLAUDE.md b/CLAUDE.md index 97f45571c0..a9bc45f5d9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -11,7 +11,7 @@ Clean Architecture with 4 layers (strict dependency direction inward): - **Infrastructure** — repository implementations, `HumansDbContext`, migrations, external API clients, jobs. - **Web** — controllers, views, view models, API endpoints, DI wiring. -See [`docs/architecture/design-rules.md`](docs/architecture/design-rules.md) for the full architecture story (the constitution): layer responsibilities, table ownership map, caching pattern (§15), authorization pattern, cross-domain rules. Read it once cover-to-cover. +See [`docs/architecture/design-rules.md`](docs/architecture/design-rules.md) for the full architecture story (the constitution): layer responsibilities, table ownership map, caching pattern (§15), authorization pattern, cross-domain rules. Read the relevant sections before architecture-sensitive work; read it cover-to-cover only when onboarding or doing broad architecture work. ## Project Rules — `memory/INDEX.md` @@ -31,6 +31,10 @@ Pattern + format spec: [`memory/META.md`](memory/META.md). Maintenance loop: [`m When drafting an issue/spec/API/refactor proposal: audit from code (not memory), draft, self-assess. If <95% confident, ask focused clarifying questions on the load-bearing guesses (`AskUserQuestion`, multi-question batches, include a "let the implementer decide" option where genuine), update, repeat. *Then* ask to submit. Catches cow-path-as-design and hallucinated requirements. Cap at ~2 rounds; punt minor stuff to the implementer. +## Reuse-First Change Discipline + +Before adding any new file, public type, interface method, service/repository method, DTO/view model, helper, endpoint, dependency, or DI registration, audit the existing owner/surface first. Prefer reuse, caller-side composition, and small local LINQ/mapping over new durable surface. If new surface is still necessary, state which existing options were rejected and why; public/interface surface requires Peter approval. + ## Concepts — Volunteer vs Tier Applications These are SEPARATE concepts; do not conflate them. diff --git a/docs/architecture/reuse-review-rules.md b/docs/architecture/reuse-review-rules.md new file mode 100644 index 0000000000..082ba0035b --- /dev/null +++ b/docs/architecture/reuse-review-rules.md @@ -0,0 +1,28 @@ +# Reuse Review Rules + +Reuse review answers one question: **did this PR add durable surface that should have reused existing code instead?** It is separate from spec review and correctness review. + +Run it after implementation/spec review and before code review. Findings are actionable only when they name the existing surface that should have been reused, or the new surface whose long-term cost is not justified. + +## Review Checklist + +- **New files:** Does each new file have a clear owner, or could it live in an existing file/component without making that owner incoherent? +- **Public types:** Does each new `public` type need to be public? Could an existing type carry the field/behavior? +- **Interfaces:** Any added interface method must satisfy `memory/architecture/interface-method-additions-are-debt.md`. +- **Services/repositories:** Did the PR add a parallel service/repository/helper where an owning service already exists? +- **DTOs/view models:** Is the new shape reused by multiple callers, or is it a one-off wrapper around an existing model? +- **Endpoints/pages:** Is this a new workflow surface, or should it be an action/view under an existing route owner? +- **DI/dependencies:** Is every new registration/package necessary, or did it bypass an existing project helper? +- **Net shape:** For cleanup/refactor PRs, is the diff net-neutral or net-negative in durable surface? If not, is the reason explicit? + +## Finding Format + +```text +REUSE — should reuse . +Why: . +Fix: . +``` + +## Non-Findings + +Do not flag new surface that is directly required by an accepted feature spec, an EF migration scaffold, generated code, or a clear section boundary. Do not flag a local variable, private helper, or one-call-site mapping block merely because it could be abstracted. diff --git a/memory/INDEX.md b/memory/INDEX.md index 6e2c82589b..ccb0949d5c 100644 --- a/memory/INDEX.md +++ b/memory/INDEX.md @@ -118,6 +118,7 @@ Atomic rules. Fetch the body when the description's trigger matches your task. S - [`pr-review-feedback-handling`](process/pr-review-feedback-handling.md) — When handling PR review feedback (Codex, Claude, human inline reviewers): fetch comments from BOTH repos via the inline-comments API, reply in each finding's own thread, resolve threads when Peter-authorized declines, never ping `@codex review` to re-trigger. - [`privilege-changes-need-explicit-approval`](process/privilege-changes-need-explicit-approval.md) — HARD RULE. Any change granting users new/elevated capability (Drive role bumps, auth-scope additions, role grants, admin flags, default permission tiers, CORS/allowlist expansions) needs Peter's explicit per-change approval before implementation, regardless of issue tier or sprint plan - [`rules-maintenance`](process/rules-maintenance.md) — when a new project rule surfaces, capture as `memory//.md` + INDEX entry in the same commit. Not external memory. +- [`reuse-first-change-discipline`](process/reuse-first-change-discipline.md) — before adding new durable surface, audit existing owners and reuse/composition options first; public/interface surface requires Peter approval - [`simplify-scope-to-section-size`](process/simplify-scope-to-section-size.md) — scale `/simplify` fix counts to section LOC, not to a smaller prior PR's count - [`todos-and-issue-tracking`](process/todos-and-issue-tracking.md) — after resolving commits: update `todos.md` Completed + close GitHub issues with summary + SHA - [`triage-protocol`](process/triage-protocol.md) — When triaging feedback: fetch full message history for every report, show reporter's verbatim Description alongside analysis, and stop the autonomous pipeline on any feedback-originated request that proposes a behavioral/policy/capability/spec change beyond a mechanical fix. diff --git a/memory/process/reuse-first-change-discipline.md b/memory/process/reuse-first-change-discipline.md new file mode 100644 index 0000000000..930834504a --- /dev/null +++ b/memory/process/reuse-first-change-discipline.md @@ -0,0 +1,26 @@ +--- +name: Reuse existing surface before adding durable surface +description: Before adding new files, public types, service/repository methods, DTOs/view models, helpers, endpoints, dependencies, or DI registrations, audit the existing owner/surface and prefer reuse or local composition; public/interface surface requires Peter approval. +--- + +New durable surface is technical debt until proven otherwise. Before adding any new file, public type, interface method, service/repository method, DTO/view model, helper, endpoint, dependency, or DI registration, audit the existing owner and prefer reuse, caller-side composition, or a small local LINQ/mapping chain. + +**Why:** Agent-generated PRs tend to solve local friction by adding methods, helpers, DTOs, services, and routes. Each addition looks reasonable in isolation, but the accumulated surface makes later maintenance and review expensive. The project already enforces interface method budgets; this rule applies the same reuse-first discipline to every other durable surface. + +**How to apply:** + +1. Identify the owning section/service/component before adding anything. +2. List the existing method/type/view/component/helper that is closest to the need. +3. Prefer caller-side composition on existing list-returning or DTO-returning methods when the extra logic is local to one caller. +4. Extract shared code only when there are multiple live call sites, one obvious owner, and the extraction makes the net system simpler. +5. If new surface is still necessary, state which existing options were rejected and why. +6. Stop and ask Peter before adding public/interface surface, or before adding a parallel service/repository/helper where an owner already exists. + +**Examples:** + +- Prefer `GetAllAsync()` + `.Where(...).FirstOrDefault()` at one call site over `GetSpecialCaseAsync()` on an interface. +- Prefer extending an existing view component or partial over creating a near-duplicate component. +- Prefer adding a field to an existing view model only when that view model already owns the page; do not create a parallel model just to avoid touching call-site mapping. + +**Related:** +- [`../architecture/interface-method-additions-are-debt.md`](../architecture/interface-method-additions-are-debt.md) diff --git a/scripts/pr-surface-report.py b/scripts/pr-surface-report.py new file mode 100644 index 0000000000..e01299f8c4 --- /dev/null +++ b/scripts/pr-surface-report.py @@ -0,0 +1,278 @@ +#!/usr/bin/env python3 +"""Generate a PR surface report from a git diff.""" + +from __future__ import annotations + +import argparse +import json +import re +import subprocess +from collections import defaultdict +from pathlib import Path + + +COMMENT_PREFIXES = ("//", "/*", "*", "*/", "#", "@*", " +## PR Surface Report + +Compared `{base}`...`{head}`. + +### LOC + +| bucket | added | deleted | +|---|---:|---:| +{rows} + +Classification notes: `tests` and `migrations` are path-based and separated before code/comment classification. Markdown under `docs/` and `memory/` is counted as `docs`. Comment lines are non-test, non-migration code-adjacent lines that start with a common comment marker. + +### EF Migrations + +Status: **{migration_status}** ({len(migration_files)} real migration file(s); max 1 per PR) + +{migration_note} + +### New Files + +{bullet_list(added_files)} + +### Surface Inventory + +**Public types** + +{bullet_list(inventory.get("public_types", []))} + +**Interface methods** + +{bullet_list(inventory.get("interface_methods", []))} + +**Service/repository methods** + +{bullet_list(inventory.get("service_repository_methods", []))} + +**Routes/actions** + +{bullet_list(inventory.get("routes_actions", []))} + +**DI registrations** + +{bullet_list(inventory.get("di_registrations", []))} + +**Package references** + +{bullet_list(inventory.get("package_references", []))} + +### Changed Files + +{len(changed_files)} file(s) changed. +""" + + +def main() -> int: + parser = argparse.ArgumentParser() + parser.add_argument("--base", required=True) + parser.add_argument("--head", required=True) + parser.add_argument("--output", default="pr-surface-report.md") + parser.add_argument("--json-output", default="pr-surface-report.json") + args = parser.parse_args() + + added_files, changed_files, migration_files = parse_name_status(args.base, args.head) + counts, inventory = parse_diff(args.base, args.head) + markdown = build_markdown(args.base, args.head, counts, added_files, changed_files, migration_files, inventory) + + Path(args.output).write_text(markdown, encoding="utf-8") + Path(args.json_output).write_text( + json.dumps( + { + "base": args.base, + "head": args.head, + "counts": counts, + "added_files": added_files, + "changed_files": changed_files, + "migration_files": migration_files, + "migration_count": len(migration_files), + "inventory": inventory, + }, + indent=2, + ), + encoding="utf-8", + ) + print(markdown) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main())