Skip to content

maintainability: three paid advisories hand-write their own spend gate, and one shipped without the commit cap #9557

Description

@JSONbored

Parent: #9541

Summary

Deliverable 2 of #9541: "The three run*ForAdvisory spend gates behind one shared precondition struct."

Three features each make a paid LLM call per PR head — ai_slop (slop-detection.ts), ai_review (ai-review-orchestration.ts), and the linked-issue satisfaction advisory (processors.ts). Each grew its own hand-written guard line, and they drifted.

#9491 found the concrete consequence: the linked-issue advisory was the one member of the family with no per-PR commit cap at all, so a long-lived PR kept paying for a fresh assessment on every push long after its two siblings had stopped. Nobody could have caught that by reading one function — the guards live in three files, and the incomplete one looked complete on its own. That is the structural drift #9541 exists to remove, in its smallest and most clearly-scoped instance.

Requirements

  1. Extract only the stops that are genuinely universal to every paid advisory.
  2. Behaviour-preserving. Adopting the shared function must not change which stop fires for any current input — the reported reason decides which audit event a PR emits, so a re-ordering would silently relabel real production history.
  3. Do not unify the eligible-author rule (see below).

What must NOT be unified, and why

!confirmedContributor appears in two of the three guards and looks shared. It is not: ai_review deliberately reviews some unconfirmed authors via resolveAiReviewableAuthor's aiReviewAllAuthors / gate-pack policy.

Folding that into a common predicate would either silently narrow ai_review's audience or silently widen the other two's. A "shared" rule that is wrong for one caller is exactly how the next #9491 gets written — so the author gate stays per-feature, and the module has to say so.

ai_review also carries its own richer, correctly-designed gate (resolvePublicAiReviewGateSkipReason: skipAiReview, aiReviewMode, env-binding checks) which already returns a named reason. Its universal stops are the same three; its feature-specific ones stay where they are.

Deliverables

  • advisorySpendStopReason(...) — pure, returning a named reason (paused | no_head_sha | commit_threshold_reached) rather than a boolean, mirroring resolvePublicAiReviewGateSkipReason's shape. Callers usually want to audit which stop fired, and a bare false forces every call site to re-derive it.
  • The caller's response to a stop stays per-feature (return silently vs. record an audit event) — those legitimately differ and must not be unified.
  • Adopt it in runAiSlopForAdvisory and runLinkedIssueSatisfactionForAdvisory.

Tests

  • Each stop fires; null when all preconditions pass; empty-string / null / undefined head SHA all stop.
  • Precedence matches the hand-written guards it replaces, asserted explicitly — this is what makes the adoption provably behaviour-preserving.
  • Pure: same input → same answer, input never mutated.
  • Adoption is pinned at the source, so the guarantee ("a fourth advisory cannot be written without these stops") stays true rather than aspirational: both adopters route through the shared gate, neither re-implements the commit-cap stop inline, and the shared module has not grown a confirmedContributor field.

Metadata

Metadata

Assignees

Labels

maintainer-onlyOwner-only work — yields no Gittensor points.orbGittensory Orb related - maintainer self-hosting analytics.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions