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/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/Packages/CrowCore/Sources/CrowCore/Models/PRStatusTransition.swift b/Packages/CrowCore/Sources/CrowCore/Models/PRStatusTransition.swift index de88e4e5..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. @@ -74,6 +85,46 @@ 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. +/// +/// `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 + } +} + 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..ad68de9f 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", reFireCount: 2)] ) let store = JSONStore(directory: dir) @@ -226,6 +228,26 @@ 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) + // 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 ee4a7bf0..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) } @@ -803,6 +809,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 9ff4d3e3..d6185b48 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,13 +163,38 @@ 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] = [:] - - /// Stable keys of transitions we've already emitted, used to suppress - /// duplicates across the in-process lifetime. See `PRStatusTransition.dedupeKey`. + /// 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 + /// 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 = [] + /// 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 + /// 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 + + /// 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 @@ -1483,9 +1519,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 +1539,27 @@ 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) } + } + // 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 { - emittedTransitionKeys.remove("\(session.id.uuidString)|checksFailing|\(sha)") + emittedTransitionMeta.removeValue(forKey: "\(session.id.uuidString)|checksFailing|\(sha)") } } } @@ -1523,8 +1571,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 +1583,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 +1605,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 +1624,195 @@ 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. + /// + /// 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 + // 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 + // 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, + maxRefires: Self.maxStalledRefires + ) 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: [], + isReFire: true + ) + out.append(synthetic) + // 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, + reFireCount: newCount + ) + let elapsed = now.timeIntervalSince(meta.emittedAt) + 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, + newCount, + Self.maxStalledRefires) + } + 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 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, + 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 + // 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 + // 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. + 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 + /// 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 @@ -1971,7 +2204,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 new file mode 100644 index 00000000..945e1fe9 --- /dev/null +++ b/Tests/CrowTests/IssueTrackerStalledRefireTests.swift @@ -0,0 +1,492 @@ +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, reFireCount: Int = 0) -> EmittedTransitionMeta { + EmittedTransitionMeta( + emittedAt: now.addingTimeInterval(-emittedSecondsAgo), + 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 + ) + } + + private func status( + review: PRStatus.ReviewStatus = .changesRequested, + sha: String? = nil, + reviewID: String? = "R_1", + isOpen: Bool = true, + mergeable: PRStatus.MergeStatus = .unknown + ) -> PRStatus { + PRStatus( + checksPass: .pending, + reviewStatus: review, + mergeable: mergeable, + failedCheckNames: [], + headSha: sha ?? shaA, + latestReviewID: reviewID, + isOpen: isOpen + ) + } + + // 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(shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA) + )) + } + + // 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(!shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaB) // SHA advanced + )) + } + + // 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(!shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + agentActivity: .working + )) + } + + @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(!shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + agentActivity: .waiting + )) + } + + @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(!shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + agentActivity: .done + )) + } + + // 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(!shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow - 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA) + )) + } + + // 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(!shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + terminalReadiness: readiness + ), "expected no re-fire at readiness \(readiness)") + } + } + + @Test + func doesNotReFireWhenTerminalReadinessUnknown() { + // No managed terminal at all → no readiness entry. Can't re-fire. + #expect(!shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA), + terminalReadiness: nil + )) + } + + // 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(!shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(review: review, sha: shaA) + ), "expected no re-fire at review status \(review)") + } + } + + // 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(!shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA, isOpen: false, mergeable: .merged) + )) + } + + @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(!shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: status(sha: shaA, isOpen: false, mergeable: .unknown) + )) + } + + @Test + func doesNotReFireWhenCurrentStatusMissing() { + // No persisted PR status for the session → we can't reason about + // SHA equality or bucket membership. Refuse. + #expect(!shouldReFire( + meta: meta(emittedSecondsAgo: quietWindow + 1, headShaAtEmit: shaA), + currentStatus: nil + )) + } + + @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(!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), + maxRefires: 3 + )) + } + + // 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) + } +} + +/// 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") + // 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 + + @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) + } +}