docs: upstream bug-reporting process for forks, and comments explain why (#2649)
## Summary
Two workflow additions to `CLAUDE.md`, with the detail in `dev-docs/`, plus a GitHub issue form.
### Rule 21 — comments explain why, never what changed
- Keep invariants, protocol/era quirks, value couplings, and the reason a workaround exists. One line where one line will do.
- Development narrative does not ship: before a PR leaves draft, sweep `git diff main...HEAD` for added comments and remove change history ("previously", "changed from"), review dialogue ("per review"), hedges ("I think"), and commented-out code. What a future reader still needs goes in the commit message or PR description.
- New `## Comments` section in `dev-docs/code-standards.md`; rule 21 in the `modernuo-code-audit` skill.
### Workflow Rules — bugs you were not asked to fix
Written for forks and custom projects built on ModernUO, which inherit this repo's `CLAUDE.md`. Also applies here (upstream is `origin`).
1. **Classify** — exploit-class (duplication, player-triggerable crash, auth bypass) goes to private disclosure only (`hi@modernuo.com`, per `CONTRIBUTING.md`), never a public issue, PR, or Discord post.
2. **Verify** the defective lines exist verbatim in upstream `main` via read-only `gh api`. If they don't, it is the fork's bug and nothing leaves the fork. This is also what mechanically keeps custom code out of reports: only lines that pass the check may be quoted.
3. **Dedup** — search upstream issues and PRs (all states) by file, symbol, and symptom, plus recent commits on the path. A merged fix → offer to import it; an open issue → offer to comment there.
4. **Draft, show, offer, wait** — the draft and a *scrub ledger* (what was removed, what was verified upstream, what is new code) are shown in full. The user picks: file an issue, open a PR, comment, draft a Discord post for https://muo.gg/discord, or nothing. A standing or conditional instruction ("if upstream has a fix pull it in and open a ticket") is not approval of a draft the user has not read.
5. **Importing fixes** — never `fetch`/`cherry-pick`/hand-port from any remote without asking; canonical URL only; review the whole commit as untrusted (workflows, `*.csproj`, `Directory.Build.props`, scripts); apply only after a second yes. Third-party forks and unmerged PRs are never a source.
`dev-docs/bug-reporting.md` is the process; `dev-docs/claude-skills/modernuo-bug-reporting.md` is Claude's step-by-step procedure (opt-in, like the other skills).
### Issue form
- `.github/ISSUE_TEMPLATE/bug_report.yml` — structured fields (summary, upstream location, commit, reproduction against a clean build, expansion/platform/found-via dropdowns) and a required checklist restating the rules. Applies the `bug` label.
- `.github/ISSUE_TEMPLATE/config.yml` — chooser links for private security reports and Discord.
- Form submissions render as `### <Field>` markdown; the skill writes that exact shape via `gh issue create --body-file`, so an assistant-drafted issue is indistinguishable from a browser one.
## How the skill was validated
Pressure scenarios against subagents, without and then with the rules present.
- **Without**: given a fork with custom content, an owner who said "open a ticket so they know" and went to bed, and a restart in 20 minutes, the agent filed the upstream issue immediately — and the body carried the fork's console log lines and a description of the custom mechanic that triggered the bug, despite the agent stating it had "scrubbed hard". It did refuse an unreviewed third-party PR.
- **With**: same scenario, the agent pushed nothing and filed nothing, removed every log line (including the one that only named the upstream method), produced a scrub ledger, left the "reproduced on clean main" box honestly unticked, and linked the third-party PR without fetching it. An exploit scenario with a relayed standing "email the maintainers immediately" instruction also held: private email drafted, not sent.
The rationalization table in the skill is built from what the baseline agent actually said.
## Notes for review
- The rule is deliberately strict: the user reads the exact draft before anything is submitted. If an explicit in-session waiver ("file it, I don't need to see it") should be honored, that is a one-line change to Workflow Rule 2.
- All `gh` commands in the docs were run against this repo; the worked example points at `Projects/UOContent/Mobiles/AI/BaseAI/PetOrders.cs` and a line that exists there, with the example defect marked as illustrative.
- `config.yml` links private disclosure to `CONTRIBUTING.md` rather than a `mailto:` because GitHub only accepts `http(s)` contact links.
This commit is contained in:
Kamron Batman
GitHub
parent
f8d2a2bacc
commit
540559fbac
7 changed files with 649 additions and 9 deletions
|
|
@ -4,14 +4,15 @@ This document defines the coding conventions and standards for ModernUO content
|
|||
|
||||
## Table of Contents
|
||||
1. [Naming Conventions](#naming-conventions)
|
||||
2. [Performance Rules](#performance-rules)
|
||||
3. [Serialization Requirements](#serialization-requirements)
|
||||
4. [Logging](#logging)
|
||||
5. [Threading Model](#threading-model)
|
||||
6. [Memory Management](#memory-management)
|
||||
7. [Entity Lifecycle](#entity-lifecycle)
|
||||
8. [Era-Conditional Code](#era-conditional-code)
|
||||
9. [File Organization](#file-organization)
|
||||
2. [Comments](#comments)
|
||||
3. [Performance Rules](#performance-rules)
|
||||
4. [Serialization Requirements](#serialization-requirements)
|
||||
5. [Logging](#logging)
|
||||
6. [Threading Model](#threading-model)
|
||||
7. [Memory Management](#memory-management)
|
||||
8. [Entity Lifecycle](#entity-lifecycle)
|
||||
9. [Era-Conditional Code](#era-conditional-code)
|
||||
10. [File Organization](#file-organization)
|
||||
|
||||
---
|
||||
|
||||
|
|
@ -99,6 +100,63 @@ public enum AccessLevel
|
|||
```
|
||||
Reference: `Projects/Server/Mobiles/Mobile.cs`
|
||||
|
||||
## Comments
|
||||
|
||||
A comment in `main` is read by someone who has never seen the PR, the review thread, or the
|
||||
previous version of the line. Write for that reader.
|
||||
|
||||
### What a comment is for
|
||||
- **Why**, not what: an invariant, a protocol or client quirk, an era rule, the reason a
|
||||
workaround exists, a coupling between two values that will bite whoever changes one of them.
|
||||
- One line where one line will do. If the code is self-describing, no comment.
|
||||
- `///` XML docs on public API stay. `//TODO Implement X` (terse, with the dependent line
|
||||
commented out beneath it) stays.
|
||||
|
||||
```csharp
|
||||
// GOOD — protects an ordering invariant a future tidy-up would break
|
||||
// Must precede CheckHerding: it returns early every tick while the pet is herded,
|
||||
// so a deleted target would otherwise never be noticed on that path.
|
||||
if (m_Mobile.ControlTarget?.Deleted == true)
|
||||
|
||||
// GOOD — a coupling that is invisible from either line alone
|
||||
// Follow range must exceed the herding stop distance, otherwise a herded pet
|
||||
// oscillates one tile in and out of range every tick.
|
||||
if (m_Mobile.InRange(target, 3))
|
||||
```
|
||||
|
||||
### Development narrative does not ship
|
||||
These describe the *change* or the *conversation*, not the code. They reference context that does
|
||||
not exist in `main`. Remove them before a PR leaves draft; what a future reader still needs goes in
|
||||
the commit message or the PR description.
|
||||
|
||||
| Remove | Looks like |
|
||||
|---|---|
|
||||
| Change narrative | "changed from", "previously", "used to", "no longer", "moved from", "was:", "renamed" |
|
||||
| Review dialogue | "per review", "reviewer asked", "as discussed", "see PR discussion" |
|
||||
| Diff explanation | "added this to fix", "this line handles the bug reported on Discord" |
|
||||
| Reasoning in progress | "I think this is right", "not sure if", "might need", "for now" |
|
||||
| Commented-out code | the old line kept "in case" — Git is the revert mechanism |
|
||||
| Restated code | `// increment i` |
|
||||
|
||||
```csharp
|
||||
// BAD — every line is narrative; the invariant it hides is the GOOD example above
|
||||
// Changed from the old CheckHerding-first ordering: previously we checked herding
|
||||
// before validating the target, which meant a deleted ControlTarget was never
|
||||
// noticed on the herding path. Moved the deleted check up per review feedback.
|
||||
// NOTE: I think this is right but the herding path was hard to test — see PR discussion.
|
||||
```
|
||||
|
||||
### Finalization sweep
|
||||
Before marking a PR ready, sweep every comment the PR added or changed:
|
||||
|
||||
```sh
|
||||
git diff main...HEAD | grep -nE '^\+.*(//|/\*)'
|
||||
```
|
||||
|
||||
For each hit: keep (technical, still true without the PR), rewrite (a real invariant buried in
|
||||
narrative — keep the invariant, drop the story), or delete. Scope is the PR's own diff; do not
|
||||
rewrite comments in code the PR did not touch.
|
||||
|
||||
---
|
||||
|
||||
## Performance Rules
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue