diff --git a/AGENTS.md b/AGENTS.md index 06be35d2..aca39741 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,6 +3,7 @@ Clusterio manages distributed Factorio servers through a controller, hosts, and plugins. Read [contributing](docs/contributing.md) for the project map, setup, commands, debugging, tests, and code style. +For PR reviews, follow the [review guide](docs/pr-review-guide.md), including its candidate ledger and final evidence audit. Use the detailed guides for the affected area: - [Communication and routing](docs/communication.md), [wire protocol](docs/devs/protocol.md), and [configuration](docs/config-system.md). diff --git a/docs/pr-review-guide.md b/docs/pr-review-guide.md new file mode 100644 index 00000000..76bf43f9 --- /dev/null +++ b/docs/pr-review-guide.md @@ -0,0 +1,60 @@ +# Reviewing pull requests + +Review observable behavior across the whole PR, including callers, consumers, and lifecycle transitions. +Reading every changed file and passing tests do not establish that those behaviors were covered. +Use [contributing](contributing.md) for setup and validation commands. + + +## Establish the scope + +- Record the PR's base and current head; preserve unrelated work with an isolated checkout when needed. +- Read the requirements and relevant documentation at the reviewed revision. + Distinguish intended policy from observed behavior when they disagree. +- Map each affected behavior from input through authorization, routing, state changes, and its consumers. + On a follow-up, verify the fixes and reassess the whole PR's affected behavior; explicitly label a deliberately narrower review. +- Trace the actual entrypoint and earlier guards before declaring a failure reachable. + Check supported configurations and compatibility policy; a merged revision or earlier clean review is not proof of correctness. + + +## Exercise the boundaries + +- Write a small matrix of relevant states and transitions before probing. + For example, distinguish configured, loaded, installed, and post-restart plugin state; test each operation permission independently. +- Check adverse paths as well as success: failure, retry, removal, restart, reconnect, and overlapping operations where applicable. + Select relevant cases from these recurring review gaps: + + | Boundary | Questions to check | + | --- | --- | + | Data contracts | Does real serializer output satisfy every consumer, including Lua restoration and inspection UI? Check omitted, empty, partial, malformed, reserved, and numeric-limit values before durable mutation. | + | State and identity | Do delayed reads and mutation completions still belong to the selected entity? Do disabled components retain their local state? Can an identifier or generation be reused after recovery? | + | Persistence and synchronization | Do save/reload, migration, deletion, and resubscription preserve versions and tombstones? Compare full snapshots, deltas, and optimized paths with ordinary transfers. | + | Ownership and errors | Can concurrent work remove another operation's lock, handle, or cache? Check synchronous throws, early promise rejection, partial mutation, and cleanup after each failure. | + | Protocol and compatibility | What does the receiver do with the message, close code, or acknowledgement? Check early startup, mixed versions, and supported older APIs. | + | Distribution | Do generated validators, browser bundles, and installed packages behave like the source checkout? Check actual export maps and plugin discovery. | + +- Use focused reproductions with concrete inputs and expected versus actual results. + Keep the logic under review real: stubbing permissions, effects, or state transitions can erase the defect being investigated. + Check that the harness reaches the intended path; compare a normal case and the base revision where practical. +- Control concurrency with explicit gates and assertions about ownership and completion, rather than timing alone. + + +## Account for every candidate + +- Keep a compact ledger of candidate issues and uncovered scenarios throughout the review. + Give each candidate a disposition: reported, rejected with evidence, or unresolved with the next check needed. +- Before finishing, compare probe outputs with the final findings. + A reproduced anomaly must not disappear just because several other issues were already found. +- Carry unresolved candidates and coverage gaps into handoffs and context summaries, alongside published findings. + For each finding, identify the trigger, consequence, source location, and supporting evidence. +- Use history to distinguish newly introduced regressions from older defects when relevant; verify the behavior at the earlier revision. + + +## Report and follow up + +- Separate source inspection, focused probes, full suites, hosted CI, browser checks, and Factorio runtime evidence. + Record failures, skips, and unavailable validation; investigate external-service failures before attributing them to the PR. +- Reproduce the original failure when checking a fix, including adjacent cases exposed by the change. + A fix for one malformed field calls for checking the remaining consumer requirements, not just that example. + Resolve a review thread only when its exact concern is addressed. +- When authorized to publish, refresh the head and existing comments, avoid duplicates, and anchor feedback to the reviewed code. + Read back each posted inline comment to verify its body and location.