From 3962417092e3b279d4522f9805e920abb24e808e Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Sun, 14 Jun 2026 17:05:06 -0400 Subject: [PATCH 1/4] Re-fire stalled .changesRequested when agent is idle (CROW-505) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When Crow tracks a PR sitting in CHANGES_REQUESTED and the agent goes idle after one auto-refine round, Crow never re-prompts β€” the dedup key keeps the round suppressed until the reviewer submits a new formal review. Now Crow re-fires the same transition when the managed terminal's agent is idle, the head SHA hasn't advanced since dispatch (anti-loop guarantee from CROW-456 preserved), and a 10-minute quiet window has elapsed. Storage: `IssueTracker.emittedTransitionKeys: Set` becomes `emittedTransitionMeta: [String: EmittedTransitionMeta]`, where the meta carries `emittedAt` (quiet-window clock) and `headShaAtEmit` (SHA-equality anti-loop gate). `PersistedIssueTrackerState` gains an optional `emittedTransitionMeta` field; pre-CROW-505 stores are migrated by synthesizing meta from the legacy key array, sourcing `headShaAtEmit` from the also-persisted `previousPRStatus` and starting the quiet-window clock at now (no immediate re-fire flood at upgrade). Also flips `AutoRespondSettings.respondToChangesRequested` default to true for new configs. Existing users' explicit choices stay sticky: `decodeIfPresent` returns the written value, so users who explicitly toggled the setting off keep it off. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: C69B361D-F6EE-49FC-9F5B-E7A3928F5F1E --- .../Sources/CrowCore/Models/AppConfig.swift | 16 +- .../CrowCore/Models/PRStatusTransition.swift | 17 ++ .../AutoRespondSettingsDefaultsTests.swift | 47 ++++ .../Sources/CrowPersistence/JSONStore.swift | 15 +- .../CrowPersistenceTests/JSONStoreTests.swift | 6 +- Sources/Crow/App/IssueTracker.swift | 194 +++++++++++-- .../IssueTrackerStalledRefireTests.swift | 259 ++++++++++++++++++ 7 files changed, 529 insertions(+), 25 deletions(-) create mode 100644 Packages/CrowCore/Tests/CrowCoreTests/AutoRespondSettingsDefaultsTests.swift create mode 100644 Tests/CrowTests/IssueTrackerStalledRefireTests.swift diff --git a/Packages/CrowCore/Sources/CrowCore/Models/AppConfig.swift b/Packages/CrowCore/Sources/CrowCore/Models/AppConfig.swift index 7f782f84..1687c95a 100644 --- a/Packages/CrowCore/Sources/CrowCore/Models/AppConfig.swift +++ b/Packages/CrowCore/Sources/CrowCore/Models/AppConfig.swift @@ -232,8 +232,16 @@ extension WorkspaceGateway { /// Opt-in settings that let Crow type instructions into a session's managed /// Claude Code terminal when a watched PR transitions into a state that -/// usually requires action. Both flags default off β€” typing into a terminal -/// unprompted is intrusive, so the user must explicitly enable each. +/// usually requires action. +/// +/// `respondToChangesRequested` defaults **on** as of CROW-505 β€” auto-refine +/// is the answer to the user's complaint that a PR sitting in +/// CHANGES_REQUESTED with an idle agent never re-prompts. Existing users' +/// explicit choices stay sticky: `decodeIfPresent` returns whatever was +/// previously written, so a user who turned this off keeps it off across +/// the upgrade. `respondToFailedChecks` still defaults off β€” typing into a +/// terminal unprompted is intrusive, and CI flakes shouldn't auto-trigger +/// a fix-attempt. public struct AutoRespondSettings: Codable, Sendable, Equatable { /// Inject a "fix the review feedback" prompt when a PR transitions into /// `reviewStatus == .changesRequested`. @@ -244,7 +252,7 @@ public struct AutoRespondSettings: Codable, Sendable, Equatable { public var respondToFailedChecks: Bool public init( - respondToChangesRequested: Bool = false, + respondToChangesRequested: Bool = true, respondToFailedChecks: Bool = false ) { self.respondToChangesRequested = respondToChangesRequested @@ -253,7 +261,7 @@ public struct AutoRespondSettings: Codable, Sendable, Equatable { public init(from decoder: Decoder) throws { let c = try decoder.container(keyedBy: CodingKeys.self) - respondToChangesRequested = try c.decodeIfPresent(Bool.self, forKey: .respondToChangesRequested) ?? false + respondToChangesRequested = try c.decodeIfPresent(Bool.self, forKey: .respondToChangesRequested) ?? true respondToFailedChecks = try c.decodeIfPresent(Bool.self, forKey: .respondToFailedChecks) ?? false } diff --git a/Packages/CrowCore/Sources/CrowCore/Models/PRStatusTransition.swift b/Packages/CrowCore/Sources/CrowCore/Models/PRStatusTransition.swift index de88e4e5..f4d84bdc 100644 --- a/Packages/CrowCore/Sources/CrowCore/Models/PRStatusTransition.swift +++ b/Packages/CrowCore/Sources/CrowCore/Models/PRStatusTransition.swift @@ -74,6 +74,23 @@ public struct PRStatusTransition: Sendable, Equatable { } } +/// 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. +public struct EmittedTransitionMeta: Codable, Sendable, Equatable { + public var emittedAt: Date + public var headShaAtEmit: String? + + public init(emittedAt: Date, headShaAtEmit: String?) { + self.emittedAt = emittedAt + self.headShaAtEmit = headShaAtEmit + } +} + extension PRStatus { /// Compute the transitions implied by moving from `old` to `new`. /// diff --git a/Packages/CrowCore/Tests/CrowCoreTests/AutoRespondSettingsDefaultsTests.swift b/Packages/CrowCore/Tests/CrowCoreTests/AutoRespondSettingsDefaultsTests.swift new file mode 100644 index 00000000..c7ce33c8 --- /dev/null +++ b/Packages/CrowCore/Tests/CrowCoreTests/AutoRespondSettingsDefaultsTests.swift @@ -0,0 +1,47 @@ +import Foundation +import Testing +@testable import CrowCore + +/// CROW-505: AutoRespondSettings default flip and decoder behavior. +@Suite("AutoRespondSettings defaults") +struct AutoRespondSettingsDefaultsTests { + @Test func defaultInitializerDefaultsRespondToChangesRequestedOn() { + // The whole point of CROW-505: a fresh install gets auto-refine on by + // default. Old default was off; users assumed it worked but the toggle + // was silently false. + let settings = AutoRespondSettings() + #expect(settings.respondToChangesRequested == true) + // Failed-checks stays off β€” separate decision; intrusive prompts on + // every CI flake aren't what most users want. + #expect(settings.respondToFailedChecks == false) + } + + @Test func decoderFallbackForMissingKeyMatchesNewDefault() throws { + // Older configs that never wrote the field at all (key absent) should + // pick up the new default β€” this is the upgrade path for users who + // never touched the setting. + let json = "{}".data(using: .utf8)! + let decoded = try JSONDecoder().decode(AutoRespondSettings.self, from: json) + #expect(decoded.respondToChangesRequested == true) + #expect(decoded.respondToFailedChecks == false) + } + + @Test func decoderPreservesExplicitFalseChoice() throws { + // Existing users who explicitly toggled the setting OFF in UI have the + // key written as `false` in their JSON. `decodeIfPresent` returns the + // written value, so their choice survives the default flip. This is + // the "existing choices stay sticky" guarantee. + let json = #"{"respondToChangesRequested": false, "respondToFailedChecks": false}"# + .data(using: .utf8)! + let decoded = try JSONDecoder().decode(AutoRespondSettings.self, from: json) + #expect(decoded.respondToChangesRequested == false) + } + + @Test func decoderHonorsExplicitTrueChoice() throws { + let json = #"{"respondToChangesRequested": true, "respondToFailedChecks": true}"# + .data(using: .utf8)! + let decoded = try JSONDecoder().decode(AutoRespondSettings.self, from: json) + #expect(decoded.respondToChangesRequested == true) + #expect(decoded.respondToFailedChecks == true) + } +} diff --git a/Packages/CrowPersistence/Sources/CrowPersistence/JSONStore.swift b/Packages/CrowPersistence/Sources/CrowPersistence/JSONStore.swift index 6705517c..b9c6daa5 100644 --- a/Packages/CrowPersistence/Sources/CrowPersistence/JSONStore.swift +++ b/Packages/CrowPersistence/Sources/CrowPersistence/JSONStore.swift @@ -5,18 +5,27 @@ import CrowCore /// without re-firing already-handled PR transitions (CROW-456). /// /// `previousPRStatus` is keyed by session UUID string so the on-disk shape -/// stays plain JSON. `emittedTransitionKeys` is serialized as an array because -/// `Set` is not a stable JSON type β€” the in-memory load converts back. +/// stays plain JSON. `emittedTransitionKeys` is the legacy array of dedup +/// keys retained for backward read; CROW-505 introduced the richer +/// `emittedTransitionMeta` map, which is now the source of truth on write. +/// Older `store.json` files lacking the map are migrated lazily in +/// `IssueTracker.hydratePersistedState`. public struct PersistedIssueTrackerState: Codable, Sendable, Equatable { public var previousPRStatus: [String: PRStatus] public var emittedTransitionKeys: [String] + /// Optional for backward compat: pre-CROW-505 stores only wrote + /// `emittedTransitionKeys`. Newer stores populate this map and leave + /// `emittedTransitionKeys` empty. + public var emittedTransitionMeta: [String: EmittedTransitionMeta]? public init( previousPRStatus: [String: PRStatus] = [:], - emittedTransitionKeys: [String] = [] + emittedTransitionKeys: [String] = [], + emittedTransitionMeta: [String: EmittedTransitionMeta]? = nil ) { self.previousPRStatus = previousPRStatus self.emittedTransitionKeys = emittedTransitionKeys + self.emittedTransitionMeta = emittedTransitionMeta } } diff --git a/Packages/CrowPersistence/Tests/CrowPersistenceTests/JSONStoreTests.swift b/Packages/CrowPersistence/Tests/CrowPersistenceTests/JSONStoreTests.swift index d0a63a50..cf134c28 100644 --- a/Packages/CrowPersistence/Tests/CrowPersistenceTests/JSONStoreTests.swift +++ b/Packages/CrowPersistence/Tests/CrowPersistenceTests/JSONStoreTests.swift @@ -214,9 +214,11 @@ import Testing latestReviewID: "R_kgD0_1" ) let dedupKey = "\(sessionID.uuidString)|changesRequested|R_kgD0_1" + let emittedAt = Date(timeIntervalSince1970: 1_700_000_000) let state = PersistedIssueTrackerState( previousPRStatus: [sessionID.uuidString: pr], - emittedTransitionKeys: [dedupKey] + emittedTransitionKeys: [dedupKey], + emittedTransitionMeta: [dedupKey: EmittedTransitionMeta(emittedAt: emittedAt, headShaAtEmit: "abc123")] ) let store = JSONStore(directory: dir) @@ -226,6 +228,8 @@ import Testing #expect(reloaded == state) #expect(reloaded?.previousPRStatus[sessionID.uuidString]?.latestReviewID == "R_kgD0_1") #expect(reloaded?.emittedTransitionKeys == [dedupKey]) + #expect(reloaded?.emittedTransitionMeta?[dedupKey]?.headShaAtEmit == "abc123") + #expect(reloaded?.emittedTransitionMeta?[dedupKey]?.emittedAt == emittedAt) } @Test func storeDataDecodesWithoutIssueTrackerStateField() throws { diff --git a/Sources/Crow/App/IssueTracker.swift b/Sources/Crow/App/IssueTracker.swift index 9ff4d3e3..4b65f30d 100644 --- a/Sources/Crow/App/IssueTracker.swift +++ b/Sources/Crow/App/IssueTracker.swift @@ -154,11 +154,21 @@ final class IssueTracker { /// behavior so existing PR state isn't replayed at startup). private var previousPRStatus: [UUID: PRStatus] = [:] - /// Stable keys of transitions we've already emitted, used to suppress - /// duplicates across the in-process lifetime. See `PRStatusTransition.dedupeKey`. + /// Per-emitted-transition metadata keyed by `PRStatusTransition.dedupeKey`, + /// used to suppress duplicates across the in-process lifetime and to power + /// the stalled `.changesRequested` re-fire pass (CROW-505): each entry + /// carries `emittedAt` (quiet-window clock) and `headShaAtEmit` (anti-loop + /// guarantee β€” re-fire only when the author hasn't pushed since dispatch). /// Cleared per-session when we observe the rule "re-arm" (e.g. checks /// move back to passing/pending) so subsequent transitions still fire. - private var emittedTransitionKeys: Set = [] + private var emittedTransitionMeta: [String: EmittedTransitionMeta] = [:] + + /// Minimum elapsed time before a stalled `.changesRequested` transition can + /// re-fire (CROW-505). 10 min β‰ˆ 10 poll cycles β€” covers Claude Code's + /// longest "thinking on a hard finding" gaps before we conclude the agent + /// has actually stalled. Constant so we can tune if real-world telemetry + /// calls for it. + nonisolated static let stalledRefireQuietWindow: TimeInterval = 10 * 60 /// Guards the GitHub-scope console warning so it fires once per session. private var didLogGitHubScopeWarning = false @@ -1483,9 +1493,10 @@ final class IssueTracker { // of the time; without this guard `persistTrackerState` re-reads, // re-encodes, and atomically rewrites `store.json` every 60s. let priorPRStatus = previousPRStatus - let priorEmittedKeys = emittedTransitionKeys + let priorEmittedMeta = emittedTransitionMeta var transitions: [PRStatusTransition] = [] + let now = Date() let sessionsWithPRs = appState.sessions.filter { !$0.isManager } for session in sessionsWithPRs { let links = appState.links(for: session.id) @@ -1502,16 +1513,16 @@ final class IssueTracker { // With CROW-456 the `.changesRequested` dedup key is keyed on the // latest CHANGES_REQUESTED review id, so a new formal review // naturally produces a different key. This cleanup bounds the - // in-memory set: when we leave the bucket entirely we drop every + // in-memory map: when we leave the bucket entirely we drop every // `changesRequested` entry for this session. if let old = oldStatus { if old.reviewStatus == .changesRequested && newStatus.reviewStatus != .changesRequested { let prefix = "\(session.id.uuidString)|changesRequested|" - emittedTransitionKeys = emittedTransitionKeys.filter { !$0.hasPrefix(prefix) } + emittedTransitionMeta = emittedTransitionMeta.filter { !$0.key.hasPrefix(prefix) } } if old.checksPass == .failing && newStatus.checksPass != .failing { if let sha = old.headSha { - emittedTransitionKeys.remove("\(session.id.uuidString)|checksFailing|\(sha)") + emittedTransitionMeta.removeValue(forKey: "\(session.id.uuidString)|checksFailing|\(sha)") } } } @@ -1523,8 +1534,11 @@ final class IssueTracker { prURL: prLink.url, prNumber: pr.number ) - for t in candidates where !emittedTransitionKeys.contains(t.dedupeKey) { - emittedTransitionKeys.insert(t.dedupeKey) + for t in candidates where emittedTransitionMeta[t.dedupeKey] == nil { + emittedTransitionMeta[t.dedupeKey] = EmittedTransitionMeta( + emittedAt: now, + headShaAtEmit: newStatus.headSha + ) transitions.append(t) } @@ -1532,11 +1546,19 @@ final class IssueTracker { appState.prStatus[session.id] = newStatus } + // CROW-505: stalled `.changesRequested` re-fire. After the per-session + // pass, walk the emitted-meta map and re-emit any review that has + // stalled β€” agent idle, head SHA unchanged since dispatch, quiet + // window elapsed. Anti-loop is preserved by the head-SHA gate: a real + // agent push advances `previousPRStatus[sid].headSha`, which kills the + // re-fire condition. + transitions.append(contentsOf: reFireStalledChangesRequested(now: now)) + if !transitions.isEmpty { onPRStatusTransitions?(transitions) } - if previousPRStatus != priorPRStatus || emittedTransitionKeys != priorEmittedKeys { + if previousPRStatus != priorPRStatus || emittedTransitionMeta != priorEmittedMeta { persistTrackerState() } @@ -1546,9 +1568,15 @@ final class IssueTracker { // MARK: - Cross-restart persistence (CROW-456) - /// Load `previousPRStatus` + `emittedTransitionKeys` from disk on startup. + /// Load `previousPRStatus` + `emittedTransitionMeta` from disk on startup. /// Drops entries whose session UUID is no longer present so the in-memory /// state can't grow without bound across long-lived installs. + /// + /// Migrates pre-CROW-505 stores that only wrote `emittedTransitionKeys`: + /// each legacy key gets a meta entry with `emittedAt = now` (starts the + /// quiet-window clock fresh β€” no immediate re-fire flood at upgrade) and + /// `headShaAtEmit` sourced from `previousPRStatus` (also persisted), so + /// the head-SHA anti-loop gate works on migrated entries too. private func hydratePersistedState() { guard let persisted = JSONStore().data.issueTrackerState else { return } let validSessionIDs = Set(appState.sessions.map(\.id)) @@ -1559,27 +1587,159 @@ final class IssueTracker { } previousPRStatus = restored - // Keep only dedup keys whose session UUID prefix matches a live session. + // Keep only entries whose session UUID prefix matches a live session. // Each key has shape "|kind|…"; reject anything else. let validUUIDPrefixes = Set(validSessionIDs.map { "\($0.uuidString)|" }) - emittedTransitionKeys = Set(persisted.emittedTransitionKeys.filter { key in - validUUIDPrefixes.contains(where: { key.hasPrefix($0) }) - }) + + if let persistedMeta = persisted.emittedTransitionMeta { + // CROW-505+: read the rich map directly. + emittedTransitionMeta = persistedMeta.filter { entry in + validUUIDPrefixes.contains(where: { entry.key.hasPrefix($0) }) + } + } else { + // Pre-CROW-505 store β€” synthesize metadata from the legacy keys. + let now = Date() + var migrated: [String: EmittedTransitionMeta] = [:] + for key in persisted.emittedTransitionKeys + where validUUIDPrefixes.contains(where: { key.hasPrefix($0) }) { + let sha = Self.parseSessionID(fromDedupKey: key) + .flatMap { restored[$0]?.headSha } + migrated[key] = EmittedTransitionMeta(emittedAt: now, headShaAtEmit: sha) + } + emittedTransitionMeta = migrated + } } - /// Persist `previousPRStatus` + `emittedTransitionKeys` after every poll + /// Persist `previousPRStatus` + `emittedTransitionMeta` after every poll /// that mutated them. `JSONStore.mutate` coalesces writes so this is cheap /// even when transitions are quiet. + /// + /// Writes both the new map and an empty legacy `emittedTransitionKeys` + /// array: older Crow builds that read the array but not the map will see + /// no keys and re-fire as if from a fresh install (graceful downgrade). private func persistTrackerState() { let snapshot = PersistedIssueTrackerState( previousPRStatus: Dictionary(uniqueKeysWithValues: previousPRStatus.map { ($0.key.uuidString, $0.value) }), - emittedTransitionKeys: Array(emittedTransitionKeys) + emittedTransitionKeys: [], + emittedTransitionMeta: emittedTransitionMeta ) JSONStore().mutate { data in data.issueTrackerState = snapshot } } + // MARK: - Stalled `.changesRequested` re-fire (CROW-505) + + /// Re-emit any `.changesRequested` transition whose original auto-respond + /// prompt has stalled β€” agent idle, terminal launched, head SHA unchanged + /// since dispatch, and the quiet window has elapsed. Anti-loop guarantee: + /// a real agent push advances `previousPRStatus[sid].headSha`, breaking the + /// SHA-match condition, so we never re-fire on the agent's own response. + /// + /// Side effects (only when a re-fire actually emits): + /// - `emittedTransitionMeta[key].emittedAt` is bumped to `now` so the next + /// quiet-window clock starts fresh. + /// - `NSLog`'d distinctive line so operators can grep for the behavior. + private func reFireStalledChangesRequested(now: Date) -> [PRStatusTransition] { + var out: [PRStatusTransition] = [] + for (key, meta) in emittedTransitionMeta { + // Only `.changesRequested` keys re-fire. Checks-failing already + // re-arms on a new commit (via the SHA-keyed dedup). + guard let sid = Self.parseSessionID(fromDedupKey: key), + Self.parseKind(fromDedupKey: key) == .changesRequested else { continue } + guard let session = appState.sessions.first(where: { $0.id == sid }) else { continue } + // Skip review sessions for the same reason `AutoRespondCoordinator` + // gates them β€” never commit on behalf of the reviewer. + guard session.kind != .review else { continue } + guard let prLink = appState.links(for: sid).first(where: { $0.linkType == .pr }) else { continue } + + let currentStatus = previousPRStatus[sid] + let agentActivity = appState.hookState(for: sid).activityState + let managedTerminal = appState.terminals(for: sid).first(where: { $0.isManaged }) + let readiness = managedTerminal.flatMap { appState.terminalReadiness[$0.id] } + + guard Self.shouldReFireStalledChangesRequested( + meta: meta, + currentStatus: currentStatus, + agentActivity: agentActivity, + terminalReadiness: readiness, + now: now, + quietWindow: Self.stalledRefireQuietWindow + ) else { continue } + + guard let status = currentStatus else { continue } + let synthetic = PRStatusTransition( + kind: .changesRequested, + sessionID: sid, + prURL: prLink.url, + prNumber: QuickActionPrompts.parsePRNumber(from: prLink.url), + headSha: status.headSha, + latestReviewID: status.latestReviewID, + failedCheckNames: [] + ) + out.append(synthetic) + // Refresh the meta so the next quiet-window clock starts now and we + // don't re-fire again on the very next poll. + emittedTransitionMeta[key] = EmittedTransitionMeta( + emittedAt: now, + headShaAtEmit: status.headSha + ) + let elapsed = now.timeIntervalSince(meta.emittedAt) + NSLog("[IssueTracker] auto-refine re-fired (stalled review) β€” session=%@, review=%@, sha=%@, elapsed=%.0fs", + sid.uuidString as NSString, + (status.latestReviewID ?? "") as NSString, + (status.headSha ?? "") as NSString, + elapsed) + } + return out + } + + /// Decide whether a stalled `.changesRequested` entry is eligible for + /// re-fire. Pure (no AppState) so the conditions can be exercised + /// independently of the polling loop. + /// + /// Returns `true` only when all four conditions from CROW-505 hold: + /// (1) PR still in CHANGES_REQUESTED, (2) agent idle on the managed + /// terminal, which must have actually reached `.agentLaunched`, + /// (3) head SHA unchanged since dispatch (anti-loop), and (4) quiet + /// window elapsed. + nonisolated static func shouldReFireStalledChangesRequested( + meta: EmittedTransitionMeta, + currentStatus: PRStatus?, + agentActivity: AgentActivityState, + terminalReadiness: TerminalReadiness?, + now: Date, + quietWindow: TimeInterval + ) -> Bool { + // (1) PR still in CHANGES_REQUESTED. + guard let s = currentStatus, s.reviewStatus == .changesRequested else { return false } + // (2) Agent idle, not actively working / waiting on input. + guard agentActivity == .idle else { return false } + // (2b) Managed terminal actually launched the agent. Pre-launch terminals + // can't be "stalled" β€” the agent never had a chance to run. + guard terminalReadiness == .agentLaunched else { return false } + // (3) Head SHA unchanged since we last emitted. A nil baseline (no SHA + // known at emit time) can't prove the author hasn't pushed, so we + // conservatively refuse to re-fire. + guard let metaSha = meta.headShaAtEmit, metaSha == s.headSha else { return false } + // (4) Quiet window has elapsed. + return now.timeIntervalSince(meta.emittedAt) >= quietWindow + } + + /// Parse the session UUID prefix of a dedup key. Returns nil for malformed + /// keys. The key format is `"||"`. + nonisolated static func parseSessionID(fromDedupKey key: String) -> UUID? { + guard let pipe = key.firstIndex(of: "|") else { return nil } + return UUID(uuidString: String(key[.. PRStatusTransition.Kind? { + let parts = key.split(separator: "|", maxSplits: 2, omittingEmptySubsequences: false) + guard parts.count >= 2 else { return nil } + return PRStatusTransition.Kind(rawValue: String(parts[1])) + } + // MARK: - Auto-Merge Watcher (CROW-299) /// Pattern matching a Crow-Session commit trailer line. Anchored to diff --git a/Tests/CrowTests/IssueTrackerStalledRefireTests.swift b/Tests/CrowTests/IssueTrackerStalledRefireTests.swift new file mode 100644 index 00000000..eb2bc961 --- /dev/null +++ b/Tests/CrowTests/IssueTrackerStalledRefireTests.swift @@ -0,0 +1,259 @@ +import Foundation +import Testing +import CrowCore +import CrowProvider +@testable import Crow + +/// CROW-505: `IssueTracker` re-fires `.changesRequested` transitions whose +/// auto-respond prompt has stalled (agent idle, head SHA unchanged, quiet +/// window elapsed). These tests cover the pure +/// `shouldReFireStalledChangesRequested` predicate plus the dedup-key +/// parse helpers that drive the wiring layer. +@Suite("IssueTracker stalled re-fire predicate (CROW-505)") +struct IssueTrackerStalledRefireTests { + + private let quietWindow: TimeInterval = 10 * 60 + private let now = Date(timeIntervalSince1970: 1_700_000_000) + private let shaA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + private let shaB = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + + private func meta(emittedSecondsAgo: TimeInterval = 11 * 60, headShaAtEmit: String? = nil) -> EmittedTransitionMeta { + EmittedTransitionMeta( + emittedAt: now.addingTimeInterval(-emittedSecondsAgo), + headShaAtEmit: headShaAtEmit ?? shaA + ) + } + + private func status( + review: PRStatus.ReviewStatus = .changesRequested, + sha: String? = nil, + reviewID: String? = "R_1" + ) -> PRStatus { + PRStatus( + checksPass: .pending, + reviewStatus: review, + mergeable: .unknown, + failedCheckNames: [], + headSha: sha ?? shaA, + latestReviewID: reviewID + ) + } + + // MARK: - Bug repro (Test 1) + + @Test + func reFiresWhenAllConditionsMet() { + // The CROW-505 bug repro: PR still in CHANGES_REQUESTED, agent went + // idle without addressing all findings, head SHA hasn't advanced + // (agent didn't push), and quiet window has elapsed. + #expect(IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + agentActivity: .idle, + terminalReadiness: .agentLaunched, + now: now, + quietWindow: quietWindow + )) + } + + // MARK: - Anti-loop guarantee (Test 2) + + @Test + func doesNotReFireWhenAgentPushedNewCommit() { + // The single most important invariant: a real agent push advances the + // head SHA, which kills the re-fire. This preserves the CROW-456 + // anti-loop guarantee β€” the agent's own response push doesn't + // re-trigger the prompt that told it to push. + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaB), // SHA advanced + agentActivity: .idle, + terminalReadiness: .agentLaunched, + now: now, + quietWindow: quietWindow + )) + } + + // MARK: - Round-2 reviewer review still fires (Test 3) + + @Test + func roundTwoFromReviewerProducesDistinctDedupKey() { + // Reviewer submitting a fresh formal review rotates `latestReviewID`, + // which produces a different `dedupeKey`. The existing + // `applyPRStatuses` path emits unconditionally on a new key β€” the + // re-fire predicate is never consulted for this case. Sanity check + // by computing the two keys and asserting they differ. + let sid = UUID() + let r1 = PRStatusTransition( + kind: .changesRequested, sessionID: sid, prURL: "https://example/1", + prNumber: 1, headSha: shaA, latestReviewID: "R_1" + ) + let r2 = PRStatusTransition( + kind: .changesRequested, sessionID: sid, prURL: "https://example/1", + prNumber: 1, headSha: shaA, latestReviewID: "R_2" + ) + #expect(r1.dedupeKey != r2.dedupeKey) + } + + // MARK: - Agent busy β€” no re-fire (Test 4) + + @Test + func doesNotReFireWhileAgentWorking() { + // Don't interrupt an agent that's actively working β€” they may already + // be addressing the feedback. + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + agentActivity: .working, + terminalReadiness: .agentLaunched, + now: now, + quietWindow: quietWindow + )) + } + + @Test + func doesNotReFireWhileAgentWaiting() { + // `.waiting` means the agent is blocked on user input β€” re-firing now + // would push another prompt into a queue the user is about to answer. + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + agentActivity: .waiting, + terminalReadiness: .agentLaunched, + now: now, + quietWindow: quietWindow + )) + } + + @Test + func doesNotReFireWhileAgentDone() { + // `.done` is the terminal "completed a turn" state β€” it's followed by + // a hook reset to `.idle`. Re-firing on `.done` would race. + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + agentActivity: .done, + terminalReadiness: .agentLaunched, + now: now, + quietWindow: quietWindow + )) + } + + // MARK: - Quiet window respected (Test 5) + + @Test + func doesNotReFireBeforeQuietWindowElapsed() { + // Quiet window: agents can spend 5+ minutes thinking on a hard + // finding. Don't re-prompt before they've had a chance to act. + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow - 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + agentActivity: .idle, + terminalReadiness: .agentLaunched, + now: now, + quietWindow: quietWindow + )) + } + + // MARK: - Terminal not launched (Test 6) + + @Test + func doesNotReFireWhenTerminalNotLaunched() { + // A pre-launch terminal (shellReady or earlier) can't be "stalled" β€” + // the agent never ran. Wait for the launch to finish first. + for readiness: TerminalReadiness in [.uninitialized, .surfaceCreated, .shellReady, .timedOut, .failed] { + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + agentActivity: .idle, + terminalReadiness: readiness, + now: now, + quietWindow: quietWindow + ), "expected no re-fire at readiness \(readiness)") + } + } + + @Test + func doesNotReFireWhenTerminalReadinessUnknown() { + // No managed terminal at all β†’ no readiness entry. Can't re-fire. + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + agentActivity: .idle, + terminalReadiness: nil, + now: now, + quietWindow: quietWindow + )) + } + + // MARK: - PR no longer in CHANGES_REQUESTED + + @Test + func doesNotReFireWhenPRLeftChangesRequestedBucket() { + // If the PR moved to APPROVED or the bucket cleared otherwise, + // re-firing is meaningless. The re-arm path in `applyPRStatuses` + // also drops the entry on this transition, but the predicate + // double-checks. + for review: PRStatus.ReviewStatus in [.approved, .reviewRequired, .unknown] { + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(review: review, sha: shaA), + agentActivity: .idle, + terminalReadiness: .agentLaunched, + now: now, + quietWindow: quietWindow + ), "expected no re-fire at review status \(review)") + } + } + + @Test + func doesNotReFireWhenCurrentStatusMissing() { + // No persisted PR status for the session β†’ we can't reason about + // SHA equality or bucket membership. Refuse. + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: nil, + agentActivity: .idle, + terminalReadiness: .agentLaunched, + now: now, + quietWindow: quietWindow + )) + } + + @Test + func doesNotReFireWhenBaselineShaNil() { + // Migrated entries from before CROW-505 may have nil headShaAtEmit. + // Without a baseline we can't prove the author hasn't pushed β€” + // refuse rather than risk an unwanted re-fire. + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: EmittedTransitionMeta(emittedAt: now.addingTimeInterval(-(quietWindow + 1)), headShaAtEmit: nil), + currentStatus: status(sha: shaA), + agentActivity: .idle, + terminalReadiness: .agentLaunched, + now: now, + quietWindow: quietWindow + )) + } + + // MARK: - Dedup-key parsers + + @Test + func parsesSessionIDAndKindFromDedupKey() { + let sid = UUID() + let crKey = "\(sid.uuidString)|changesRequested|R_1" + let cfKey = "\(sid.uuidString)|checksFailing|\(shaA)" + + #expect(IssueTracker.parseSessionID(fromDedupKey: crKey) == sid) + #expect(IssueTracker.parseSessionID(fromDedupKey: cfKey) == sid) + #expect(IssueTracker.parseKind(fromDedupKey: crKey) == .changesRequested) + #expect(IssueTracker.parseKind(fromDedupKey: cfKey) == .checksFailing) + } + + @Test + func parsersReturnNilForMalformedKeys() { + #expect(IssueTracker.parseSessionID(fromDedupKey: "not-a-uuid|changesRequested|R_1") == nil) + #expect(IssueTracker.parseSessionID(fromDedupKey: "no-pipe-anywhere") == nil) + #expect(IssueTracker.parseKind(fromDedupKey: "\(UUID().uuidString)|unknownKind|x") == nil) + #expect(IssueTracker.parseKind(fromDedupKey: "missing-discriminator") == nil) + } +} From d4cc7c0fd38bb8ea59604c1ad63297a86a11a3d2 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Sun, 14 Jun 2026 17:16:24 -0400 Subject: [PATCH 2/4] Gate stalled re-fire on respondToChangesRequested toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without this gate a user who explicitly opted out of auto-respond would still see a fresh "Changes Requested" macOS notification every quiet window β€” `onPRStatusTransitions` calls `notifyPRTransition` unconditionally and only the actual prompt dispatch is gated downstream in `AutoRespondCoordinator.handle`, so the synthetic re-fire produced pure notification noise for zero useful action. Adds `respondToChangesRequestedProvider: () -> Bool` to `IssueTracker` mirroring `autoMergeWatcherEnabledProvider` et al., early-returns from `reFireStalledChangesRequested` when the toggle is off, and wires the provider from `AppDelegate`. Also adds an integration test suite ("IssueTracker stalled re-fire wiring") that drives the map-walking method directly so future wiring drift is caught: toggle-off suppresses + does not bump emittedAt, toggle- on emits + bumps, review sessions are skipped, agent activity gate, and unlaunched/missing-terminal gates. The pure-predicate suite remains as the unit-level coverage. Addresses review feedback on PR #507. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: C69B361D-F6EE-49FC-9F5B-E7A3928F5F1E --- Sources/Crow/App/AppDelegate.swift | 3 + Sources/Crow/App/IssueTracker.swift | 32 +++- .../IssueTrackerStalledRefireTests.swift | 146 ++++++++++++++++++ 3 files changed, 178 insertions(+), 3 deletions(-) diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index ee4a7bf0..53ef7443 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -803,6 +803,9 @@ final class AppDelegate: NSObject, NSApplicationDelegate { tracker.autoRebaseWatcherEnabledProvider = { [weak self] in self?.appConfig?.autoRebaseWatcherEnabled ?? false } + tracker.respondToChangesRequestedProvider = { [weak self] in + self?.appConfig?.autoRespond.respondToChangesRequested ?? false + } tracker.onAutoRebasePushed = { [weak self] sessionID, _, number in self?.notificationManager?.notifyAutoRebasePushed(number: number, sessionID: sessionID) } diff --git a/Sources/Crow/App/IssueTracker.swift b/Sources/Crow/App/IssueTracker.swift index 4b65f30d..84cbe299 100644 --- a/Sources/Crow/App/IssueTracker.swift +++ b/Sources/Crow/App/IssueTracker.swift @@ -76,6 +76,17 @@ final class IssueTracker { /// watcher is inert until AppDelegate wires it (CROW-318). var autoRebaseWatcherEnabledProvider: () -> Bool = { false } + /// Reads the latest `AutoRespondSettings.respondToChangesRequested` + /// snapshot on every poll. Used solely to gate the stalled-`.changesRequested` + /// re-fire pass (CROW-505): without this gate an opted-out user would still + /// see a fresh "Changes Requested" macOS notification every quiet window + /// (via `onPRStatusTransitions` β†’ `notifyPRTransition`), even though + /// `AutoRespondCoordinator` correctly suppresses the actual dispatch β€” + /// pure notification noise that defeats their opt-out. Defaults to a + /// closure returning `false` so the re-fire path stays inert until + /// AppDelegate wires it. + var respondToChangesRequestedProvider: () -> Bool = { false } + /// Fires after Crow rebased a PR branch and force-pushed it. Wired in /// AppDelegate to post a notification. var onAutoRebasePushed: ((UUID, String, Int) -> Void)? @@ -152,7 +163,9 @@ final class IssueTracker { /// Populated lazily on first observation; that first poll never fires /// transitions (matches the `previousReviewRequestIDs` first-fetch /// behavior so existing PR state isn't replayed at startup). - private var previousPRStatus: [UUID: PRStatus] = [:] + /// Internal (not private) so `@testable` tests can seed it when driving the + /// stalled-re-fire pass (CROW-505) without going through a full poll. + var previousPRStatus: [UUID: PRStatus] = [:] /// Per-emitted-transition metadata keyed by `PRStatusTransition.dedupeKey`, /// used to suppress duplicates across the in-process lifetime and to power @@ -161,7 +174,9 @@ final class IssueTracker { /// guarantee β€” re-fire only when the author hasn't pushed since dispatch). /// Cleared per-session when we observe the rule "re-arm" (e.g. checks /// move back to passing/pending) so subsequent transitions still fire. - private var emittedTransitionMeta: [String: EmittedTransitionMeta] = [:] + /// Internal (not private) for the same `@testable` seeding reason as + /// `previousPRStatus`. + var emittedTransitionMeta: [String: EmittedTransitionMeta] = [:] /// Minimum elapsed time before a stalled `.changesRequested` transition can /// re-fire (CROW-505). 10 min β‰ˆ 10 poll cycles β€” covers Claude Code's @@ -1640,7 +1655,18 @@ final class IssueTracker { /// - `emittedTransitionMeta[key].emittedAt` is bumped to `now` so the next /// quiet-window clock starts fresh. /// - `NSLog`'d distinctive line so operators can grep for the behavior. - private func reFireStalledChangesRequested(now: Date) -> [PRStatusTransition] { + func reFireStalledChangesRequested(now: Date) -> [PRStatusTransition] { + // Gate on the user-facing `AutoRespondSettings.respondToChangesRequested` + // toggle. Without this gate a user who explicitly opted out would still + // get a fresh `Changes Requested` macOS notification every quiet window + // (`onPRStatusTransitions` β†’ `notifyPRTransition` posts unconditionally; + // only the actual prompt dispatch is gated downstream in + // `AutoRespondCoordinator.handle`) β€” pure notification noise that + // defeats their opt-out. New configs default to `true` post-CROW-505, + // so this gate only suppresses behavior for users who deliberately + // disabled it. + guard respondToChangesRequestedProvider() else { return [] } + var out: [PRStatusTransition] = [] for (key, meta) in emittedTransitionMeta { // Only `.changesRequested` keys re-fire. Checks-failing already diff --git a/Tests/CrowTests/IssueTrackerStalledRefireTests.swift b/Tests/CrowTests/IssueTrackerStalledRefireTests.swift index eb2bc961..55fa7bb6 100644 --- a/Tests/CrowTests/IssueTrackerStalledRefireTests.swift +++ b/Tests/CrowTests/IssueTrackerStalledRefireTests.swift @@ -257,3 +257,149 @@ struct IssueTrackerStalledRefireTests { #expect(IssueTracker.parseKind(fromDedupKey: "missing-discriminator") == nil) } } + +/// Integration coverage for `IssueTracker.reFireStalledChangesRequested`: the +/// map-walking pass that consults the predicate, builds synthetic transitions, +/// and bumps the dedup-meta timestamp. The predicate is covered above in +/// isolation; these tests catch wiring drift β€” gate-off behavior, terminal +/// resolution, and the `emittedAt` refresh that prevents same-poll re-fires. +@Suite("IssueTracker stalled re-fire wiring (CROW-505)") +@MainActor +struct IssueTrackerStalledRefireWiringTests { + + private let shaA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + + /// Build an `IssueTracker` whose appState contains a single work session + /// with a PR link, a managed terminal at `.agentLaunched`, an idle hook + /// state, and a prior PRStatus in CHANGES_REQUESTED on `shaA`. Seeds an + /// `emittedTransitionMeta` entry that has been sitting for ~2Γ— the quiet + /// window β€” so the pure predicate would say "re-fire". + private func makeStalledTracker( + toggleOn: Bool, + sessionKind: SessionKind = .work, + agentActivity: AgentActivityState = .idle, + readiness: TerminalReadiness? = .agentLaunched + ) -> (IssueTracker, UUID, String) { + let state = AppState() + let sid = UUID() + let session = Session(id: sid, name: "feature-test", kind: sessionKind) + state.sessions = [session] + + let prLink = SessionLink( + sessionID: sid, + label: "PR #1", + url: "https://github.com/radiusmethod/crow/pull/1", + linkType: .pr + ) + state.links[sid] = [prLink] + + let terminal = SessionTerminal( + sessionID: sid, + name: "Claude Code", + cwd: "/tmp", + isManaged: true + ) + state.terminals[sid] = [terminal] + if let readiness { + state.terminalReadiness[terminal.id] = readiness + } + state.hookState(for: sid).activityState = agentActivity + + let tracker = IssueTracker(appState: state, providerManager: ProviderManager()) + tracker.respondToChangesRequestedProvider = { toggleOn } + + let status = PRStatus( + checksPass: .pending, + reviewStatus: .changesRequested, + mergeable: .unknown, + failedCheckNames: [], + headSha: shaA, + latestReviewID: "R_1" + ) + tracker.previousPRStatus[sid] = status + + let key = "\(sid.uuidString)|changesRequested|R_1" + let now = Date() + tracker.emittedTransitionMeta[key] = EmittedTransitionMeta( + emittedAt: now.addingTimeInterval(-IssueTracker.stalledRefireQuietWindow * 2), + headShaAtEmit: shaA + ) + + return (tracker, sid, key) + } + + // MARK: - The gate + + @Test + func toggleOffSuppressesAllSyntheticTransitions() { + // Reviewer's regression catch (review on PR #507): an opted-out user + // must not see synthetic transitions, even when every predicate + // condition is satisfied. Without the gate, `onPRStatusTransitions` + // would fire `notifyPRTransition` every quiet window β€” pure macOS + // notification noise for zero useful action. + let (tracker, _, key) = makeStalledTracker(toggleOn: false) + let metaBefore = tracker.emittedTransitionMeta[key] + + let transitions = tracker.reFireStalledChangesRequested(now: Date()) + + #expect(transitions.isEmpty) + // Meta must not be touched by the no-op path β€” bumping `emittedAt` + // when nothing fired would lose the original dispatch timestamp. + #expect(tracker.emittedTransitionMeta[key] == metaBefore) + } + + @Test + func toggleOnPermitsSyntheticTransitionAndBumpsEmittedAt() { + let now = Date() + let (tracker, sid, key) = makeStalledTracker(toggleOn: true) + let originalEmittedAt = tracker.emittedTransitionMeta[key]?.emittedAt + + let transitions = tracker.reFireStalledChangesRequested(now: now) + + #expect(transitions.count == 1) + #expect(transitions[0].kind == .changesRequested) + #expect(transitions[0].sessionID == sid) + #expect(transitions[0].headSha == shaA) + #expect(transitions[0].latestReviewID == "R_1") + // emittedAt bumped to `now` so the next poll inside the quiet window + // doesn't immediately re-fire again. + #expect(tracker.emittedTransitionMeta[key]?.emittedAt == now) + #expect(tracker.emittedTransitionMeta[key]?.emittedAt != originalEmittedAt) + } + + // MARK: - Review-session gate + + @Test + func reviewSessionsAreSkippedEvenWhenToggleOn() { + // Mirrors the `AutoRespondCoordinator.shouldSkipReviewSession` policy: + // never commit on behalf of a reviewer. The map-walking pass checks + // `session.kind != .review` so the synthetic transition never reaches + // the notification fan-out either. + let (tracker, _, _) = makeStalledTracker(toggleOn: true, sessionKind: .review) + let transitions = tracker.reFireStalledChangesRequested(now: Date()) + #expect(transitions.isEmpty) + } + + // MARK: - Activity / readiness gates (integration view of the predicate) + + @Test + func agentWorkingSuppressesSyntheticTransition() { + let (tracker, _, _) = makeStalledTracker(toggleOn: true, agentActivity: .working) + #expect(tracker.reFireStalledChangesRequested(now: Date()).isEmpty) + } + + @Test + func unlaunchedTerminalSuppressesSyntheticTransition() { + let (tracker, _, _) = makeStalledTracker(toggleOn: true, readiness: .shellReady) + #expect(tracker.reFireStalledChangesRequested(now: Date()).isEmpty) + } + + @Test + func missingManagedTerminalSuppressesSyntheticTransition() { + // Edge: no terminal entry at all (e.g. session created but terminal + // never materialized). Predicate sees `terminalReadiness: nil` and + // refuses. + let (tracker, _, _) = makeStalledTracker(toggleOn: true, readiness: nil) + #expect(tracker.reFireStalledChangesRequested(now: Date()).isEmpty) + } +} From b391cee938649c28db601ff8a83cb47b589e712e Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Sun, 14 Jun 2026 18:20:42 -0400 Subject: [PATCH 3/4] =?UTF-8?q?Gate=20stalled=20re-fire=20on=20PR=20isOpen?= =?UTF-8?q?=20+=20drop=20meta=20on=20open=E2=86=92closed=20edge?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer caught a correctness gap on PR #507: the standing-state re-fire pass reads `previousPRStatus[sid]` directly rather than firing on transition edges, and a PR that merged or closed-unmerged while in CHANGES_REQUESTED keeps `reviewDecision` unchanged. With the agent idle, head SHA unchanged (closing doesn't move HEAD), and the quiet window elapsed, re-fire would prompt the agent to "address review feedback" on a dead PR every quiet window indefinitely. Adds `PRStatus.isOpen: Bool` (default `true` for backward decode), populated from `pr.state == "OPEN"` in `buildPRStatus`. Gates `shouldReFireStalledChangesRequested` on `currentStatus.isOpen`. Also extends the `applyPRStatuses` re-arm cleanup to drop changesRequested meta entries on the openβ†’closed edge β€” defense in depth so the map can't accumulate dead entries that the predicate has to repeatedly reject. Adds two new predicate tests for merged and closed-unmerged PRs covering both `mergeable == .merged` and `mergeable == .unknown`. Also addresses the non-blocking notes from the same review: - Adds a docstring on `reFireStalledChangesRequested` explaining that the in-loop mutation of `emittedTransitionMeta` is safe thanks to Swift dictionary copy-on-write iteration semantics. The other two Green notes are confirmations and need no code change: the default-flip's reach (encoder writes the field unconditionally, so the new `true` default only lands on genuinely fresh configs β€” matches CROW-505's stated intent), and the documented one-time downgrade prompt burst. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: C69B361D-F6EE-49FC-9F5B-E7A3928F5F1E --- .../Sources/CrowCore/Models/PRStatus.swift | 22 +++++++++- Sources/Crow/App/IssueTracker.swift | 30 ++++++++++++-- .../IssueTrackerStalledRefireTests.swift | 41 +++++++++++++++++-- 3 files changed, 85 insertions(+), 8 deletions(-) diff --git a/Packages/CrowCore/Sources/CrowCore/Models/PRStatus.swift b/Packages/CrowCore/Sources/CrowCore/Models/PRStatus.swift index cec8d883..bd0fe007 100644 --- a/Packages/CrowCore/Sources/CrowCore/Models/PRStatus.swift +++ b/Packages/CrowCore/Sources/CrowCore/Models/PRStatus.swift @@ -16,6 +16,21 @@ public struct PRStatus: Codable, Sendable, Equatable { /// 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. + public var isOpen: Bool public init( checksPass: CheckStatus = .unknown, @@ -23,7 +38,8 @@ public struct PRStatus: Codable, Sendable, Equatable { mergeable: MergeStatus = .unknown, failedCheckNames: [String] = [], headSha: String? = nil, - latestReviewID: String? = nil + latestReviewID: String? = nil, + isOpen: Bool = true ) { self.checksPass = checksPass self.reviewStatus = reviewStatus @@ -31,6 +47,7 @@ public struct PRStatus: Codable, Sendable, Equatable { self.failedCheckNames = failedCheckNames self.headSha = headSha self.latestReviewID = latestReviewID + self.isOpen = isOpen } public init(from decoder: Decoder) throws { @@ -41,10 +58,11 @@ public struct PRStatus: Codable, Sendable, Equatable { 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 } private enum CodingKeys: String, CodingKey { - case checksPass, reviewStatus, mergeable, failedCheckNames, headSha, latestReviewID + case checksPass, reviewStatus, mergeable, failedCheckNames, headSha, latestReviewID, isOpen } public enum CheckStatus: String, Codable, Sendable { diff --git a/Sources/Crow/App/IssueTracker.swift b/Sources/Crow/App/IssueTracker.swift index 84cbe299..6d8bee31 100644 --- a/Sources/Crow/App/IssueTracker.swift +++ b/Sources/Crow/App/IssueTracker.swift @@ -1535,6 +1535,17 @@ final class IssueTracker { let prefix = "\(session.id.uuidString)|changesRequested|" emittedTransitionMeta = emittedTransitionMeta.filter { !$0.key.hasPrefix(prefix) } } + // CROW-505: a PR that merged or closed-unmerged while in + // `CHANGES_REQUESTED` keeps its `reviewDecision` unchanged + // (GitHub doesn't reset reviewDecision on close), so the + // bucket-exit rearm above never fires for it. Drop the + // meta entries explicitly on the openβ†’closed edge so the + // map can't accumulate dead entries that the re-fire + // predicate has to repeatedly reject. + if old.isOpen && !newStatus.isOpen { + let prefix = "\(session.id.uuidString)|changesRequested|" + emittedTransitionMeta = emittedTransitionMeta.filter { !$0.key.hasPrefix(prefix) } + } if old.checksPass == .failing && newStatus.checksPass != .failing { if let sha = old.headSha { emittedTransitionMeta.removeValue(forKey: "\(session.id.uuidString)|checksFailing|\(sha)") @@ -1655,6 +1666,12 @@ final class IssueTracker { /// - `emittedTransitionMeta[key].emittedAt` is bumped to `now` so the next /// quiet-window clock starts fresh. /// - `NSLog`'d distinctive line so operators can grep for the behavior. + /// + /// Note on iteration: we mutate `emittedTransitionMeta` during the loop, + /// but Swift dictionaries are value types β€” `for (key, meta) in + /// emittedTransitionMeta` iterates over a copy-on-write snapshot taken at + /// loop entry, so the in-loop assignment to `self.emittedTransitionMeta` + /// doesn't perturb the iteration order or visit set. func reFireStalledChangesRequested(now: Date) -> [PRStatusTransition] { // Gate on the user-facing `AutoRespondSettings.respondToChangesRequested` // toggle. Without this gate a user who explicitly opted out would still @@ -1737,8 +1754,14 @@ final class IssueTracker { now: Date, quietWindow: TimeInterval ) -> Bool { - // (1) PR still in CHANGES_REQUESTED. - guard let s = currentStatus, s.reviewStatus == .changesRequested else { return false } + // (1) PR still in CHANGES_REQUESTED and still OPEN. The `isOpen` check + // is what stops the re-fire from prompting the agent to "address + // review feedback" on a merged or closed-unmerged PR: GitHub leaves + // `reviewDecision == CHANGES_REQUESTED` on close, so without this + // gate a dead PR would re-prompt indefinitely. The re-arm path in + // `applyPRStatuses` also drops the meta entry when `isOpen` flips + // false; this predicate-level check is defense in depth. + guard let s = currentStatus, s.reviewStatus == .changesRequested, s.isOpen else { return false } // (2) Agent idle, not actively working / waiting on input. guard agentActivity == .idle else { return false } // (2b) Managed terminal actually launched the agent. Pre-launch terminals @@ -2157,7 +2180,8 @@ final class IssueTracker { mergeable: mergeStatus, failedCheckNames: failedChecks, headSha: pr.headRefOid.isEmpty ? nil : pr.headRefOid, - latestReviewID: pr.latestReviewID + latestReviewID: pr.latestReviewID, + isOpen: pr.state == "OPEN" ) } diff --git a/Tests/CrowTests/IssueTrackerStalledRefireTests.swift b/Tests/CrowTests/IssueTrackerStalledRefireTests.swift index 55fa7bb6..64a4cb40 100644 --- a/Tests/CrowTests/IssueTrackerStalledRefireTests.swift +++ b/Tests/CrowTests/IssueTrackerStalledRefireTests.swift @@ -27,15 +27,18 @@ struct IssueTrackerStalledRefireTests { private func status( review: PRStatus.ReviewStatus = .changesRequested, sha: String? = nil, - reviewID: String? = "R_1" + reviewID: String? = "R_1", + isOpen: Bool = true, + mergeable: PRStatus.MergeStatus = .unknown ) -> PRStatus { PRStatus( checksPass: .pending, reviewStatus: review, - mergeable: .unknown, + mergeable: mergeable, failedCheckNames: [], headSha: sha ?? shaA, - latestReviewID: reviewID + latestReviewID: reviewID, + isOpen: isOpen ) } @@ -206,6 +209,38 @@ struct IssueTrackerStalledRefireTests { } } + // MARK: - PR no longer open (CROW-505 review #2) + + @Test + func doesNotReFireOnMergedPR() { + // A PR that merged while in CHANGES_REQUESTED keeps reviewDecision + // unchanged, but the re-fire must not prompt the agent to "address + // review feedback" on a merged PR β€” there's nothing to push to. + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA, isOpen: false, mergeable: .merged), + agentActivity: .idle, + terminalReadiness: .agentLaunched, + now: now, + quietWindow: quietWindow + )) + } + + @Test + func doesNotReFireOnClosedUnmergedPR() { + // Same concern, the more common case: PR closed without merging. + // `mergeable` stays `.unknown` on closed PRs β€” only `isOpen` flags + // this as dead. The reviewer's specific blocker. + #expect(!IssueTracker.shouldReFireStalledChangesRequested( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA, isOpen: false, mergeable: .unknown), + agentActivity: .idle, + terminalReadiness: .agentLaunched, + now: now, + quietWindow: quietWindow + )) + } + @Test func doesNotReFireWhenCurrentStatusMissing() { // No persisted PR status for the session β†’ we can't reason about From 8cae4bb66c6fe48291b57f8001c6455d139d7df3 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Sun, 14 Jun 2026 18:38:26 -0400 Subject: [PATCH 4/4] Cap stalled re-fires at 1 and tag synthetic transitions as isReFire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the agent legitimately replies on the PR instead of pushing β€” a behavior the auto-respond prompt explicitly invites ("If a comment is unclear or you disagree, leave a reply…") β€” the head SHA stays unchanged and the agent goes idle. The previous re-fire pass had no upper bound, so the identical prompt would re-inject every quiet window forever, potentially driving duplicate reply comments on the reviewer's PR. Adds `EmittedTransitionMeta.reFireCount: Int` (backward-decode `?? 0`) and `IssueTracker.maxStalledRefires: Int = 1`. The pure predicate's new condition (5) refuses re-fire once `reFireCount >= maxRefires`; each successful re-fire bumps the count. Edge events (review id rotates, author pushes, bucket exits, PR closes) clear the entry and naturally reset the counter via the existing re-arm path. Also tags synthetic transitions with `PRStatusTransition.isReFire = true`. `AppDelegate.onPRStatusTransitions` skips `notifyPRTransition` for re-fires, so a stalled review doesn't generate a fresh macOS notification every quiet window β€” the re-prompt itself (still routed through `AutoRespondCoordinator.handle`) is the useful part; the notification was pure noise. The default-flip Yellow from the same review is a description-only concern (encoder writes the field unconditionally β†’ existing users who never touched the toggle keep `false`; the new `true` default only lands on fresh configs). PR description updated to reflect the limited reach honestly. A one-time opt-in migration prompt is a reasonable follow-up but is out of scope. New tests: - `doesNotReFireOncePerEmissionCapReached` + `reFiresWhileUnderCap` (pure predicate, cap semantics) - `secondReFireOnSameMetaIsSuppressedByCap` (integration over the map-walking pass β€” exercises the bump + re-evaluation) - `toggleOnPermitsSyntheticTransitionAndBumpsEmittedAt` extended to assert `isReFire == true` and `reFireCount == 1` - `emittedTransitionMetaDecodesLegacyEntryWithoutReFireCount` (backward decode default) - `issueTrackerStateRoundTripsThroughDisk` extended with `reFireCount` πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: C69B361D-F6EE-49FC-9F5B-E7A3928F5F1E --- .../CrowCore/Models/PRStatusTransition.swift | 38 +++- .../CrowPersistenceTests/JSONStoreTests.swift | 20 +- Sources/Crow/App/AppDelegate.swift | 6 + Sources/Crow/App/IssueTracker.swift | 52 +++-- .../IssueTrackerStalledRefireTests.swift | 200 +++++++++++------- 5 files changed, 225 insertions(+), 91 deletions(-) diff --git a/Packages/CrowCore/Sources/CrowCore/Models/PRStatusTransition.swift b/Packages/CrowCore/Sources/CrowCore/Models/PRStatusTransition.swift index f4d84bdc..121a4333 100644 --- a/Packages/CrowCore/Sources/CrowCore/Models/PRStatusTransition.swift +++ b/Packages/CrowCore/Sources/CrowCore/Models/PRStatusTransition.swift @@ -35,6 +35,15 @@ public struct PRStatusTransition: Sendable, Equatable { 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 public init( kind: Kind, @@ -43,7 +52,8 @@ public struct PRStatusTransition: Sendable, Equatable { prNumber: Int? = nil, headSha: String? = nil, latestReviewID: String? = nil, - failedCheckNames: [String] = [] + failedCheckNames: [String] = [], + isReFire: Bool = false ) { self.kind = kind self.sessionID = sessionID @@ -52,6 +62,7 @@ public struct PRStatusTransition: Sendable, Equatable { self.headSha = headSha self.latestReviewID = latestReviewID self.failedCheckNames = failedCheckNames + self.isReFire = isReFire } /// Stable key used to suppress duplicate fires across polling cycles. @@ -81,13 +92,36 @@ public struct PRStatusTransition: Sendable, Equatable { /// 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?) { + 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 } } diff --git a/Packages/CrowPersistence/Tests/CrowPersistenceTests/JSONStoreTests.swift b/Packages/CrowPersistence/Tests/CrowPersistenceTests/JSONStoreTests.swift index cf134c28..ad68de9f 100644 --- a/Packages/CrowPersistence/Tests/CrowPersistenceTests/JSONStoreTests.swift +++ b/Packages/CrowPersistence/Tests/CrowPersistenceTests/JSONStoreTests.swift @@ -218,7 +218,7 @@ import Testing let state = PersistedIssueTrackerState( previousPRStatus: [sessionID.uuidString: pr], emittedTransitionKeys: [dedupKey], - emittedTransitionMeta: [dedupKey: EmittedTransitionMeta(emittedAt: emittedAt, headShaAtEmit: "abc123")] + emittedTransitionMeta: [dedupKey: EmittedTransitionMeta(emittedAt: emittedAt, headShaAtEmit: "abc123", reFireCount: 2)] ) let store = JSONStore(directory: dir) @@ -230,6 +230,24 @@ import Testing #expect(reloaded?.emittedTransitionKeys == [dedupKey]) #expect(reloaded?.emittedTransitionMeta?[dedupKey]?.headShaAtEmit == "abc123") #expect(reloaded?.emittedTransitionMeta?[dedupKey]?.emittedAt == emittedAt) + // CROW-505 review #3: reFireCount survives the round trip so the + // per-emission cap persists across Crow restarts. + #expect(reloaded?.emittedTransitionMeta?[dedupKey]?.reFireCount == 2) +} + +@Test func emittedTransitionMetaDecodesLegacyEntryWithoutReFireCount() throws { + // Backward compat: pre-cap stores wrote `EmittedTransitionMeta` without + // the `reFireCount` field. The decode must default it to 0 so an + // upgrade doesn't wedge the re-fire path by inheriting a stale count. + // Uses the same `.iso8601` date strategy as `JSONStore` so the test + // matches the real on-disk shape. + let legacyJSON = #"{"emittedAt": "2023-11-14T22:13:20Z", "headShaAtEmit": "abc123"}"# + .data(using: .utf8)! + let decoder = JSONDecoder() + decoder.dateDecodingStrategy = .iso8601 + let decoded = try decoder.decode(EmittedTransitionMeta.self, from: legacyJSON) + #expect(decoded.headShaAtEmit == "abc123") + #expect(decoded.reFireCount == 0) } @Test func storeDataDecodesWithoutIssueTrackerStateField() throws { diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index 53ef7443..f4f67fe9 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -778,6 +778,12 @@ final class AppDelegate: NSObject, NSApplicationDelegate { tracker.onPRStatusTransitions = { [weak self] transitions in guard let self else { return } for transition in transitions { + // CROW-505 review #3: re-fire transitions skip the macOS + // notification so a stalled review doesn't generate a fresh + // banner every quiet window for the same review id. The + // re-prompt itself (below) is the useful effect; the + // notification was pure noise. + if transition.isReFire { continue } if let session = self.appState.sessions.first(where: { $0.id == transition.sessionID }) { self.notificationManager?.notifyPRTransition(transition, session: session) } diff --git a/Sources/Crow/App/IssueTracker.swift b/Sources/Crow/App/IssueTracker.swift index 6d8bee31..d6185b48 100644 --- a/Sources/Crow/App/IssueTracker.swift +++ b/Sources/Crow/App/IssueTracker.swift @@ -185,6 +185,17 @@ final class IssueTracker { /// calls for it. nonisolated static let stalledRefireQuietWindow: TimeInterval = 10 * 60 + /// Max times a single `.changesRequested` emission may re-fire before + /// requiring a real edge event (reviewer rotates `latestReviewID`, + /// author pushes, bucket exits) to reset. Capped at 1 so the agent + /// receives at most one re-prompt per stall: the auto-respond prompt + /// explicitly invites the agent to reply rather than push when a finding + /// is unclear or disputed, and a reply is a legitimate terminal state + /// the head-SHA gate alone can't recognize. Without this cap the same + /// prompt would re-inject every quiet window indefinitely, potentially + /// driving duplicate reply comments on the reviewer's PR. + nonisolated static let maxStalledRefires: Int = 1 + /// Guards the GitHub-scope console warning so it fires once per session. private var didLogGitHubScopeWarning = false @@ -1707,7 +1718,8 @@ final class IssueTracker { agentActivity: agentActivity, terminalReadiness: readiness, now: now, - quietWindow: Self.stalledRefireQuietWindow + quietWindow: Self.stalledRefireQuietWindow, + maxRefires: Self.maxStalledRefires ) else { continue } guard let status = currentStatus else { continue } @@ -1718,21 +1730,27 @@ final class IssueTracker { prNumber: QuickActionPrompts.parsePRNumber(from: prLink.url), headSha: status.headSha, latestReviewID: status.latestReviewID, - failedCheckNames: [] + failedCheckNames: [], + isReFire: true ) out.append(synthetic) - // Refresh the meta so the next quiet-window clock starts now and we - // don't re-fire again on the very next poll. + // Refresh emittedAt + bump the count so the next quiet-window + // clock starts now and the per-emission cap (CROW-505 review #3) + // ratchets toward exhaustion. + let newCount = meta.reFireCount + 1 emittedTransitionMeta[key] = EmittedTransitionMeta( emittedAt: now, - headShaAtEmit: status.headSha + headShaAtEmit: status.headSha, + reFireCount: newCount ) let elapsed = now.timeIntervalSince(meta.emittedAt) - NSLog("[IssueTracker] auto-refine re-fired (stalled review) β€” session=%@, review=%@, sha=%@, elapsed=%.0fs", + NSLog("[IssueTracker] auto-refine re-fired (stalled review) β€” session=%@, review=%@, sha=%@, elapsed=%.0fs, count=%d/%d", sid.uuidString as NSString, (status.latestReviewID ?? "") as NSString, (status.headSha ?? "") as NSString, - elapsed) + elapsed, + newCount, + Self.maxStalledRefires) } return out } @@ -1741,18 +1759,20 @@ final class IssueTracker { /// re-fire. Pure (no AppState) so the conditions can be exercised /// independently of the polling loop. /// - /// Returns `true` only when all four conditions from CROW-505 hold: - /// (1) PR still in CHANGES_REQUESTED, (2) agent idle on the managed - /// terminal, which must have actually reached `.agentLaunched`, - /// (3) head SHA unchanged since dispatch (anti-loop), and (4) quiet - /// window elapsed. + /// Returns `true` only when all conditions from CROW-505 hold: + /// (1) PR still in CHANGES_REQUESTED and still open, (2) agent idle on + /// the managed terminal, which must have actually reached + /// `.agentLaunched`, (3) head SHA unchanged since dispatch (anti-loop), + /// (4) quiet window elapsed, and (5) re-fire count below the cap so a + /// legitimate reply-instead-of-push doesn't drive infinite re-prompts. nonisolated static func shouldReFireStalledChangesRequested( meta: EmittedTransitionMeta, currentStatus: PRStatus?, agentActivity: AgentActivityState, terminalReadiness: TerminalReadiness?, now: Date, - quietWindow: TimeInterval + quietWindow: TimeInterval, + maxRefires: Int ) -> Bool { // (1) PR still in CHANGES_REQUESTED and still OPEN. The `isOpen` check // is what stops the re-fire from prompting the agent to "address @@ -1772,7 +1792,11 @@ final class IssueTracker { // conservatively refuse to re-fire. guard let metaSha = meta.headShaAtEmit, metaSha == s.headSha else { return false } // (4) Quiet window has elapsed. - return now.timeIntervalSince(meta.emittedAt) >= quietWindow + guard now.timeIntervalSince(meta.emittedAt) >= quietWindow else { return false } + // (5) Re-fire cap not yet reached. Edge events (review id rotates, + // author pushes, bucket exits, PR closes) clear the entry and reset + // the count by virtue of producing a new emission. + return meta.reFireCount < maxRefires } /// Parse the session UUID prefix of a dedup key. Returns nil for malformed diff --git a/Tests/CrowTests/IssueTrackerStalledRefireTests.swift b/Tests/CrowTests/IssueTrackerStalledRefireTests.swift index 64a4cb40..945e1fe9 100644 --- a/Tests/CrowTests/IssueTrackerStalledRefireTests.swift +++ b/Tests/CrowTests/IssueTrackerStalledRefireTests.swift @@ -17,10 +17,33 @@ struct IssueTrackerStalledRefireTests { private let shaA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" private let shaB = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" - private func meta(emittedSecondsAgo: TimeInterval = 11 * 60, headShaAtEmit: String? = nil) -> EmittedTransitionMeta { + private func meta(emittedSecondsAgo: TimeInterval = 11 * 60, headShaAtEmit: String? = nil, reFireCount: Int = 0) -> EmittedTransitionMeta { EmittedTransitionMeta( emittedAt: now.addingTimeInterval(-emittedSecondsAgo), - headShaAtEmit: headShaAtEmit ?? shaA + headShaAtEmit: headShaAtEmit ?? shaA, + reFireCount: reFireCount + ) + } + + /// Convenience wrapper so each predicate call doesn't have to repeat + /// `quietWindow` + `maxRefires`. The defaults match the production + /// `IssueTracker.maxStalledRefires`, so the test reads the constant + /// (locking against silent cap bumps). + private func shouldReFire( + meta: EmittedTransitionMeta, + currentStatus: PRStatus?, + agentActivity: AgentActivityState = .idle, + terminalReadiness: TerminalReadiness? = .agentLaunched, + maxRefires: Int = IssueTracker.maxStalledRefires + ) -> Bool { + IssueTracker.shouldReFireStalledChangesRequested( + meta: meta, + currentStatus: currentStatus, + agentActivity: agentActivity, + terminalReadiness: terminalReadiness, + now: now, + quietWindow: quietWindow, + maxRefires: maxRefires ) } @@ -49,13 +72,9 @@ struct IssueTrackerStalledRefireTests { // The CROW-505 bug repro: PR still in CHANGES_REQUESTED, agent went // idle without addressing all findings, head SHA hasn't advanced // (agent didn't push), and quiet window has elapsed. - #expect(IssueTracker.shouldReFireStalledChangesRequested( + #expect(shouldReFire( meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), - currentStatus: status(sha: shaA), - agentActivity: .idle, - terminalReadiness: .agentLaunched, - now: now, - quietWindow: quietWindow + currentStatus: status(sha: shaA) )) } @@ -67,13 +86,9 @@ struct IssueTrackerStalledRefireTests { // head SHA, which kills the re-fire. This preserves the CROW-456 // anti-loop guarantee β€” the agent's own response push doesn't // re-trigger the prompt that told it to push. - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), - currentStatus: status(sha: shaB), // SHA advanced - agentActivity: .idle, - terminalReadiness: .agentLaunched, - now: now, - quietWindow: quietWindow + currentStatus: status(sha: shaB) // SHA advanced )) } @@ -104,13 +119,10 @@ struct IssueTrackerStalledRefireTests { func doesNotReFireWhileAgentWorking() { // Don't interrupt an agent that's actively working β€” they may already // be addressing the feedback. - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), currentStatus: status(sha: shaA), - agentActivity: .working, - terminalReadiness: .agentLaunched, - now: now, - quietWindow: quietWindow + agentActivity: .working )) } @@ -118,13 +130,10 @@ struct IssueTrackerStalledRefireTests { func doesNotReFireWhileAgentWaiting() { // `.waiting` means the agent is blocked on user input β€” re-firing now // would push another prompt into a queue the user is about to answer. - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), currentStatus: status(sha: shaA), - agentActivity: .waiting, - terminalReadiness: .agentLaunched, - now: now, - quietWindow: quietWindow + agentActivity: .waiting )) } @@ -132,13 +141,10 @@ struct IssueTrackerStalledRefireTests { func doesNotReFireWhileAgentDone() { // `.done` is the terminal "completed a turn" state β€” it's followed by // a hook reset to `.idle`. Re-firing on `.done` would race. - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), currentStatus: status(sha: shaA), - agentActivity: .done, - terminalReadiness: .agentLaunched, - now: now, - quietWindow: quietWindow + agentActivity: .done )) } @@ -148,13 +154,9 @@ struct IssueTrackerStalledRefireTests { func doesNotReFireBeforeQuietWindowElapsed() { // Quiet window: agents can spend 5+ minutes thinking on a hard // finding. Don't re-prompt before they've had a chance to act. - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: meta(emittedSecondsAgo: quietWindow - 1, headShaAtEmit: shaA), - currentStatus: status(sha: shaA), - agentActivity: .idle, - terminalReadiness: .agentLaunched, - now: now, - quietWindow: quietWindow + currentStatus: status(sha: shaA) )) } @@ -165,13 +167,10 @@ struct IssueTrackerStalledRefireTests { // A pre-launch terminal (shellReady or earlier) can't be "stalled" β€” // the agent never ran. Wait for the launch to finish first. for readiness: TerminalReadiness in [.uninitialized, .surfaceCreated, .shellReady, .timedOut, .failed] { - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), currentStatus: status(sha: shaA), - agentActivity: .idle, - terminalReadiness: readiness, - now: now, - quietWindow: quietWindow + terminalReadiness: readiness ), "expected no re-fire at readiness \(readiness)") } } @@ -179,13 +178,10 @@ struct IssueTrackerStalledRefireTests { @Test func doesNotReFireWhenTerminalReadinessUnknown() { // No managed terminal at all β†’ no readiness entry. Can't re-fire. - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), currentStatus: status(sha: shaA), - agentActivity: .idle, - terminalReadiness: nil, - now: now, - quietWindow: quietWindow + terminalReadiness: nil )) } @@ -198,13 +194,9 @@ struct IssueTrackerStalledRefireTests { // also drops the entry on this transition, but the predicate // double-checks. for review: PRStatus.ReviewStatus in [.approved, .reviewRequired, .unknown] { - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), - currentStatus: status(review: review, sha: shaA), - agentActivity: .idle, - terminalReadiness: .agentLaunched, - now: now, - quietWindow: quietWindow + currentStatus: status(review: review, sha: shaA) ), "expected no re-fire at review status \(review)") } } @@ -216,13 +208,9 @@ struct IssueTrackerStalledRefireTests { // A PR that merged while in CHANGES_REQUESTED keeps reviewDecision // unchanged, but the re-fire must not prompt the agent to "address // review feedback" on a merged PR β€” there's nothing to push to. - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), - currentStatus: status(sha: shaA, isOpen: false, mergeable: .merged), - agentActivity: .idle, - terminalReadiness: .agentLaunched, - now: now, - quietWindow: quietWindow + currentStatus: status(sha: shaA, isOpen: false, mergeable: .merged) )) } @@ -231,13 +219,9 @@ struct IssueTrackerStalledRefireTests { // Same concern, the more common case: PR closed without merging. // `mergeable` stays `.unknown` on closed PRs β€” only `isOpen` flags // this as dead. The reviewer's specific blocker. - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), - currentStatus: status(sha: shaA, isOpen: false, mergeable: .unknown), - agentActivity: .idle, - terminalReadiness: .agentLaunched, - now: now, - quietWindow: quietWindow + currentStatus: status(sha: shaA, isOpen: false, mergeable: .unknown) )) } @@ -245,13 +229,9 @@ struct IssueTrackerStalledRefireTests { func doesNotReFireWhenCurrentStatusMissing() { // No persisted PR status for the session β†’ we can't reason about // SHA equality or bucket membership. Refuse. - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), - currentStatus: nil, - agentActivity: .idle, - terminalReadiness: .agentLaunched, - now: now, - quietWindow: quietWindow + currentStatus: nil )) } @@ -260,13 +240,46 @@ struct IssueTrackerStalledRefireTests { // Migrated entries from before CROW-505 may have nil headShaAtEmit. // Without a baseline we can't prove the author hasn't pushed β€” // refuse rather than risk an unwanted re-fire. - #expect(!IssueTracker.shouldReFireStalledChangesRequested( + #expect(!shouldReFire( meta: EmittedTransitionMeta(emittedAt: now.addingTimeInterval(-(quietWindow + 1)), headShaAtEmit: nil), + currentStatus: status(sha: shaA) + )) + } + + // MARK: - Re-fire cap (CROW-505 review #3) + + @Test + func doesNotReFireOncePerEmissionCapReached() { + // The whole reason this cap exists: when the agent legitimately + // *replies* on the PR instead of pushing (a behavior the + // auto-respond prompt itself encourages), SHA stays put and the + // agent goes idle. Without a cap the same prompt re-injects every + // quiet window forever, potentially producing duplicate reply + // comments on the reviewer's PR. After `maxStalledRefires`, refuse + // until an edge event creates a new meta entry. + #expect(!shouldReFire( + meta: meta( + emittedSecondsAgo: quietWindow + 1, + headShaAtEmit: shaA, + reFireCount: IssueTracker.maxStalledRefires + ), + currentStatus: status(sha: shaA) + )) + } + + @Test + func reFiresWhileUnderCap() { + // Count below the cap still permits a re-fire. With the production + // cap of 1, only `reFireCount == 0` qualifies β€” but a customizable + // cap (passed explicitly here) lets us verify the < relation. + #expect(shouldReFire( + meta: meta( + emittedSecondsAgo: quietWindow + 1, + headShaAtEmit: shaA, + reFireCount: 2 + ), currentStatus: status(sha: shaA), - agentActivity: .idle, - terminalReadiness: .agentLaunched, - now: now, - quietWindow: quietWindow + maxRefires: 3 )) } @@ -396,10 +409,49 @@ struct IssueTrackerStalledRefireWiringTests { #expect(transitions[0].sessionID == sid) #expect(transitions[0].headSha == shaA) #expect(transitions[0].latestReviewID == "R_1") + // CROW-505 review #3: synthetic re-fires are tagged so AppDelegate + // can suppress the macOS notification while still letting + // AutoRespondCoordinator dispatch the re-prompt. + #expect(transitions[0].isReFire) // emittedAt bumped to `now` so the next poll inside the quiet window // doesn't immediately re-fire again. #expect(tracker.emittedTransitionMeta[key]?.emittedAt == now) #expect(tracker.emittedTransitionMeta[key]?.emittedAt != originalEmittedAt) + // CROW-505 review #3: re-fire count incremented so the per-emission + // cap eventually shuts the re-fire down even if the agent never + // pushes (e.g. legitimately replied instead). + #expect(tracker.emittedTransitionMeta[key]?.reFireCount == 1) + } + + @Test + func secondReFireOnSameMetaIsSuppressedByCap() { + // The cap regression test: after one re-fire on a stalled emission + // (production `maxStalledRefires = 1`), a subsequent poll under the + // same conditions must NOT emit another synthetic transition. + // Otherwise an agent that legitimately replied instead of pushing + // would receive the identical prompt every quiet window forever. + let (tracker, _, key) = makeStalledTracker(toggleOn: true) + + // First re-fire β€” emits, increments count, refreshes emittedAt. + let first = tracker.reFireStalledChangesRequested(now: Date()) + #expect(first.count == 1) + #expect(tracker.emittedTransitionMeta[key]?.reFireCount == 1) + + // Roll the clock past another quiet window so emittedAt is stale + // again, then walk the pass β€” the cap must hold. + let laterMeta = EmittedTransitionMeta( + emittedAt: Date().addingTimeInterval(-IssueTracker.stalledRefireQuietWindow * 2), + headShaAtEmit: shaA, + reFireCount: 1 + ) + tracker.emittedTransitionMeta[key] = laterMeta + + let second = tracker.reFireStalledChangesRequested(now: Date()) + #expect(second.isEmpty) + // Cap reached β†’ meta is left untouched (no count overflow, no + // emittedAt drift) so it remains a stable record of the original + // stall until an edge event clears it. + #expect(tracker.emittedTransitionMeta[key] == laterMeta) } // MARK: - Review-session gate