Skip to content

Require a code-quality review pass before every contribution - #24

Closed
eldarkurtic wants to merge 1 commit into
mobius-os:mainfrom
eldarkurtic:feat/pr-quality-review-gate
Closed

Require a code-quality review pass before every contribution#24
eldarkurtic wants to merge 1 commit into
mobius-os:mainfrom
eldarkurtic:feat/pr-quality-review-gate

Conversation

@eldarkurtic

Copy link
Copy Markdown
Contributor

What this changes

Adds a mandatory code-quality review step to the contribution loop, so the
branch an owner reviews is already the cleaned-up version rather than a first
draft.

contributing.md gains a Review the code before every contribution
section, placed between the approval gate and Prepare for review — after the
code is written, before the review commit is built. It defines two passes over
the branch diff:

  1. Strip the slop — comments restating the line, defensive try/except on
    trusted paths, casts that only silence a complaint, nesting an early return
    would flatten, near-duplicates of an existing helper. Behavior unchanged,
    edits minimal.
  2. Audit the structure — a list of presumptive blockers the agent must fix
    or explicitly justify: a file crossing 1000 lines, ad-hoc conditionals bolted
    into unrelated flows, feature logic leaking into shared paths, indirection
    that buys no clarity, loose payloads papering over an invariant, logic in the
    wrong layer, non-atomic updates.

Because the agent is reviewing its own diff, it acts on findings rather than
listing them, and anything deliberately declined gets one sentence of
justification in the PR body — so a reviewer never has to ask why the obvious
decomposition was skipped. One scope limit keeps this from sprawling: the
restructuring stays inside the change already agreed with the owner.

The staged-record self-review checklist now also confirms both passes actually
ran against the committed branch.

review-followup.md gets the same requirement as a new step 5, deliberately
scoped down: an autopilot grant authorizes answering a review, not restructuring
a codebase. It strips slop and fixes local structural problems in its own new
code, and reports anything larger in the reply or escalates, rather than
force-pushing a rewrite.

Notes for review

  • The section is intentionally self-contained prose with no external
    dependency
    , so it works on every instance. It closes by noting it is the
    floor rather than the ceiling, so instances with a richer code-quality skill
    installed apply that too. The standards were inspired by the publicly
    published deslop and thermo-nuclear-code-quality-review skills in
    cursor/plugins; the wording here is original, and that repository ships no
    license, so nothing is copied from it.
  • Both files are declared under skills in the manifest, so version is
    bumped 0.5.6 → 0.5.7 in mobius.json and package.json for the update to
    reach installed instances. The repo's own manifest.test.mjs pins those two
    to each other.
  • Documentation only — no runtime code paths change. Full suite passes
    (120 pass, 7 pre-existing skips).

Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com>
@eldarkurtic eldarkurtic added the enhancement New feature or request label Jul 26, 2026

@hamzamerzic hamzamerzic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — the "strip the slop" pass (Pass 1) is exactly right and fits how Möbius already thinks about contributions (comments restating the line, defensive try/except on trusted paths, casts that only silence a complaint, near-duplicate helpers). Happy to land that as-is.

Before merging I'd like to soften Pass 2 and a couple of thresholds, because as written they sit in tension with the rest of contributing.md's minimal-diff / narrow-scope discipline and could end up enlarging diffs rather than cleaning them:

  1. Make Pass 2 proportional/advisory instead of a mandatory "ambitious" restructure on every PR. The "audit structure, ambitiously … branches, flags, helpers, or whole layers disappear" framing as a presumptive blocker on every contribution can push toward larger, scope-crept diffs — the opposite of the goal. Suggest reframing to: pursue the structural simplification the change itself motivates, act only when the problem earns it, and keep unrelated refactors out of the PR.

  2. Soften the hard 1000-lines-per-file trigger. As a context-agnostic presumptive blocker it misfires on legitimately long modules, generated files, data/fixtures, and long Markdown. Suggest making it advisory, or scoping it to hand-written source modules.

  3. Allow proportionality for trivial PRs. "MANDATORY … no exceptions" is heavy for a one-line or docs-only change; give the agent license to right-size the effort.

  4. Route decline-justifications to private prior_work evidence rather than the public body_draft. body_draft is published verbatim upstream, so internal "why I didn't decompose X" notes read better as private review evidence — the review-followup counterpart already routes the equivalent into /reply.

The naming ("every contribution" vs. the PR-only flow, since issues/comments have no branch) is minor and can stay. With Pass 2 softened along these lines I'm glad to merge. Thanks again — the core idea is a good one.

@hamzamerzic

Copy link
Copy Markdown
Contributor

Superseded by the reviewed integration in #33, which preserves this PR's commits and authorship. Thank you — the combined change is now on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants