fix(decide): reach mergeable without formal approval; broaden consistency test

Address PR review (aa-1):
- ResolveOpenPRDecision now keys MERGEABLE on Mergeable alone (checked
  before Approved). Mergeability is the authoritative merge gate, so a PR
  on a no-required-review repo no longer falls through to PR_OPEN. The
  approved+mergeable and approved-only cases are unchanged.
- Broaden the derive-consistency test to cover the probe and terminal
  deciders too, not just the open-PR ladder.
- Document the HashEvidence epoch-stripping regex's breadth.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
harshitsinghbhandari 2026-05-27 00:06:57 +05:30
parent 6e90734276
commit 9920c6daaa
2 changed files with 48 additions and 8 deletions

View File

@ -166,7 +166,11 @@ func ResolveOpenPRDecision(in OpenPRInput) LifecycleDecision {
return base(domain.StatusCIFailed, domain.PRReasonCIFailing, domain.SessionWorking, domain.ReasonFixingCI)
case in.ChangesRequested:
return base(domain.StatusChangesRequested, domain.PRReasonChangesRequested, domain.SessionWorking, domain.ReasonResolvingReviewComments)
case in.Approved && in.Mergeable:
case in.Mergeable:
// Mergeability is the authoritative merge gate, so it already folds in
// "approved if review is required". Checking it before Approved means a
// PR on a no-required-review repo (mergeable, not formally approved) is
// still surfaced as ready-to-merge instead of falling through to PR_OPEN.
return base(domain.StatusMergeable, domain.PRReasonMergeReady, domain.SessionIdle, domain.ReasonAwaitingExternalReview)
case in.Approved:
return base(domain.StatusApproved, domain.PRReasonApproved, domain.SessionIdle, domain.ReasonAwaitingExternalReview)
@ -270,6 +274,11 @@ func HashEvidence(evidence string) string {
// timestampPatterns strip the time-varying parts of an evidence string before
// hashing. Order matters: the full datetime form is removed before the bare
// time-of-day and epoch forms so they don't partially match.
//
// The epoch pattern strips any bare 10-13 digit run (unix seconds/millis). That
// is broad enough to also clobber a same-width numeric ID if one ever appears in
// an evidence string; evidence is decider-authored, so today nothing else lands
// in that range, but keep IDs out of evidence strings to preserve hash fidelity.
var timestampPatterns = []*regexp.Regexp{
regexp.MustCompile(`\d{4}-\d{2}-\d{2}[T ]\d{2}:\d{2}:\d{2}(?:\.\d+)?(?:Z|[+-]\d{2}:?\d{2})?`),
regexp.MustCompile(`\d{2}:\d{2}:\d{2}(?:\.\d+)?`),

View File

@ -182,6 +182,13 @@ func TestResolveOpenPRDecision(t *testing.T) {
wantPR: domain.PRReasonMergeReady,
wantState: domain.SessionIdle,
},
{
name: "mergeable without formal approval (no required review) -> mergeable",
in: OpenPRInput{Mergeable: true},
wantStatus: domain.StatusMergeable,
wantPR: domain.PRReasonMergeReady,
wantState: domain.SessionIdle,
},
{
name: "approved but not mergeable -> approved",
in: OpenPRInput{Approved: true},
@ -238,26 +245,50 @@ func TestResolveOpenPRDecision(t *testing.T) {
}
}
func TestResolveOpenPRDecisionDerivesConsistently(t *testing.T) {
// The display Status produced by the ladder must equal what DeriveLegacyStatus
// would produce from the same canonical (session, pr) it emits.
inputs := []OpenPRInput{
func TestDecidersDeriveConsistently(t *testing.T) {
// Every decision a decider produces must be self-consistent: the display
// Status it reports must equal what DeriveLegacyStatus produces from the
// canonical (session, pr) sub-states it emits. This locks the deciders and
// the display-derivation against drifting apart.
//
// The ResolveTerminalPRStateDecision none/open default is intentionally
// excluded — it is a documented no-op for misuse, not a real verdict.
var decisions []LifecycleDecision
for _, in := range []OpenPRInput{
{CIFailing: true},
{ChangesRequested: true},
{Approved: true, Mergeable: true},
{Mergeable: true},
{Approved: true},
{ReviewPending: true},
{IdleBeyond: true},
{},
} {
decisions = append(decisions, ResolveOpenPRDecision(in))
}
for _, in := range inputs {
d := ResolveOpenPRDecision(in)
decisions = append(decisions,
ResolveTerminalPRStateDecision(domain.PRMerged),
ResolveTerminalPRStateDecision(domain.PRClosed),
)
for _, in := range []ProbeInput{
{KillRequested: true, Now: t0},
{Runtime: domain.RuntimeAlive, Process: ProcessAlive, Now: t0},
{Runtime: domain.RuntimeMissing, Process: ProcessIndeterminate, Now: t0},
{Runtime: domain.RuntimeExited, Process: ProcessDead, Now: t0},
} {
decisions = append(decisions, ResolveProbeDecision(in))
}
for _, d := range decisions {
l := domain.CanonicalSessionLifecycle{
Session: domain.SessionSubstate{State: d.SessionState, Reason: d.SessionReason},
PR: domain.PRSubstate{State: d.PRState, Reason: d.PRReason},
}
if got := domain.DeriveLegacyStatus(l); got != d.Status {
t.Errorf("input %+v: decision Status=%q but DeriveLegacyStatus=%q", in, d.Status, got)
t.Errorf("decision %+v: Status=%q but DeriveLegacyStatus=%q", d, d.Status, got)
}
}
}