diff --git a/.agents/skills/engineering-workflow/SKILL.md b/.agents/skills/engineering-workflow/SKILL.md index 06aef55..a0502cb 100644 --- a/.agents/skills/engineering-workflow/SKILL.md +++ b/.agents/skills/engineering-workflow/SKILL.md @@ -14,13 +14,16 @@ description: > writing new C# files, and before adding a new coding pattern not already established in the repo — even a request that just looks like "add a quick feature" or "fix this bug" should route through it first. Also - covers reviewing a PR/diff in this repo and responding to feedback left - on one (`tasks/` procedures, not just reference lookup) — see How to use - this skill below. Trigger on: "how should I structure this", "coding - standards", "is this secure", "how do we handle X in the database", - "where does this belong", "contributing", "code of conduct", "review - this PR", "review my changes", "address that feedback", "handle the - review comments", or any request to add a feature to SharpMud.Engine / + covers designing a feature/architecture decision end-to-end (ADR + plan), + implementing against an already-`Accepted` ADR, reviewing a PR/diff in + this repo, and responding to feedback left on one (`tasks/` procedures, + not just reference lookup) — see How to use this skill below. Trigger + on: "how should I structure this", "coding standards", "is this + secure", "how do we handle X in the database", "where does this + belong", "contributing", "code of conduct", "design this feature", + "let's build/implement this", "review this PR", "review my changes", + "address that feedback", "handle the review comments", "what's next on + the roadmap", or any request to add a feature to SharpMud.Engine / SharpMud.Ruleset.Classic / SharpMud.Persistence / SharpMud.Host / SharpMud.Adapters.*. --- @@ -63,9 +66,17 @@ lives in the file itself. | When you're asked to... | Run | |---|---| +| Design a feature/architecture decision (light or deep dive → ADR → plan) | `tasks/design.md` | +| Implement code against an already-`Accepted` ADR/plan | `tasks/implement.md` | | Review a PR/diff in this repo | `tasks/pr-review.md` | | Address/respond to feedback already posted on a PR | `tasks/respond-to-pr-feedback.md` | +`design.md` → `implement.md` → `pr-review.md` → `respond-to-pr-feedback.md` +is the full lifecycle of a non-trivial change in this repo, in order — +most requests only need one or two of these (e.g. a light-dive design +folded straight into implementation), but a genuinely new feature or +roadmap slice touches all four in sequence. + ### References | When you're about to... | Read | diff --git a/.agents/skills/engineering-workflow/tasks/design.md b/.agents/skills/engineering-workflow/tasks/design.md new file mode 100644 index 0000000..d765c70 --- /dev/null +++ b/.agents/skills/engineering-workflow/tasks/design.md @@ -0,0 +1,114 @@ +# Task: Design + +**Trigger:** asked to design a new feature, an architecture change, or +answer "how should I structure this" — including picking up the next item +off a roadmap (e.g. the next slice in `PLAN-0001`). Produces a decision +(an ADR, and usually a plan) that `tasks/implement.md` then builds against. +This task **does not write application code** — if you catch yourself +editing a `.cs` file mid-design, stop; that belongs in `tasks/implement.md` +once the ADR is `Accepted`. + +## Load these references first + +Always: +- `references/design-decisions.md` — this task is that file's process, + turned into a checklist; read it in full, not just skimmed, since the + light/deep-dive distinction and the ADR/plan mechanics are the actual + content of this task +- `references/documentation.md` + +Conditionally, based on what the design touches — load these *before* +generating alternatives, not after, since they constrain which +alternatives are even legal: +- `references/coding-standards.md` — the design will produce code; know + the constraints (composition over inheritance, DI-only, no MEF/ + singletons) before proposing a shape that violates them +- `references/persistence.md` — touches `SharpMud.Persistence`, a new/ + changed `Behavior`, or anything that needs to survive a restart +- `references/security.md` — touches auth, secrets, config, or the Telnet + input surface +- `references/testing.md` — the design has a non-obvious test strategy + worth deciding up front (e.g. how something inherently timing-/ + concurrency-sensitive gets covered) + +## Procedure + +1. **Check for existing precedent before assuming this is new.** Grep for + the likely type name (`Behavior`, `Manager`, `Command`) and skim the + relevant `docs/*.md` — a request phrased as "add X" is often "X already + exists, here's where" (`design-decisions.md`'s opening rule). A design + session grounded in what's already there beats one that re-derives a + solved problem. +2. **Decide light vs. deep**, per `design-decisions.md`: + - **Light** — problem and solution shape are already clear (adding a + `Behavior` following a pattern another `Behavior` already uses, + adopting an established org-wide tooling convention, etc). Skip + straight to step 4; most ADR sections compress to a sentence. + - **Deep** — the problem is ambiguous, spans multiple subsystems, or has + more than one plausible shape. Do step 3 first. +3. **Deep dive only: brainstorm before drafting anything.** Restate the + problem in your own words. Surface the constraints that actually bind + (existing `Behavior` composition model, no-Ruleset-reference rule, + DI-only wiring, no migrations — pull the specifics from whichever + conditional references you loaded above, don't restate them generically + here). Generate **at least two genuinely different alternatives** — not + a strawman and a preferred option; if you can only think of one real + shape, it probably wasn't a deep dive. Research existing prior art if + relevant (e.g. how WheelMUD solved the analogous problem — see + `docs/research/wheelmud-findings.md` for the citation format) and record + what's adopted vs. deliberately changed. **Talk it through with the + user before committing to one** — if there's a genuine fork (e.g. "match + an external reference's shape 1:1" vs. "a leaner reimplementation fit to + this repo's actual needs"), surface it explicitly (a direct question + works better than silently picking one and writing the ADR around it) — + the point of this phase is to catch a disagreement before it's baked + into an `Accepted` ADR, not after. +4. **Write the ADR.** Copy `docs/adr/0000-adr-template.md` to + `docs/adr/NNNN-kebab-case-title.md` (next sequential number — check + `docs/adr/README.md`'s index for the last one used). Fill in Context, + Decision Drivers, Considered Options, Decision Outcome, Consequences, + Links — compressed to a sentence/"N/A" per section for a light dive, + full detail for a deep one. Keep it to decision content only — no task + checklist, no file list (that's the plan, step 6). Set `Status: + Proposed` while still unsettled; once the user has actually confirmed + the direction (the deep dive's step 3 conversation, or an explicit "yes, + do that" for a light one), flip it to `Accepted` — that status change + *is* the record of the decision being made. +5. **Index and cross-link — without describing unimplemented behavior as + current.** Add a row to `docs/adr/README.md`'s index table. Subsystem + docs describe *current* state (`documentation.md`), and this task + hasn't written any code yet, so don't rewrite a doc's prose to describe + the new decision as already true. The only doc touch that belongs here + is a forward-reference — an Open Items entry (or equivalent) noting the + gap now has a decision, linked to the ADR, e.g. "resolved by ADR-NNNN, + not yet implemented — see `tasks/implement.md`." Actually describing the + new current state is `tasks/implement.md`'s step 6, once the code + backing it exists. If this ADR supersedes an earlier one, set that + ADR's `Status` to `Superseded by ADR-XXXX` — don't edit its Context/ + Decision/Options, an accepted ADR is a historical record. +6. **Decide whether a plan is warranted.** Per `docs/plans/README.md`: not + every ADR needs one — a light dive whose outcome is an obvious one-file + change doesn't. Write one when the implementation is multi-file, spans + more than one sitting, or has enough real sequencing that a checklist + earns its keep (this covers most deep dives and some light ones). If + warranted: copy `docs/plans/0000-plan-template.md` to + `docs/plans/NNNN-kebab-case-title.md` (same number as the ADR it + implements), fill in Goal/Scope/Tasks/Critical files/Test plan/ + Verification, `Status: Not Started`. A plan can be drafted alongside a + still-`Proposed` ADR (designing the how often surfaces questions about + the what) — but it stays `Not Started` until the ADR is `Accepted`; a + plan moving to `In Progress` against a `Proposed` ADR means code is + being written for an unsettled decision. +7. **Index the plan** in `docs/plans/README.md` if one was created, and + — if this design is a slice of the WheelMUD reconciliation roadmap — + update `docs/plans/0001-wheelmud-reconciliation-roadmap.md`'s row for + that slice to link the new ADR/plan. + +## Output + +An `Accepted` ADR (or a `Proposed` one, if genuinely still under +discussion and this task is being paused rather than finished), indexed, +cross-linked from the affected subsystem doc(s), with a `Plan` alongside +it if the implementation is non-trivial. That pair is the handoff to +`tasks/implement.md` — implementation shouldn't start (a plan shouldn't +move to `In Progress`) until the ADR reads `Accepted`. diff --git a/.agents/skills/engineering-workflow/tasks/implement.md b/.agents/skills/engineering-workflow/tasks/implement.md new file mode 100644 index 0000000..dac79ff --- /dev/null +++ b/.agents/skills/engineering-workflow/tasks/implement.md @@ -0,0 +1,89 @@ +# Task: Implement + +**Trigger:** asked to build/implement a feature, write the code for a +slice, or "let's build this now" — following up on `tasks/design.md` +having produced an `Accepted` ADR (and usually a `Plan`). Distinct from +`tasks/design.md`, which decides *what* and *why* and never touches code; +this task is *how*, and assumes the decision is already made. If asked to +implement something with no `Accepted` ADR behind it and the change is +non-trivial (per `design-decisions.md`'s light/deep-dive bar), stop and +run `tasks/design.md` first rather than writing code against an +undocumented decision — retrofitting an ADR after the fact produces a +worse record than writing it first. + +## Load these references first + +Always: +- `references/coding-standards.md` +- `references/testing.md` +- `references/documentation.md` +- `references/design-decisions.md` (just the ADR/plan status rules — you + already did the design thinking in `tasks/design.md`) + +Conditionally, based on what the ADR/plan actually touches: +- `references/persistence.md` — anything under `SharpMud.Persistence`, a + new/changed `Behavior`, or an EF Core config class +- `references/security.md` — auth, secrets, config, or anything on the + Telnet (untrusted) input path + +## Procedure + +1. **Confirm there's an `Accepted` ADR before writing any code.** Find it + (check `docs/adr/README.md`'s index, or the plan referenced by the + request) and read its Decision Outcome — that's the spec you're + building against, not your own read of the request. If a `Plan` exists, + read its current Tasks/Critical files/Test plan sections too; if its + `Status` is still `Not Started` and the ADR is now `Accepted`, flip it + to `In Progress` as you begin. +2. **Implement per `coding-standards.md`** — naming, nullable annotations + (comment every `!` outside the `= null!` EF case), async patterns, + traditional constructors for new classes, DI-only wiring (no MEF, no + static `XManager.Instance`), 4-param-max methods, file layout. Prefer + composition over inheritance for game objects — a new `Behavior`, never + a new `Thing` subtype. XML doc comments on every new/substantially + -changed public member (`documentation.md`) — this is not optional + "nice to have" for new code, only pre-existing code gets the debt + carve-out. +3. **Add tests as you build, not after.** New engine/ruleset/persistence + behavior gets test coverage per `testing.md` (xUnit v3 on Microsoft + Testing Platform, AAA with blank-line separation, per-project + `[XxxAutoData]` attributes, `TestLogger` over `NSubstitute.For>()` + for logging). Use the `dotnet-unit-testing-patterns` skill for the + mechanics (SpecimenBuilders, NSubstitute setup) if you need the deeper + reference. +4. **Update the plan as reality meets it, not just at the end.** Check off + tasks as they're actually done; if the implementation diverges from + what the plan scoped (a file wasn't needed, an extra one was), edit the + plan's task list to match — a plan being wrong about *how* doesn't + require touching the ADR, which stays immutable (`design-decisions.md`). +5. **Build and run the full test suite** before treating anything as done + — `dotnet build` then `dotnet test`, not just "it compiled." +6. **Update `docs/*.md` in the same change**, not a follow-up + (`documentation.md`, `contributing.md`) — mark previously "not + implemented"/stub sections as done, close the Open Items this work + resolves, add "Verified" detail once you've actually checked the + behavior, link back to the ADR rather than re-deriving its reasoning. +7. **Do real manual verification for anything network/session/persistence + -facing** — this repo's established pattern (see `testing.md`, + `contributing.md`) is that unit tests alone don't count as "verified" + for that surface. Actually run the host, connect a real client, exercise + the golden path and the edge case the ADR called out, and record what + you did in the doc's Verified section — not just "tests pass." +8. **Close out the plan.** Once every task is checked off and verification + is done, set the plan's `Status: Done`. If this implements a slice of + `PLAN-0001` (the WheelMUD reconciliation roadmap), check off that + slice's row there too. +9. **Commit, and open a PR if asked**, per `contributing.md`'s "keep PRs + scoped to one decision or one feature" rule — don't bundle an unrelated + fix or refactor into the same diff just because you're already touching + nearby code (`code-of-conduct.md`). Route review through + `tasks/pr-review.md` once a PR exists, rather than self-reviewing + informally. + +## Output + +Working, tested code matching the ADR's Decision Outcome; the plan (if one +exists) at `Status: Done` with every task checked off; `docs/*.md` updated +in the same change, not deferred; and, for anything network/session/ +persistence-facing, a real recorded manual verification — not just a +green test suite. diff --git a/.github/workflows/pr-build.yaml b/.github/workflows/pr-build.yaml index 28fb83b..d74403b 100644 --- a/.github/workflows/pr-build.yaml +++ b/.github/workflows/pr-build.yaml @@ -2,6 +2,7 @@ name: PR Build on: pull_request: + types: [opened, synchronize, reopened, ready_for_review] branches: - main workflow_dispatch: