Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion .claude/agents/batch-worker.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -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 <N> reuse issues`.

### Phase 3: Code Review (after all issues in batch)

Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
59 changes: 59 additions & 0 deletions .claude/skills/reuse-review/SKILL.md
Original file line number Diff line number Diff line change
@@ -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 <number>` - 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 <number> --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 - <new surface> should reuse <existing surface>.
Why: <cost/risk>.
Fix: <specific consolidation or caller-side composition>.

### 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.
26 changes: 14 additions & 12 deletions .claude/tech-debt-prompt.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

---

Expand Down Expand Up @@ -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
Expand All @@ -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.

---

Expand All @@ -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.
12 changes: 7 additions & 5 deletions .claude/tech-debt-tasks-prompt.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

---

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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.
19 changes: 19 additions & 0 deletions .github/ISSUE_TEMPLATE/bug_report.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
15 changes: 15 additions & 0 deletions .github/ISSUE_TEMPLATE/feature_request.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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.
5 changes: 5 additions & 0 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ Tick items as you go, or strike through (`~~item~~`) with a reason if a row genu

<!-- The motivation. Skip if obvious from the linked issue. -->

## Existing surface checked

<!-- List existing services/components/helpers/routes/DTOs considered before adding new surface. Use "No new durable surface" when true. -->

## UI changes / screenshots

<!-- Drop screenshots or short clips for any user-visible change. Delete this section if there are none. -->
Expand All @@ -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/<bucket>/<name>.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.
Expand Down
Loading
Loading