Skip to content

Stateless needsRefine: derive CHANGES_REQUESTED state from PR alone (CROW-508) #509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dgershman merged 3 commits into from
Jun 15, 2026
Merged

Stateless needsRefine: derive CHANGES_REQUESTED state from PR alone (CROW-508) #509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c853f89
Stateless needsRefine: derive CHANGES_REQUESTED state from PR alone (…
dgershman Jun 15, 2026
bfff26f
Address review feedback on CROW-508 (#509)
dgershman Jun 15, 2026
34cac11
Fix .withFractionalSeconds parse failure that made CROW-508 inert (#509)
dgershman Jun 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 59 additions & 26 deletions Packages/CrowCore/Sources/CrowCore/Models/PRStatus.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,48 +6,45 @@ public struct PRStatus: Codable, Sendable, Equatable {
public var reviewStatus: ReviewStatus
public var mergeable: MergeStatus
public var failedCheckNames: [String]
/// Head commit SHA. Used to dedupe per-commit transition events
/// (e.g. don't re-fire "checks failing" when the same commit is re-run).
/// Head commit SHA. Used to dedupe per-commit `.checksFailing` events
/// (don't re-fire when the same commit is re-run).
public var headSha: String?
/// Node ID of the most recent CHANGES_REQUESTED review on this PR, when
/// the provider surfaces stable review identifiers. Round-2 dedup uses
/// this so a fresh "Request changes" submission re-arms auto-respond
/// even when the bucket stays in `.changesRequested`. `nil` when no
/// CHANGES_REQUESTED review is currently visible, or on providers that
/// don't expose review IDs in the monitored-PR query (e.g. GitLab).
public var latestReviewID: String?
/// Whether the underlying PR is currently OPEN (as opposed to MERGED or
/// closed-unmerged). Distinct from `mergeable == .merged`: a CLOSED PR
/// has `mergeable == .unknown` but is still not actionable.
///
/// Required by the stalled-`.changesRequested` re-fire pass (CROW-505):
/// the standing-state predicate reads `previousPRStatus[sid]` directly
/// rather than firing on transition edges, so without this flag a PR
/// that merged or closed while in `CHANGES_REQUESTED` (its
/// `reviewDecision` doesn't reset on close) would keep re-prompting the
/// agent every quiet window to "address review feedback" on a dead PR.
/// Defaults to `true` for backward decode — pre-CROW-505 stores have no
/// `isOpen` field, and the very next poll rewrites the persisted status
/// from the live viewer payload before re-fire is consulted, so the
/// transient default never affects an actual re-fire decision.
/// has `mergeable == .unknown` but is still not actionable. Gates
/// `needsRefine` so a dead PR can never re-prompt.
public var isOpen: Bool
/// Max `submittedAt` across CHANGES_REQUESTED reviews currently visible
/// on the PR. `nil` when no CHANGES_REQUESTED review is present (or the
/// provider doesn't surface review timestamps, e.g. GitLab today).
/// The stateless "needs refine" rule compares this against
/// `lastSubstantiveCommitAt` to decide whether the agent owes a response.
public var lastChangesRequestedAt: Date?
/// Max `committedDate` across the PR's commits that are NOT rebases or
/// merges (parent count < 2 AND message does not start with a merge
/// prefix). `nil` when no commit timestamp data is available. Used by
/// the stateless rule to know whether the author has substantively
/// responded since the latest CHANGES_REQUESTED review.
public var lastSubstantiveCommitAt: Date?

public init(
checksPass: CheckStatus = .unknown,
reviewStatus: ReviewStatus = .unknown,
mergeable: MergeStatus = .unknown,
failedCheckNames: [String] = [],
headSha: String? = nil,
latestReviewID: String? = nil,
isOpen: Bool = true
isOpen: Bool = true,
lastChangesRequestedAt: Date? = nil,
lastSubstantiveCommitAt: Date? = nil
) {
self.checksPass = checksPass
self.reviewStatus = reviewStatus
self.mergeable = mergeable
self.failedCheckNames = failedCheckNames
self.headSha = headSha
self.latestReviewID = latestReviewID
self.isOpen = isOpen
self.lastChangesRequestedAt = lastChangesRequestedAt
self.lastSubstantiveCommitAt = lastSubstantiveCommitAt
}

public init(from decoder: Decoder) throws {
Expand All @@ -57,12 +54,14 @@ public struct PRStatus: Codable, Sendable, Equatable {
mergeable = try c.decodeIfPresent(MergeStatus.self, forKey: .mergeable) ?? .unknown
failedCheckNames = try c.decodeIfPresent([String].self, forKey: .failedCheckNames) ?? []
headSha = try c.decodeIfPresent(String.self, forKey: .headSha)
latestReviewID = try c.decodeIfPresent(String.self, forKey: .latestReviewID)
isOpen = try c.decodeIfPresent(Bool.self, forKey: .isOpen) ?? true
lastChangesRequestedAt = try c.decodeIfPresent(Date.self, forKey: .lastChangesRequestedAt)
lastSubstantiveCommitAt = try c.decodeIfPresent(Date.self, forKey: .lastSubstantiveCommitAt)
}

private enum CodingKeys: String, CodingKey {
case checksPass, reviewStatus, mergeable, failedCheckNames, headSha, latestReviewID, isOpen
case checksPass, reviewStatus, mergeable, failedCheckNames, headSha, isOpen
case lastChangesRequestedAt, lastSubstantiveCommitAt
}

public enum CheckStatus: String, Codable, Sendable {
Expand Down Expand Up @@ -112,4 +111,38 @@ public struct PRStatus: Codable, Sendable, Equatable {
public var hasBlockers: Bool {
!isMerged && (checksPass == .failing || reviewStatus == .changesRequested || mergeable == .conflicting)
}

/// The stateless "needs refine" rule (CROW-508). Returns `true` when the
/// PR is sitting in CHANGES_REQUESTED, is still open, and the agent has
/// not made a substantive commit since the latest CHANGES_REQUESTED
/// review. `terminalIdle` gates the outer IssueTracker dispatch — keeping
/// it as a parameter here makes the rule fully derivable from the PR plus
/// terminal state, with no per-session bookkeeping.
///
/// The rule is intentionally tolerant of nil timestamps:
/// - `lastChangesRequestedAt == nil`: GitHub said `reviewDecision ==
/// CHANGES_REQUESTED` but didn't surface a timestamped CR review (rare;
/// `latestReviews` paginates and can omit one). We refuse to fire — a
/// missing timestamp can't anchor "since when," and a false fire would
/// re-prompt the agent for no reviewer action.
/// - `lastSubstantiveCommitAt == nil`: the PR has no qualifying commits
/// yet (or commit data wasn't fetched). Treat as "no response since
/// review" → still needs refine.
///
/// Anti-loop is automatic: as soon as the agent makes a non-merge,
/// non-rebase commit, `lastSubstantiveCommitAt` advances past
/// `lastChangesRequestedAt` and this returns false on the next poll.
/// A `Merge branch 'main'` commit does NOT advance the timestamp (it's
/// filtered out upstream when computing `lastSubstantiveCommitAt`), so
/// the GitHub "Update branch" button can't trick the rule into a false
/// negative.
public static func needsRefine(status: PRStatus, terminalIdle: Bool) -> Bool {
guard terminalIdle else { return false }
guard status.reviewStatus == .changesRequested, status.isOpen else { return false }
guard let lastReview = status.lastChangesRequestedAt else { return false }
if let lastCommit = status.lastSubstantiveCommitAt {
return lastCommit < lastReview
}
return true
}
}
196 changes: 29 additions & 167 deletions Packages/CrowCore/Sources/CrowCore/Models/PRStatusTransition.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ import Foundation

/// A detected change in a session's `PRStatus` that warrants user attention.
///
/// Computed by `IssueTracker` once per polling cycle by comparing the new
/// `PRStatus` against the previous one. Pure value type — no UI/AppState deps —
/// so the comparison logic stays unit-testable.
/// `.changesRequested` is emitted by the stateless `PRStatus.needsRefine`
/// path in `IssueTracker` (CROW-508). `.checksFailing` is still edge-detected
/// from the previous PR status — checks that just flipped from passing/pending
/// to failing on a new commit.
public struct PRStatusTransition: Sendable, Equatable {
public enum Kind: String, Sendable, Equatable {
/// A reviewer transitioned the PR into "changes requested", or a fresh
/// formal "Request changes" review was submitted while the PR was
/// already in `.changesRequested` (round-2; identified by review-id
/// rotation). See `transitions(from:to:…)` for the detection rules.
/// A reviewer has CHANGES_REQUESTED and the agent hasn't responded
/// with a substantive commit since. Emitted on every poll the rule
/// holds, gated by the per-PR cooldown in `IssueTracker`.
case changesRequested
/// At least one CI/CD check newly transitioned to failing on the
/// current head commit.
Expand All @@ -21,131 +21,49 @@ public struct PRStatusTransition: Sendable, Equatable {
public let sessionID: UUID
public let prURL: String
public let prNumber: Int?
/// Head commit SHA at the moment of the transition. Part of the dedupe key
/// for `.checksFailing` so re-runs on the same commit don't re-fire.
/// Carried on `.changesRequested` for downstream telemetry/UI but is not
/// part of that dedupe key (see `dedupeKey`).
/// Head commit SHA at the moment of the transition. Carried through to
/// downstream consumers (logging, AutoRespondCoordinator) but is no
/// longer part of any dedupe key (CROW-508 removed the meta map).
public let headSha: String?
/// Node ID of the most recent CHANGES_REQUESTED review when the transition
/// fired. Sole round-2 discriminator in the `.changesRequested` dedupe
/// key — a fresh formal review rotates this and re-arms auto-respond,
/// while the author's own response push (which doesn't dismiss the
/// reviewer's CHANGES_REQUESTED decision) leaves it alone and stays
/// deduped. `nil` for providers that don't expose review IDs.
public let latestReviewID: String?
/// Names of failing checks (empty for `.changesRequested`).
public let failedCheckNames: [String]
/// True when this transition is a synthetic re-emission produced by the
/// stalled-`.changesRequested` re-fire pass (CROW-505), not a fresh
/// edge-detected transition. Consumers route on this to suppress noisy
/// downstream effects on re-fires (e.g. AppDelegate skips
/// `notifyPRTransition` so the user doesn't get a fresh macOS
/// notification every quiet window for the same review). The
/// `AutoRespondCoordinator` dispatch still runs — re-prompting the
/// stalled agent is the whole point.
public let isReFire: Bool
/// True when this `.changesRequested` is the second-or-later dispatch
/// for the same reviewer submission (cooldown re-fire on a stuck-idle
/// agent). The re-prompt to the agent is still useful — that's the
/// whole point of the cooldown — but a fresh macOS notification every
/// cooldown window for the same review id is pure noise, so
/// `AppDelegate.onPRStatusTransitions` skips `notifyPRTransition` when
/// this flag is set. Always `false` for `.checksFailing`.
public let isCooldownReFire: Bool

public init(
kind: Kind,
sessionID: UUID,
prURL: String,
prNumber: Int? = nil,
headSha: String? = nil,
latestReviewID: String? = nil,
failedCheckNames: [String] = [],
isReFire: Bool = false
isCooldownReFire: Bool = false
) {
self.kind = kind
self.sessionID = sessionID
self.prURL = prURL
self.prNumber = prNumber
self.headSha = headSha
self.latestReviewID = latestReviewID
self.failedCheckNames = failedCheckNames
self.isReFire = isReFire
}

/// Stable key used to suppress duplicate fires across polling cycles.
///
/// `.changesRequested` keys on `(session, kind, latestReviewID)`. The
/// review ID rotates only when a reviewer submits a new formal review —
/// not when the author pushes a fix — so the auto-respond prompt that
/// instructs the agent to "commit the fix, push, re-request review"
/// can't trigger itself on the next poll (CROW-456 review feedback).
///
/// `.checksFailing` keys on `(session, kind, headSha)` so a new commit
/// that also fails CI can re-fire.
public var dedupeKey: String {
switch kind {
case .changesRequested:
return "\(sessionID.uuidString)|changesRequested|\(latestReviewID ?? "")"
case .checksFailing:
return "\(sessionID.uuidString)|checksFailing|\(headSha ?? "")"
}
}
}

/// Per-emitted-transition metadata persisted alongside the dedup key.
///
/// `emittedAt` lets the stalled-re-fire pass (CROW-505) enforce a quiet
/// window before re-prompting; `headShaAtEmit` is the head SHA observed at
/// dispatch, which preserves the anti-loop guarantee — a real agent push
/// advances the head SHA, so re-fire only triggers when the author has not
/// pushed since the original prompt fired.
///
/// `reFireCount` bounds the number of times the same emission may re-fire
/// before requiring a real edge event (reviewer rotates `latestReviewID`,
/// author pushes, bucket exits) to reset. Without this cap the re-fire would
/// loop forever when the agent legitimately *replies* on the PR instead of
/// pushing — a behavior the auto-respond prompt itself encourages ("If a
/// comment is unclear or you disagree, leave a reply…"). The reply leaves
/// the SHA unchanged but is a valid terminal state, not a stall.
public struct EmittedTransitionMeta: Codable, Sendable, Equatable {
public var emittedAt: Date
public var headShaAtEmit: String?
public var reFireCount: Int

public init(emittedAt: Date, headShaAtEmit: String?, reFireCount: Int = 0) {
self.emittedAt = emittedAt
self.headShaAtEmit = headShaAtEmit
self.reFireCount = reFireCount
}

public init(from decoder: Decoder) throws {
let c = try decoder.container(keyedBy: CodingKeys.self)
emittedAt = try c.decode(Date.self, forKey: .emittedAt)
headShaAtEmit = try c.decodeIfPresent(String.self, forKey: .headShaAtEmit)
// Backward decode for pre-cap stores: a missing field means the entry
// has never been re-fired yet, so 0 is the safe default.
reFireCount = try c.decodeIfPresent(Int.self, forKey: .reFireCount) ?? 0
}

private enum CodingKeys: String, CodingKey {
case emittedAt, headShaAtEmit, reFireCount
self.isCooldownReFire = isCooldownReFire
}
}

extension PRStatus {
/// Compute the transitions implied by moving from `old` to `new`.
///
/// On the first observation (`old == nil`), emits a transition only when
/// the new state is itself a "needs attention" condition — `.changesRequested`
/// or failing checks. A PR first observed as `.open` with passing/pending
/// checks emits nothing (the startup-flood safety the bare guard used to
/// provide). Persisted `emittedTransitionKeys` in `IssueTracker` (CROW-456)
/// suppress restart re-fires of transitions already emitted by a prior run.
/// Compute the `.checksFailing` transition (if any) implied by moving from
/// `old` to `new`. `.changesRequested` is no longer computed here — the
/// stateless `needsRefine` path in `IssueTracker` handles it directly off
/// the PR snapshot (CROW-508).
///
/// Otherwise emits at most one `.changesRequested` and one `.checksFailing`,
/// in that order.
///
/// `.changesRequested` fires when the bucket is entered (`old` wasn't
/// `.changesRequested`) or when the latest CHANGES_REQUESTED review ID
/// rotates while still in the bucket (a fresh formal "Request changes"
/// submission). A head-SHA change alone does **not** fire — `reviewDecision`
/// stays `CHANGES_REQUESTED` after the author pushes, so any such trigger
/// would re-fire on the agent's own response push and create an auto-respond
/// loop. Real "round 2 after a fix" always involves the reviewer submitting
/// a new formal review, which rotates the review ID.
/// On the first observation (`old == nil`), emits a `.checksFailing`
/// transition only when the new state itself is failing — same
/// startup-flood safety the prior implementation provided for CI.
///
/// - Note: This is the pure piece of the transition pipeline; the
/// stateful dedupe (across polls) lives in `IssueTracker`.
Expand All @@ -156,66 +74,10 @@ extension PRStatus {
prURL: String,
prNumber: Int?
) -> [PRStatusTransition] {
guard let old else {
// First observation (CROW-477): emit only when the new state is
// itself attention-needing, so a PR Crow first sees already in
// CHANGES_REQUESTED (or with failing checks) still routes to
// auto-respond. Restart-spurious re-fires are prevented by the
// persisted dedupe keys at the IssueTracker layer, not here.
var out: [PRStatusTransition] = []
if new.reviewStatus == .changesRequested {
out.append(PRStatusTransition(
kind: .changesRequested,
sessionID: sessionID,
prURL: prURL,
prNumber: prNumber,
headSha: new.headSha,
latestReviewID: new.latestReviewID,
failedCheckNames: []
))
}
if new.checksPass == .failing {
out.append(PRStatusTransition(
kind: .checksFailing,
sessionID: sessionID,
prURL: prURL,
prNumber: prNumber,
headSha: new.headSha,
failedCheckNames: new.failedCheckNames
))
}
return out
}

var out: [PRStatusTransition] = []

if new.reviewStatus == .changesRequested {
let entering = old.reviewStatus != .changesRequested
// Require both ids to be non-nil. If a CHANGES_REQUESTED review is
// briefly absent from `latestReviews(first: 5)` (buried behind 5+
// other reviewers' latest reviews) and then resurfaces with a new
// id, the nil → id step intentionally won't fire. We prefer a
// missed re-prompt to a spurious one — `reviewDecision` and the
// `latestReviews`-derived id can legitimately disagree during a
// single CHANGES_REQUESTED tenure, and a spurious fire would
// re-prompt the agent for no reviewer action.
let newReview = old.latestReviewID != nil
&& new.latestReviewID != nil
&& old.latestReviewID != new.latestReviewID
if entering || newReview {
out.append(PRStatusTransition(
kind: .changesRequested,
sessionID: sessionID,
prURL: prURL,
prNumber: prNumber,
headSha: new.headSha,
latestReviewID: new.latestReviewID,
failedCheckNames: []
))
}
}

if old.checksPass != .failing && new.checksPass == .failing {
let wasFailing = old?.checksPass == .failing
if !wasFailing, new.checksPass == .failing {
out.append(PRStatusTransition(
kind: .checksFailing,
sessionID: sessionID,
Expand Down
Loading
Loading