Skip to content

refactor(disposition): one shared mergeable-state interpretation consumed by planner, comment, and labels - #8800

Merged
JSONbored merged 2 commits into
mainfrom
refactor/shared-pr-disposition
Jul 26, 2026
Merged

refactor(disposition): one shared mergeable-state interpretation consumed by planner, comment, and labels#8800
JSONbored merged 2 commits into
mainfrom
refactor/shared-pr-disposition

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

The final structural item of adversarial-audit epic #8757. #8758 fixed the divergent predicates; this removes the CLASS by giving every disposition surface one shared interpretation of GitHub's raw mergeable_state string, with a cross-surface invariant suite that turns any future re-divergence into a test failure.

Changes

  • src/settings/pr-disposition.ts (new, pure, dependency-free — cannot participate in an import cycle): assessMergeableState is now THE single point interpreting the raw string (clean / conflict / behind / unstable / indeterminate, each meaning documented as a contract), and derivePrDisposition computes the shared held/approve/merge core plus the comment surface's downgrade set.
  • agent-actions.ts derives the disposition once per plan and consumes its fields (heldForManualReview, wouldApprove at the approve gate, wouldMerge inside canMerge); isConflict routes through assessMergeableState. Planner-private terms (autonomy, idempotency, approvals, RC3 terminal-block) stay conjoined at their gates and are pinned as planner-private by test. Byte-identical: all 308 existing planner tests pass unchanged.
  • unified-comment.ts readiness gains mergeStateHeld?: boolean, resolved by processors.ts via isCommentMergeStateHeld — the self-contained renderer consumes plain data (zero-import contract intact, parity suite green); callers that don't pass it keep the byte-identical pre-refactor(disposition): single shared PrDisposition object consumed by comment renderer, planner, executor, and labels #8759 raw-string behavior, with an equivalence test proving the fallback matches the shared interpretation for every state.
  • test/unit/pr-disposition-invariants.test.ts: module invariants over the full state matrix (wouldMerge⇒wouldApprove; held⇒neither; behind/blocked/unknown approvable-never-held; conflict unapprovable-never-held), planner-vs-disposition agreement for every state on a green PR (actions + label), renderer-vs-disposition agreement through the bridge boolean, boolean-authoritative-over-raw, and RC3 planner-privacy.

Deliberate deviation from the issue's literal text, documented in the module header: the surfaces share the pure derivation function rather than one threaded object instance — same can-never-drift guarantee (same function + same inputs), a fraction of the plumbing. The executor's live rechecks already re-verify through the same predicates post-#8758 and are unchanged.

Test plan

  • npm run typecheck — clean
  • 11-test invariant suite (new) + 1,091 tests green across all eight adjacent suites including unified-comment-parity
  • Changed hunks incl. the new module: 0 uncovered statements/branches (the one moved pre-existing line's arms pinned by a new RC3 test)

Closes #8759

…umed by planner, comment, and labels (#8759)

The #8711 incident's root CLASS: four surfaces each re-derived their own
meaning for GitHub's raw mergeable_state string (comment {dirty,behind,
unstable}; merge !== clean; approve {dirty}; hold-label none pre-#8758).
#8758 unified the predicates; this removes the class:

- src/settings/pr-disposition.ts (new, pure, dependency-free):
  assessMergeableState — THE single raw-string interpretation point — and
  derivePrDisposition (heldForManualReview / wouldApprove / wouldMerge /
  commentMergeStateHeld), each semantic documented as a contract.
- agent-actions.ts derives the disposition once and consumes its fields for
  heldForManualReview, the approve gate, and canMerge; isConflict routes
  through assessMergeableState. Byte-identical: all 308 existing planner
  tests pass unchanged.
- unified-comment.ts's readiness gains mergeStateHeld — resolved by
  processors.ts via isCommentMergeStateHeld so the self-contained renderer
  consumes a boolean instead of re-deriving meaning (zero-import contract
  intact); legacy callers keep the byte-identical raw-string fallback.
- test/unit/pr-disposition-invariants.test.ts: the cross-surface invariant
  suite — module invariants over the full state matrix, planner-vs-
  disposition agreement per state, renderer-vs-disposition agreement through
  the bridge boolean, and the legacy-fallback equivalence pin.
@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Logic backtest

Replayed 0 historical case(s) for linked_issue_scope_mismatch through the base (11fb8ee) and head (babe826) versions of its detection logic (corpus checksum 4f53cda18c2b).

Backtest comparison: linked_issue_scope_mismatch

Verdict: unchanged — no comparable axis moved.

Advisory only — this check never blocks merge (#8105).

@JSONbored JSONbored self-assigned this Jul 26, 2026
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.94%. Comparing base (b0bdaaa) to head (babe826).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8800      +/-   ##
==========================================
- Coverage   93.81%   92.94%   -0.88%     
==========================================
  Files         797      702      -95     
  Lines       79608    57138   -22470     
  Branches    24127    20253    -3874     
==========================================
- Hits        74686    53107   -21579     
+ Misses       3556     3145     -411     
+ Partials     1366      886     -480     
Flag Coverage Δ
backend 92.94% <100.00%> (-2.16%) ⬇️
control-plane ?
rees ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/queue/processors.ts 95.68% <100.00%> (+<0.01%) ⬆️
src/review/unified-comment.ts 98.48% <100.00%> (+0.01%) ⬆️
src/settings/agent-actions.ts 98.51% <100.00%> (+0.38%) ⬆️
src/settings/pr-disposition.ts 100.00% <100.00%> (ø)

... and 99 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 26, 2026
@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-26 07:03:27 UTC

5 files · 1 AI reviewer · no blockers · CI failing · blocked

🛑 Suggested Action - Manual Review

Review summary
This is a clean structural refactor extracting one shared `assessMergeableState`/`derivePrDisposition` core into a new pure, dependency-free module, then having the planner (agent-actions.ts), the comment bridge (processors.ts), and the renderer (unified-comment.ts) all consume it instead of each re-deriving `mergeable_state` semantics independently. I traced the planner's old `heldForManualReview`/`canMerge`/approve-gate booleans against the new `disposition.wouldApprove`/`wouldMerge`/`heldForManualReview` fields and they are algebraically equivalent (`mergeableClean` → `mergeable === "clean"`, `isConflict` → `mergeable === "conflict"`), and the new invariant suite (test/unit/pr-disposition-invariants.test.ts) directly cross-checks planner output and the comment-status bridge against the shared module for every raw state, which is exactly the kind of test that would catch a re-divergence. The unified-comment.ts fallback path (`mergeStateHeld !== undefined` vs legacy `mergeStateLabel` string check) is well-guarded and preserves byte-identical behavior for older callers.

Nits — 5 non-blocking

CI checks failing

  • validate
  • validate-tests

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8759
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 13 registered-repo PR(s), 13 merged, 275 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 13 PR(s), 275 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Linked issue satisfaction

Partially addressed
The PR unifies raw mergeable_state interpretation into pr-disposition.ts and wires the planner and the comment bridge to it, plus adds a solid invariant test suite, but it does not deliver the single PrDisposition object with verdict/plannedActions/holdReasons/labelSet computed once in processors.ts that the issue specifies, and it does not touch the executor's live rechecks or make label resoluti

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 13 PR(s), 275 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 26, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

1 similar comment
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored
JSONbored merged commit b8b054e into main Jul 26, 2026
6 checks passed
@JSONbored
JSONbored deleted the refactor/shared-pr-disposition branch July 26, 2026 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(disposition): single shared PrDisposition object consumed by comment renderer, planner, executor, and labels

1 participant