From 290ecb66bda95aa31337d81ae0d3b7894cb9ad47 Mon Sep 17 00:00:00 2001 From: angelraph Date: Wed, 29 Jul 2026 03:20:46 +0100 Subject: [PATCH] docs: add meaningful change threshold guide Adds docs/meaningful-change-threshold.md explaining that line count alone is not the standard for judging PR completeness, with small-but-complete vs small-but-incomplete examples, insufficient-change patterns, and reviewer scope-assessment guidance. Cross-links it from README.md, the Meaningful Implementation Checklist, and the Contributor Evaluation Policy so it doesn't duplicate their existing content. Closes #148 --- README.md | 1 + docs/contributor-evaluation-policy.md | 1 + docs/meaningful-change-threshold.md | 128 ++++++++++++++++++++ docs/meaningful-implementation-checklist.md | 4 + 4 files changed, 134 insertions(+) create mode 100644 docs/meaningful-change-threshold.md diff --git a/README.md b/README.md index f503f32..b6e7db7 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,7 @@ See [Failing CI Response Guide](docs/failing-ci-guide.md) for detailed causes an - [Failing CI Response Guide](docs/failing-ci-guide.md) — how to reproduce and fix Rust, Soroban, Makefile, dependency, and workflow failures (failing checks can block approval) - [Payment-Period Conduct Note](docs/payment-period-conduct.md) — contributor expectations during paid periods: no spam, self-review, GrantFox evaluation, CI/testing - [Meaningful Implementation Checklist](docs/meaningful-implementation-checklist.md) — what counts as real contract work: behaviour, security, tests, events, acceptance criteria + reviewer checks +- [Meaningful Change Threshold Guide](docs/meaningful-change-threshold.md) — why line count alone is not the standard, small-but-complete vs. small-but-incomplete examples, and reviewer scope-assessment guidance - [Aegis Contracts Contribution Examples](docs/aegis-contracts-examples.md) — side-by-side comparisons of low-effort, partial, under-tested, failing-CI, and acceptable contributions (reference before opening a PR) - [Minimum Testing Standards](docs/testing-standards.md) — **mandatory** testing requirements per module, happy-path and negative-path expectations, integration fixtures, manual verification guidance, and no-test justification policy diff --git a/docs/contributor-evaluation-policy.md b/docs/contributor-evaluation-policy.md index 0b87eaa..26d37a0 100644 --- a/docs/contributor-evaluation-policy.md +++ b/docs/contributor-evaluation-policy.md @@ -176,5 +176,6 @@ or circumventing evaluation — will be addressed proportionally: - [Evaluation Readiness Summary](./evaluation-readiness.md) — central checklist for contributors before evaluation day - [PR Evidence Checklist](./pr-evidence-checklist.md) — mandatory evidence checklist for every PR - [Meaningful Implementation Checklist](./meaningful-implementation-checklist.md) — what counts as real contract work +- [Meaningful Change Threshold Guide](./meaningful-change-threshold.md) — why line count alone is not the standard, and how reviewers assess scope - [Aegis Contracts Contribution Examples](./aegis-contracts-examples.md) — side-by-side comparisons of acceptable vs unacceptable contributions - [CONTRIBUTING.md](../CONTRIBUTING.md) — development workflow and branching conventions diff --git a/docs/meaningful-change-threshold.md b/docs/meaningful-change-threshold.md new file mode 100644 index 0000000..7e75ffe --- /dev/null +++ b/docs/meaningful-change-threshold.md @@ -0,0 +1,128 @@ +# Aegis Contracts — Meaningful Change Threshold Guide + +Guidance for contributors and reviewers on how to judge whether a PR is +**substantively done**, independent of how many lines it touches. + +> This guide answers one question: *"Is this change enough to close the +> issue?"* For the technical bar a contract change must clear (authorization, +> events, error codes, tests), see the +> [Meaningful Implementation Checklist](meaningful-implementation-checklist.md). +> This guide focuses on **scope judgment** — the line-count fallacy, and how +> to tell a small-but-complete fix apart from a small-but-incomplete one. + +## Why this exists + +Some PRs land with very small diffs that don't solve the underlying issue — +a renamed variable, a comment, a single test, or a partial fix that leaves +the reported behaviour unfixed. Line count is not a reliable signal either +way: + +- A **one-line fix** can fully resolve an issue (e.g. adding a missing + `require_role` check). +- A **300-line PR** can still leave the issue open (new module, no tests, no + enforcement of the actual invariant reported). + +**The standard is completeness against the issue, not diff size.** + +## The threshold: three questions + +A change clears the meaningful-change threshold only if the answer to all +three is yes: + +1. **Does it solve the reported problem**, not a symptom or a piece of it? +2. **Is it verified** — tests that would fail without the change, and pass + with it (see [Testing Standards](testing-standards.md))? +3. **Is it traceable to the issue's acceptance criteria**, with every + criterion met or explicitly called out as deferred (see + [Requirement Traceability Mapping](traceability-mapping.md))? + +If any answer is no, the change is incomplete regardless of its size. + +## Small but complete vs. small but incomplete + +Both examples below are ~2 lines. Only one is meaningful. + +**Small and complete** — closes the issue outright: +```rust +// Issue: unauthorized callers can update the holding cap. +pub fn set_holding_cap(env: Env, admin: Address, cap: i128) -> Result<(), Error> { + require_role(&env, &admin, Role::AssetManager)?; // <-- the fix + if cap < 0 { + return Err(Error::InvalidAmount); + } + env.storage().instance().set(&HOLDING_CAP, &cap); + env.events().publish(("holding_cap_updated",), (admin, cap)); + Ok(()) +} +``` +This is a tight diff, but it enforces the missing invariant, keeps the +existing event and error handling, and is covered by a test that proves a +non-admin caller is rejected. Nothing further is needed to close the issue. + +**Small and incomplete** — looks similar, doesn't close the issue: +```rust +// Issue: unauthorized callers can update the holding cap. +pub fn set_holding_cap(env: Env, admin: Address, cap: i128) { + // TODO: add auth check once role system is finalized + env.storage().instance().set(&HOLDING_CAP, &cap); +} +``` +Same line count, but the actual vulnerability (missing authorization) is +untouched — it's deferred behind a comment. No test proves rejection because +nothing is rejected. This does not clear the threshold no matter how "clean" +the diff looks. + +The distinguishing factor is never size — it's whether the reported problem +is actually gone and proven gone by a test. + +## Examples of insufficient changes + +These patterns show up as real diffs but do not meet the threshold, even +when merged: + +- **Cosmetic-only changes** — renaming variables, reformatting, or comment + edits presented as a fix for a behavioural issue. +- **Partial fixes** — handling one of several reported cases (e.g. rejecting + a negative amount but not a zero address) while marking the issue resolved. +- **Tests added without a corresponding fix** — a new test that documents + the bug but doesn't assert the corrected behaviour, or a test that passes + whether or not the fix is present. +- **Suppressing instead of fixing** — silencing a warning, adding `#[allow]`, + or catching an error without addressing why it occurs. +- **Scope narrowed silently** — implementing only the easy half of the issue + and not disclosing the remainder as out-of-scope in the PR description or + [Completion Table](traceability-mapping.md#completion-table-format). +- **Documentation-only response to a behavioural issue** — updating a doc + comment to describe the bug instead of fixing the code. + +For a broader gallery of failure categories with side-by-side comparisons, +see [Aegis Contracts Contribution Examples](aegis-contracts-examples.md). + +## Reviewer assessment guidance + +When reviewing a PR, don't gauge effort by diff size. Instead: + +- [ ] Read the linked issue's acceptance criteria first, then check each one + off against the diff — not the PR description's claims. +- [ ] Ask: "If I revert just this diff, does the original bug/gap come back?" + If yes, the diff is load-bearing. If the repo behaves the same either + way, the diff isn't meaningful yet. +- [ ] Check that new/changed tests actually fail on the pre-change code + (mentally or by checking out `main` and running them). +- [ ] Look for TODOs, `#[allow(...)]`, or deferred-work comments standing in + for the real fix. +- [ ] Confirm partial scope is disclosed, not silently merged as "done." +- [ ] Weigh a large diff the same way — more code is not evidence of more + correctness. Apply the same three questions from + [above](#the-threshold-three-questions). + +A PR that fails this assessment is not meaningful yet, regardless of size — +request the missing behaviour, test, or disclosure before approval. + +## Related Documents + +- [Meaningful Implementation Checklist](meaningful-implementation-checklist.md) — the technical completeness bar (auth, invariants, events, errors, tests) +- [Aegis Contracts Contribution Examples](aegis-contracts-examples.md) — side-by-side comparisons of low-effort, partial, and acceptable contributions +- [Requirement Traceability Mapping](traceability-mapping.md) — mandatory completion table format for acceptance criteria +- [Reviewer Checklist](reviewer-checklist.md) — standardized quality and security checklist for PR reviewers +- [Contributor Evaluation Policy](contributor-evaluation-policy.md) — formal policy this guide supports diff --git a/docs/meaningful-implementation-checklist.md b/docs/meaningful-implementation-checklist.md index 0b2ae61..4d91673 100644 --- a/docs/meaningful-implementation-checklist.md +++ b/docs/meaningful-implementation-checklist.md @@ -19,6 +19,10 @@ A small PR can be meaningful (a tight, well-tested fix). A large PR can be *un*meaningful (adds code without behaviour, tests, or security). Size is not the measure — **completeness is**. +📖 For a deeper look at judging small-vs-incomplete changes and reviewer +scope assessment, see the +[Meaningful Change Threshold Guide](meaningful-change-threshold.md). + --- ## Before you start