diff --git a/.github/workflows/test-depot.yml b/.github/workflows/test-depot.yml index d394291686fd..4697a2221767 100644 --- a/.github/workflows/test-depot.yml +++ b/.github/workflows/test-depot.yml @@ -165,7 +165,7 @@ jobs: suite_output=$(run_unit_suite "-only-testing:cmuxTests/$suite") || suite_status=$? printf '%s\n' "$suite_output" if [ "$suite_status" -ne 0 ]; then return "$suite_status"; fi - if ! printf '%s\n' "$suite_output" | grep -Eq 'Test run with [1-9][0-9]* tests|Executed [1-9][0-9]* tests'; then + if ! grep -Eq 'Test run with [1-9][0-9]* tests|Executed [1-9][0-9]* tests' <<< "$suite_output"; then echo "No tests executed for $suite" >&2 return 1 fi diff --git a/CLI/AgentHookNotificationPolicy.swift b/CLI/AgentHookNotificationPolicy.swift index 9194a86c31de..df1b6bc2b30c 100644 --- a/CLI/AgentHookNotificationPolicy.swift +++ b/CLI/AgentHookNotificationPolicy.swift @@ -1,4 +1,5 @@ import CmuxSettings +import CryptoKit import Foundation enum AgentHookNotificationStatus: String, Codable { @@ -23,17 +24,39 @@ enum AgentHookNotifyCategory: String { } } - /// Legacy delimiter-safe meta segment: `c=;p=<0|1>`. The - /// contextual overload below adds the agent and alert identity. + /// Delimiter-safe legacy meta segment: `c=;p=<0|1>`. The + /// contextual overload below can add agent, alert, and approval identity. func metaSegment(pending: Bool) -> String? { metaSegment(pending: pending, agentKind: nil, isSubagent: nil) } + /// Meta segment carrying an opaque Codex approval correlation id. + /// `.other` is the explicit ungated category and never rides the wire. + func metaSegment( + pending: Bool, + approvalID: String? = nil, + approvalIDIsDerived: Bool = false, + approvalSource: String? = nil + ) -> String? { + metaSegment( + pending: pending, + approvalID: approvalID, + approvalIDIsDerived: approvalIDIsDerived, + agentKind: nil, + isSubagent: nil, + approvalSource: approvalSource + ) + } + /// Extended meta segment carrying optional agent-event context for the - /// app's notification-policy hooks: - /// `c=;p=<0|1>[;a=][;n=<0|1>][;k=]` (canonical - /// field order; `a=` is the case-preserving registry identifier, `n=` marks a - /// nested subagent session, and `k=` is an opaque notification identity). + /// app's notification-policy hooks. Correlated Codex approvals retain the + /// historical `a=` spelling for compatibility. Other + /// producers may attach a validated agent slug and opaque UUID key using + /// the canonical `a=`, `d=`, `n=`, and `k=` fields. + /// `c=;p=<0|1>[;a=][;d=1][;n=<0|1>][;k=]` + /// (canonical field order; `d=1` marks a derived approval identity, `a=` is + /// the stable lowercase agent slug otherwise, `n=` marks a nested subagent + /// session, and `k=` is an opaque notification identity). /// An agent kind or correlation key that fails validation is dropped rather /// than risking the app-side parser folding the whole meta back into the /// body. @@ -42,10 +65,36 @@ enum AgentHookNotifyCategory: String { agentKind: String?, isSubagent: Bool?, correlationKey: String? = nil + ) -> String? { + metaSegment( + pending: pending, + approvalID: nil, + agentKind: agentKind, + isSubagent: isSubagent, + correlationKey: correlationKey + ) + } + + private func metaSegment( + pending: Bool, + approvalID: String?, + approvalIDIsDerived: Bool = false, + agentKind: String?, + isSubagent: Bool?, + correlationKey: String? = nil, + approvalSource: String? = nil ) -> String? { guard self != .other else { return nil } var segment = "c=\(rawValue);p=\(pending ? 1 : 0)" - if let agentKind, Self.isValidAgentKindTag(agentKind) { + if self == .needsPermission, let approvalID { + segment += ";a=\(approvalID)" + if approvalIDIsDerived { + segment += ";d=1" + if let approvalSource, Self.isValidApprovalSource(approvalSource) { + segment += ";o=\(approvalSource)" + } + } + } else if let agentKind, Self.isValidAgentKindTag(agentKind) { segment += ";a=\(agentKind)" } if let isSubagent { @@ -57,6 +106,10 @@ enum AgentHookNotifyCategory: String { return segment } + static func isValidApprovalSource(_ value: String) -> Bool { + value == "hook" || value == "feed" + } + /// Mirror of the app-side `AgentNotificationMeta` slug grammar: 1-64 ASCII /// characters of `[A-Za-z0-9._-]`, excluding `.` and `..`. Both sides must /// agree exactly or the app folds the meta back into the notification body. @@ -104,6 +157,387 @@ enum AgentHookNotifyCategory: String { } } +/// Correlation shared by the generic Codex hook and the newer Feed hook. +/// `PermissionRequest` does not expose a tool-use id, so both sides derive an +/// opaque identity from the stable session/turn/tool/input tuple that the +/// request and completion share. +struct CodexApprovalNotificationIdentity: Equatable, Sendable { + /// Keep model-controlled input from turning approval correlation into an + /// unbounded serialization/hash operation on the hook's synchronous path. + /// Inputs larger than this are not safely correlatable and fail closed. + private static let maxCanonicalToolInputBytes = 64 * 1024 + private static let maxIdentityComponentBytes = 1024 + private static let hexadecimalDigits = Array("0123456789abcdef".utf8) + // Hook envelopes are allowed a few wrapper layers (for example an + // app-server `notification` containing `params` and `toolCall`). Keep + // the recursive fallback bounded so hostile JSON cannot turn identity + // derivation into an unbounded tree walk. + private static let maxWrapperDepth = 4 + + let scope: String + let approvalID: String + /// Whether the provider supplied a stable per-request discriminator. When + /// false, repeated identical tuples are treated as ambiguous by the app + /// coordinator and require a scope-level resolution. + let isAuthoritative: Bool + let fallbackApprovalID: String? + + init(scope: String, approvalID: String, isAuthoritative: Bool = true, fallbackApprovalID: String? = nil) { + self.scope = scope + self.approvalID = approvalID + self.isAuthoritative = isAuthoritative + self.fallbackApprovalID = fallbackApprovalID + } + + var resolutionOptions: String { + "--approval-id=\(approvalID)" + (fallbackApprovalID.map { " --approval-fallback-id=\($0)" } ?? "") + } + + static func nativeRequestID(in object: [String: Any]?) -> String? { + guard let object else { return nil } + return firstNonemptyStringDeep(in: object, keys: [ + "approval_id", "approvalId", "tool_call_id", "toolCallId", "call_id", "callId", + "request_id", "requestId", "item_id", "itemId", + ]) + } + + /// Derives the session/turn scope even when a lifecycle payload has no + /// tool input (for example Codex's `Stop` hook). Scope clears are safe for + /// that payload shape; a request-level id still requires deterministic + /// tool input below. + static func makeScope( + rawObject: [String: Any]?, + fallbackSessionID: String? + ) -> String? { + guard let seed = scopeSeed( + rawObject: rawObject ?? [:], + fallbackSessionID: fallbackSessionID + ) else { return nil } + return digestPrefix(seed) + } + + static func make( + rawObject: [String: Any]?, + fallbackSessionID: String? + ) -> Self? { + let object = rawObject ?? [:] + guard let scopeSeed = scopeSeed( + rawObject: object, + fallbackSessionID: fallbackSessionID + ) else { return nil } + guard firstNonemptyStringDeep(in: object, keys: ["turn_id", "turnId"]) != nil else { + return nil + } + let toolCall = firstNestedObject(in: object, keys: ["toolCall", "tool_call"]) + guard let toolName = ( + firstNonemptyStringDeep(in: object, keys: ["tool_name", "toolName"]) + ?? toolCall.flatMap { firstNonemptyString(in: $0, keys: ["name", "tool_name", "toolName"]) } + ) else { + return nil + } + // Explicit provider ids are authoritative when present. In + // particular, do not let a canonical input value silently override a + // `call_id`/`tool_call_id` supplied by the provider: those ids are the + // only stable discriminator when two calls have identical arguments. + let explicitCallID = nativeRequestID(in: object) + let toolInput = firstNestedValue(in: object, keys: ["tool_input", "toolInput"]) + ?? toolCall?["args"] + // Prefer an explicit provider call id, then the canonical request + // tuple. Codex versions in the wild add `tool_use_id` only to + // PostToolUse; it is deliberately not used here because a + // completion-only id would make the request and completion diverge. + let canonicalToolInput = toolInput.flatMap(canonicalJSON) + guard explicitCallID != nil || canonicalToolInput != nil else { return nil } + let scope = digestPrefix(scopeSeed) + let requestSeed: String + if let explicitCallID { + requestSeed = "\(scopeSeed)\ncall=\(explicitCallID)\ntool=\(toolName)" + } else { + requestSeed = "\(scopeSeed)\ntool=\(toolName)\ninput=\(canonicalToolInput!)" + } + let request = digestPrefix(requestSeed) + return Self( + scope: scope, + approvalID: "\(scope).\(request)", + isAuthoritative: explicitCallID != nil, + fallbackApprovalID: explicitCallID == nil ? nil : canonicalToolInput.map { + "\(scope).\(digestPrefix("\(scopeSeed)\ntool=\(toolName)\ninput=\($0)"))" + } + ) + } + + private static func scopeSeed( + rawObject: [String: Any], + fallbackSessionID: String? + ) -> String? { + guard let sessionID = firstNonemptyStringDeep( + in: rawObject, + keys: ["session_id", "sessionId", "conversation_id", "conversationId"] + ) ?? normalized(fallbackSessionID) else { return nil } + guard let turnID = firstNonemptyStringDeep(in: rawObject, keys: ["turn_id", "turnId"]) else { + return nil + } + return "session=\(sessionID)\nturn=\(turnID)" + } + + /// Searches only the wrapper containers used by supported Codex hook + /// producers. Keeping this bounded avoids recursively walking arbitrary + /// model-controlled JSON while still handling app-server envelopes. + private static func firstNonemptyStringDeep( + in object: [String: Any], + keys: [String], + depth: Int = 0 + ) -> String? { + guard depth <= maxWrapperDepth else { return nil } + if let direct = firstNonemptyString(in: object, keys: keys) { + return direct + } + for containerKey in ["notification", "data", "context", "extra", "params", "toolCall", "tool_call"] { + guard let nested = object[containerKey] as? [String: Any] else { continue } + if let value = firstNonemptyStringDeep(in: nested, keys: keys, depth: depth + 1) { + return value + } + } + return nil + } + + private static func firstNestedObject( + in object: [String: Any], + keys: [String], + depth: Int = 0 + ) -> [String: Any]? { + guard depth <= maxWrapperDepth else { return nil } + for key in keys { + if let nested = object[key] as? [String: Any] { + return nested + } + } + for containerKey in ["notification", "data", "context", "extra", "params"] { + guard let nested = object[containerKey] as? [String: Any] else { continue } + if let result = firstNestedObject(in: nested, keys: keys, depth: depth + 1) { + return result + } + } + return nil + } + + private static func firstNestedValue( + in object: [String: Any], + keys: [String], + depth: Int = 0 + ) -> Any? { + guard depth <= maxWrapperDepth else { return nil } + for key in keys { + if let value = object[key] { + return value + } + } + for containerKey in ["notification", "data", "context", "extra", "params"] { + guard let nested = object[containerKey] as? [String: Any] else { continue } + if let value = firstNestedValue(in: nested, keys: keys, depth: depth + 1) { + return value + } + } + return nil + } + + private static func normalized(_ value: String?) -> String? { + guard let value = value?.trimmingCharacters(in: .whitespacesAndNewlines), + !value.isEmpty, + value.utf8.count <= maxIdentityComponentBytes else { return nil } + return value + } + + private static func firstNonemptyString( + in object: [String: Any], + keys: [String] + ) -> String? { + for key in keys { + if let value = normalized(object[key] as? String) { + return value + } + } + return nil + } + + private static func canonicalJSON(_ value: Any?) -> String? { + guard let value else { return nil } + let wrapped: [String: Any] = ["value": value] + guard JSONSerialization.isValidJSONObject(wrapped), + let data = try? JSONSerialization.data(withJSONObject: wrapped, options: [.sortedKeys]), + data.count <= maxCanonicalToolInputBytes, + let string = String(data: data, encoding: .utf8) else { + return nil + } + return string + } + + private static func digestPrefix(_ value: String) -> String { + let digest = SHA256.hash(data: Data(value.utf8)) + var bytes: [UInt8] = [] + bytes.reserveCapacity(24) + for byte in digest.prefix(12) { + bytes.append(hexadecimalDigits[Int(byte >> 4)]) + bytes.append(hexadecimalDigits[Int(byte & 0x0f)]) + } + return String(decoding: bytes, as: UTF8.self) + } +} + +enum CodexApprovalReviewRoute: Equatable, Sendable { + case user + case autoReview +} + +/// Reads Codex's effective per-turn reviewer before deciding whether a +/// `PermissionRequest` can represent user-visible work. +/// +/// Codex runs permission hooks before either reviewer. Current Codex persists +/// the effective `approvals_reviewer` in the turn-context rollout item first, +/// so `auto_review` is authoritative evidence that the request will be decided +/// by Codex's reviewer rather than by the user. Older Codex versions omit the +/// field and fall back to the app-side settle window. +struct CodexApprovalNotificationPolicy: Sendable { + static let rolloutTailBytes: UInt64 = 1024 * 1024 + + /// Returns true only when a readable bounded rollout tail proves auto-review. + func isAutoReviewed( + rawObject: [String: Any], + transcriptPath: String?, + readRolloutLines: (_ path: String, _ maxBytes: UInt64) -> [String]? + ) -> Bool { + // An explicit user reviewer is authoritative and can never result in + // auto-review. Resolve that cheap in-memory case before opening and + // parsing the rollout tail. Keep the auto-review side fail-closed: + // it still requires a readable transcript as proof of the effective + // turn context. + if reviewRoute(rawObject: rawObject, rolloutLines: []) == .user { + return false + } + guard let transcriptPath = transcriptPath?.trimmingCharacters(in: .whitespacesAndNewlines), + !transcriptPath.isEmpty, + let rolloutLines = readRolloutLines(transcriptPath, Self.rolloutTailBytes), + !rolloutLines.isEmpty else { + return false + } + return reviewRoute(rawObject: rawObject, rolloutLines: rolloutLines) == .autoReview + } + + func reviewRoute( + rawObject: [String: Any], + rolloutLines: [String] + ) -> CodexApprovalReviewRoute? { + // A top-level reviewer on the request itself is authoritative, including + // for MCP calls. Current hook payloads omit it, but accept that stronger + // future signal before falling back to turn-wide context. + if let direct = reviewRoute(in: rawObject) { + return direct + } + + // Codex Apps may override the turn's reviewer per MCP connector. The + // hook payload does not expose that effective override, so no turn-wide + // reviewer value is authoritative for MCP requests. Fall back to + // correlated settling rather than risk silencing a user prompt. + let toolName = firstToolName(in: rawObject) + if toolName?.lowercased().hasPrefix("mcp__") == true { + return nil + } + + let requestedTurnID = firstStringDeep( + in: rawObject, + keys: ["turn_id", "turnId"] + ) + guard let requestedTurnID else { return nil } + for line in rolloutLines.reversed() { + guard let data = line.data(using: .utf8), + let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any], + object["type"] as? String == "turn_context", + let payload = object["payload"] as? [String: Any] else { + continue + } + if firstString(in: payload, keys: ["turn_id", "turnId"]) == requestedTurnID { + return reviewRoute(in: payload) + } + } + return nil + } + + private func reviewRoute(in object: [String: Any]) -> CodexApprovalReviewRoute? { + guard let value = firstString( + in: object, + keys: ["approvals_reviewer", "approvalsReviewer", "approval_reviewer", "approvalReviewer"] + )?.lowercased() else { + return nil + } + switch value { + case "user": + return .user + case "auto_review", "auto-review", "guardian_subagent": + return .autoReview + default: + return nil + } + } + + private func firstString( + in object: [String: Any], + keys: [String] + ) -> String? { + for key in keys { + guard let raw = object[key] as? String else { continue } + let value = raw.trimmingCharacters(in: .whitespacesAndNewlines) + if !value.isEmpty { + return value + } + } + return nil + } + + /// Finds a supported Codex field through the bounded app-server wrapper + /// layers. The hook payload is model-controlled JSON, so never recurse + /// without a depth limit. + private func firstStringDeep( + in object: [String: Any], + keys: [String], + depth: Int = 0 + ) -> String? { + guard depth <= 4 else { return nil } + if let direct = firstString(in: object, keys: keys) { + return direct + } + for containerKey in ["notification", "data", "context", "extra", "params"] { + guard let nested = object[containerKey] as? [String: Any] else { continue } + if let value = firstStringDeep(in: nested, keys: keys, depth: depth + 1) { + return value + } + } + return nil + } + + private func firstToolName( + in object: [String: Any], + depth: Int = 0 + ) -> String? { + guard depth <= 4 else { return nil } + if let direct = firstString(in: object, keys: ["tool_name", "toolName"]) { + return direct + } + for toolKey in ["toolCall", "tool_call"] { + if let toolCall = object[toolKey] as? [String: Any], + let name = firstString(in: toolCall, keys: ["name", "tool_name", "toolName"]) { + return name + } + } + for containerKey in ["notification", "data", "context", "extra", "params"] { + guard let nested = object[containerKey] as? [String: Any] else { continue } + if let value = firstToolName(in: nested, depth: depth + 1) { + return value + } + } + return nil + } +} + struct AgentHookNotificationSummary { let subtitle: String let body: String @@ -508,6 +942,7 @@ enum AgentHookNotificationPolicy { hash ^= UInt64(byte) hash &*= 0x100000001b3 } - return String(format: "%016llx", hash) + let hexadecimal = String(hash, radix: 16) + return String(repeating: "0", count: max(0, 16 - hexadecimal.count)) + hexadecimal } } diff --git a/CLI/CMUXCLI+AgentHookPayload.swift b/CLI/CMUXCLI+AgentHookPayload.swift index fa31046a7cc2..9cd0f0d2ad32 100644 --- a/CLI/CMUXCLI+AgentHookPayload.swift +++ b/CLI/CMUXCLI+AgentHookPayload.swift @@ -1,3 +1,4 @@ +import Darwin import Foundation extension CMUXCLI { @@ -352,46 +353,39 @@ extension CMUXCLI { maxBytes: UInt64 ) -> [String]? { let expandedPath = NSString(string: path).expandingTildeInPath - guard let handle = try? FileHandle(forReadingFrom: URL(fileURLWithPath: expandedPath)) else { + // Open and validate one descriptor. A path-only stat followed by a + // separate FileHandle open is a TOCTOU window in which a FIFO/device + // could replace the transcript and block the synchronous hook. + // `O_NOFOLLOW` rejects a final symlink and `O_NONBLOCK` keeps even a + // raced special-file open from waiting before fstat can reject it. + let descriptor = Darwin.open( + expandedPath, + O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK + ) + guard descriptor >= 0 else { return nil } + defer { _ = Darwin.close(descriptor) } + var metadata = stat() + guard Darwin.fstat(descriptor, &metadata) == 0, + (metadata.st_mode & mode_t(S_IFMT)) == mode_t(S_IFREG) else { return nil } - defer { try? handle.close() } - - func isASCIIWhitespace(_ byte: UInt8) -> Bool { - byte == 0x09 || byte == 0x0A || byte == 0x0D || byte == 0x20 - } - - func hasCompleteLineAfterLeadingBoundary(_ data: Data, readStart: UInt64) -> Bool { - guard readStart > 0 else { return true } - guard let newline = data.firstIndex(of: 0x0A) else { return false } - return data[data.index(after: newline)...].contains { !isASCIIWhitespace($0) } - } + let handle = FileHandle(fileDescriptor: descriptor, closeOnDealloc: false) let size: UInt64 do { size = try handle.seekToEnd() - var readStart = size > maxBytes ? size - maxBytes : 0 + let readStart = size > maxBytes ? size - maxBytes : 0 try handle.seek(toOffset: readStart) - guard var data = try handle.readToEnd(), !data.isEmpty else { + // Read exactly the bounded window. `readToEnd()` can observe bytes + // appended after `seekToEnd()` and silently exceed the rollout + // budget on a busy Codex transcript. + let initialReadLength = Int(min(maxBytes, UInt64(Int.max))) + guard var data = try handle.read(upToCount: initialReadLength), !data.isEmpty else { return nil } - let maxWindowBytes = maxBytes > UInt64.max / 8 ? UInt64.max : maxBytes * 8 - - while !hasCompleteLineAfterLeadingBoundary(data, readStart: readStart), readStart > 0 { - let currentWindowBytes = size - readStart - guard currentWindowBytes < maxWindowBytes else { break } - let remainingWindowBytes = maxWindowBytes - currentWindowBytes - let expansionBytes = min(readStart, maxBytes, remainingWindowBytes) - guard expansionBytes > 0 else { break } - - readStart -= expansionBytes - try handle.seek(toOffset: readStart) - guard let expandedData = try handle.readToEnd(), !expandedData.isEmpty else { - return nil - } - data = expandedData - } - + // Keep the hook read strictly bounded. If the window begins in the + // middle of a very long line, dropping that partial line is safer + // than expanding into an unbounded transcript prefix. if readStart > 0, let newline = data.firstIndex(of: 0x0A) { data.removeSubrange(data.startIndex...newline) } diff --git a/CLI/CMUXCLI+CodexFireAndForgetHooks.swift b/CLI/CMUXCLI+CodexFireAndForgetHooks.swift index 0eeb8ae14885..20445466ed5d 100644 --- a/CLI/CMUXCLI+CodexFireAndForgetHooks.swift +++ b/CLI/CMUXCLI+CodexFireAndForgetHooks.swift @@ -271,9 +271,11 @@ extension CMUXCLI { } /// Removes obsolete regular files only when their names prove cmux ownership. - /// Live Codex sessions may still hold paths from another tagged build, and - /// concurrent launches can briefly overlap script generation, so collection - /// waits until no Codex process is running and leaves recent files alone. + /// This runs only during an explicit hook install, never on wrapper launch + /// or automatic reconciliation. Since an older immutable script may still + /// be referenced by a long-lived Codex process, fail closed whenever any + /// Codex process is running; the process probe is intentionally outside the + /// launch path. static func garbageCollectCodexHookScripts(retaining filenames: Set) { guard !hasRunningCodexProcess(), let directory = codexHookScriptsDirectory(), @@ -285,8 +287,9 @@ extension CMUXCLI { return } - let newestRemovableDate = Date().addingTimeInterval(-60) - for url in contents where !filenames.contains(url.lastPathComponent) { + let newestRemovableDate = Date().addingTimeInterval(-24 * 60 * 60) + let removableCandidates = contents.compactMap { url -> (url: URL, date: Date)? in + guard !filenames.contains(url.lastPathComponent) else { return nil } let values = try? url.resourceValues(forKeys: [ .contentModificationDateKey, .isRegularFileKey, @@ -295,13 +298,28 @@ extension CMUXCLI { values?.isRegularFile == true, let modificationDate = values?.contentModificationDate, modificationDate < newestRemovableDate else { - continue + return nil } - try? FileManager.default.removeItem(at: url) + return (url, modificationDate) + } + // Keep cleanup bounded if a damaged or very old installation has + // accumulated an unexpectedly large number of generated files, while + // deleting the oldest batch so later explicit installs make progress. + let boundedCandidates = removableCandidates + .sorted { lhs, rhs in + if lhs.date != rhs.date { return lhs.date < rhs.date } + return lhs.url.lastPathComponent < rhs.url.lastPathComponent + } + .prefix(256) + for url in boundedCandidates { + try? FileManager.default.removeItem(at: url.url) } } - /// Conservatively detects sessions that may still reference an older hook generation. + /// A running Codex process may have loaded an immutable hook path that is + /// absent from the current config. Cleanup is explicit-install-only, so a + /// synchronous fail-closed probe protects that process without adding + /// launch latency or a background polling task. private static func hasRunningCodexProcess() -> Bool { let process = Process() process.executableURL = URL(fileURLWithPath: "/usr/bin/pgrep") @@ -319,7 +337,11 @@ extension CMUXCLI { static func codexFireAndForgetAgentHookShellCommand(_ command: String, for def: AgentHookDef) -> String { let routedArguments = command.hasPrefix("cmux ") ? String(command.dropFirst("cmux ".count)) : command - let runner = "payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( timer=; trap \"kill \\$timer 2>/dev/null || true; wait \\$timer 2>/dev/null || true; exit 0\" HUP INT TERM; sleep 30 & timer=\"$!\"; wait \"$timer\" 2>/dev/null || true; timer=; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; wait \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"" + // Keep timer ownership in one process and stop it through a file + // handshake. A signal-based supervisor can receive TERM before its + // trap/timer assignment is installed, orphaning the timer for a fast + // child; the explicit parent-owned timer is always waited/reaped. + let runner = "payload=\"$1\"; shift; timer_stop=\"$payload.timer-stop\"; timer_done=\"$payload.timer-done\"; kill_timer_tree() { timer_root=\"$1\"; for timer_child in $(/usr/bin/pgrep -P \"$timer_root\" 2>/dev/null || true); do kill_timer_tree \"$timer_child\"; done; kill -KILL \"$timer_root\" 2>/dev/null || true; }; rm -f \"$timer_stop\" \"$timer_done\"; ( sleep 30 & timer=\"$!\"; while [ ! -e \"$timer_stop\" ]; do timer_state=$(/bin/ps -o state= -p \"$timer\" 2>/dev/null | /usr/bin/tr -d \"[:space:]\"); case \"$timer_state\" in \"\"|Z*) wait \"$timer\" 2>/dev/null || true; printf done >\"$timer_done\"; exit 0;; esac; /bin/sleep 0.05; done; kill_timer_tree \"$timer\"; wait \"$timer\" 2>/dev/null || true; exit 0 ) & timer_supervisor=\"$!\"; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( while [ ! -s \"$timer_done\" ] && [ ! -e \"$timer_stop\" ]; do /bin/sleep 0.05; done; if [ -s \"$timer_done\" ]; then kill \"$child\" 2>/dev/null || true; fi ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; : >\"$timer_stop\"; kill \"$watchdog\" 2>/dev/null || true; wait \"$watchdog\" 2>/dev/null || true; wait \"$timer_supervisor\" 2>/dev/null || true; rm -f \"$payload\" \"$timer_stop\" \"$timer_done\"" let noOp = stdinDrainingHookNoOpShellCommand return [ "cmux_cli=\"${CMUX_BUNDLED_CLI_PATH:-}\"", diff --git a/CLI/CMUXCLI+SemanticNotifications.swift b/CLI/CMUXCLI+SemanticNotifications.swift index 7073b88405f9..b939e28fb248 100644 --- a/CLI/CMUXCLI+SemanticNotifications.swift +++ b/CLI/CMUXCLI+SemanticNotifications.swift @@ -12,6 +12,12 @@ extension CMUXCLI { let fields = payload.split(separator: "|", omittingEmptySubsequences: false).map(String.init) guard fields.count >= 3 else { throw CLIError(message: String(localized: "cli.notification.invalidPayload", defaultValue: "Invalid notification payload")) } let meta = fields.count > 3 ? fields[3].split(separator: ";").map(String.init) : [] + if source == "codex", kind == .approvalRequested, + Self.semanticAttentionContext(rawObject, source: source).requestIdentity == nil, + let approval = meta.first(where: { $0.hasPrefix("a=") }), + approval.range(of: "^a=[0-9a-f]{24}\\.[0-9a-f]{24}$", options: .regularExpression) != nil { + return "notify_target_async \(workspaceId) \(surfaceId) \(payload)" + } let category = meta.first { $0.hasPrefix("c=") }.map { String($0.dropFirst(2)) } ?? (kind == .turnCompleted ? "turn-complete" : "other") let notification = AgentJournalNotification(title: fields[0], subtitle: fields[1], body: fields[2], @@ -26,7 +32,7 @@ extension CMUXCLI { kind: AgentJournalEventKind, rawObject: [String: Any]?, notification: AgentJournalNotification, pendingWork: Bool = false, isSubagent: Bool = false ) throws -> String { - var context = Self.semanticAttentionContext(rawObject) + var context = Self.semanticAttentionContext(rawObject, source: source) var notification = notification switch kind { case .errorReported, .messagePublished: notification.category = "other" @@ -47,7 +53,7 @@ extension CMUXCLI { return "agent_journal_append \(String(decoding: data, as: UTF8.self))" } - static func semanticAttentionContext(_ object: [String: Any]?) -> AgentAttentionContext { + static func semanticAttentionContext(_ object: [String: Any]?, source: String? = nil) -> AgentAttentionContext { func identifier(_ keys: [String]) -> String? { for key in keys { if let value = object?[key] as? String, !value.isEmpty { return value } @@ -59,7 +65,8 @@ extension CMUXCLI { return AgentAttentionContext( eventIdentity: identifier(["event_id", "eventId", "message_id", "uuid"]), turnIdentity: identifier(["turn_id", "turnId"]), - requestIdentity: identifier(["tool_use_id", "toolUseId", "toolUseID", "tool_call_id", "toolCallId", "request_id", "requestId"])) + requestIdentity: (source == "codex" ? CodexApprovalNotificationIdentity.nativeRequestID(in: object) : nil) + ?? identifier(["tool_use_id", "toolUseId", "toolUseID", "tool_call_id", "toolCallId", "request_id", "requestId"])) } static func semanticOccurredAtMs(_ object: [String: Any]?) -> Int64? { diff --git a/CLI/FeedEventClassifier.swift b/CLI/FeedEventClassifier.swift index 1789cf94c54c..82aa94c5d10d 100644 --- a/CLI/FeedEventClassifier.swift +++ b/CLI/FeedEventClassifier.swift @@ -36,21 +36,16 @@ struct FeedEventClassification: Equatable { /// A tool COMPLETED for an agent whose approval prompts notify via /// ``notifiesNativeApprovalPrompt`` — execution strictly follows any /// approval, so the prompt resolved (approved by the user or by the - /// agent's own auto-reviewer) and the bridge clears the pane's stale - /// notifications. Only tool COMPLETION qualifies: pre-tool events fire + /// agent's own auto-reviewer) and the bridge resolves the correlated + /// approval notification. Only tool COMPLETION qualifies: pre-tool events fire /// when the agent intends to run a tool, with no ordering guarantee /// against the approval-prompt hook, so clearing there could erase a /// just-raised prompt while the agent is still blocked. /// - /// The clear is deliberately pane-wide and uncorrelated with any single - /// request: notifications carry no request identity anywhere in cmux, - /// and every agent integration clears the same way on progress signals — - /// Claude's `session-start`/`prompt-submit`/`pre-tool-use` hooks, the - /// generic `.approvalResponse` action (Hermes' resolved native - /// approvals), and codex's own `prompt-submit` hook (which also clears - /// deny-without-further-tools residue at the next turn). Pane - /// notifications are attention signals; agent progress in the pane makes - /// them stale as a set. + /// Newer Codex payloads carry an approval/call id; older payloads fall back + /// to a bounded session/turn/tool/input identity. The app-side coordinator + /// marks repeated derived identities ambiguous, so one completion cannot + /// settle multiple identical requests. let clearsNativeApprovalPrompt: Bool } @@ -198,8 +193,8 @@ struct FeedEventClassifier { case .toolEnd: // A completed tool ran, and execution strictly follows any // approval — so this is the earliest progress signal that can - // safely clear a resolved native approval prompt (approved by - // the user or by the agent's own auto-reviewer). Scoped to + // safely resolve its correlated native approval prompt (approved + // by the user or by the agent's own auto-reviewer). Scoped to // sources that raise those prompts so other agents' tool // telemetry never touches the notification queue. return FeedEventClassification( @@ -436,14 +431,14 @@ struct FeedEventClassifier { /// Builds the pane-attention V1 socket command a classified feed event /// carries — the `needs-permission`-gated `notify_target_async` for a - /// native approval prompt, or the pane-scoped `clear_notifications` for - /// a resolved one. Pure so the exact wire command (UUID gating, payload - /// shape, gate meta) is unit-testable; the CLI feed hook sends the + /// native approval prompt, or the correlated `clear_notifications` for a + /// resolved one. Pure so the exact wire command (UUID gating, payload + /// shape, gate/correlation meta) is unit-testable; the CLI feed hook sends the /// returned line request/response and awaits the app's acknowledgement. /// - /// Returns `nil` when the classification carries no attention side - /// effect or when either identity is missing/not a UUID: the command is - /// advisory and must never fail the hook. + /// Returns `nil` when the classification carries no attention side effect, + /// when targets are invalid, or when an exact Codex completion lacks its + /// identity. A legacy prompt may still use the pane-scoped notify form. /// /// The notification body deliberately names only the TOOL — mirroring /// the in-app Feed approval banner (`feed.notification.permission.body`) @@ -457,7 +452,9 @@ struct FeedEventClassifier { workspaceId: String?, surfaceId: String?, agentID: String = "codex", - includeAgentContext: Bool = false + includeAgentContext: Bool = false, + source: String? = nil, + approvalIdentity: CodexApprovalNotificationIdentity? = nil ) -> String? { guard classification.notifiesNativeApprovalPrompt || classification.clearsNativeApprovalPrompt else { return nil } @@ -466,7 +463,28 @@ struct FeedEventClassifier { let surfaceRaw = surfaceId?.trimmingCharacters(in: .whitespacesAndNewlines), let surfaceUUID = UUID(uuidString: surfaceRaw) else { return nil } + // A Codex completion is correlated by an exact identity. Other native + // approval producers retain the historical pane-scoped command shape; + // callers that omit `source` are treated as Codex for compatibility + // with the strict completion path. + let requiresCorrelatedIdentity = source? + .trimmingCharacters(in: .whitespacesAndNewlines) + .lowercased() == "codex" || source == nil + // A hook payload is bounded, but a single tool-name field could still + // consume nearly the entire budget and force large socket/UI copies. + // Keep attention lines small and predictable on the synchronous path. + guard !classification.clearsNativeApprovalPrompt + || !requiresCorrelatedIdentity + || approvalIdentity != nil else { + return nil + } if classification.clearsNativeApprovalPrompt { + if let approvalIdentity { + return "clear_notifications --tab=\(workspaceUUID.uuidString) --panel=\(surfaceUUID.uuidString) \(approvalIdentity.resolutionOptions)" + } + // Legacy non-Codex producers have no exact approval identity and + // historically clear their own pane-scoped prompt. + guard !requiresCorrelatedIdentity else { return nil } return "clear_notifications --tab=\(workspaceUUID.uuidString) --panel=\(surfaceUUID.uuidString)" } let subtitle = String( @@ -487,7 +505,14 @@ struct FeedEventClassifier { ) } let meta: String? - if includeAgentContext { + if let approvalIdentity { + meta = AgentHookNotifyCategory.needsPermission.metaSegment( + pending: false, + approvalID: approvalIdentity.approvalID, + approvalIDIsDerived: !approvalIdentity.isAuthoritative, + approvalSource: "feed" + ) + } else if includeAgentContext { meta = AgentHookNotifyCategory.needsPermission.metaSegment( pending: false, agentID: agentID @@ -507,7 +532,8 @@ struct FeedEventClassifier { /// tool names are payload-controlled input, so normalize them the same /// way `notificationPayload` sanitizes its fields. private static func attentionNotificationField(_ value: String) -> String { - value + let bounded = String(value.prefix(240)) + return bounded .components(separatedBy: .newlines) .joined(separator: " ") .trimmingCharacters(in: .whitespacesAndNewlines) diff --git a/CLI/cmux.swift b/CLI/cmux.swift index 0744df6a44db..3d7ba0b32138 100644 --- a/CLI/cmux.swift +++ b/CLI/cmux.swift @@ -408,7 +408,11 @@ final class ClaudeHookSessionStore { func lookup(sessionId: String, deadline: Date? = nil) throws -> ClaudeHookSessionRecord? { let normalized = normalizeSessionId(sessionId) guard !normalized.isEmpty else { return nil } - return try withLockedState(deadline: deadline, persist: false) { state in + // Feed fallback lookups run for every native Codex approval event. + // They never mutate state, so use a shared lock and avoid the full + // JSON encode/atomic replace that `withLockedState` performs after + // every read. This keeps hook bursts from turning into disk I/O. + return try withSharedState(deadline: deadline) { state in state.sessions[normalized] } } @@ -2567,7 +2571,46 @@ final class ClaudeHookSessionStore { ) } - private func loadUnlocked(deadline: Date? = nil) throws -> ClaudeHookSessionStoreFile { + private func withSharedState( + deadline: Date? = nil, + _ body: (ClaudeHookSessionStoreFile) throws -> T + ) throws -> T { + let lockPath = statePath + ".lock" + try fileManager.createDirectory( + at: URL(fileURLWithPath: lockPath).deletingLastPathComponent(), + withIntermediateDirectories: true, + attributes: [.posixPermissions: NSNumber(value: Int16(0o700))] + ) + let fd = open(lockPath, O_CREAT | O_RDWR, mode_t(S_IRUSR | S_IWUSR)) + if fd < 0 { + throw CLIError(message: "Failed to open Claude hook state lock: \(lockPath)") + } + defer { Darwin.close(fd) } + + if let deadline { + while flock(fd, LOCK_SH | LOCK_NB) != 0 { + guard errno == EWOULDBLOCK || errno == EAGAIN, Date.now < deadline else { + throw CLIError(message: "Timed out locking Claude hook state: \(lockPath)") + } + usleep(5_000) + } + } else if flock(fd, LOCK_SH) != 0 { + throw CLIError(message: "Failed to lock Claude hook state: \(lockPath)") + } + defer { _ = flock(fd, LOCK_UN) } + if let deadline, Date.now >= deadline { + throw CLIError(message: "Claude hook state deadline exceeded: \(lockPath)") + } + var state = try loadUnlocked(deadline: deadline, allowsRecovery: false) + if let deadline, Date.now >= deadline { + throw CLIError(message: "Claude hook state deadline exceeded: \(lockPath)") + } + pruneExpired(&state) + return try body(state) + } + + private func loadUnlocked(deadline: Date? = nil, allowsRecovery: Bool = true) throws -> ClaudeHookSessionStoreFile { + guard fileManager.fileExists(atPath: statePath) else { return ClaudeHookSessionStoreFile() } @@ -2581,10 +2624,12 @@ final class ClaudeHookSessionStore { throw CLIError(message: "Claude hook state file is too large for the hook deadline: \(statePath)") } if fileSize > Self.maxRecoverableHookStateFileBytes { + guard allowsRecovery else { return ClaudeHookSessionStoreFile() } return try quarantineOversizedState(at: stateURL) } let data = try Data(contentsOf: stateURL) guard var decoded = try? decoder.decode(ClaudeHookSessionStoreFile.self, from: data) else { + guard allowsRecovery else { return ClaudeHookSessionStoreFile() } return try quarantineOversizedState(at: stateURL) } if fileSize > Self.maxHookStateFileBytes { @@ -33973,9 +34018,18 @@ export default CMUXSessionRestore; resolvedDirectWorkspaceArg ?? processBinding()?.workspaceId } - let rawInput = rawInputOverride - ?? String(data: FileHandle.standardInput.readDataToEndOfFile(), encoding: .utf8) - ?? "" + let rawInputData: Data = if let rawInputOverride { + Data(rawInputOverride.utf8) + } else if def.name == "codex" { + // Codex hooks are invoked synchronously by the agent. Keep a + // malformed/model-controlled stdin payload from holding the hook + // (and its socket) indefinitely or allocating unbounded memory. + Self.readBoundedFeedHookStdin(maxBytes: Self.genericCodexHookMaxStdinBytes) + ?? Data() + } else { + FileHandle.standardInput.readDataToEndOfFile() + } + let rawInput = String(data: rawInputData, encoding: .utf8) ?? "" let input = parseClaudeHookInput(rawInput: rawInput) let persistedHookEventName = reportedHookEventName(from: input) ?? Self.feedEventName(forClaudeSubcommand: subcommand) @@ -34279,7 +34333,7 @@ export default CMUXSessionRestore; nativeEvent: reportedHookEventName(from: input) ?? subcommand, declaredPhase: declaredPhase, detail: detail, - attention: Self.semanticAttentionContext(input.rawObject), + attention: Self.semanticAttentionContext(input.rawObject, source: def.name), occurredAtMs: Self.semanticOccurredAtMs(input.rawObject), responseTimeout: responseTimeout, deadline: deadline ?? cursorShellDeadline, @@ -36070,6 +36124,21 @@ export default CMUXSessionRestore; responseTimeout: def.name == "cursor" ? cursorCriticalTimeout() : nil ) + // Scope-clearing is intentionally after the freshness checks above. + // A delayed Stop from an older turn must not tombstone a newer + // approval before we know this stop still owns the pane. + if def.name == "codex", + !suppressVisibleMutations, + let approvalScope = CodexApprovalNotificationIdentity.makeScope( + rawObject: input.rawObject ?? input.object, + fallbackSessionID: input.sessionId + ) { + _ = try? sendV1Command( + "clear_notifications --tab=\(workspaceId)\(socketPanelOption(surfaceId)) --approval-scope=\(approvalScope)", + client: client + ) + } + if !sessionId.isEmpty, !suppressVisibleMutations { _ = try? store.upsert(sessionId: sessionId, workspaceId: workspaceId, surfaceId: surfaceId, cwd: cwd, transcriptPath: input.transcriptPath ?? mapped?.transcriptPath, @@ -36560,6 +36629,41 @@ export default CMUXSessionRestore; return } + let approvalIdentity = def.name == "codex" && summary.notifyCategory == .needsPermission + ? CodexApprovalNotificationIdentity.make( + rawObject: input.rawObject ?? input.object, + fallbackSessionID: input.sessionId + ) + : nil + if def.name == "codex", summary.notifyCategory == .needsPermission { + let isAutoReviewed = CodexApprovalNotificationPolicy().isAutoReviewed( + rawObject: input.rawObject ?? input.object ?? [:], + transcriptPath: input.transcriptPath ?? mapped?.transcriptPath, + readRolloutLines: { path, maxBytes in + readRecentTextFileLines(path: path, maxBytes: maxBytes) + } + ) + if isAutoReviewed { +#if DEBUG + agentHookDebugLog( + "agentHook.notification.skip agent=codex session=\(agentHookDebugShort(sessionId)) reason=autoReview", + socketPath: client.socketPath, + env: env + ) +#endif + sendAgentFeedTelemetryUnlessSuppressed( + workspaceId: workspaceId, + surfaceId: surfaceId + ) + print("{}") + return + } + // Older Codex payloads can omit the structured turn/tool + // fields. They still represent a blocked native prompt, so + // keep the legacy pane-scoped notification fallback; only an + // exact completion is allowed to clear by approval ID. + } + if antigravitySuppressDuplicateIdleWhileBackgroundWork { #if DEBUG agentHookDebugLog( @@ -36717,7 +36821,7 @@ export default CMUXSessionRestore; category: summary.notifyCategory, body: summary.body ) - if !summary.body.isEmpty { + if !summary.body.isEmpty || approvalIdentity != nil { // One ancestry walk per delivered notification, feeding the // notify payload's subagent tag below. let notificationEventPID = preferredAgentHookEventPID( @@ -36739,20 +36843,38 @@ export default CMUXSessionRestore; // Completions AND waiting nags are both "pending" while // background work is live, so a fullyIdle=false Antigravity // waiting cue doesn't deliver a false "waiting for input". - // Error status wins over a classifier category so every - // error carries the contextual error sound tag, even if an - // integration supplied an inconsistent category. - let notificationMeta = Self.agentNotificationMeta( - category: summary.notifyCategory, - isError: summary.status == .error, - pending: (summary.notifyCategory == .turnComplete || summary.notifyCategory == .idleReminder) - && hasActiveAntigravityBackgroundWork(), - agentID: def.name, - isSubagent: isNestedAgentSession, - correlationKey: cursorShellNeedsApproval - ? cursorApprovalNotificationCorrelationKey - : nil - ) + let notificationPending = (summary.notifyCategory == .turnComplete || summary.notifyCategory == .idleReminder) + && hasActiveAntigravityBackgroundWork() + let notificationMeta: String? + if let approvalIdentity { + notificationMeta = summary.notifyCategory.metaSegment( + pending: notificationPending, + approvalID: approvalIdentity.approvalID, + approvalIDIsDerived: !approvalIdentity.isAuthoritative, + approvalSource: "hook" + ) + } else if def.name == "codex", summary.notifyCategory == .needsPermission { + // Preserve the pre-correlation Codex wire form when the + // payload cannot provide a stable identity. + notificationMeta = summary.notifyCategory.metaSegment( + pending: notificationPending, + approvalID: nil + ) + } else { + // Error status wins over a classifier category so every + // error carries the contextual error sound tag, even if + // an integration supplied an inconsistent category. + notificationMeta = Self.agentNotificationMeta( + category: summary.notifyCategory, + isError: summary.status == .error, + pending: notificationPending, + agentID: def.name, + isSubagent: isNestedAgentSession, + correlationKey: cursorShellNeedsApproval + ? cursorApprovalNotificationCorrelationKey + : nil + ) + } let payload = notificationPayload( title: notificationTitle(workspaceId: workspaceId, surfaceId: surfaceId), subtitle: summary.subtitle, @@ -36787,7 +36909,9 @@ export default CMUXSessionRestore; env: env ) #endif - markNotificationSent(fingerprint: notificationFingerprint) + if approvalIdentity == nil { + markNotificationSent(fingerprint: notificationFingerprint) + } } catch { #if DEBUG agentHookDebugLog( @@ -36959,7 +37083,26 @@ export default CMUXSessionRestore; } case .noop: - break + if def.name == "codex", subcommand == "post-tool-use" { + let mapped = sessionId.isEmpty ? nil : (try? store.lookup(sessionId: sessionId)) + if let target = resolveAgentHookTarget(mapped: mapped) { + emitJournal(.attentionResolved, workspaceId: target.workspaceId, surfaceId: target.surfaceId, + responseTimeout: Self.codexApprovalResolutionResponseTimeoutSeconds) + guard let approvalIdentity = CodexApprovalNotificationIdentity.make( + rawObject: input.rawObject ?? input.object, fallbackSessionID: input.sessionId + ) else { break } + // This wrapper path runs in a detached fire-and-forget + // worker. Never let a stalled daemon hold one worker for + // the SocketClient's 15-second default; Codex can emit a + // completion for every tool and otherwise accumulate + // blocked child processes. The synchronous feed lane has + // its own longer acknowledgement budget. + _ = try? client.send( + command: "clear_notifications --tab=\(target.workspaceId)\(socketPanelOption(target.surfaceId)) \(approvalIdentity.resolutionOptions)", + responseTimeout: Self.codexApprovalResolutionResponseTimeoutSeconds + ) + } + } } print(def.name == "pi" ? piHookResolvedTargetOutput(strictPiTarget) : hookResponse) @@ -36993,6 +37136,11 @@ export default CMUXSessionRestore; /// acknowledged send) before the feed hook gives up and returns. static let feedAttentionAcknowledgeTimeoutSeconds: TimeInterval = 2 + /// Short response budget for correlated clears emitted by Codex's detached + /// wrapper worker. This path is best-effort and must not accumulate one + /// blocked process per completed tool while the app socket is unhealthy. + static let codexApprovalResolutionResponseTimeoutSeconds: TimeInterval = 0.5 + /// Portion of the attention deadline reserved for the essential /// notify/clear send. The optional live-target probe may never consume /// this: a stalled probe that ate the whole budget would starve the very @@ -37141,15 +37289,11 @@ export default CMUXSessionRestore; guard let data = try? JSONSerialization.data(withJSONObject: frame), let line = String(data: data, encoding: .utf8) else { return } - // Deliberately NO native-approval-prompt clear on this lane: the - // wrapper-injected codex hooks that reach it (`hooks codex - // post-tool-use`) run as fire-and-forget nohup workers with no - // ordering guarantee, so a delayed completion worker's clear could - // erase a NEWER request's live permission notification — silencing a - // blocked agent (#9592). Only the synchronous feed-hook path - // (`runFeedHook`), whose events arrive in codex's own order, emits - // the clear; wrapper-path staleness self-heals at the next - // `prompt-submit`, which already clears the pane. + // Attention commands do not ride this one-way telemetry connection. + // The generic Codex notification and post-tool handlers send their + // correlated stage/resolve commands through the acknowledged V1 lane; + // keeping them separate also prevents a delayed fire-and-forget + // completion worker from clearing a newer request (#9592). sendBestEffortFeedTelemetry(socketPath: client.socketPath, line: line, socketPassword: socketPassword) } @@ -39084,6 +39228,7 @@ export default CMUXSessionRestore; classification: FeedEventClassification, source: String, toolName: String, + approvalIdentity: CodexApprovalNotificationIdentity?, eventDict: [String: Any], env: [String: String], client: SocketClient?, @@ -39136,10 +39281,17 @@ export default CMUXSessionRestore; workspaceId: liveTarget?.workspaceId ?? ambientWorkspaceId, surfaceId: liveTarget?.surfaceId ?? ambientSurfaceId, agentID: source, - includeAgentContext: true + includeAgentContext: true, + source: source, + approvalIdentity: approvalIdentity ) else { return } - let evidence = Self.semanticAttentionContext(eventDict) + let evidence = Self.semanticAttentionContext(eventDict, source: source) if classification.clearsNativeApprovalPrompt { + if source == "codex", approvalIdentity != nil { + _ = try? activeClient.send(command: attentionLine, + responseTimeout: min(Self.codexApprovalResolutionResponseTimeoutSeconds, remainingBudget() / 2), + deadline: deadline) + } guard evidence.requestIdentity != nil, let workspaceID = liveTarget?.workspaceId ?? ambientWorkspaceId, let surfaceID = liveTarget?.surfaceId ?? ambientSurfaceId else { return } @@ -39352,10 +39504,12 @@ export default CMUXSessionRestore; print(compactedFeedOutput) return } - let sessionId = firstString( + let rawSessionId = firstString( in: stdinObj, keys: ["session_id", "sessionId", "conversation_id", "conversationId"] - ) ?? stableFallbackFeedSessionId(source: source, rawObject: stdinObj, agentPid: agentPid) + ) + let sessionId = rawSessionId + ?? stableFallbackFeedSessionId(source: source, rawObject: stdinObj, agentPid: agentPid) guard let workstreamID = Self.feedWorkstreamID( source: source, sessionID: sessionId @@ -39363,6 +39517,27 @@ export default CMUXSessionRestore; print("{}") return } + let rawTranscriptPath = firstString( + in: stdinObj, + keys: ["transcript_path", "transcriptPath"] + ) + let mappedTranscriptPath: String? = { + guard source == "codex", + classification.notifiesNativeApprovalPrompt, + rawTranscriptPath == nil, + let rawSessionId, + let def = Self.agentDef(named: source) else { + return nil + } + let store = ClaudeHookSessionStore(processEnv: env.merging( + ["CMUX_CLAUDE_HOOK_STATE_PATH": agentHookStatePath( + sessionStoreSuffix: def.sessionStoreSuffix, + env: env + )], + uniquingKeysWith: { _, new in new } + )) + return (try? store.lookup(sessionId: rawSessionId))?.transcriptPath + }() var validatedCodexFeedTarget: (workspaceId: String, surfaceId: String)? // Native Codex child events are committed before their telemetry frame @@ -39517,6 +39692,30 @@ export default CMUXSessionRestore; return } } + // Identity derivation canonicalizes the tool input and computes two + // SHA-256 digests. Most feed events are ordinary telemetry and can + // never touch the native approval banner, so avoid that work unless + // this event actually raises or resolves one. + let needsApprovalCorrelation = source == "codex" + && (classification.notifiesNativeApprovalPrompt + || classification.clearsNativeApprovalPrompt) + let approvalIdentity = needsApprovalCorrelation + ? CodexApprovalNotificationIdentity.make( + rawObject: stdinObj, + // Never use the synthetic Feed fallback (which is derived + // from PID/pane context) as an approval identity. + fallbackSessionID: rawSessionId + ) + : nil + let codexApprovalIsAutoReviewed = source == "codex" + && classification.notifiesNativeApprovalPrompt + && CodexApprovalNotificationPolicy().isAutoReviewed( + rawObject: stdinObj, + transcriptPath: rawTranscriptPath ?? mappedTranscriptPath, + readRolloutLines: { path, maxBytes in + readRecentTextFileLines(path: path, maxBytes: maxBytes) + } + ) var eventDict: [String: Any] = [ "session_id": workstreamID, @@ -39579,7 +39778,7 @@ export default CMUXSessionRestore; hookEventName: hookEventName, promptText: promptText ) - let causalEvidence = Self.semanticAttentionContext(stdinObj) + let causalEvidence = Self.semanticAttentionContext(stdinObj, source: source) let requestId = stdinObj["_opencode_request_id"] as? String ?? causalEvidence.requestIdentity.map { "\(workstreamID):\(Data($0.utf8).base64EncodedString())" } ?? UUID().uuidString @@ -39628,21 +39827,21 @@ export default CMUXSessionRestore; // implicit reconnect inside `sendOneWay` is not bounded by the // write timeout. // - // Known accepted residual: codex's fire-and-forget prompt-submit - // worker clears the pane at turn start from a DETACHED process. - // If that worker is slower than the model's first approval-needing - // tool call, its late clear can remove this notification — the - // same pre-existing exposure the wrapper-path notification - // (`hooks codex notification`) has always had. Fencing clears by - // origin time is a cross-layer protocol change deliberately out - // of scope here. - let sendsAttention = classification.notifiesNativeApprovalPrompt + // Prompt and completion are correlated in the app-side settle + // coordinator. A completion that reaches the app first leaves a + // short tombstone, while an old completion can only resolve its + // own request id — never a newer blocking prompt in the pane. + let sendsAttention = ( + classification.notifiesNativeApprovalPrompt + && !codexApprovalIsAutoReviewed + ) || classification.clearsNativeApprovalPrompt if sendsAttention { deliverNativeApprovalPromptAttention( classification: classification, source: source, toolName: toolName, + approvalIdentity: approvalIdentity, eventDict: eventDict, env: env, client: client, @@ -39776,24 +39975,30 @@ export default CMUXSessionRestore; } private static let feedHookMaxStdinBytes = 1 * 1024 * 1024 + /// Generic Codex hooks also read model-controlled JSON from stdin, but + /// unlike the Feed path they historically used read-to-EOF. Keep the + /// synchronous hook bounded to the same one-megabyte safety envelope. + private static let genericCodexHookMaxStdinBytes = 1 * 1024 * 1024 private static let piFeedHookMaxStdinBytes = 128 * 1024 private static func readBoundedFeedHookStdin( maxBytes: Int, handle: FileHandle = .standardInput ) -> Data? { + guard maxBytes > 0 else { return Data() } var data = Data() - while data.count <= maxBytes { - let remainingBytes = maxBytes + 1 - data.count + while data.count < maxBytes { + let remainingBytes = maxBytes - data.count let chunkSize = min(64 * 1024, remainingBytes) let chunk = (try? handle.read(upToCount: chunkSize)) ?? Data() guard !chunk.isEmpty else { return data } data.append(chunk) } - guard data.count <= maxBytes else { - while !((try? handle.read(upToCount: 64 * 1024)) ?? Data()).isEmpty {} - return nil - } + // Stop as soon as the bounded window is full. Do not perform a probe + // read for a possible extra byte: a producer may keep stdin open while + // waiting for its hook consumer, and that probe would reintroduce a + // hang at exactly the limit. A payload that fills the window is parsed + // as usual; truncated/oversized JSON fails closed at the parser. return data } diff --git a/Resources/Localizable.xcstrings b/Resources/Localizable.xcstrings index 28cdc93eb911..1c5bc4fca457 100644 --- a/Resources/Localizable.xcstrings +++ b/Resources/Localizable.xcstrings @@ -44457,13 +44457,13 @@ "en": { "stringUnit": { "state": "translated", - "value": "clear_notifications [--tab=X] [--panel=ID] [--correlation-key=UUID]" + "value": "clear_notifications [--tab=X] [--panel=ID] [--approval-id=ID|--approval-scope=SCOPE|--correlation-key=UUID]" } }, "ja": { "stringUnit": { "state": "translated", - "value": "clear_notifications [--tab=X] [--panel=ID] [--correlation-key=UUID]" + "value": "clear_notifications [--tab=X] [--panel=ID] [--approval-id=ID|--approval-scope=SCOPE|--correlation-key=UUID]" } } } diff --git a/Sources/AgentApprovalCorrelationID.swift b/Sources/AgentApprovalCorrelationID.swift new file mode 100644 index 000000000000..bc944ebb856f --- /dev/null +++ b/Sources/AgentApprovalCorrelationID.swift @@ -0,0 +1,47 @@ +import Foundation +import CryptoKit + +/// A delimiter-safe correlation identifier for one native agent approval. +/// +/// The first digest scopes the request to a session turn; the second digest +/// identifies the tool request within that turn. Both are 96-bit lowercase +/// hexadecimal prefixes generated by the CLI. +struct AgentApprovalCorrelationID: Hashable, RawRepresentable, Sendable { + struct Scope: Hashable, RawRepresentable, Sendable { + let rawValue: String + + init?(rawValue: String) { + guard AgentApprovalCorrelationID.isDigest(rawValue) else { return nil } + self.rawValue = rawValue + } + } + + let rawValue: String + let scope: Scope + + init?(rawValue: String) { + let pieces = rawValue.split(separator: ".", omittingEmptySubsequences: false) + guard pieces.count == 2, + let scope = Scope(rawValue: String(pieces[0])), + Self.isDigest(String(pieces[1])) else { + return nil + } + self.rawValue = rawValue + self.scope = scope + } + + static func journal(sessionID: String, correlationKey: String) -> Self? { + func digest(_ value: String) -> String { + SHA256.hash(data: Data(value.utf8)).prefix(12) + .map { String(format: "%02x", $0) }.joined() + } + return Self(rawValue: "\(digest("codex-journal:\(sessionID)")).\(digest(correlationKey))") + } + + private static func isDigest(_ value: String) -> Bool { + value.utf8.count == 24 + && value.utf8.allSatisfy { byte in + (0x30...0x39).contains(byte) || (0x61...0x66).contains(byte) + } + } +} diff --git a/Sources/AgentApprovalNotificationCoordinator.swift b/Sources/AgentApprovalNotificationCoordinator.swift new file mode 100644 index 000000000000..4259db4aed19 --- /dev/null +++ b/Sources/AgentApprovalNotificationCoordinator.swift @@ -0,0 +1,640 @@ +import Foundation + +/// Settles native-agent approval signals before they become notifications. +/// +/// Codex emits `PermissionRequest` before its own approval reviewer runs, so +/// the request is not proof that the user will ever need to act. This +/// coordinator holds correlated requests through the settle window, cancels +/// requests that resolve before delivery, and keeps at most one delivered +/// notification per pane while any correlated requests remain outstanding. +@MainActor +final class AgentApprovalNotificationCoordinator { + typealias Action = @MainActor @Sendable () -> Void + typealias Cancellation = @MainActor @Sendable () -> Void + typealias Scheduler = @MainActor (TimeInterval, @escaping Action) -> Cancellation + typealias ScheduledActionDispatcher = @MainActor (@escaping Action) -> Void + + struct Delivery: Equatable, Sendable { + let workspaceID: UUID + let surfaceID: UUID + let title: String + let subtitle: String + let body: String + /// Agent context is carried through delayed approval delivery so + /// notification-policy hooks see the same category/pending metadata + /// as they would for an immediate agent notification. + let agent: TerminalNotificationPolicyAgentContext? + let correlationKey: String + /// Optional producer key supplied by the source notification. The + /// coordinator still owns the episode key, while the queue keeps this + /// alias so a producer can clear its exact notification. + let producerCorrelationKey: String? + } + + struct Clear: Equatable, Sendable { + let workspaceID: UUID + let surfaceID: UUID + let correlationKey: String + } + + private struct Candidate { + let workspaceID: UUID + let title: String + let subtitle: String + let body: String + let approvalID: AgentApprovalCorrelationID + let isDerived: Bool + let approvalSources: Set + let agent: TerminalNotificationPolicyAgentContext? + let producerCorrelationKey: String? + let readyAt: TimeInterval + let sequence: UInt64 + } + + private struct PaneState { + var workspaceID: UUID + /// Monotonic activity marker used to evict the least-recently-used + /// pane when stale hook traffic exceeds the global registry bound. + var lastTouchedAt: TimeInterval + var lastTouchedSequence: UInt64 + // Key by the logical approval id so duplicate hook deliveries replace + // one record instead of growing an unbounded sequence-keyed bag. + var candidates: [String: Candidate] = [:] + // A derived tuple identity can represent either a retried hook or two + // distinct identical calls. Keep that ambiguity explicit so one exact + // completion cannot settle more than one logical request. + var ambiguousApprovalIDs: Set = [] + var scheduledID: UUID? + var scheduledAt: TimeInterval? + var cancelScheduled: Cancellation? + var deliveredCorrelationKey: String? + var deliveredProducerCorrelationKey: String? = nil + var deliveredApprovalID: String? = nil + var cancelEpisodeExpiry: Cancellation? + var episodeID: UUID? + } + + private struct ResolutionKey: Hashable { + let surfaceID: UUID + let value: String + } + + private let settleDelay: TimeInterval + private let tombstoneLifetime: TimeInterval + private let episodeLifetime: TimeInterval + private let now: @MainActor () -> TimeInterval + private let schedule: Scheduler + private let dispatchScheduledAction: ScheduledActionDispatcher + private let deliver: @MainActor (Delivery) -> Void + private let clear: @MainActor (Clear) -> Void + private static let maxCandidatesPerPane = 64 + /// A malformed or stale hook can name an arbitrary surface. Keep that + /// untrusted fan-out bounded even when no live owner exists to cancel it. + nonisolated static let maxTrackedPanes = 256 + /// Delivered episodes remain visible until an authoritative resolution or + /// dismissal. This fallback only retires unresolved panes when the + /// scheduler is unavailable. + private static let unresolvedPaneLifetime: TimeInterval = 10 * 60 + private static let maxTombstonesPerKind = 1_024 + nonisolated static let defaultEpisodeLifetime: TimeInterval = .infinity + nonisolated static let approvalCorrelationPrefix = "agent-approval:" + private var panes: [UUID: PaneState] = [:] + private var exactResolutionTombstones: [ResolutionKey: TimeInterval] = [:] + private var scopeResolutionTombstones: [ResolutionKey: TimeInterval] = [:] + private var nextSequence: UInt64 = 0 + + init( + settleDelay: TimeInterval = 0.1, + tombstoneLifetime: TimeInterval = 1, + episodeLifetime: TimeInterval = AgentApprovalNotificationCoordinator.defaultEpisodeLifetime, + now: @escaping @MainActor () -> TimeInterval = { ProcessInfo.processInfo.systemUptime }, + schedule: @escaping Scheduler = AgentApprovalNotificationCoordinator.scheduleOnMainActor(delay:action:), + dispatchScheduledAction: @escaping ScheduledActionDispatcher, + deliver: @escaping @MainActor (Delivery) -> Void, + clear: @escaping @MainActor (Clear) -> Void + ) { + self.settleDelay = settleDelay.isFinite ? max(0, settleDelay) : 0.1 + self.tombstoneLifetime = tombstoneLifetime.isFinite ? max(0, tombstoneLifetime) : 1 + self.episodeLifetime = episodeLifetime.isFinite ? max(0, episodeLifetime) : .infinity + self.now = now + self.schedule = schedule + self.dispatchScheduledAction = dispatchScheduledAction + self.deliver = deliver + self.clear = clear + } + + func stage( + workspaceID: UUID, + surfaceID: UUID, + title: String, + subtitle: String, + body: String, + approvalID: AgentApprovalCorrelationID, + isDerived: Bool = false, + approvalSource: String? = nil, + agent: TerminalNotificationPolicyAgentContext? = nil, + producerCorrelationKey: String? = nil + ) { + let timestamp = now() + pruneTombstones(at: timestamp) + prunePanes(at: timestamp) + let exactKey = ResolutionKey(surfaceID: surfaceID, value: approvalID.rawValue) + guard exactResolutionTombstones[exactKey] == nil else { return } + let scopeKey = ResolutionKey(surfaceID: surfaceID, value: approvalID.scope.rawValue) + guard scopeResolutionTombstones[scopeKey] == nil else { return } + + nextSequence &+= 1 + let candidate = Candidate( + workspaceID: workspaceID, + title: title, + subtitle: subtitle, + body: body, + approvalID: approvalID, + isDerived: isDerived, + approvalSources: Set(approvalSource.flatMap { ["hook", "feed"].contains($0) ? [$0] : nil } ?? []), + agent: agent, + producerCorrelationKey: producerCorrelationKey, + readyAt: timestamp + settleDelay, + sequence: nextSequence + ) + var state = panes[surfaceID] ?? PaneState( + workspaceID: workspaceID, + lastTouchedAt: timestamp, + lastTouchedSequence: nextSequence + ) + state.workspaceID = workspaceID + state.lastTouchedAt = timestamp + state.lastTouchedSequence = nextSequence + if let existing = state.candidates[approvalID.rawValue] { + // Preserve the original deadline/order for a duplicate signal; + // only the latest display text may have changed. If no provider + // discriminator exists, an exact completion is intentionally + // treated as ambiguous and waits for a scope-level resolution. + let duplicateFromDistinctHookPaths = isDerived + && existing.isDerived + && !candidate.approvalSources.isEmpty + && !existing.approvalSources.isEmpty + && candidate.approvalSources.isDisjoint(with: existing.approvalSources) + if (isDerived || existing.isDerived) && !duplicateFromDistinctHookPaths { + state.ambiguousApprovalIDs.insert(approvalID.rawValue) + } + state.candidates[approvalID.rawValue] = Candidate( + workspaceID: candidate.workspaceID, + title: candidate.title, + subtitle: candidate.subtitle, + body: candidate.body, + approvalID: candidate.approvalID, + isDerived: candidate.isDerived, + approvalSources: existing.approvalSources.union(candidate.approvalSources), + agent: candidate.agent, + producerCorrelationKey: candidate.producerCorrelationKey, + readyAt: min(existing.readyAt, candidate.readyAt), + sequence: existing.sequence + ) + } else { + state.candidates[approvalID.rawValue] = candidate + } + if state.candidates.count > Self.maxCandidatesPerPane { + let overflow = state.candidates.count - Self.maxCandidatesPerPane + let staleIDs = state.candidates.values + .sorted { $0.sequence < $1.sequence } + // Never evict the candidate represented by the visible + // banner. It remains in `candidates` until its exact or scope + // resolution arrives, so dropping it here would make later + // unrelated resolutions clear the wrong episode. + .filter { $0.approvalID.rawValue != state.deliveredApprovalID } + .prefix(overflow) + .map { $0.approvalID.rawValue } + for staleID in staleIDs { + state.candidates.removeValue(forKey: staleID) + state.ambiguousApprovalIDs.remove(staleID) + } + } + panes[surfaceID] = state + prunePanes(at: timestamp) + + // Once a pane has one visible approval notification, additional + // requests join that pending episode without producing more banners. + guard state.deliveredCorrelationKey == nil else { return } + scheduleNextFlush(surfaceID: surfaceID, timestamp: timestamp) + } + + func resolve(surfaceID: UUID, approvalID: AgentApprovalCorrelationID, fallbackApprovalID: AgentApprovalCorrelationID? = nil) { + let timestamp = now() + pruneTombstones(at: timestamp) + prunePanes(at: timestamp) + let exactKey = ResolutionKey(surfaceID: surfaceID, value: approvalID.rawValue) + let alreadyResolved = exactResolutionTombstones[exactKey] != nil + exactResolutionTombstones[ + exactKey + ] = timestamp + tombstoneLifetime + if !alreadyResolved, panes[surfaceID]?.candidates[approvalID.rawValue] == nil, + let fallbackApprovalID, fallbackApprovalID != approvalID, + fallbackApprovalID.scope == approvalID.scope, + panes[surfaceID]?.candidates[fallbackApprovalID.rawValue]?.isDerived != false { + resolve(surfaceID: surfaceID, approvalID: fallbackApprovalID) + return + } + guard var state = panes[surfaceID] else { return } + if state.ambiguousApprovalIDs.contains(approvalID.rawValue) { + // The provider did not give us enough information to know which + // identical request completed. Leave the candidate visible until + // the authoritative turn/scope resolution arrives. + panes[surfaceID] = state + return + } + guard state.candidates.removeValue(forKey: approvalID.rawValue) != nil else { return } + finishResolution(surfaceID: surfaceID, state: &state, timestamp: timestamp) + } + + func resolve(surfaceID: UUID, approvalScope: AgentApprovalCorrelationID.Scope) { + let timestamp = now() + pruneTombstones(at: timestamp) + prunePanes(at: timestamp) + scopeResolutionTombstones[ + ResolutionKey(surfaceID: surfaceID, value: approvalScope.rawValue) + ] = timestamp + tombstoneLifetime + guard var state = panes[surfaceID] else { return } + state.candidates = state.candidates.filter { + $0.value.approvalID.scope != approvalScope + } + state.ambiguousApprovalIDs = state.ambiguousApprovalIDs.filter { + state.candidates[$0] != nil + } + finishResolution(surfaceID: surfaceID, state: &state, timestamp: timestamp) + } + + func cancel(surfaceID: UUID, clearDelivered: Bool = true) { + guard let state = panes.removeValue(forKey: surfaceID) else { return } + state.cancelScheduled?() + state.cancelEpisodeExpiry?() + if clearDelivered, let correlationKey = state.deliveredCorrelationKey { + clear(Clear( + workspaceID: state.workspaceID, + surfaceID: surfaceID, + correlationKey: correlationKey + )) + } + } + + func cancel(workspaceID: UUID, clearDelivered: Bool = true) { + cancelPanes(clearDelivered: clearDelivered) { claimedWorkspaceID, _ in + claimedWorkspaceID == workspaceID + } + } + + func cancelPanes( + clearDelivered: Bool = true, + where shouldCancel: (_ claimedWorkspaceID: UUID, _ surfaceID: UUID) -> Bool + ) { + let surfaceIDs = panes.compactMap { surfaceID, state in + shouldCancel(state.workspaceID, surfaceID) ? surfaceID : nil + } + for surfaceID in surfaceIDs { + cancel(surfaceID: surfaceID, clearDelivered: clearDelivered) + } + } + + func cancelAll(clearDelivered: Bool = true) { + for surfaceID in Array(panes.keys) { + cancel(surfaceID: surfaceID, clearDelivered: clearDelivered) + } + } + + func hasEpisode(surfaceID: UUID) -> Bool { + panes[surfaceID] != nil + } + + /// Keep a live approval episode attached to the surface's current owner. + /// Candidates already carry their enqueue-time owner, so update them as a + /// unit; otherwise a later resolution would clear the source workspace + /// after the pane moved. + func rebind(surfaceID: UUID, toWorkspaceID workspaceID: UUID) { + guard var state = panes[surfaceID], state.workspaceID != workspaceID else { return } + state.workspaceID = workspaceID + state.lastTouchedAt = now() + nextSequence &+= 1 + state.lastTouchedSequence = nextSequence + state.candidates = state.candidates.mapValues { candidate in + Candidate( + workspaceID: workspaceID, + title: candidate.title, + subtitle: candidate.subtitle, + body: candidate.body, + approvalID: candidate.approvalID, + isDerived: candidate.isDerived, + approvalSources: candidate.approvalSources, + agent: candidate.agent, + producerCorrelationKey: candidate.producerCorrelationKey, + readyAt: candidate.readyAt, + sequence: candidate.sequence + ) + } + panes[surfaceID] = state + } + + /// Removes a delivered approval episode after the user (or another + /// notification path) dismissed its banner. This intentionally does not + /// emit a second clear: the store has already removed the row. Pending + /// candidates are discarded too, so a late staged hook cannot resurrect a + /// banner the user explicitly dismissed. + @discardableResult + func dismissDelivered(correlationKey: String) -> UUID? { + guard let match = panes.first(where: { $0.value.deliveredCorrelationKey == correlationKey }) else { + return nil + } + let surfaceID = match.key + let timestamp = now() + pruneTombstones(at: timestamp) + let state = panes.removeValue(forKey: surfaceID) + state?.cancelScheduled?() + state?.cancelEpisodeExpiry?() + if let state { + // A dismissed banner must not be recreated by a delayed duplicate + // hook. Fence every approval that was part of the dismissed episode + // for the same bounded tombstone window. + let expiry = timestamp + tombstoneLifetime + for candidate in state.candidates.values { + exactResolutionTombstones[ + ResolutionKey(surfaceID: surfaceID, value: candidate.approvalID.rawValue) + ] = expiry + } + } + return surfaceID + } + + private func finishResolution( + surfaceID: UUID, + state: inout PaneState, + timestamp: TimeInterval + ) { + guard !state.candidates.isEmpty else { + state.cancelScheduled?() + state.cancelEpisodeExpiry?() + panes.removeValue(forKey: surfaceID) + if let correlationKey = state.deliveredCorrelationKey { + clear(Clear( + workspaceID: state.workspaceID, + surfaceID: surfaceID, + correlationKey: correlationKey + )) + } + return + } + + if let latest = state.candidates.values.max(by: { $0.sequence < $1.sequence }) { + state.workspaceID = latest.workspaceID + } + let displayedApprovalResolved = state.deliveredCorrelationKey != nil + && state.deliveredApprovalID.map { state.candidates[$0] == nil } == true + let replacementClear: Clear? + if displayedApprovalResolved, let deliveredCorrelationKey = state.deliveredCorrelationKey { + replacementClear = Clear( + workspaceID: state.workspaceID, + surfaceID: surfaceID, + correlationKey: deliveredCorrelationKey + ) + state.deliveredCorrelationKey = nil + state.deliveredProducerCorrelationKey = nil + state.deliveredApprovalID = nil + state.cancelEpisodeExpiry?() + state.cancelEpisodeExpiry = nil + state.episodeID = nil + } else { + replacementClear = nil + } + panes[surfaceID] = state + if let replacementClear { + clear(replacementClear) + scheduleNextFlush( + surfaceID: surfaceID, + timestamp: timestamp, + replacingExistingSchedule: true + ) + } else if state.deliveredCorrelationKey == nil { + scheduleNextFlush( + surfaceID: surfaceID, + timestamp: timestamp, + replacingExistingSchedule: true + ) + } + } + + private func scheduleNextFlush( + surfaceID: UUID, + timestamp: TimeInterval, + replacingExistingSchedule: Bool = false + ) { + guard var state = panes[surfaceID], + state.deliveredCorrelationKey == nil, + let deadline = state.candidates.values.map(\.readyAt).min() else { + return + } + if !replacingExistingSchedule, + let scheduledAt = state.scheduledAt, + scheduledAt <= deadline { + return + } + + state.cancelScheduled?() + let scheduledID = UUID() + state.scheduledID = scheduledID + state.scheduledAt = deadline + state.cancelScheduled = nil + panes[surfaceID] = state + + let cancellation = schedule(max(0, deadline - timestamp)) { [weak self] in + guard let self else { return } + self.dispatchScheduledAction { [weak self] in + self?.flush(surfaceID: surfaceID, scheduledID: scheduledID) + } + } + guard var current = panes[surfaceID], current.scheduledID == scheduledID else { + cancellation() + return + } + current.cancelScheduled = cancellation + panes[surfaceID] = current + } + + private func flush(surfaceID: UUID, scheduledID: UUID) { + guard var state = panes[surfaceID], + state.scheduledID == scheduledID, + state.deliveredCorrelationKey == nil else { + return + } + state.scheduledID = nil + state.scheduledAt = nil + state.cancelScheduled = nil + panes[surfaceID] = state + + let timestamp = now() + guard let candidate = state.candidates.values + .filter({ $0.readyAt <= timestamp }) + .max(by: { $0.sequence < $1.sequence }) else { + scheduleNextFlush(surfaceID: surfaceID, timestamp: timestamp) + return + } + + let correlationKey = Self.approvalCorrelationPrefix + UUID().uuidString + state.workspaceID = candidate.workspaceID + state.deliveredCorrelationKey = correlationKey + state.deliveredProducerCorrelationKey = candidate.producerCorrelationKey + state.deliveredApprovalID = candidate.approvalID.rawValue + state.cancelEpisodeExpiry?() + let episodeID = UUID() + state.episodeID = episodeID + let expiryCancellation: Cancellation? + if episodeLifetime.isFinite, episodeLifetime > 0 { + expiryCancellation = schedule(episodeLifetime) { [weak self] in + guard let self else { return } + self.dispatchScheduledAction { [weak self] in + self?.expireEpisode(surfaceID: surfaceID, correlationKey: correlationKey, episodeID: episodeID) + } + } + } else { + expiryCancellation = nil + } + // Store the cancellation only if this delivery is still current. + // `deliver` may synchronously enqueue a clear on another lane. + state.cancelEpisodeExpiry = expiryCancellation + panes[surfaceID] = state + deliver(Delivery( + workspaceID: candidate.workspaceID, + surfaceID: surfaceID, + title: candidate.title, + subtitle: candidate.subtitle, + body: candidate.body, + agent: candidate.agent, + correlationKey: correlationKey, + producerCorrelationKey: candidate.producerCorrelationKey + )) + } + + private func pruneTombstones(at timestamp: TimeInterval) { + exactResolutionTombstones = exactResolutionTombstones.filter { $0.value > timestamp } + scopeResolutionTombstones = scopeResolutionTombstones.filter { $0.value > timestamp } + if exactResolutionTombstones.count > Self.maxTombstonesPerKind { + exactResolutionTombstones = Dictionary( + uniqueKeysWithValues: exactResolutionTombstones + .sorted { $0.value > $1.value } + .prefix(Self.maxTombstonesPerKind) + .map { ($0.key, $0.value) } + ) + } + if scopeResolutionTombstones.count > Self.maxTombstonesPerKind { + scopeResolutionTombstones = Dictionary( + uniqueKeysWithValues: scopeResolutionTombstones + .sorted { $0.value > $1.value } + .prefix(Self.maxTombstonesPerKind) + .map { ($0.key, $0.value) } + ) + } + } + + /// Retires stale or least-recently-used pane episodes before untrusted hook + /// traffic can grow the registry without limit. Evicted delivered episodes + /// are explicitly cleared so the persisted notification cannot outlive its + /// in-memory owner. + private func prunePanes(at timestamp: TimeInterval) { + if timestamp.isFinite { + let staleIDs = panes.compactMap { surfaceID, state -> UUID? in + guard state.deliveredCorrelationKey == nil, + state.lastTouchedAt.isFinite, + timestamp - state.lastTouchedAt >= Self.unresolvedPaneLifetime else { + return nil + } + return surfaceID + } + for surfaceID in staleIDs { + cancel(surfaceID: surfaceID, clearDelivered: true) + } + } + + let overflow = panes.count - Self.maxTrackedPanes + guard overflow > 0 else { return } + let evictedIDs = panes + .sorted { lhs, rhs in + if lhs.value.lastTouchedSequence != rhs.value.lastTouchedSequence { + return lhs.value.lastTouchedSequence < rhs.value.lastTouchedSequence + } + return lhs.key.uuidString < rhs.key.uuidString + } + .prefix(overflow) + .map(\.key) + for surfaceID in evictedIDs { + cancel(surfaceID: surfaceID, clearDelivered: true) + } + } + + private func expireEpisode( + surfaceID: UUID, + correlationKey: String, + episodeID: UUID + ) { + guard var state = panes[surfaceID], + state.deliveredCorrelationKey == correlationKey, + state.episodeID == episodeID else { return } + let timestamp = now() + state.cancelEpisodeExpiry = nil + state.episodeID = nil + state.cancelScheduled?() + state.cancelScheduled = nil + guard !state.candidates.isEmpty else { + panes.removeValue(forKey: surfaceID) + clear(Clear( + workspaceID: state.workspaceID, + surfaceID: surfaceID, + correlationKey: correlationKey + )) + return + } + + // A finite episode lifetime is a recovery/re-notification deadline, + // not permission to hide an approval that still awaits the user. Retire + // the old persisted row, reset the episode, and deliver the outstanding + // candidate again with a fresh correlation key. + let previousClear = Clear( + workspaceID: state.workspaceID, + surfaceID: surfaceID, + correlationKey: correlationKey + ) + state.deliveredCorrelationKey = nil + state.deliveredProducerCorrelationKey = nil + state.lastTouchedAt = timestamp + nextSequence &+= 1 + state.lastTouchedSequence = nextSequence + panes[surfaceID] = state + clear(previousClear) + scheduleNextFlush( + surfaceID: surfaceID, + timestamp: timestamp, + replacingExistingSchedule: true + ) + } + + nonisolated static func isApprovalCorrelationKey(_ value: String?) -> Bool { + value?.hasPrefix(approvalCorrelationPrefix) == true + } + + private static func scheduleOnMainActor( + delay: TimeInterval, + action: @escaping Action + ) -> Cancellation { + let boundedDelay = delay.isFinite ? max(0, delay) : 0 + // This is a genuine one-shot presentation deadline, not a polling + // sleep. A main-run-loop timer keeps cancellation explicit and lets the + // coordinator remain entirely on MainActor; tests inject a virtual + // scheduler instead of waiting on wall-clock time. + let timer = Timer(timeInterval: boundedDelay, repeats: false) { _ in + // The timer is registered on `RunLoop.main`, so its callback is + // guaranteed to execute on MainActor. Tell Swift's isolation + // checker about that Foundation callback boundary synchronously. + MainActor.assumeIsolated { + action() + } + } + RunLoop.main.add(timer, forMode: .common) + return { timer.invalidate() } + } +} diff --git a/Sources/AgentJournalLifecycleCenter+Notifications.swift b/Sources/AgentJournalLifecycleCenter+Notifications.swift index aef6a3ff2b3b..e67799b63536 100644 --- a/Sources/AgentJournalLifecycleCenter+Notifications.swift +++ b/Sources/AgentJournalLifecycleCenter+Notifications.swift @@ -24,8 +24,17 @@ extension AgentJournalLifecycleCenter { guard let workspaceID = event.draft.workspaceId.flatMap(UUID.init(uuidString:)), let surfaceID = event.draft.surfaceId.flatMap(UUID.init(uuidString:)) else { return } for key in decision.invalidatedCorrelationKeys { - TerminalMutationBus.shared.enqueueClearNotifications(forTabId: workspaceID, - surfaceId: surfaceID, correlationKey: key) + let bus = TerminalMutationBus.shared + if event.draft.source == "codex", let sessionID = event.draft.sessionId, + let approvalID = AgentApprovalCorrelationID.journal(sessionID: sessionID, correlationKey: key) { + bus.enqueueAgentApprovalResolution(surfaceId: surfaceID, approvalID: approvalID) + bus.enqueueMainActorMutation { + guard bus.resolvedApprovalCorrelationKey(surfaceID: surfaceID, producerCorrelationKey: key) == nil else { return } + bus.enqueueClearNotifications(forTabId: workspaceID, surfaceId: surfaceID, correlationKey: key) + } + } else { + bus.enqueueClearNotifications(forTabId: workspaceID, surfaceId: surfaceID, correlationKey: key) + } if let sessionID = event.draft.sessionId { FeedCoordinator.shared.invalidateSemanticRequest(requestId: key, source: event.draft.source, sessionId: sessionID) } @@ -92,12 +101,17 @@ extension AgentJournalLifecycleCenter { let category = AgentNotifyCategory(rawValue: notification.category) let alert: NotificationSoundAlertType? = draft.kind == .errorReported ? .errorStalled : category?.soundAlertType let sound = alert.flatMap { NotificationSoundOverrideContext(agentID: draft.source, alertType: $0) } + let correlationKey = notification.correlationKey ?? identity + let approvalID = draft.source == "codex" && draft.kind == .approvalRequested + ? draft.sessionId.flatMap { AgentApprovalCorrelationID.journal(sessionID: $0, correlationKey: correlationKey) } + : nil let delivered = delivery.enqueue( workspaceID: live.tabId, surfaceID: liveSurfaceID, title: notification.title, subtitle: notification.subtitle, body: notification.body, category: category, pending: draft.pendingWork, soundContext: sound, + approvalID: approvalID, agentKind: draft.source, isSubagent: draft.isSubagent, - correlationKey: notification.correlationKey ?? identity, + correlationKey: correlationKey, sessionId: draft.sessionId, coalesces: false ) diff --git a/Sources/AgentNotificationDelivery.swift b/Sources/AgentNotificationDelivery.swift index 03a79b83ab79..fee7f2454599 100644 --- a/Sources/AgentNotificationDelivery.swift +++ b/Sources/AgentNotificationDelivery.swift @@ -18,8 +18,8 @@ struct AgentNotificationDelivery: Sendable { } /// Gates and enqueues the same notification event for hooks and PTY prompt detectors. - /// `agentKind`/`isSubagent` are informational agent-event context forwarded - /// to the user's notification-policy hooks; they never affect the gate. + /// Approval correlation is used only for needs-permission events; agent + /// context remains available for all other categories. @discardableResult func enqueue( workspaceID: UUID, @@ -30,6 +30,9 @@ struct AgentNotificationDelivery: Sendable { category: AgentNotifyCategory?, pending: Bool, soundContext: NotificationSoundOverrideContext? = nil, + approvalID: AgentApprovalCorrelationID? = nil, + approvalIDIsDerived: Bool = false, + approvalSource: String? = nil, agentKind: String? = nil, isSubagent: Bool? = nil, correlationKey: String? = nil, @@ -37,6 +40,27 @@ struct AgentNotificationDelivery: Sendable { coalesces: Bool = false ) -> Bool { guard allows(category: category, pending: pending) else { return false } + if category == .needsPermission, let approvalID { + TerminalMutationBus.shared.enqueueAgentApprovalNotification( + tabId: workspaceID, + surfaceId: surfaceID, + title: title, + subtitle: subtitle, + body: body, + approvalID: approvalID, + approvalIDIsDerived: approvalIDIsDerived, + approvalSource: approvalSource, + agent: Self.agentContext( + category: category, + pending: pending, + agentKind: agentKind, + isSubagent: isSubagent, + sessionId: sessionId + ), + producerCorrelationKey: correlationKey + ) + return true + } TerminalMutationBus.shared.enqueueNotification( tabId: workspaceID, surfaceId: surfaceID, diff --git a/Sources/AgentNotificationGate.swift b/Sources/AgentNotificationGate.swift index 80a2f55c4fc2..255de9b6274b 100644 --- a/Sources/AgentNotificationGate.swift +++ b/Sources/AgentNotificationGate.swift @@ -26,20 +26,24 @@ enum AgentTurnCompleteMode: String { case never } -/// Parsed `c=;p=<0|1>[;a=][;n=<0|1>][;s=][;k=]` meta segment. +/// Parsed `c=;p=<0|1>[;a=][;d=1][;o=][;n=<0|1>][;s=][;k=]` meta segment. /// Returns `nil` unless BOTH a KNOWN category literal and a valid `p=0|1` /// pending flag are present, so the reserved suffix grammar stays exactly the /// three known categories — any other `c=...` tail stays part of the legacy -/// notification body. (`.other` never rides the wire: senders omit the meta -/// entirely for ungated alerts.) +/// notification body. (`.other` only rides the wire when it carries an +/// explicit error sound context.) /// /// The optional trailing fields carry agent-event context for the user's -/// notification-policy hooks: `a=` is the case-preserving registry identifier -/// (`claude`, `codex`, `MyAgent`, …) and `n=` marks a nested subagent session. -/// Pre-extension senders emit only `c=;p=` and parse exactly as before. +/// notification-policy hooks: `a=` is either the case-preserving registry +/// identifier or a correlated approval id, `d=1` marks a derived approval id, +/// and `n=` marks a nested subagent session. Pre-extension senders emit only +/// `c=;p=` and parse exactly as before. struct AgentNotificationMeta { let category: AgentNotifyCategory let pending: Bool + let approvalID: AgentApprovalCorrelationID? + let approvalIDIsDerived: Bool + let approvalSource: String? let agentKind: String? let isSubagent: Bool? let soundContext: NotificationSoundOverrideContext? @@ -49,31 +53,59 @@ struct AgentNotificationMeta { init?(meta: String) { // Accept ONLY the canonical serialization the CLI emits (`c=` then - // `p=`, optionally followed by `a=`, `n=`, `s=`, then `k=`, this order, - // no duplicates or extras). Anything else — reordered, duplicated, or - // unknown trailing fields — is not metadata and stays part of the - // legacy notification body. + // `p=`, optionally followed by `a=`, `d=`, `o=`, `n=`, `s=`, then `k=`, this + // order, no duplicates or extras). Anything else — reordered, + // duplicated, or unknown trailing fields — is not metadata and stays + // part of the legacy notification body. let fields = meta.split(separator: ";", omittingEmptySubsequences: false) - guard (2...6).contains(fields.count), + guard (2...8).contains(fields.count), fields[0].hasPrefix("c="), fields[1].hasPrefix("p=") else { return nil } guard let known = AgentNotifyCategory(rawValue: String(fields[0].dropFirst(2))) else { return nil } + let pending: Bool switch fields[1].dropFirst(2) { - case "1": self.pending = true - case "0": self.pending = false + case "1": pending = true + case "0": pending = false default: return nil } var agentKind: String? = nil var isSubagent: Bool? = nil var soundContext: NotificationSoundOverrideContext? = nil var correlationKey: String? = nil + var approvalID: AgentApprovalCorrelationID? = nil + var approvalIDIsDerived = false var index = 2 if index < fields.count, fields[index].hasPrefix("a=") { - let kind = String(fields[index].dropFirst(2)) - guard Self.isValidAgentKindTag(kind) else { return nil } - agentKind = kind + let value = String(fields[index].dropFirst(2)) + if let parsedApprovalID = AgentApprovalCorrelationID(rawValue: value) { + guard known == .needsPermission else { return nil } + approvalID = parsedApprovalID + } else { + // A value shaped like an approval id but failing its strict + // lowercase-hex grammar must not be reinterpreted as an agent + // slug. That would turn malformed correlated metadata into a + // seemingly valid notification and defeat exact clearing. + guard !Self.looksLikeApprovalID(value), Self.isValidAgentKindTag(value) else { + return nil + } + agentKind = value + } + index += 1 + } + if index < fields.count, fields[index].hasPrefix("d=") { + guard approvalID != nil, fields[index].dropFirst(2) == "1" else { return nil } + approvalIDIsDerived = true + index += 1 + } + var approvalSource: String? = nil + if index < fields.count, fields[index].hasPrefix("o=") { + let value = String(fields[index].dropFirst(2)) + guard approvalID != nil, + approvalIDIsDerived, + Self.isValidApprovalSource(value) else { return nil } + approvalSource = value index += 1 } if index < fields.count, fields[index].hasPrefix("n=") { @@ -108,12 +140,20 @@ struct AgentNotificationMeta { guard index == fields.count else { return nil } guard known != .other || soundContext != nil else { return nil } self.category = known + self.pending = pending + self.approvalID = approvalID + self.approvalIDIsDerived = approvalIDIsDerived + self.approvalSource = approvalSource self.agentKind = agentKind self.isSubagent = isSubagent self.soundContext = soundContext self.correlationKey = correlationKey } + private static func isValidApprovalSource(_ value: String) -> Bool { + value == "hook" || value == "feed" + } + /// Mirror of the CLI's `AgentHookNotifyCategory.isValidAgentKindTag` slug /// grammar: 1-64 ASCII characters of `[A-Za-z0-9._-]`, excluding `.` and /// `..`. Both sides must agree exactly or the meta folds back into the body. @@ -124,6 +164,11 @@ struct AgentNotificationMeta { static func isValidCorrelationKey(_ value: String) -> Bool { UUID(uuidString: value) != nil } + + private static func looksLikeApprovalID(_ value: String) -> Bool { + let pieces = value.split(separator: ".", omittingEmptySubsequences: false) + return pieces.count == 2 && pieces.allSatisfy { $0.utf8.count == 24 } + } } /// Pure delivery decision for agent-tagged notifications. Kept free of any I/O diff --git a/Sources/TerminalController.swift b/Sources/TerminalController.swift index 6dd1505bfd88..ddebd2ea1fed 100644 --- a/Sources/TerminalController.swift +++ b/Sources/TerminalController.swift @@ -12184,7 +12184,7 @@ class TerminalController { notify_target - Notify by workspace+surface notify_target_async - Queue notification by workspace+surface list_notifications - List all notifications - clear_notifications [--tab=X] [--panel=ID] - Clear notifications (all, per-tab, or per-panel) + clear_notifications [--tab=X] [--panel=ID] [--approval-id=ID|--approval-scope=SCOPE|--correlation-key=UUID] - Clear notifications (all, per-tab, per-panel, or one correlated approval) set_app_focus - Override app focus state simulate_app_active - Trigger app active handler set_status [--icon=X] [--color=#hex] [--url=X] [--priority=N] [--format=plain|markdown] [--tab=X] - Set a status entry @@ -13510,6 +13510,9 @@ class TerminalController { category: meta?.category, pending: meta?.pending ?? false, soundContext: meta?.soundContext, + approvalID: meta?.approvalID, + approvalIDIsDerived: meta?.approvalIDIsDerived ?? false, + approvalSource: meta?.approvalSource, agentKind: meta?.agentKind, isSubagent: meta?.isSubagent, correlationKey: meta?.correlationKey @@ -13570,26 +13573,51 @@ class TerminalController { return "OK" } let parsed = parseOptions(trimmed) + let usage = String( + localized: "cli.error.clearNotificationsUsage", + defaultValue: "clear_notifications [--tab=X] [--panel=ID] [--approval-id=ID|--approval-scope=SCOPE|--correlation-key=UUID]" + ) guard let tabOption = parsed.options["tab"], !tabOption.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { - let usage = String( - localized: "cli.error.clearNotificationsUsage", - defaultValue: "clear_notifications [--tab=X] [--panel=ID] [--correlation-key=UUID]" - ) return "ERROR: Usage: \(usage)" } let targetResolution = parseSidebarMutationTabTarget(options: parsed.options) guard let target = targetResolution.target else { return targetResolution.error ?? "ERROR: Tab not found" } - let usage = String( - localized: "cli.error.clearNotificationsUsage", - defaultValue: "clear_notifications [--tab=X] [--panel=ID] [--correlation-key=UUID]" - ) let panelResolution = parseOptionalPanelIdOption(options: parsed.options, usage: usage) if let error = panelResolution.error { return error } + let approvalID: AgentApprovalCorrelationID? + if let rawApprovalID = parsed.options["approval-id"] { + guard let parsedID = AgentApprovalCorrelationID(rawValue: rawApprovalID) else { + return "ERROR: Usage: \(usage)" + } + approvalID = parsedID + } else { + approvalID = nil + } + let fallbackApprovalID: AgentApprovalCorrelationID? + if let rawFallbackID = parsed.options["approval-fallback-id"] { + guard let approvalID, + let parsedID = AgentApprovalCorrelationID(rawValue: rawFallbackID), + parsedID.scope == approvalID.scope else { + return "ERROR: Usage: \(usage)" + } + fallbackApprovalID = parsedID + } else { + fallbackApprovalID = nil + } + let approvalScope: AgentApprovalCorrelationID.Scope? + if let rawApprovalScope = parsed.options["approval-scope"] { + guard let parsedScope = AgentApprovalCorrelationID.Scope(rawValue: rawApprovalScope) else { + return "ERROR: Usage: \(usage)" + } + approvalScope = parsedScope + } else { + approvalScope = nil + } let correlationKey: String? if let rawCorrelationKey = parsed.options["correlation-key"] { let normalized = rawCorrelationKey.trimmingCharacters(in: .whitespacesAndNewlines) @@ -13600,18 +13628,29 @@ class TerminalController { ) } correlationKey = uuid.uuidString.lowercased() - guard panelResolution.panelId != nil else { - return "ERROR: " + String( - localized: "cli.error.clearNotificationsCorrelationKeyRequiresPanel", - defaultValue: "--correlation-key requires --panel" - ) - } } else { correlationKey = nil } + guard [approvalID != nil, approvalScope != nil, correlationKey != nil].filter({ $0 }).count <= 1 else { + return "ERROR: Usage: \(usage)" + } + if (approvalID != nil || approvalScope != nil || correlationKey != nil), panelResolution.panelId == nil { + return "ERROR: Usage: \(usage)" + } if case .workspace(let tabId) = target { if let panelId = panelResolution.panelId { - if let correlationKey { + if let approvalID { + TerminalMutationBus.shared.enqueueAgentApprovalResolution( + surfaceId: panelId, + approvalID: approvalID, + fallbackApprovalID: fallbackApprovalID + ) + } else if let approvalScope { + TerminalMutationBus.shared.enqueueAgentApprovalResolution( + surfaceId: panelId, + approvalScope: approvalScope + ) + } else if let correlationKey { TerminalMutationBus.shared.enqueueClearNotifications( forTabId: tabId, surfaceId: panelId, @@ -13624,6 +13663,26 @@ class TerminalController { TerminalMutationBus.shared.enqueueClearNotifications(forTabId: tabId) } } else { + if let panelId = panelResolution.panelId, + (approvalID != nil || approvalScope != nil) { + TerminalMutationBus.shared.enqueueMainActorMutation { [weak self] in + guard let self, let tab = self.resolveSidebarMutationTab(target), + tab.panels.keys.contains(panelId) else { return } + if let approvalID { + TerminalMutationBus.shared.enqueueAgentApprovalResolution( + surfaceId: panelId, + approvalID: approvalID, + fallbackApprovalID: fallbackApprovalID + ) + } else if let approvalScope { + TerminalMutationBus.shared.enqueueAgentApprovalResolution( + surfaceId: panelId, + approvalScope: approvalScope + ) + } + } + return "OK" + } let clearBoundary = TerminalMutationBus.shared.markNotificationClearBoundary() TerminalMutationBus.shared.enqueueMainActorMutation { [weak self] in guard let self, let tab = self.resolveSidebarMutationTab(target) else { return } @@ -13650,7 +13709,8 @@ class TerminalController { TerminalNotificationStore.shared.clearNotifications( forTabId: tab.id, surfaceId: panelId, - discardQueuedNotifications: false, throughNotificationGeneration: clearBoundary + discardQueuedNotifications: false, + throughNotificationGeneration: clearBoundary ) } } else { @@ -13660,7 +13720,8 @@ class TerminalController { ) TerminalNotificationStore.shared.clearNotifications( forTabId: tab.id, - discardQueuedNotifications: false, throughNotificationGeneration: clearBoundary + discardQueuedNotifications: false, + throughNotificationGeneration: clearBoundary ) } } diff --git a/Sources/TerminalNotificationLiveRetargetDelivery.swift b/Sources/TerminalNotificationLiveRetargetDelivery.swift index d5ae2bb25f07..749cdbe5e745 100644 --- a/Sources/TerminalNotificationLiveRetargetDelivery.swift +++ b/Sources/TerminalNotificationLiveRetargetDelivery.swift @@ -77,6 +77,7 @@ extension TerminalNotificationStore { /// the workspace that currently owns it (issues #7939/#5781) instead of /// being dropped on a stale workspace claim; only a gone target (closed /// surface/workspace) skips. + @discardableResult func deliverQueuedNotification( claimedTabId: UUID, surfaceId: UUID?, @@ -88,7 +89,7 @@ extension TerminalNotificationStore { correlationKey: String? = nil, notificationGeneration: UInt64, soundContext: NotificationSoundOverrideContext? = nil - ) { + ) -> Bool { guard let target = AppDelegate.shared?.agentNotificationDeliveryTarget( claimedTabId: claimedTabId, surfaceId: surfaceId @@ -98,7 +99,7 @@ extension TerminalNotificationStore { "notification.queue.deliver.skip workspace=\(claimedTabId.uuidString.prefix(8)) surface=\(surfaceId?.uuidString.prefix(8) ?? "nil") reason=targetMissing titleLen=\(title.count) subtitleLen=\(subtitle.count) bodyLen=\(body.count)" ) #endif - return + return false } #if DEBUG cmuxDebugLog( @@ -118,6 +119,7 @@ extension TerminalNotificationStore { agent: agent, soundContext: soundContext ) + return true } /// Re-resolves canonical surface identity at the final apply boundary, diff --git a/Sources/TerminalNotificationQueue.swift b/Sources/TerminalNotificationQueue.swift index dcd14c9ef275..9c982af09040 100644 --- a/Sources/TerminalNotificationQueue.swift +++ b/Sources/TerminalNotificationQueue.swift @@ -8,6 +8,32 @@ fileprivate struct QueuedTerminalNotificationKey: Hashable, Sendable { let surfaceId: UUID? } +fileprivate struct QueuedAgentApprovalStage { + let workspaceID: UUID + let surfaceID: UUID + let title: String + let subtitle: String + let body: String + let approvalID: AgentApprovalCorrelationID + let approvalIDIsDerived: Bool + let approvalSource: String? + let agent: TerminalNotificationPolicyAgentContext? + let producerCorrelationKey: String? +} + +fileprivate struct AgentApprovalCorrelationAliasKey: Hashable, Sendable { + let surfaceID: UUID + let producerCorrelationKey: String +} + +fileprivate struct AgentApprovalMutationToken { + let global: UInt64 + let workspace: UInt64 + let surface: UInt64 + let workspaceID: UUID? + let surfaceID: UUID? +} + fileprivate struct QueuedTerminalNotification: Sendable { let key: QueuedTerminalNotificationKey let title: String @@ -25,6 +51,10 @@ fileprivate enum TerminalSocketMutation { case clearNotificationsForTab(UUID, through: UInt64) case clearNotificationsForSurface(UUID, UUID, through: UInt64) case clearNotificationsForCorrelation(UUID, UUID, String, through: UInt64) + case clearNotificationCorrelation(QueuedTerminalNotificationKey, String) + case stageAgentApproval(QueuedAgentApprovalStage, AgentApprovalMutationToken) + case resolveAgentApproval(UUID, AgentApprovalCorrelationID, AgentApprovalCorrelationID?, AgentApprovalMutationToken) + case resolveAgentApprovalScope(UUID, AgentApprovalCorrelationID.Scope, AgentApprovalMutationToken) case perform(@MainActor () -> Void) } @@ -67,11 +97,67 @@ final class TerminalMutationBus: @unchecked Sendable { private var drainScheduled = false private var nextSequence: UInt64 = 0 private var currentNotificationGeneration: UInt64 = 0 + private var approvalGlobalGeneration: UInt64 = 0 + private var approvalWorkspaceGenerations: [UUID: UInt64] = [:] + private var approvalSurfaceGenerations: [UUID: UInt64] = [:] + private var approvalWorkspaceBySurface: [UUID: UUID] = [:] + /// Producer-supplied correlation keys remain aliases of the coordinator's + /// episode key until that episode is cleared. Keeping the alias in the + /// mutation bus lets a later `--correlation-key` clear find the delivered + /// row without weakening the `agent-approval:` lifecycle marker. + private var approvalCorrelationAliases: [AgentApprovalCorrelationAliasKey: String] = [:] + private var pendingApprovalMutationCount = 0 + // Hook traffic can outlive the workspaces/surfaces that produced it. Keep + // generation fencing bounded even if a long-running process churns through + // thousands of UUIDs. When the cap is reached, a global epoch bump safely + // invalidates every outstanding token before the old key tables are reset. + private let maxApprovalGenerationEntries = 4_096 + private let maxApprovalCorrelationAliases = 1_024 + /// Bound approval-only traffic while the main actor is unavailable. Stage + /// mutations are explicitly droppable; resolution mutations evict the + /// oldest stage first so a stale approval cannot outlive a newer clear. + private let maxPendingApprovalMutations = 256 private let maxMutationsPerDrain = 16 #if DEBUG private var drainsSuspendedForTesting = false #endif + @MainActor + private lazy var agentApprovalNotifications = AgentApprovalNotificationCoordinator( + dispatchScheduledAction: { [weak self] action in + self?.enqueueMainActorMutation(action) + }, + deliver: { [weak self] delivery in + if let producerCorrelationKey = delivery.producerCorrelationKey { + self?.registerApprovalCorrelationAlias( + surfaceID: delivery.surfaceID, + producerCorrelationKey: producerCorrelationKey, + episodeCorrelationKey: delivery.correlationKey + ) + } + self?.enqueueNotification( + tabId: delivery.workspaceID, + surfaceId: delivery.surfaceID, + title: delivery.title, + subtitle: delivery.subtitle, + body: delivery.body, + replyShape: .none, + agent: delivery.agent, + correlationKey: delivery.correlationKey, + // The coordinator already owns approval coalescing. Preserve + // unrelated notifications that are queued for the same pane. + coalesces: false + ) + }, + clear: { [weak self] clear in + self?.enqueueClearNotification( + tabId: clear.workspaceID, + surfaceId: clear.surfaceID, + correlationKey: clear.correlationKey + ) + } + ) + nonisolated func enqueueNotification( tabId: UUID, surfaceId: UUID?, @@ -96,13 +182,167 @@ final class TerminalMutationBus: @unchecked Sendable { ), coalesces: coalesces) } + /// User-facing removal paths must also retire the in-memory approval + /// episode. Otherwise a later queued stage can recreate a banner that was + /// already dismissed from the notification store. + @MainActor + func dismissAgentApproval( + correlationKey: String, + workspaceID: UUID? = nil, + surfaceID: UUID? = nil + ) { + guard AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(correlationKey) else { return } + let dismissedSurfaceID = agentApprovalNotifications.dismissDelivered(correlationKey: correlationKey) + let resolvedSurfaceID = surfaceID ?? dismissedSurfaceID + removeApprovalCorrelationAliases( + surfaceID: resolvedSurfaceID, + episodeCorrelationKey: correlationKey + ) + lock.lock() + ensureApprovalGenerationCapacityLocked(workspaceID: workspaceID, surfaceID: resolvedSurfaceID) + let resolvedWorkspaceID = workspaceID ?? resolvedSurfaceID.flatMap { approvalWorkspaceBySurface[$0] } + if let resolvedWorkspaceID { + approvalWorkspaceGenerations[resolvedWorkspaceID, default: 0] &+= 1 + } + if let resolvedSurfaceID { + approvalSurfaceGenerations[resolvedSurfaceID, default: 0] &+= 1 + approvalWorkspaceBySurface.removeValue(forKey: resolvedSurfaceID) + } + lock.unlock() + } + + @MainActor + func cancelAgentApproval(surfaceID: UUID) { + agentApprovalNotifications.cancel(surfaceID: surfaceID, clearDelivered: false) + removeApprovalCorrelationAliases(surfaceID: surfaceID, episodeCorrelationKey: nil) + invalidateApprovalGenerations(global: false, workspaceID: nil, surfaceID: surfaceID) + } + + @MainActor + func cancelAgentApprovals(workspaceID: UUID) { + agentApprovalNotifications.cancel(workspaceID: workspaceID, clearDelivered: false) + removeApprovalCorrelationAliases(workspaceID: workspaceID) + lock.lock() + ensureApprovalGenerationCapacityLocked(workspaceID: workspaceID, surfaceID: nil) + approvalWorkspaceGenerations[workspaceID, default: 0] &+= 1 + let surfaceIDs = approvalWorkspaceBySurface.compactMap { surfaceID, ownerID in + ownerID == workspaceID ? surfaceID : nil + } + for surfaceID in surfaceIDs { + approvalSurfaceGenerations[surfaceID, default: 0] &+= 1 + approvalWorkspaceBySurface.removeValue(forKey: surfaceID) + } + lock.unlock() + } + + /// Rebind an approval episode when its terminal surface moves between + /// workspaces. The move fences queued mutations captured under the old + /// owner and updates the coordinator's eventual clear target. + @MainActor + func rebindAgentApproval( + surfaceID: UUID, + fromWorkspaceID: UUID, + toWorkspaceID: UUID + ) { + guard fromWorkspaceID != toWorkspaceID else { return } + agentApprovalNotifications.rebind( + surfaceID: surfaceID, + toWorkspaceID: toWorkspaceID + ) + lock.lock() + ensureApprovalGenerationCapacityLocked( + workspaceID: toWorkspaceID, + surfaceID: surfaceID + ) + if let previousWorkspaceID = approvalWorkspaceBySurface[surfaceID], + previousWorkspaceID != toWorkspaceID { + approvalWorkspaceGenerations[previousWorkspaceID, default: 0] &+= 1 + // Keep the surface generation stable for a live move: a + // completion already queued before the move is still for the + // same approval episode. Old workspace-scoped mutations are + // fenced by the workspace generation above. + } + approvalWorkspaceBySurface[surfaceID] = toWorkspaceID + lock.unlock() + } + + nonisolated func enqueueAgentApprovalNotification( + tabId: UUID, + surfaceId: UUID, + title: String, + subtitle: String, + body: String, + approvalID: AgentApprovalCorrelationID, + approvalIDIsDerived: Bool = false, + approvalSource: String? = nil, + agent: TerminalNotificationPolicyAgentContext? = nil, + producerCorrelationKey: String? = nil + ) { + lock.lock() + ensureApprovalGenerationCapacityLocked(workspaceID: tabId, surfaceID: surfaceId) + // A surface move (or a recycled surface identity) must fence stages + // captured under its previous workspace, even before the next hook + // arrives with the new workspace id. + if let previousWorkspaceID = approvalWorkspaceBySurface[surfaceId], + previousWorkspaceID != tabId { + approvalWorkspaceGenerations[previousWorkspaceID, default: 0] &+= 1 + } + approvalWorkspaceBySurface[surfaceId] = tabId + let token = approvalMutationToken(workspaceID: tabId, surfaceID: surfaceId) + let shouldScheduleDrain = enqueueApprovalMutationLocked(.stageAgentApproval(QueuedAgentApprovalStage( + workspaceID: tabId, + surfaceID: surfaceId, + title: title, + subtitle: subtitle, + body: body, + approvalID: approvalID, + approvalIDIsDerived: approvalIDIsDerived, + approvalSource: approvalSource, + agent: agent, + producerCorrelationKey: producerCorrelationKey + ), token)) + lock.unlock() + if shouldScheduleDrain { scheduleDrain() } + } + + nonisolated func enqueueAgentApprovalResolution( + surfaceId: UUID, + approvalID: AgentApprovalCorrelationID, + fallbackApprovalID: AgentApprovalCorrelationID? = nil + ) { + lock.lock() + let token = approvalMutationToken( + workspaceID: nil, + surfaceID: surfaceId + ) + let shouldScheduleDrain = enqueueApprovalMutationLocked(.resolveAgentApproval(surfaceId, approvalID, fallbackApprovalID, token)) + lock.unlock() + if shouldScheduleDrain { scheduleDrain() } + } + + nonisolated func enqueueAgentApprovalResolution( + surfaceId: UUID, + approvalScope: AgentApprovalCorrelationID.Scope + ) { + lock.lock() + let token = approvalMutationToken( + workspaceID: nil, + surfaceID: surfaceId + ) + let shouldScheduleDrain = enqueueApprovalMutationLocked(.resolveAgentApprovalScope(surfaceId, approvalScope, token)) + lock.unlock() + if shouldScheduleDrain { scheduleDrain() } + } + nonisolated func enqueueClearAllNotifications() { + invalidateApprovalGenerations(global: true, workspaceID: nil, surfaceID: nil) enqueueClear({ .clearAllNotifications(through: $0) }) { _ in true } } nonisolated func enqueueClearNotifications(forTabId tabId: UUID) { // Surface-addressed entries may have moved since enqueue. Keep them // ahead of the barrier so delivery can resolve their live owner first. + invalidateApprovalGenerations(global: false, workspaceID: tabId, surfaceID: nil) enqueueClear({ .clearNotificationsForTab(tabId, through: $0) }) { notification in notification.key.tabId == tabId && notification.key.surfaceId == nil } @@ -110,6 +350,7 @@ final class TerminalMutationBus: @unchecked Sendable { nonisolated func enqueueClearNotifications(forTabId tabId: UUID, surfaceId: UUID) { // Canonical surface identity: a stale-keyed entry would retarget here at drain. + invalidateApprovalGenerations(global: false, workspaceID: tabId, surfaceID: surfaceId) enqueueClear({ .clearNotificationsForSurface(tabId, surfaceId, through: $0) }) { notification in notification.key.surfaceId == surfaceId } @@ -124,14 +365,35 @@ final class TerminalMutationBus: @unchecked Sendable { surfaceId: UUID, correlationKey: String ) { + let effectiveCorrelationKey = resolvedApprovalCorrelationKey( + surfaceID: surfaceId, + producerCorrelationKey: correlationKey + ) ?? correlationKey enqueueClear({ - .clearNotificationsForCorrelation(tabId, surfaceId, correlationKey, through: $0) + .clearNotificationsForCorrelation(tabId, surfaceId, effectiveCorrelationKey, through: $0) }) { notification in notification.key.surfaceId == surfaceId - && notification.correlationKey == correlationKey + && notification.correlationKey == effectiveCorrelationKey } } + private nonisolated func enqueueClearNotification( + tabId: UUID, + surfaceId: UUID, + correlationKey: String + ) { + // This is a correlation-only reconciliation emitted by the approval + // coordinator. Do not advance pane generations here: a newer approval + // stage already queued behind this clear must remain admissible. The + // coordinator records its own exact tombstone before this mutation is + // enqueued, so a late duplicate is still fenced without invalidating + // unrelated stages. + enqueueBarrierMutation(.clearNotificationCorrelation( + QueuedTerminalNotificationKey(tabId: tabId, surfaceId: surfaceId), + correlationKey + )) + } + nonisolated func enqueueMainActorMutation(_ mutation: @escaping @MainActor () -> Void) { enqueueBarrierMutation(.perform(mutation)) } @@ -165,18 +427,35 @@ final class TerminalMutationBus: @unchecked Sendable { correlationKey: String, through boundary: UInt64 ) { + let effectiveCorrelationKey = resolvedApprovalCorrelationKey( + surfaceID: surfaceId, + producerCorrelationKey: correlationKey + ) ?? correlationKey discardPendingNotifications { notification, generation in notification.key.surfaceId == surfaceId - && notification.correlationKey == correlationKey + && notification.correlationKey == effectiveCorrelationKey && generation <= boundary } } nonisolated func discardPendingNotifications() { discardPendingNotifications(advanceGeneration: true) { _, _ in true } + invalidateApprovalGenerations(global: true, workspaceID: nil, surfaceID: nil) + } + + /// Direct store clears bypass the socket mutation cases, so cancel their + /// staged approval state before discarding queued notification deliveries. + @MainActor + func discardPendingNotificationsForClearAll() { + agentApprovalNotifications.cancelAll(clearDelivered: false) + lock.lock() + approvalCorrelationAliases.removeAll() + lock.unlock() + discardPendingNotifications() } nonisolated func discardPendingNotifications(forTabId tabId: UUID) { + invalidateApprovalGenerations(global: false, workspaceID: tabId, surfaceID: nil) discardPendingNotifications { notification, _ in notification.key.tabId == tabId } @@ -186,6 +465,7 @@ final class TerminalMutationBus: @unchecked Sendable { /// `rebindSurfaceNotifications`, where a surface-wide discard could drop a /// newer entry legitimately queued under the destination key mid-move. nonisolated func discardPendingNotifications(forTabId tabId: UUID, surfaceId: UUID?) { + invalidateApprovalGenerations(global: false, workspaceID: tabId, surfaceID: surfaceId) discardPendingNotifications { notification, _ in notification.key.tabId == tabId && notification.key.surfaceId == surfaceId } @@ -196,6 +476,7 @@ final class TerminalMutationBus: @unchecked Sendable { /// surface's live owner (#7939), so a clear that matched only the claimed /// key would let a stale-keyed entry resurrect the notification at drain. nonisolated func discardPendingNotifications(forSurfaceId surfaceId: UUID) { + invalidateApprovalGenerations(global: false, workspaceID: nil, surfaceID: surfaceId) discardPendingNotifications { notification, _ in notification.key.surfaceId == surfaceId } @@ -205,8 +486,38 @@ final class TerminalMutationBus: @unchecked Sendable { /// live destination workspace when workspace-scoped. @MainActor func discardPendingNotificationsForClear(tabId: UUID, surfaceId: UUID?) { - if let surfaceId { discardPendingNotifications(forSurfaceId: surfaceId) } - else { discardPendingNotificationsResolvingLiveOwner(forTabId: tabId) } + if let surfaceId { + agentApprovalNotifications.cancel(surfaceID: surfaceId, clearDelivered: false) + removeApprovalCorrelationAliases(surfaceID: surfaceId, episodeCorrelationKey: nil) + discardPendingNotifications(forSurfaceId: surfaceId) + invalidateApprovalGenerations(global: false, workspaceID: nil, surfaceID: surfaceId) + } else { + cancelAgentApprovalNotifications( + forLiveTabId: tabId, + clearDelivered: false + ) + removeApprovalCorrelationAliases(workspaceID: tabId) + discardPendingNotificationsResolvingLiveOwner(forTabId: tabId) + invalidateApprovalGenerations(global: false, workspaceID: tabId, surfaceID: nil) + } + } + + @MainActor + private func cancelAgentApprovalNotifications( + forLiveTabId tabId: UUID, + clearDelivered: Bool + ) { + agentApprovalNotifications.cancelPanes(clearDelivered: clearDelivered) { + claimedTabId, + surfaceId in + guard let target = AppDelegate.shared?.agentNotificationDeliveryTarget( + claimedTabId: claimedTabId, + surfaceId: surfaceId + ) else { + return claimedTabId == tabId + } + return target.tabId == tabId + } } /// Phase 1 of the live-owner workspace clear (see @@ -333,6 +644,244 @@ final class TerminalMutationBus: @unchecked Sendable { scheduleDrain() } + private func registerApprovalCorrelationAlias( + surfaceID: UUID, + producerCorrelationKey: String, + episodeCorrelationKey: String + ) { + guard !producerCorrelationKey.isEmpty else { return } + lock.lock() + let key = AgentApprovalCorrelationAliasKey( + surfaceID: surfaceID, + producerCorrelationKey: producerCorrelationKey + ) + if approvalCorrelationAliases.count >= maxApprovalCorrelationAliases, + approvalCorrelationAliases[key] == nil, + let oldest = approvalCorrelationAliases.keys.first { + approvalCorrelationAliases.removeValue(forKey: oldest) + } + approvalCorrelationAliases[key] = episodeCorrelationKey + lock.unlock() + } + + nonisolated func resolvedApprovalCorrelationKey( + surfaceID: UUID, + producerCorrelationKey: String + ) -> String? { + lock.lock() + defer { lock.unlock() } + return approvalCorrelationAliases[AgentApprovalCorrelationAliasKey( + surfaceID: surfaceID, + producerCorrelationKey: producerCorrelationKey + )] + } + + nonisolated func resolvedApprovalCorrelationKey( + producerCorrelationKey: String + ) -> String { + lock.lock() + defer { lock.unlock() } + return approvalCorrelationAliases.first { + $0.key.producerCorrelationKey == producerCorrelationKey + }?.value ?? producerCorrelationKey + } + + private func removeApprovalCorrelationAliases( + surfaceID: UUID?, + episodeCorrelationKey: String? + ) { + lock.lock() + approvalCorrelationAliases = approvalCorrelationAliases.filter { key, value in + let matchesSurface = surfaceID.map { key.surfaceID == $0 } ?? true + let matchesEpisode = episodeCorrelationKey.map { value == $0 } ?? true + return !(matchesSurface && matchesEpisode) + } + lock.unlock() + } + + private func removeApprovalCorrelationAliases(workspaceID: UUID) { + lock.lock() + let surfaceIDs = Set(approvalWorkspaceBySurface.compactMap { surfaceID, ownerID in + ownerID == workspaceID ? surfaceID : nil + }) + approvalCorrelationAliases = approvalCorrelationAliases.filter { key, _ in + !surfaceIDs.contains(key.surfaceID) + } + lock.unlock() + } + + private nonisolated func approvalMutationToken( + workspaceID: UUID?, + surfaceID: UUID? + ) -> AgentApprovalMutationToken { + // Resolution tokens intentionally remain workspace-agnostic. A live + // surface may move between workspaces while a completion is queued; + // the surface generation is the identity fence for that mutation. + let resolvedWorkspaceID = workspaceID + return AgentApprovalMutationToken( + global: approvalGlobalGeneration, + workspace: resolvedWorkspaceID.map { approvalWorkspaceGenerations[$0] ?? 0 } ?? 0, + surface: surfaceID.map { approvalSurfaceGenerations[$0] ?? 0 } ?? 0, + workspaceID: resolvedWorkspaceID, + surfaceID: surfaceID + ) + } + + /// Must be called with `lock` held. A global reset is preferable to + /// evicting individual UUID generations: evicting one key could make an + /// old token look current again when its generation falls back to zero. + private func ensureApprovalGenerationCapacityLocked( + workspaceID: UUID?, + surfaceID: UUID? + ) { + var projected = approvalWorkspaceGenerations.count + + approvalSurfaceGenerations.count + + approvalWorkspaceBySurface.count + if let workspaceID, approvalWorkspaceGenerations[workspaceID] == nil { + projected += 1 + } + if let surfaceID { + if approvalSurfaceGenerations[surfaceID] == nil { projected += 1 } + if approvalWorkspaceBySurface[surfaceID] == nil { projected += 1 } + } + guard projected <= maxApprovalGenerationEntries else { + approvalGlobalGeneration &+= 1 + approvalWorkspaceGenerations.removeAll(keepingCapacity: true) + approvalSurfaceGenerations.removeAll(keepingCapacity: true) + approvalWorkspaceBySurface.removeAll(keepingCapacity: true) + return + } + } + + private nonisolated func invalidateApprovalGenerations( + global: Bool, + workspaceID: UUID?, + surfaceID: UUID? + ) { + lock.lock() + ensureApprovalGenerationCapacityLocked(workspaceID: workspaceID, surfaceID: surfaceID) + if global { approvalGlobalGeneration &+= 1 } + if let workspaceID { + approvalWorkspaceGenerations[workspaceID, default: 0] &+= 1 + } + if let surfaceID { + approvalSurfaceGenerations[surfaceID, default: 0] &+= 1 + approvalWorkspaceBySurface.removeValue(forKey: surfaceID) + } + lock.unlock() + } + + /// Approval stages/resolutions are keyed mutations. Coalesce duplicate + /// entries while the main actor is busy. A bounded admission rule then + /// drops a new stage (or evicts the oldest stage for a resolution) once + /// `maxPendingApprovalMutations` is reached, so a hook burst cannot turn + /// the pending array into an unbounded backlog. + private nonisolated func enqueueApprovalMutationLocked(_ mutation: TerminalSocketMutation) -> Bool { + let shouldScheduleDrain: Bool + switch mutation { + case .stageAgentApproval(let stage, _): + let beforeCount = pending.count + pending.removeAll { entry in + guard case .stageAgentApproval(let existing, _) = entry.mutation else { return false } + return existing.surfaceID == stage.surfaceID + && existing.approvalID == stage.approvalID + && !existing.approvalIDIsDerived + && !stage.approvalIDIsDerived + } + pendingApprovalMutationCount -= beforeCount - pending.count + case .resolveAgentApproval(let surfaceID, let approvalID, let fallbackID, _): + let beforeCount = pending.count + pending.removeAll { entry in + guard case .resolveAgentApproval(let existingSurfaceID, let existingID, let existingFallbackID, _) = entry.mutation else { return false } + return existingSurfaceID == surfaceID && existingID == approvalID && existingFallbackID == fallbackID + } + pendingApprovalMutationCount -= beforeCount - pending.count + case .resolveAgentApprovalScope(let surfaceID, let scope, _): + let beforeCount = pending.count + pending.removeAll { entry in + guard case .resolveAgentApprovalScope(let existingSurfaceID, let existingScope, _) = entry.mutation else { return false } + return existingSurfaceID == surfaceID && existingScope == scope + } + pendingApprovalMutationCount -= beforeCount - pending.count + default: + break + } + if pendingApprovalMutationCount >= maxPendingApprovalMutations { + if Self.isApprovalResolution(mutation), + let evictionIndex = pending.firstIndex(where: { entry in + if case .stageAgentApproval = entry.mutation { return true } + return false + }) ?? pending.firstIndex(where: { Self.isApprovalMutation($0.mutation) }) { + pending.remove(at: evictionIndex) + pendingApprovalMutationCount -= 1 + } else { + // Stage admission is explicitly lossy under pressure. The + // coordinator's settle window and later authoritative hook + // events still provide the next opportunity to surface it. + return false + } + } + nextSequence &+= 1 + pending.append(TerminalSocketMutationEntry( + sequence: nextSequence, + mutation: mutation, + notificationGeneration: nil, + notificationCoalescingKey: nil, + performReplaceKey: nil + )) + pendingApprovalMutationCount += 1 + shouldScheduleDrain = !drainScheduled + if shouldScheduleDrain { drainScheduled = true } + return shouldScheduleDrain + } + + private static func isApprovalMutation(_ mutation: TerminalSocketMutation) -> Bool { + switch mutation { + case .stageAgentApproval, .resolveAgentApproval, .resolveAgentApprovalScope: + return true + default: + return false + } + } + + private static func isApprovalResolution(_ mutation: TerminalSocketMutation) -> Bool { + switch mutation { + case .resolveAgentApproval, .resolveAgentApprovalScope: + return true + default: + return false + } + } + + private nonisolated func approvalTokenIsCurrent( + _ token: AgentApprovalMutationToken, + workspaceID: UUID?, + surfaceID: UUID? + ) -> Bool { + lock.lock() + defer { lock.unlock() } + guard token.global == approvalGlobalGeneration else { return false } + guard token.workspaceID == workspaceID || workspaceID == nil else { return false } + guard token.surfaceID == surfaceID || surfaceID == nil else { return false } + if let tokenWorkspaceID = token.workspaceID, + token.workspace != (approvalWorkspaceGenerations[tokenWorkspaceID] ?? 0) { + return false + } + if let tokenSurfaceID = token.surfaceID, + token.surface != (approvalSurfaceGenerations[tokenSurfaceID] ?? 0) { + return false + } + // Resolution tokens may be workspace-agnostic after an owner mapping + // is retired; the surface generation above remains the stale-event + // fence. Only compare the live owner when the token captured one. + if let tokenSurfaceID = token.surfaceID, + let tokenWorkspaceID = token.workspaceID, + approvalWorkspaceBySurface[tokenSurfaceID] != tokenWorkspaceID { + return false + } + return true + } + /// Last-write-wins `enqueueMainActorMutation`: drops any still-pending /// mutation with the same `replaceKey` before appending, so the survivor /// applies at its new enqueue position (the notification coalescing @@ -457,6 +1006,11 @@ final class TerminalMutationBus: @unchecked Sendable { let batch = Array(pending.prefix(count)) if !batch.isEmpty { pending.removeFirst(count) + pendingApprovalMutationCount -= batch.reduce(into: 0) { count, entry in + if Self.isApprovalMutation(entry.mutation) { + count += 1 + } + } } let remaining = pending.count lock.unlock() @@ -492,7 +1046,7 @@ final class TerminalMutationBus: @unchecked Sendable { "notification.queue.perform seq=\(entry.sequence) workspace=\(notification.key.tabId.uuidString.prefix(8)) surface=\(notification.key.surfaceId?.uuidString.prefix(8) ?? "nil") titleLen=\(notification.title.count) subtitleLen=\(notification.subtitle.count) bodyLen=\(notification.body.count)" ) #endif - TerminalNotificationStore.shared.deliverQueuedNotification( + let delivered = TerminalNotificationStore.shared.deliverQueuedNotification( claimedTabId: notification.key.tabId, surfaceId: notification.key.surfaceId, title: notification.title, @@ -504,15 +1058,38 @@ final class TerminalMutationBus: @unchecked Sendable { notificationGeneration: entry.notificationGeneration ?? 0, soundContext: notification.soundContext ) + if AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(notification.correlationKey) { + // A missing live target is terminal for this episode: keep + // neither its expiry task nor its candidate strings around + // waiting for a pane that cannot receive the banner. + if !delivered, let correlationKey = notification.correlationKey { + dismissAgentApproval( + correlationKey: correlationKey, + workspaceID: notification.key.tabId, + surfaceID: notification.key.surfaceId + ) + } + } case .clearAllNotifications(let boundary): + agentApprovalNotifications.cancelAll(clearDelivered: false) + lock.lock() + approvalCorrelationAliases.removeAll() + lock.unlock() TerminalNotificationStore.shared.clearAll(discardQueuedNotifications: false, throughNotificationGeneration: boundary) case .clearNotificationsForTab(let tabId, let boundary): + cancelAgentApprovalNotifications( + forLiveTabId: tabId, + clearDelivered: false + ) + removeApprovalCorrelationAliases(workspaceID: tabId) TerminalNotificationStore.shared.clearNotifications( forTabId: tabId, discardQueuedNotifications: false, throughNotificationGeneration: boundary ) case .clearNotificationsForSurface(let tabId, let surfaceId, let boundary): + agentApprovalNotifications.cancel(surfaceID: surfaceId, clearDelivered: false) + removeApprovalCorrelationAliases(surfaceID: surfaceId, episodeCorrelationKey: nil) TerminalNotificationStore.shared.clearNotifications( forTabId: tabId, surfaceId: surfaceId, @@ -520,12 +1097,65 @@ final class TerminalMutationBus: @unchecked Sendable { throughNotificationGeneration: boundary ) case .clearNotificationsForCorrelation(let tabId, let surfaceId, let correlationKey, let boundary): + if AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(correlationKey) { + _ = agentApprovalNotifications.dismissDelivered(correlationKey: correlationKey) + removeApprovalCorrelationAliases( + surfaceID: surfaceId, + episodeCorrelationKey: correlationKey + ) + } TerminalNotificationStore.shared.clearNotifications( forTabId: tabId, surfaceId: surfaceId, correlationKey: correlationKey, throughNotificationGeneration: boundary ) + case .clearNotificationCorrelation(let key, let correlationKey): + // A pane may disappear between delivery and resolution. Clear + // the claimed workspace when no live owner can be retargeted. + let tabId = AppDelegate.shared?.agentNotificationDeliveryTarget( + claimedTabId: key.tabId, + surfaceId: key.surfaceId + )?.tabId ?? key.tabId + // Correlation clears can originate outside the coordinator's + // own resolve callback. Retire any matching episode before + // removing the persisted row so a late stage cannot recreate + // the banner. + _ = agentApprovalNotifications.dismissDelivered(correlationKey: correlationKey) + removeApprovalCorrelationAliases( + surfaceID: key.surfaceId, + episodeCorrelationKey: correlationKey + ) + TerminalNotificationStore.shared.clearNotifications( + forTabId: tabId, + correlationKey: correlationKey + ) + case .stageAgentApproval(let stage, let token): + guard approvalTokenIsCurrent(token, workspaceID: stage.workspaceID, surfaceID: stage.surfaceID) else { continue } + agentApprovalNotifications.stage( + workspaceID: stage.workspaceID, + surfaceID: stage.surfaceID, + title: stage.title, + subtitle: stage.subtitle, + body: stage.body, + approvalID: stage.approvalID, + isDerived: stage.approvalIDIsDerived, + approvalSource: stage.approvalSource, + agent: stage.agent, + producerCorrelationKey: stage.producerCorrelationKey + ) + case .resolveAgentApproval(let surfaceID, let approvalID, let fallbackID, let token): + guard approvalTokenIsCurrent(token, workspaceID: nil, surfaceID: surfaceID) else { continue } + agentApprovalNotifications.resolve(surfaceID: surfaceID, approvalID: approvalID, fallbackApprovalID: fallbackID) + if !agentApprovalNotifications.hasEpisode(surfaceID: surfaceID) { + removeApprovalCorrelationAliases(surfaceID: surfaceID, episodeCorrelationKey: nil) + } + case .resolveAgentApprovalScope(let surfaceID, let approvalScope, let token): + guard approvalTokenIsCurrent(token, workspaceID: nil, surfaceID: surfaceID) else { continue } + agentApprovalNotifications.resolve(surfaceID: surfaceID, approvalScope: approvalScope) + if !agentApprovalNotifications.hasEpisode(surfaceID: surfaceID) { + removeApprovalCorrelationAliases(surfaceID: surfaceID, episodeCorrelationKey: nil) + } case .perform(let mutation): mutation() } diff --git a/Sources/TerminalNotificationStore.swift b/Sources/TerminalNotificationStore.swift index e8b58ca9166e..a880d7f08305 100644 --- a/Sources/TerminalNotificationStore.swift +++ b/Sources/TerminalNotificationStore.swift @@ -1211,9 +1211,9 @@ final class TerminalNotificationStore: ObservableObject { body: String, replyShape: TerminalNotificationReplyShape = .none, retargetsToLiveSurfaceOwner: Bool = true, + correlationKey: String? = nil, cooldownKey: String? = nil, cooldownInterval: TimeInterval? = nil, - correlationKey: String? = nil, clickAction: TerminalNotificationClickAction? = nil, notificationGeneration: UInt64? = nil, resolvedHooks: [CmuxResolvedNotificationHook]? = nil, preRegisteredPolicyRequestId: UUID? = nil, @@ -1650,8 +1650,13 @@ final class TerminalNotificationStore: ObservableObject { ) { var updated = notifications var idsToClear: [String] = [] + var supersededApprovalKeys: [String] = [] updated.removeAll { existing in guard existing.tabId == notification.tabId, existing.surfaceId == notification.surfaceId else { return false } + if AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(existing.correlationKey), + !AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(notification.correlationKey) { + return false + } if let correlationKey = notification.correlationKey { // Correlated producers (Cursor approvals and other // identity-scoped events) replace only their own prior entry. @@ -1660,9 +1665,18 @@ final class TerminalNotificationStore: ObservableObject { guard existing.correlationKey == correlationKey else { return false } } idsToClear.append(existing.id.uuidString) + if let correlationKey = existing.correlationKey, + AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(correlationKey), + correlationKey != notification.correlationKey { + supersededApprovalKeys.append(correlationKey) + } return true } + for correlationKey in supersededApprovalKeys { + TerminalMutationBus.shared.dismissAgentApproval(correlationKey: correlationKey) + } + if let existingIndicatorSurfaceId = focusedReadIndicatorByTabId[notification.tabId], existingIndicatorSurfaceId != notification.surfaceId { focusedReadIndicatorByTabId.removeValue(forKey: notification.tabId) @@ -1840,10 +1854,15 @@ final class TerminalNotificationStore: ObservableObject { guard !ids.isEmpty else { return marked } var updated = notifications var activeIDs: [String] = [] + var retiredApprovalKeys: [String] = [] var drainedSuperseded: [String] = [] for index in updated.indices where ids.contains(updated[index].id) && !updated[index].isRead { updated[index].isRead = true activeIDs.append(updated[index].id.uuidString) + if let correlationKey = updated[index].correlationKey, + AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(correlationKey) { + retiredApprovalKeys.append(correlationKey) + } drainedSuperseded.append(contentsOf: supersededPhoneDismissBuffer.flush( forKey: SupersededPhoneDismissBuffer.key( tabId: updated[index].tabId, @@ -1852,6 +1871,9 @@ final class TerminalNotificationStore: ObservableObject { )) } if !activeIDs.isEmpty { + for correlationKey in retiredApprovalKeys { + TerminalMutationBus.shared.dismissAgentApproval(correlationKey: correlationKey) + } notifications = updated removeNotificationRequestsAndReleaseSoundReferences(withIdentifiers: activeIDs) emitNotificationsDismissed( @@ -1904,10 +1926,15 @@ final class TerminalNotificationStore: ObservableObject { notificationFeedHistory.markRead(inWorkspace: tabId) var updated = notifications var idsToClear: [String] = [] + var retiredApprovalKeys: [String] = [] for index in updated.indices { if updated[index].tabId == tabId && !updated[index].isRead { updated[index].isRead = true idsToClear.append(updated[index].id.uuidString) + if let correlationKey = updated[index].correlationKey, + AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(correlationKey) { + retiredApprovalKeys.append(correlationKey) + } } } if !idsToClear.isEmpty { @@ -1920,6 +1947,9 @@ final class TerminalNotificationStore: ObservableObject { setPanelDerivedWorkspaceUnread(false, forTabId: tabId) setWorkspaceRestoredUnread(false, forTabId: tabId) if !idsToClear.isEmpty { + for correlationKey in retiredApprovalKeys { + TerminalMutationBus.shared.dismissAgentApproval(correlationKey: correlationKey) + } removeNotificationRequestsAndReleaseSoundReferences(withIdentifiers: idsToClear) emitNotificationsDismissed( ids: idsToClear, @@ -1933,6 +1963,7 @@ final class TerminalNotificationStore: ObservableObject { notificationFeedHistory.markRead(inWorkspace: tabId, surfaceId: surfaceId) var updated = notifications var idsToClear: [String] = [] + var retiredApprovalKeys: [String] = [] var supersededDrained = supersededPhoneDismissBuffer.flush( forKey: SupersededPhoneDismissBuffer.key(tabId: tabId, surfaceId: surfaceId) ) @@ -1941,6 +1972,10 @@ final class TerminalNotificationStore: ObservableObject { !updated[index].isRead { updated[index].isRead = true idsToClear.append(updated[index].id.uuidString) + if let correlationKey = updated[index].correlationKey, + AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(correlationKey) { + retiredApprovalKeys.append(correlationKey) + } supersededDrained.append(contentsOf: supersededPhoneDismissBuffer.flush( forKey: SupersededPhoneDismissBuffer.key( tabId: updated[index].tabId, @@ -1970,6 +2005,9 @@ final class TerminalNotificationStore: ObservableObject { setWorkspaceRestoredUnread(false, forTabId: tabId) } if !idsToClear.isEmpty { + for correlationKey in retiredApprovalKeys { + TerminalMutationBus.shared.dismissAgentApproval(correlationKey: correlationKey) + } removeNotificationRequestsAndReleaseSoundReferences(withIdentifiers: idsToClear) emitNotificationsDismissed(ids: idsToClear, drainedSuperseded: supersededDrained) } @@ -2075,12 +2113,17 @@ final class TerminalNotificationStore: ObservableObject { notificationFeedHistory.markAllRead() var updated = notifications var idsToClear: [String] = [] + var retiredApprovalKeys: [String] = [] var tabIdsToClearPanelUnread = panelDerivedUnreadWorkspaceIds for index in updated.indices { if !updated[index].isRead { tabIdsToClearPanelUnread.insert(updated[index].tabId) updated[index].isRead = true idsToClear.append(updated[index].id.uuidString) + if let correlationKey = updated[index].correlationKey, + AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(correlationKey) { + retiredApprovalKeys.append(correlationKey) + } } } if !idsToClear.isEmpty { @@ -2092,6 +2135,9 @@ final class TerminalNotificationStore: ObservableObject { clearPanelDerivedWorkspaceUnread() clearWorkspaceRestoredUnread() if !idsToClear.isEmpty { + for correlationKey in retiredApprovalKeys { + TerminalMutationBus.shared.dismissAgentApproval(correlationKey: correlationKey) + } removeNotificationRequestsAndReleaseSoundReferences(withIdentifiers: idsToClear) emitNotificationsDismissed( ids: idsToClear, @@ -2106,6 +2152,10 @@ final class TerminalNotificationStore: ObservableObject { let originalCount = updated.count updated.removeAll { $0.id == id } guard updated.count != originalCount else { return } + if let correlationKey = removed?.correlationKey, + AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(correlationKey) { + TerminalMutationBus.shared.dismissAgentApproval(correlationKey: correlationKey) + } notifications = updated notificationFeedHistory.markRead(ids: [id]) if let removed { @@ -2124,13 +2174,24 @@ final class TerminalNotificationStore: ObservableObject { } func clearNotifications(forTabId tabId: UUID, correlationKey: String) { - inFlightPolicyRequests.discard(forTabId: tabId, correlationKey: correlationKey) - let ids = notifications.compactMap { - $0.tabId == tabId && $0.correlationKey == correlationKey ? $0.id : nil + let effectiveCorrelationKey = TerminalMutationBus.shared + .resolvedApprovalCorrelationKey(producerCorrelationKey: correlationKey) + inFlightPolicyRequests.discard(forTabId: tabId, correlationKey: effectiveCorrelationKey) + let matching = notifications.filter { + $0.tabId == tabId && $0.correlationKey == effectiveCorrelationKey + } + let ids = matching.map(\.id) + if matching.isEmpty { + if AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(effectiveCorrelationKey) { + TerminalMutationBus.shared.dismissAgentApproval(correlationKey: effectiveCorrelationKey) + } + return } + // `remove(id:)` performs the combined UserNotifications clear and + // retires any delivered approval episode for each active record. ids.forEach(remove) - // `remove(id:)` already performs the combined UserNotifications clear - // for each active record; do not issue a second pending-removal batch. + // Do not issue a second pending-removal batch: `remove(id:)` owns the + // complete cleanup for each active record. } /// Clears one surface notification by its producer correlation key. This @@ -2143,15 +2204,20 @@ final class TerminalNotificationStore: ObservableObject { correlationKey: String, throughNotificationGeneration: UInt64? = nil ) { + let effectiveCorrelationKey = TerminalMutationBus.shared + .resolvedApprovalCorrelationKey( + surfaceID: surfaceId, + producerCorrelationKey: correlationKey + ) ?? correlationKey inFlightPolicyRequests.discard( forSurfaceId: surfaceId, - correlationKey: correlationKey, + correlationKey: effectiveCorrelationKey, through: throughNotificationGeneration ) let liveTabId = AppDelegate.shared? .agentNotificationDeliveryTarget(claimedTabId: tabId, surfaceId: surfaceId)?.tabId ?? tabId let ids: [UUID] = notifications.compactMap { notification -> UUID? in - guard notification.correlationKey == correlationKey, + guard notification.correlationKey == effectiveCorrelationKey, notification.matchesClear( tabId: tabId, liveTabId: liveTabId, @@ -2161,10 +2227,19 @@ final class TerminalNotificationStore: ObservableObject { } return notification.id } + if ids.isEmpty, + AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(effectiveCorrelationKey) { + TerminalMutationBus.shared.dismissAgentApproval(correlationKey: effectiveCorrelationKey) + } ids.forEach(remove) } func restoreSessionNotifications(_ restoredNotifications: [TerminalNotification], forTabId tabId: UUID) { + // Approval episodes are process-local and cannot be reconstructed from + // the persisted banner rows below. Retire any episode owned by this + // workspace before dropping its old approval rows, otherwise a later + // hook would join a stale delivered episode and never present again. + TerminalMutationBus.shared.cancelAgentApprovals(workspaceID: tabId) TerminalMutationBus.shared.discardPendingNotifications(forTabId: tabId) let removedIds = notifications @@ -2172,7 +2247,13 @@ final class TerminalNotificationStore: ObservableObject { .map { $0.id.uuidString } var usedNotificationIds = Set(notifications.filter { $0.tabId != tabId }.map(\.id)) let restoredForTab = restoredNotifications - .filter { $0.tabId == tabId } + .filter { + $0.tabId == tabId + // Approval coordinator state is deliberately ephemeral; + // restoring its banner without its in-memory candidates + // would create an orphan that no later hook can settle. + && !AgentApprovalNotificationCoordinator.isApprovalCorrelationKey($0.correlationKey) + } .sorted(by: Self.notificationSortPrecedes) .map { Self.notificationWithUniqueId($0, usedIds: &usedNotificationIds) } let keptNotifications = notifications.filter { $0.tabId != tabId } @@ -2229,7 +2310,9 @@ final class TerminalNotificationStore: ObservableObject { private func replaceNotificationsForClear(_ next: [TerminalNotification]) { suppressNotificationDiffPublishing = true; notifications = next; suppressNotificationDiffPublishing = false } func clearAll(discardQueuedNotifications: Bool = true, throughNotificationGeneration: UInt64? = nil) { inFlightPolicyRequests.discardAll(through: throughNotificationGeneration) - if discardQueuedNotifications { TerminalMutationBus.shared.discardPendingNotifications() } + if discardQueuedNotifications { + TerminalMutationBus.shared.discardPendingNotificationsForClearAll() + } guard !notifications.isEmpty || !focusedReadIndicatorByTabId.isEmpty || !manualUnreadWorkspaceIds.isEmpty || @@ -2238,6 +2321,14 @@ final class TerminalNotificationStore: ObservableObject { !restoredUnreadWorkspaceIds.isEmpty else { return } let tabIdsToClearPanelUnread = panelDerivedUnreadWorkspaceIds.union(notifications.map(\.tabId)) let ids = notifications.map { $0.id.uuidString } + let approvalKeys = notifications.compactMap { notification -> String? in + guard let key = notification.correlationKey, + AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(key) else { return nil } + return key + } + for key in approvalKeys { + TerminalMutationBus.shared.dismissAgentApproval(correlationKey: key) + } notificationFeedHistory.markRead( ids: Set(ids.compactMap { UUID(uuidString: $0) }) ) @@ -2317,6 +2408,11 @@ final class TerminalNotificationStore: ObservableObject { func rebindSurfaceNotifications(fromTabId sourceTabId: UUID, toTabId destinationTabId: UUID, surfaceId: UUID) { guard sourceTabId != destinationTabId else { return } + TerminalMutationBus.shared.rebindAgentApproval( + surfaceID: surfaceId, + fromWorkspaceID: sourceTabId, + toWorkspaceID: destinationTabId + ) inFlightPolicyRequests.rebindSurface(fromTabId: sourceTabId, toTabId: destinationTabId, surfaceId: surfaceId) notificationFeedHistory.rebindSurface( fromTabId: sourceTabId, diff --git a/cmux.xcodeproj/project.pbxproj b/cmux.xcodeproj/project.pbxproj index 83893ca4e97e..d6c6ad10ae69 100644 --- a/cmux.xcodeproj/project.pbxproj +++ b/cmux.xcodeproj/project.pbxproj @@ -17,6 +17,8 @@ A5C017000000000000000007 /* AccountSignInView.swift in Sources */ = {isa = PBXBuildFile; fileRef = A5C017000000000000000008 /* AccountSignInView.swift */; }; A9E020000000000000000006 /* agent-session-react in Resources */ = {isa = PBXBuildFile; fileRef = A9E010000000000000000006 /* agent-session-react */; }; A9E020000000000000000007 /* agent-session-solid in Resources */ = {isa = PBXBuildFile; fileRef = A9E010000000000000000007 /* agent-session-solid */; }; + A10017010000000000000003 /* AgentApprovalCorrelationID.swift in Sources */ = {isa = PBXBuildFile; fileRef = A10017010000000000000004 /* AgentApprovalCorrelationID.swift */; }; + A10017010000000000000001 /* AgentApprovalNotificationCoordinator.swift in Sources */ = {isa = PBXBuildFile; fileRef = A10017010000000000000002 /* AgentApprovalNotificationCoordinator.swift */; }; C7AF10000000000000000005 /* AgentChatArtifactGalleryBuilder.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7AF10000000000000000006 /* AgentChatArtifactGalleryBuilder.swift */; }; C7AF10000000000000000001 /* AgentChatArtifactIndex.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7AF10000000000000000002 /* AgentChatArtifactIndex.swift */; }; 5FAC71D5AC71D5AC71D50001 /* AgentChatChildRunTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5FAC71D5AC71D5AC71D50002 /* AgentChatChildRunTests.swift */; }; @@ -644,6 +646,7 @@ 6379A0016379A0016379A001 /* CLISSHPTYResizeInputTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6379A0026379A0026379A002 /* CLISSHPTYResizeInputTests.swift */; }; A7367001A1B2C3D4E5F60718 /* CLISSHSessionAttachAnchorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A7367002A1B2C3D4E5F60718 /* CLISSHSessionAttachAnchorTests.swift */; }; C12984000000000000000004 /* CLIStdioSIGPIPERegressionTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C12984000000000000000003 /* CLIStdioSIGPIPERegressionTests.swift */; }; + 6EC43EBD9373A772913CB237 /* CLITestProcessRunner.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5E880B3826C1A154F9C36A89 /* CLITestProcessRunner.swift */; }; B05553B10000000000000001 /* CLITmuxCompatRemoteSplitTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = B05553B10000000000000002 /* CLITmuxCompatRemoteSplitTests.swift */; }; 7837E0057837E0057837E005 /* CLITmuxCompatResizePaneTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7837E0067837E0067837E006 /* CLITmuxCompatResizePaneTests.swift */; }; C0DE11260000000000000001 /* CLITmuxCompatStoreConcurrencyTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C0DE11260000000000000002 /* CLITmuxCompatStoreConcurrencyTests.swift */; }; @@ -3484,6 +3487,8 @@ A5C017000000000000000008 /* AccountSignInView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AccountSignInView.swift; sourceTree = ""; }; A9E010000000000000000006 /* agent-session-react */ = {isa = PBXFileReference; lastKnownFileType = folder; path = "agent-session-react"; sourceTree = ""; }; A9E010000000000000000007 /* agent-session-solid */ = {isa = PBXFileReference; lastKnownFileType = folder; path = "agent-session-solid"; sourceTree = ""; }; + A10017010000000000000004 /* AgentApprovalCorrelationID.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AgentApprovalCorrelationID.swift; sourceTree = ""; }; + A10017010000000000000002 /* AgentApprovalNotificationCoordinator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AgentApprovalNotificationCoordinator.swift; sourceTree = ""; }; C7AF10000000000000000006 /* AgentChatArtifactGalleryBuilder.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = "AgentChatArtifactGalleryBuilder.swift"; sourceTree = ""; }; C7AF10000000000000000002 /* AgentChatArtifactIndex.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = "AgentChatArtifactIndex.swift"; sourceTree = ""; }; 5FAC71D5AC71D5AC71D50002 /* AgentChatChildRunTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AgentChatChildRunTests.swift; sourceTree = ""; }; @@ -4096,6 +4101,7 @@ 6379A0026379A0026379A002 /* CLISSHPTYResizeInputTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CLISSHPTYResizeInputTests.swift; sourceTree = ""; }; A7367002A1B2C3D4E5F60718 /* CLISSHSessionAttachAnchorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CLISSHSessionAttachAnchorTests.swift; sourceTree = ""; }; C12984000000000000000003 /* CLIStdioSIGPIPERegressionTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CLIStdioSIGPIPERegressionTests.swift; sourceTree = ""; }; + 5E880B3826C1A154F9C36A89 /* CLITestProcessRunner.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = CLITestProcessRunner.swift; sourceTree = ""; }; B05553B10000000000000002 /* CLITmuxCompatRemoteSplitTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CLITmuxCompatRemoteSplitTests.swift; sourceTree = ""; }; 7837E0067837E0067837E006 /* CLITmuxCompatResizePaneTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CLITmuxCompatResizePaneTests.swift; sourceTree = ""; }; C0DE11260000000000000002 /* CLITmuxCompatStoreConcurrencyTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CLITmuxCompatStoreConcurrencyTests.swift; sourceTree = ""; }; @@ -8257,6 +8263,8 @@ B8B056D80000000000000002 /* MobileHostIdentityTests.swift */ = {isa = PBXFileRef B79500090000000000000002 /* PortScanner+Publication.swift */, 6313B1A1 /* PaneMemoryDescriptor.swift */, 6313A1A1 /* PaneMemoryGuardrail.swift */, + A10017010000000000000004 /* AgentApprovalCorrelationID.swift */, + A10017010000000000000002 /* AgentApprovalNotificationCoordinator.swift */, A5D41235A1B2C3D4E5F60718 /* AgentNotificationGate.swift */, A77A0006A1B2C3D4E5F60001 /* AgentJournalLifecycleCenter.swift */, A0F23A5D074809C236E5C607 /* AgentNotificationAdmissionWaiters.swift */, @@ -10077,6 +10085,7 @@ B8B056D80000000000000002 /* MobileHostIdentityTests.swift */ = {isa = PBXFileRef C0D3F1F00000000000000104 /* CLICodexHookTimeoutRegressionTests.swift */, C0D3F1F20000000000000104 /* CLICodexYoloResumePersistenceTests.swift */, C0D3F1F10000000000000104 /* CLICodexHookTimeoutRegressionTestSupport.swift */, + 5E880B3826C1A154F9C36A89 /* CLITestProcessRunner.swift */, 918100000000000000000001 /* CLICodexResumeNotificationTests.swift */, C6711B010000000000000001 /* CLICodexWeakEnvironmentRestoreBindingTests.swift */, D96290020000000000000001 /* CodexResumeBindingVerificationRegressionTests.swift */, @@ -10813,6 +10822,8 @@ B8B056D80000000000000002 /* MobileHostIdentityTests.swift */ = {isa = PBXFileRef A5C017000000000000000009 /* AccountSignInPanel.swift in Sources */, A5C01700000000000000000B /* AccountSignInPanelView.swift in Sources */, A5C017000000000000000007 /* AccountSignInView.swift in Sources */, + A10017010000000000000003 /* AgentApprovalCorrelationID.swift in Sources */, + A10017010000000000000001 /* AgentApprovalNotificationCoordinator.swift in Sources */, C7AF10000000000000000005 /* AgentChatArtifactGalleryBuilder.swift in Sources */, C7AF10000000000000000001 /* AgentChatArtifactIndex.swift in Sources */, C7A52E000000000000000002 /* AgentChatEndedTranscriptListabilityCache.swift in Sources */, @@ -13424,6 +13435,7 @@ B8B056D80000000000000002 /* MobileHostIdentityTests.swift */ = {isa = PBXFileRef 6379A0016379A0016379A001 /* CLISSHPTYResizeInputTests.swift in Sources */, A7367001A1B2C3D4E5F60718 /* CLISSHSessionAttachAnchorTests.swift in Sources */, C12984000000000000000004 /* CLIStdioSIGPIPERegressionTests.swift in Sources */, + 6EC43EBD9373A772913CB237 /* CLITestProcessRunner.swift in Sources */, B05553B10000000000000001 /* CLITmuxCompatRemoteSplitTests.swift in Sources */, 7837E0057837E0057837E005 /* CLITmuxCompatResizePaneTests.swift in Sources */, C0DE11260000000000000001 /* CLITmuxCompatStoreConcurrencyTests.swift in Sources */, diff --git a/cmuxTests/AgentNotificationGateTests.swift b/cmuxTests/AgentNotificationGateTests.swift index a8b34edd69c0..94c1ebe45903 100644 --- a/cmuxTests/AgentNotificationGateTests.swift +++ b/cmuxTests/AgentNotificationGateTests.swift @@ -1,3 +1,4 @@ +import Foundation import CmuxSettings import Testing @@ -8,7 +9,7 @@ import Testing #endif /// Exhaustive decision-table coverage for the app-side agent notification gate -/// and the `c=;p=<0|1>` meta parser it consumes. +/// and the category/approval meta parser it consumes. @Suite struct AgentNotificationGateTests { @Test func needsPermissionFollowsToggleAndIgnoresPending() { for pending in [false, true] { @@ -75,6 +76,34 @@ import Testing #expect(c?.pending == true) } + @Test func metaParsesCanonicalApprovalCorrelation() { + let approvalID = "111111111111111111111111.aaaaaaaaaaaaaaaaaaaaaaaa" + let meta = AgentNotificationMeta( + meta: "c=needs-permission;p=0;a=\(approvalID)" + ) + + #expect(meta?.category == .needsPermission) + #expect(meta?.pending == false) + #expect(meta?.approvalID?.rawValue == approvalID) + #expect(meta?.approvalID?.scope.rawValue == "111111111111111111111111") + #expect(meta?.approvalIDIsDerived == false) + + let derived = AgentNotificationMeta( + meta: "c=needs-permission;p=0;a=\(approvalID);d=1" + ) + #expect(derived?.approvalIDIsDerived == true) + } + + @Test(arguments: ["hook", "feed"]) + func derivedApprovalMetadataAcceptsKnownTransport(source: String) throws { + let approvalID = "111111111111111111111111.aaaaaaaaaaaaaaaaaaaaaaaa" + let meta = try #require(AgentNotificationMeta(meta: "c=needs-permission;p=0;a=\(approvalID);d=1;o=\(source)")) + #expect(meta.approvalID?.rawValue == approvalID) + #expect(meta.approvalIDIsDerived) + #expect(AgentNotificationMeta(meta: "c=needs-permission;p=0;a=\(approvalID);d=1;o=unknown") == nil) + #expect(AgentNotificationMeta(meta: "c=needs-permission;p=0;a=\(approvalID);o=\(source)") == nil) + } + @Test func metaUnknownCategoryIsRejected() { // Only the three known category literals are wire-valid; anything else // (including "c=other") stays part of the legacy notification body. @@ -98,12 +127,16 @@ import Testing } @Test func metaRequiresExactCanonicalForm() { - // Only the CLI's exact canonical serialization parses; reordered, - // duplicated, or trailing fields stay part of the legacy body. + // Only the CLI's exact canonical serialization (including the + // approval extension) parses; reordered, duplicated, or trailing fields + // stay part of the legacy body. #expect(AgentNotificationMeta(meta: "c=turn-complete;p=1;note") == nil) #expect(AgentNotificationMeta(meta: "p=1;c=turn-complete") == nil) #expect(AgentNotificationMeta(meta: "c=turn-complete;c=turn-complete;p=1") == nil) #expect(AgentNotificationMeta(meta: "c=turn-complete;p=1;") == nil) + #expect(AgentNotificationMeta(meta: "c=turn-complete;p=1;a=111111111111111111111111.aaaaaaaaaaaaaaaaaaaaaaaa") == nil) + #expect(AgentNotificationMeta(meta: "c=needs-permission;p=0;a=not a token") == nil) + #expect(AgentNotificationMeta(meta: "c=needs-permission;p=0;a=111111111111111111111111.AAAAAAAAAAAAAAAAAAAAAAAA") == nil) } @Test func legacyTwoFieldMetaParsesWithoutAgentContext() { @@ -112,6 +145,8 @@ import Testing let parsed = AgentNotificationMeta(meta: "c=turn-complete;p=0") #expect(parsed?.category == .turnComplete) #expect(parsed?.pending == false) + #expect(parsed?.approvalID == nil) + #expect(parsed?.approvalIDIsDerived == false) #expect(parsed?.agentKind == nil) #expect(parsed?.isSubagent == nil) } @@ -227,3 +262,616 @@ import Testing ) } } + +@MainActor +@Suite struct AgentApprovalNotificationCoordinatorTests { + private static let workspaceID = UUID(uuidString: "11111111-2222-3333-4444-555555555555")! + private static let surfaceID = UUID(uuidString: "66666666-7777-8888-9999-AAAAAAAAAAAA")! + private static let firstApprovalID = AgentApprovalCorrelationID( + rawValue: "111111111111111111111111.aaaaaaaaaaaaaaaaaaaaaaaa" + )! + private static let secondApprovalID = AgentApprovalCorrelationID( + rawValue: "111111111111111111111111.bbbbbbbbbbbbbbbbbbbbbbbb" + )! + + @Test func autoResolvedApprovalProducesNoNotification() { + let fixture = Fixture() + + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "shell needs approval", + approvalID: Self.firstApprovalID + ) + fixture.coordinator.resolve( + surfaceID: Self.surfaceID, + approvalID: Self.firstApprovalID + ) + fixture.scheduler.runAll() + + #expect(fixture.deliveries.isEmpty) + #expect(fixture.clears.isEmpty) + } + + @Test func genuinelyBlockingApprovalNotifiesAfterSettleWindow() { + let fixture = Fixture() + + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "shell needs approval", + approvalID: Self.firstApprovalID + ) + #expect(fixture.deliveries.isEmpty) + + fixture.scheduler.runAll() + + #expect(fixture.deliveries.count == 1) + #expect(fixture.deliveries.first?.body == "shell needs approval") + } + + @Test func delayedApprovalPreservesAgentContextAndProducerCorrelation() throws { + let fixture = Fixture() + let context = TerminalNotificationPolicyAgentContext( + kind: "codex", + category: AgentNotifyCategory.needsPermission.rawValue, + pending: true, + isSubagent: true + ) + let producerKey = "11111111-1111-1111-1111-111111111111" + + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "shell needs approval", + approvalID: Self.firstApprovalID, + agent: context, + producerCorrelationKey: producerKey + ) + fixture.scheduler.runAll() + + let delivery = try #require(fixture.deliveries.first) + #expect(delivery.agent == context) + #expect(delivery.producerCorrelationKey == producerKey) + } + + @Test func candidateOverflowKeepsDisplayedApprovalResolvable() throws { + let fixture = Fixture() + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "displayed approval", + approvalID: Self.firstApprovalID + ) + fixture.scheduler.runAll() + _ = try #require(fixture.deliveries.first) + + for index in 0..<64 { + let suffix = String(format: "%024x", index + 1) + let approvalID = AgentApprovalCorrelationID( + rawValue: "111111111111111111111111.\(suffix)" + )! + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "queued \(index)", + approvalID: approvalID + ) + } + + fixture.coordinator.resolve( + surfaceID: Self.surfaceID, + approvalID: Self.firstApprovalID + ) + #expect(fixture.clears.count == 1) + } + + @Test func deliveredApprovalClearsWhenItResolves() throws { + let fixture = Fixture() + + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "shell needs approval", + approvalID: Self.firstApprovalID + ) + fixture.scheduler.runAll() + let delivery = try #require(fixture.deliveries.first) + + fixture.coordinator.resolve( + surfaceID: Self.surfaceID, + approvalID: Self.firstApprovalID + ) + + #expect(fixture.clears.count == 1) + #expect(fixture.clears.first?.surfaceID == Self.surfaceID) + #expect(fixture.clears.first?.correlationKey == delivery.correlationKey) + } + + @Test func pendingApprovalsInOnePaneCoalesceIntoOneNotification() { + let fixture = Fixture() + + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "first tool needs approval", + approvalID: Self.firstApprovalID + ) + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "second tool needs approval", + approvalID: Self.secondApprovalID + ) + fixture.coordinator.resolve( + surfaceID: Self.surfaceID, + approvalID: Self.firstApprovalID + ) + fixture.scheduler.runAll() + + #expect(fixture.deliveries.count == 1) + #expect(fixture.deliveries.first?.body == "second tool needs approval") + #expect(fixture.clears.isEmpty) + + // The older request may resolve after the shared banner is already + // visible. Its completion must not clear the still-pending second one. + fixture.coordinator.resolve( + surfaceID: Self.surfaceID, + approvalID: Self.firstApprovalID + ) + #expect(fixture.deliveries.count == 1) + #expect(fixture.clears.isEmpty) + } + + @Test func resolvingDisplayedApprovalRefreshesCoalescedNotification() throws { + let fixture = Fixture() + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "first tool needs approval", + approvalID: Self.firstApprovalID + ) + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "newer tool needs approval", + approvalID: Self.secondApprovalID + ) + fixture.scheduler.runAll() + let displayed = try #require(fixture.deliveries.first) + #expect(displayed.body == "newer tool needs approval") + + fixture.coordinator.resolve( + surfaceID: Self.surfaceID, + approvalID: Self.secondApprovalID + ) + fixture.scheduler.runAll() + + #expect(fixture.clears.count == 1) + #expect(fixture.clears.first?.correlationKey == displayed.correlationKey) + #expect(fixture.deliveries.count == 2) + #expect(fixture.deliveries.last?.body == "first tool needs approval") + } + + @Test func turnResolutionCancelsDeniedApprovalBeforeDelivery() { + let fixture = Fixture() + + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "shell needs approval", + approvalID: Self.firstApprovalID + ) + fixture.coordinator.resolve( + surfaceID: Self.surfaceID, + approvalScope: Self.firstApprovalID.scope + ) + fixture.scheduler.runAll() + + #expect(fixture.deliveries.isEmpty) + } + + @Test func reorderedOldResolutionDoesNotCancelNewApproval() { + let fixture = Fixture() + + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "new tool needs approval", + approvalID: Self.secondApprovalID + ) + fixture.coordinator.resolve( + surfaceID: Self.surfaceID, + approvalID: Self.firstApprovalID + ) + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "old tool needs approval", + approvalID: Self.firstApprovalID + ) + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "duplicate old tool needs approval", + approvalID: Self.firstApprovalID + ) + fixture.scheduler.runAll() + + #expect(fixture.deliveries.count == 1) + #expect(fixture.deliveries.first?.body == "new tool needs approval") + } + + @Test(arguments: [false, true]) + func derivedTransportDuplicatesOnlyBecomeAmbiguousOnRepeatedSource(repeatSource: Bool) { + let fixture = Fixture() + let sources = repeatSource ? ["hook", "feed", "hook", "feed"] : ["hook", "feed"] + for source in sources { + fixture.coordinator.stage(workspaceID: Self.workspaceID, surfaceID: Self.surfaceID, + title: "Codex", subtitle: "Permission", body: "approval", approvalID: Self.firstApprovalID, + isDerived: true, approvalSource: source) + } + fixture.coordinator.resolve(surfaceID: Self.surfaceID, approvalID: Self.firstApprovalID) + fixture.scheduler.advance(by: 0.2) + #expect(fixture.deliveries.count == (repeatSource ? 1 : 0)) + fixture.coordinator.resolve(surfaceID: Self.surfaceID, approvalScope: Self.firstApprovalID.scope) + #expect(fixture.clears.count == (repeatSource ? 1 : 0)) + } + + @Test(arguments: [false, true]) + func completionFallbackOnlyResolvesUnambiguousDerivedRequests(ambiguous: Bool) { + let fixture = Fixture() + for _ in 0..<(ambiguous ? 2 : 1) { + fixture.coordinator.stage(workspaceID: Self.workspaceID, surfaceID: Self.surfaceID, + title: "Codex", subtitle: "Permission", body: "derived request", approvalID: Self.firstApprovalID, + isDerived: true) + } + fixture.coordinator.resolve(surfaceID: Self.surfaceID, approvalID: Self.secondApprovalID, + fallbackApprovalID: Self.firstApprovalID) + fixture.scheduler.advance(by: 0.2) + #expect(fixture.deliveries.count == (ambiguous ? 1 : 0)) + } + + @Test func nativeCompletionDoesNotClearAnotherDerivedCandidate() { + let fixture = Fixture() + fixture.coordinator.stage(workspaceID: Self.workspaceID, surfaceID: Self.surfaceID, + title: "Codex", subtitle: "Permission", body: "derived request", approvalID: Self.firstApprovalID, + isDerived: true) + fixture.coordinator.stage(workspaceID: Self.workspaceID, surfaceID: Self.surfaceID, + title: "Codex", subtitle: "Permission", body: "native request", approvalID: Self.secondApprovalID) + fixture.coordinator.resolve(surfaceID: Self.surfaceID, approvalID: Self.secondApprovalID, + fallbackApprovalID: Self.firstApprovalID) + fixture.coordinator.resolve(surfaceID: Self.surfaceID, approvalID: Self.secondApprovalID, + fallbackApprovalID: Self.firstApprovalID) + fixture.scheduler.advance(by: 0.2) + #expect(fixture.deliveries.map(\.body) == ["derived request"]) + } + + @Test func duplicateApprovalSignalsRequireScopeResolution() { + let fixture = Fixture() + + for body in ["first identical call", "second identical call"] { + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: body, + approvalID: Self.firstApprovalID, + isDerived: true + ) + } + fixture.scheduler.runAll() + #expect(fixture.deliveries.count == 1) + + fixture.coordinator.resolve( + surfaceID: Self.surfaceID, + approvalID: Self.firstApprovalID + ) + // Without a provider call id, the identical signals may represent two + // distinct executions. An exact completion must fail closed instead of + // clearing both through one derived identity. + #expect(fixture.clears.isEmpty) + + fixture.coordinator.resolve( + surfaceID: Self.surfaceID, + approvalScope: Self.firstApprovalID.scope + ) + #expect(fixture.clears.count == 1) + + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: "delayed duplicate", + approvalID: Self.firstApprovalID + ) + fixture.scheduler.runAll() + + #expect(fixture.deliveries.count == 1) + #expect(fixture.clears.count == 1) + } + + @Test func authoritativeDuplicateSignalsResolveByExactID() { + let fixture = Fixture() + + for body in ["first delivery", "retry delivery"] { + fixture.coordinator.stage( + workspaceID: Self.workspaceID, + surfaceID: Self.surfaceID, + title: "Codex", + subtitle: "Permission", + body: body, + approvalID: Self.firstApprovalID, + isDerived: false + ) + } + fixture.scheduler.runAll() + fixture.coordinator.resolve( + surfaceID: Self.surfaceID, + approvalID: Self.firstApprovalID + ) + + #expect(fixture.clears.count == 1) + } + + @Test func staleSurfaceFanoutIsBounded() { + let fixture = Fixture() + let count = AgentApprovalNotificationCoordinator.maxTrackedPanes + 32 + + for index in 0.. AgentApprovalNotificationCoordinator.Cancellation { + let entry = Entry(deadline: now + delay, action: action) + entries.append(entry) + return { entry.isCancelled = true } + } + + func runAll() { + run(until: .infinity) + } + + func advance(by interval: TimeInterval) { + run(until: now + interval) + } + + private func run(until deadline: TimeInterval) { + while let index = entries.indices.min(by: { + entries[$0].deadline < entries[$1].deadline + }) { + let entry = entries[index] + if entry.isCancelled { + entries.remove(at: index) + continue + } + guard entry.deadline <= deadline + 0.000_001 else { break } + entries.remove(at: index) + now = max(now, entry.deadline) + entry.action() + } + if deadline.isFinite { + now = max(now, deadline) + } + } + } +} diff --git a/cmuxTests/AgentNotificationMutationBoundaryTests.swift b/cmuxTests/AgentNotificationMutationBoundaryTests.swift index de307e367dcf..0a531eeb9610 100644 --- a/cmuxTests/AgentNotificationMutationBoundaryTests.swift +++ b/cmuxTests/AgentNotificationMutationBoundaryTests.swift @@ -10,6 +10,152 @@ import Testing #endif extension AgentNotificationRegressionTests { + private func waitForApprovalControl(_ fixture: Fixture, body: String) async throws { + let deadline = ContinuousClock.now + .seconds(15) + while !fixture.store.notifications.contains(where: { $0.body == body }), ContinuousClock.now < deadline { + TerminalMutationBus.shared.drainForTesting() + try await Task.sleep(for: .milliseconds(10)) + } + TerminalMutationBus.shared.drainForTesting() + #expect(fixture.store.notifications.contains { $0.body == body }) + } + + @Test func queuedApprovalResolutionsSurviveFirstUseAndLaterEpisodes() async throws { + let fixture = try makeFixture() + defer { fixture.restore() } + let bus = TerminalMutationBus.shared + let controlSurface = try #require(fixture.destination.focusedPanelId) + defer { + bus.setDrainsSuspendedForTesting(false) + bus.cancelAgentApproval(surfaceID: fixture.panelId) + bus.cancelAgentApproval(surfaceID: controlSurface) + bus.discardPendingNotifications() + } + for episode in 1...2 { + bus.setDrainsSuspendedForTesting(true) + let first = try #require(AgentApprovalCorrelationID(rawValue: "111111111111111111111111.\(String(format: "%024x", episode * 2))")) + let second = try #require(AgentApprovalCorrelationID(rawValue: "111111111111111111111111.\(String(format: "%024x", episode * 2 + 1))")) + for approvalID in [first, second] { + bus.enqueueAgentApprovalResolution(surfaceId: fixture.panelId, approvalID: approvalID) + } + for approvalID in [first, second] { + bus.enqueueAgentApprovalNotification(tabId: fixture.source.id, surfaceId: fixture.panelId, + title: "Codex", subtitle: "", body: "auto-approved", approvalID: approvalID) + } + let controlID = try #require(AgentApprovalCorrelationID(rawValue: "222222222222222222222222.\(String(format: "%024x", episode))")) + bus.enqueueAgentApprovalNotification(tabId: fixture.destination.id, surfaceId: controlSurface, + title: "Control", subtitle: "", body: "control-\(episode)", approvalID: controlID) + bus.setDrainsSuspendedForTesting(false) + bus.drainForTesting() + try await waitForApprovalControl(fixture, body: "control-\(episode)") + #expect(!fixture.store.notifications.contains { $0.body == "auto-approved" }) + bus.enqueueAgentApprovalResolution(surfaceId: controlSurface, approvalID: controlID) + bus.drainForTesting() + } + } + + @Test func queuedDerivedRepetitionsRemainAmbiguous() async throws { + let fixture = try makeFixture() + defer { fixture.restore() } + let bus = TerminalMutationBus.shared + let approvalID = try #require(AgentApprovalCorrelationID(rawValue: "333333333333333333333333.aaaaaaaaaaaaaaaaaaaaaaaa")) + bus.setDrainsSuspendedForTesting(true) + defer { + bus.setDrainsSuspendedForTesting(false) + bus.cancelAgentApproval(surfaceID: fixture.panelId) + bus.discardPendingNotifications() + } + for _ in 0..<2 { + bus.enqueueAgentApprovalNotification(tabId: fixture.source.id, surfaceId: fixture.panelId, + title: "Codex", subtitle: "", body: "ambiguous approval", approvalID: approvalID, approvalIDIsDerived: true) + } + bus.enqueueAgentApprovalResolution(surfaceId: fixture.panelId, approvalID: approvalID) + bus.setDrainsSuspendedForTesting(false) + bus.drainForTesting() + try await waitForApprovalControl(fixture, body: "ambiguous approval") + bus.enqueueAgentApprovalResolution(surfaceId: fixture.panelId, approvalScope: approvalID.scope) + bus.drainForTesting() + #expect(fixture.store.notifications.isEmpty) + } + + @Test func newerWorkspaceClaimDoesNotInvalidateQueuedApprovalCompletion() async throws { + let fixture = try makeFixture() + defer { fixture.restore() } + let bus = TerminalMutationBus.shared + let resolvedID = try #require(AgentApprovalCorrelationID(rawValue: "555555555555555555555555.aaaaaaaaaaaaaaaaaaaaaaaa")) + let waitingID = try #require(AgentApprovalCorrelationID(rawValue: "555555555555555555555555.bbbbbbbbbbbbbbbbbbbbbbbb")) + defer { + bus.setDrainsSuspendedForTesting(false) + bus.cancelAgentApproval(surfaceID: fixture.panelId) + bus.discardPendingNotifications() + } + bus.enqueueAgentApprovalNotification(tabId: fixture.source.id, surfaceId: fixture.panelId, + title: "Codex", subtitle: "", body: "already resolved", approvalID: resolvedID) + bus.drainForTesting() + bus.setDrainsSuspendedForTesting(true) + bus.enqueueAgentApprovalResolution(surfaceId: fixture.panelId, approvalID: resolvedID) + bus.enqueueAgentApprovalNotification(tabId: fixture.destination.id, surfaceId: fixture.panelId, + title: "Codex", subtitle: "", body: "already resolved", approvalID: resolvedID) + bus.enqueueAgentApprovalNotification(tabId: fixture.destination.id, surfaceId: fixture.panelId, + title: "Codex", subtitle: "", body: "new waiting approval", approvalID: waitingID) + try movePanel(fixture) + bus.setDrainsSuspendedForTesting(false) + bus.drainForTesting() + try await waitForApprovalControl(fixture, body: "new waiting approval") + #expect(fixture.store.notifications.map(\.body) == ["new waiting approval"]) + #expect(fixture.store.notifications.first?.tabId == fixture.destination.id) + } + + @Test(arguments: [false, true]) + func ordinaryNotificationDoesNotRetirePendingApproval(correlated: Bool) async throws { + let fixture = try makeFixture() + defer { fixture.restore() } + let bus = TerminalMutationBus.shared + let approvalID = try #require(AgentApprovalCorrelationID(rawValue: "666666666666666666666666.aaaaaaaaaaaaaaaaaaaaaaaa")) + defer { + bus.cancelAgentApproval(surfaceID: fixture.panelId) + bus.discardPendingNotifications() + } + bus.enqueueAgentApprovalNotification(tabId: fixture.source.id, surfaceId: fixture.panelId, + title: "Codex", subtitle: "", body: "still waiting", approvalID: approvalID) + try await waitForApprovalControl(fixture, body: "still waiting") + bus.enqueueNotification(tabId: fixture.source.id, surfaceId: fixture.panelId, + title: "Other", subtitle: "", body: "unrelated result", + correlationKey: correlated ? UUID().uuidString : nil) + try await waitForApprovalControl(fixture, body: "unrelated result") + #expect(fixture.store.notifications.contains { $0.body == "still waiting" }) + bus.enqueueAgentApprovalResolution(surfaceId: fixture.panelId, approvalID: approvalID) + bus.drainForTesting() + #expect(fixture.store.notifications.map(\.body) == ["unrelated result"]) + } + + @Test func cancellingOneApprovalKeepsOtherPaneAliases() async throws { + let fixture = try makeFixture() + defer { fixture.restore() } + let bus = TerminalMutationBus.shared + let secondSurface = try #require(fixture.destination.focusedPanelId) + let approvalID = try #require(AgentApprovalCorrelationID(rawValue: "444444444444444444444444.aaaaaaaaaaaaaaaaaaaaaaaa")) + defer { + bus.cancelAgentApproval(surfaceID: fixture.panelId) + bus.cancelAgentApproval(surfaceID: secondSurface) + bus.discardPendingNotifications() + } + for (workspaceID, surfaceID, producer) in [ + (fixture.source.id, fixture.panelId, "first-producer"), + (fixture.destination.id, secondSurface, "second-producer"), + ] { + bus.enqueueAgentApprovalNotification(tabId: workspaceID, surfaceId: surfaceID, + title: "Codex", subtitle: "", body: producer, approvalID: approvalID, producerCorrelationKey: producer) + } + try await waitForApprovalControl(fixture, body: "first-producer") + try await waitForApprovalControl(fixture, body: "second-producer") + #expect(bus.resolvedApprovalCorrelationKey(surfaceID: fixture.panelId, producerCorrelationKey: "first-producer") != nil) + let secondAlias = try #require(bus.resolvedApprovalCorrelationKey(surfaceID: secondSurface, producerCorrelationKey: "second-producer")) + bus.cancelAgentApproval(surfaceID: fixture.panelId) + #expect(bus.resolvedApprovalCorrelationKey(surfaceID: fixture.panelId, producerCorrelationKey: "first-producer") == nil) + #expect(bus.resolvedApprovalCorrelationKey(surfaceID: secondSurface, producerCorrelationKey: "second-producer") == secondAlias) + } + // Generous for loaded CI runners: subprocess spawn, signal propagation, // and marker writes can take multiple seconds there. A long timeout only // slows the failure path. diff --git a/cmuxTests/AgentSemanticNotificationDeliveryTests.swift b/cmuxTests/AgentSemanticNotificationDeliveryTests.swift index 6709b401d897..974dba4c7ca1 100644 --- a/cmuxTests/AgentSemanticNotificationDeliveryTests.swift +++ b/cmuxTests/AgentSemanticNotificationDeliveryTests.swift @@ -43,7 +43,7 @@ extension AgentNotificationRegressionTests { } @Test(arguments: ["claude", "codex"]) - func semanticReplayAfterReadDoesNotRepeatEffects(source: String) throws { + func semanticReplayAfterReadDoesNotRepeatEffects(source: String) async throws { let fixture = try makeFixture() defer { fixture.restore() } let url = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) @@ -58,6 +58,7 @@ extension AgentNotificationRegressionTests { #expect(AgentJournalLifecycleCenter.claimNotification(first, decision: decision, store: journal)) AgentJournalLifecycleCenter.deliverNotification(first, identity: try #require(decision.identity)) TerminalMutationBus.shared.drainForTesting() + await waitForNotification(in: fixture.store) #expect(deliveries.count == 1) let effects = try #require(deliveries.first) #expect(effects.desktop && effects.sound && effects.command) @@ -74,7 +75,7 @@ extension AgentNotificationRegressionTests { } @Test(arguments: ["claude", "codex"]) - func semanticNotificationFollowsMovedSurfaceAndRejectsMissingSurface(source: String) throws { + func semanticNotificationFollowsMovedSurfaceAndRejectsMissingSurface(source: String) async throws { let fixture = try makeFixture() defer { fixture.restore() } let event = semanticEvent(fixture, source: source) @@ -82,6 +83,7 @@ extension AgentNotificationRegressionTests { #expect(AgentJournalLifecycleCenter.notificationTargetIsCurrent(event.draft)) AgentJournalLifecycleCenter.deliverNotification(event, identity: "semantic-event") TerminalMutationBus.shared.drainForTesting() + await waitForNotification(in: fixture.store) #expect(fixture.store.notifications.map(\.tabId) == [fixture.destination.id]) #expect(!fixture.store.hasUnreadNotification(forTabId: fixture.source.id, surfaceId: fixture.panelId)) var stale = event.draft @@ -90,7 +92,7 @@ extension AgentNotificationRegressionTests { } @Test(arguments: ["claude", "codex"]) - func continuationCancelsQueuedSemanticEffectsWithoutClearingLaterApproval(source: String) throws { + func continuationCancelsQueuedSemanticEffectsWithoutClearingLaterApproval(source: String) async throws { let fixture = try makeFixture() defer { fixture.restore() } var reconciler = AgentNotificationReconciler() @@ -107,8 +109,53 @@ extension AgentNotificationRegressionTests { let next = reconciler.apply(later) AgentJournalLifecycleCenter.deliverNotification(later, identity: try #require(next.identity)) TerminalMutationBus.shared.drainForTesting() + await waitForNotification(in: fixture.store) #expect(fixture.store.notifications.count == 1) - #expect(fixture.store.notifications.first?.correlationKey == next.identity) + if source == "codex" { + #expect(AgentApprovalNotificationCoordinator.isApprovalCorrelationKey(fixture.store.notifications.first?.correlationKey)) + } else { + #expect(fixture.store.notifications.first?.correlationKey == next.identity) + } + } + + @Test func semanticCodexApprovalSettlesAndResolvesWithoutHidingOtherRequests() async throws { + let fixture = try makeFixture() + defer { fixture.restore() } + let bus = TerminalMutationBus.shared + defer { + bus.cancelAgentApproval(surfaceID: fixture.panelId) + bus.discardPendingNotifications() + } + var reconciler = AgentNotificationReconciler() + func stage(_ request: String, sequence: Int64) throws -> AgentJournalEvent { + let event = semanticEvent(fixture, source: "codex", sequence: sequence, request: request) + let decision = reconciler.apply(event) + AgentJournalLifecycleCenter.deliverNotification(event, identity: try #require(decision.identity)) + return event + } + func resolve(_ request: AgentJournalEvent, sequence: Int64) { + var draft = request.draft + draft.kind = .attentionResolved + draft.occurredAtMs = sequence + draft.attention?.notification = nil + let event = AgentJournalEvent(sequence: sequence, committedAtMs: sequence, draft: draft) + AgentJournalLifecycleCenter.clearInvalidatedNotifications(event, decision: reconciler.apply(event)) + } + let transient = try stage("auto-approved", sequence: 1) + bus.drainForTesting() + #expect(fixture.store.notifications.isEmpty) + resolve(transient, sequence: 2) + let blocking = try stage("blocking", sequence: 3) + let concurrent = try stage("concurrent", sequence: 4) + bus.drainForTesting() + await waitForNotification(in: fixture.store) + #expect(fixture.store.notifications.count == 1) + resolve(blocking, sequence: 5) + bus.drainForTesting() + #expect(fixture.store.notifications.count == 1) + resolve(concurrent, sequence: 6) + bus.drainForTesting() + #expect(fixture.store.notifications.isEmpty) } @Test(arguments: ["claude", "codex"]) func finalDeliveryRejectsReplacedSessionButAcceptsItsResume(source: String) throws { diff --git a/cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift b/cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift index b71c62fb1900..c148ba12e088 100644 --- a/cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift +++ b/cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift @@ -105,7 +105,9 @@ func startCodexHookMockSocketServerAccepting( surfaceId: String, connectionLimit: Int ) { - DispatchQueue.global(qos: .userInitiated).async { + // Accept and per-client loops block indefinitely; keep them off the + // libdispatch pool so parallel suites cannot starve each other. + CLITestProcessRunner.detachBlockingThread(name: "cmux-test-codex-mock-accept") { var accepted = 0 while accepted < connectionLimit { var clientAddr = sockaddr_un() @@ -120,7 +122,7 @@ func startCodexHookMockSocketServerAccepting( return } accepted += 1 - DispatchQueue.global(qos: .userInitiated).async { + CLITestProcessRunner.detachBlockingThread(name: "cmux-test-codex-mock-client") { handleCodexHookMockSocketClient(fd: clientFD, commands: commands, surfaceId: surfaceId) } } @@ -168,6 +170,20 @@ func codexHookMockSocketResponse(for line: String, surfaceId: String) -> String result: ["surfaces": [["id": surfaceId, "ref": surfaceId, "focused": true]]] ) } + if payload["method"] as? String == "agent.resolve_delivery_target", + let params = payload["params"] as? [String: Any], + let workspaceId = params["workspace_id"] as? String, + let requestedSurfaceId = params["surface_id"] as? String { + return codexHookV2Response( + id: id, + ok: true, + result: [ + "source": "surface", + "workspace_id": workspaceId, + "surface_id": requestedSurfaceId == surfaceId ? surfaceId : requestedSurfaceId, + ] + ) + } return codexHookV2Response(id: id, ok: true, result: [:]) } @@ -194,49 +210,20 @@ func runCodexHookProcess( standardInput: String? = nil, timeout: TimeInterval ) -> CodexHookProcessRunResult { - let process = Process() - let stdoutPipe = Pipe() - let stderrPipe = Pipe() - let stdinPipe = standardInput == nil ? nil : Pipe() - process.executableURL = URL(fileURLWithPath: executablePath) - process.arguments = arguments - process.environment = environment - process.standardInput = stdinPipe ?? FileHandle.nullDevice - process.standardOutput = stdoutPipe - process.standardError = stderrPipe - - do { - try process.run() - } catch { - return CodexHookProcessRunResult(status: -1, stdout: "", stderr: String(describing: error), timedOut: false) - } - if let standardInput, let stdinPipe { - stdinPipe.fileHandleForWriting.write(Data(standardInput.utf8)) - try? stdinPipe.fileHandleForWriting.close() - } - - let exitSignal = DispatchSemaphore(value: 0) - DispatchQueue.global(qos: .userInitiated).async { - process.waitUntilExit() - exitSignal.signal() - } - - let timedOut = exitSignal.wait(timeout: .now() + timeout) == .timedOut - if timedOut { - process.terminate() - if exitSignal.wait(timeout: .now() + 1) == .timedOut { - kill(process.processIdentifier, SIGKILL) - _ = exitSignal.wait(timeout: .now() + 1) - } - } - - let stdoutData = stdoutPipe.fileHandleForReading.readDataToEndOfFile() - let stderrData = stderrPipe.fileHandleForReading.readDataToEndOfFile() + // Runs on dedicated threads (CLITestProcessRunner) so a saturated + // libdispatch pool during parallel Swift Testing cannot hide the exit. + let outcome = CLITestProcessRunner.run( + executablePath: executablePath, + arguments: arguments, + environment: environment, + standardInput: standardInput, + timeout: timeout + ) return CodexHookProcessRunResult( - status: process.terminationStatus, - stdout: String(data: stdoutData, encoding: .utf8) ?? "", - stderr: String(data: stderrData, encoding: .utf8) ?? "", - timedOut: timedOut + status: outcome.status, + stdout: outcome.stdout, + stderr: outcome.stderr, + timedOut: outcome.timedOut ) } @@ -272,3 +259,19 @@ func waitForConditionBlocking( } return condition() } + +/// Requires `condition` to remain true for the full interval. This is useful +/// for negative assertions around delayed hook delivery: a single snapshot can +/// observe the settle window before a notification is emitted. +func waitForConditionToRemainTrueBlocking( + duration: TimeInterval, + pollInterval: TimeInterval = 0.02, + _ condition: () -> Bool +) -> Bool { + let deadline = Date().addingTimeInterval(duration) + while Date() < deadline { + guard condition() else { return false } + Thread.sleep(forTimeInterval: pollInterval) + } + return condition() +} diff --git a/cmuxTests/CLICodexHookTimeoutRegressionTests.swift b/cmuxTests/CLICodexHookTimeoutRegressionTests.swift index 0f98bafeca45..5d0871335c6a 100644 --- a/cmuxTests/CLICodexHookTimeoutRegressionTests.swift +++ b/cmuxTests/CLICodexHookTimeoutRegressionTests.swift @@ -4,6 +4,24 @@ import Testing @Suite(.serialized) struct CLICodexHookTimeoutRegressionTests { + @Test func codexPermissionPromptProtocolEndToEnd() throws { + let cliPath = try bundledCLIPath() + let root = FileManager.default.temporaryDirectory + .appendingPathComponent("cmux-codex-protocol-\(UUID().uuidString)", isDirectory: true) + try FileManager.default.createDirectory(at: root, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: root) } + let script = URL(fileURLWithPath: #filePath) + .deletingLastPathComponent().deletingLastPathComponent() + .appendingPathComponent("tests/test_codex_permission_prompt_notification.py") + var environment = codexHookTestEnvironment(root: root, codexHome: root.appendingPathComponent(".codex")) + environment["CMUX_CLI_BIN"] = cliPath + environment["CMUX_AGENT_HOOK_STATE_DIR"] = root.appendingPathComponent("hook-state").path + let result = runCodexHookProcess(executablePath: "/usr/bin/python3", arguments: [script.path], + environment: environment, timeout: 120) + #expect(result.status == 0, Comment(rawValue: result.stdout + result.stderr)) + #expect(result.stdout.contains("PASS: codex permission prompts notify, acknowledge, and resolve")) + } + @Test func codexHookInstallReplacesSynchronousBundledHook() throws { let cliPath = try bundledCLIPath() let root = FileManager.default.temporaryDirectory @@ -167,6 +185,22 @@ struct CLICodexHookTimeoutRegressionTests { connectionLimit: 16 ) + let approvalObject: [String: Any] = [ + "session_id": "codex-permission-session", + "turn_id": "turn-1", + "cwd": root.path, + "hook_event_name": "PermissionRequest", + "tool_name": "shell", + "tool_input": ["command": "git status"], + "message": "approval required", + ] + let approvalData = try JSONSerialization.data(withJSONObject: approvalObject, options: [.sortedKeys]) + let approvalInput = try #require(String(data: approvalData, encoding: .utf8)) + let approvalIdentity = try #require(CodexApprovalNotificationIdentity.make( + rawObject: approvalObject, + fallbackSessionID: nil + )) + let result = runCodexHookProcess( executablePath: cliPath, arguments: ["hooks", "codex", "notification"], @@ -180,7 +214,7 @@ struct CLICodexHookTimeoutRegressionTests { "CMUX_AGENT_HOOK_STATE_DIR": root.path, "CMUX_CLI_SENTRY_DISABLED": "1", ], - standardInput: #"{"session_id":"codex-permission-session","cwd":"\#(root.path)","hook_event_name":"PermissionRequest","message":"approval required"}"#, + standardInput: approvalInput, timeout: 5 ) @@ -204,6 +238,153 @@ struct CLICodexHookTimeoutRegressionTests { && capture.workspaceId == workspaceId && capture.surfaceId == surfaceId }) + #expect(commands.snapshot().contains { + $0.hasPrefix("notify_target_async \(workspaceId) \(surfaceId) ") + && $0.hasSuffix("|c=needs-permission;p=0;a=\(approvalIdentity.approvalID);d=1;o=hook") + }) + + let completionObject: [String: Any] = [ + "session_id": "codex-permission-session", + "turn_id": "turn-1", + "cwd": root.path, + "hook_event_name": "PostToolUse", + "tool_name": "shell", + "tool_input": ["command": "git status"], + "tool_response": ["exit_code": 0], + "tool_use_id": "post-review-only-id", + ] + let completionData = try JSONSerialization.data(withJSONObject: completionObject, options: [.sortedKeys]) + let completionInput = try #require(String(data: completionData, encoding: .utf8)) + let completion = runCodexHookProcess( + executablePath: cliPath, + arguments: ["hooks", "codex", "post-tool-use"], + environment: [ + "HOME": root.path, + "PATH": "/usr/bin:/bin:/usr/sbin:/sbin", + "PWD": root.path, + "CMUX_SOCKET_PATH": socketPath, + "CMUX_WORKSPACE_ID": workspaceId, + "CMUX_SURFACE_ID": surfaceId, + "CMUX_AGENT_HOOK_STATE_DIR": root.path, + "CMUX_CLI_SENTRY_DISABLED": "1", + ], + standardInput: completionInput, + timeout: 5 + ) + + #expect(!completion.timedOut, Comment(rawValue: completion.stderr)) + #expect(completion.status == 0, Comment(rawValue: completion.stderr)) + #expect(waitForConditionBlocking(timeout: 1) { + commands.snapshot().contains( + "clear_notifications --tab=\(workspaceId) --panel=\(surfaceId) --approval-id=\(approvalIdentity.approvalID)" + ) + }) + + let stop = runCodexHookProcess( + executablePath: cliPath, + arguments: ["hooks", "codex", "stop"], + environment: [ + "HOME": root.path, + "PATH": "/usr/bin:/bin:/usr/sbin:/sbin", + "PWD": root.path, + "CMUX_SOCKET_PATH": socketPath, + "CMUX_WORKSPACE_ID": workspaceId, + "CMUX_SURFACE_ID": surfaceId, + "CMUX_AGENT_HOOK_STATE_DIR": root.path, + "CMUX_CLI_SENTRY_DISABLED": "1", + ], + standardInput: """ + {"session_id":"codex-permission-session","turn_id":"turn-1","cwd":"\(root.path)","hook_event_name":"Stop","last_assistant_message":"Denied"} + """, + timeout: 5 + ) + + #expect(!stop.timedOut, Comment(rawValue: stop.stderr)) + #expect(stop.status == 0, Comment(rawValue: stop.stderr)) + #expect(waitForConditionBlocking(timeout: 1) { + commands.snapshot().contains( + "clear_notifications --tab=\(workspaceId) --panel=\(surfaceId) --approval-scope=\(approvalIdentity.scope)" + ) + }) + } + + @Test func codexAutoReviewedPermissionRequestProducesNoUserAttention() throws { + let cliPath = try bundledCLIPath() + let root = FileManager.default.temporaryDirectory + .appendingPathComponent("cmux-codex-auto-review-\(UUID().uuidString)", isDirectory: true) + let transcriptURL = root.appendingPathComponent("rollout.jsonl", isDirectory: false) + let socketPath = makeCodexHookSocketPath("codex-auto-review") + let listenerFD = try bindCodexHookUnixSocket(at: socketPath) + let commands = CodexHookCapturedSocketCommands() + let workspaceId = "33333333-3333-3333-3333-333333333333" + let surfaceId = "44444444-4444-4444-4444-444444444444" + try FileManager.default.createDirectory(at: root, withIntermediateDirectories: true) + try """ + {"type":"turn_context","payload":{"turn_id":"turn-auto","approvals_reviewer":"auto_review"}} + """.write(to: transcriptURL, atomically: true, encoding: .utf8) + defer { + Darwin.close(listenerFD) + unlink(socketPath) + try? FileManager.default.removeItem(at: root) + } + startCodexHookMockSocketServerAccepting( + listenerFD: listenerFD, + commands: commands, + surfaceId: surfaceId, + connectionLimit: 8 + ) + + let approvalObject: [String: Any] = [ + "session_id": "codex-auto-review-session", + "turn_id": "turn-auto", + "transcript_path": transcriptURL.path, + "cwd": root.path, + "hook_event_name": "PermissionRequest", + "tool_name": "shell", + "tool_input": ["command": "sleep 10"], + "message": "approval required", + ] + let approvalData = try JSONSerialization.data( + withJSONObject: approvalObject, + options: [.sortedKeys] + ) + let approvalInput = try #require(String(data: approvalData, encoding: .utf8)) + let result = runCodexHookProcess( + executablePath: cliPath, + arguments: ["hooks", "codex", "notification"], + environment: [ + "HOME": root.path, + "PATH": "/usr/bin:/bin:/usr/sbin:/sbin", + "PWD": root.path, + "CMUX_SOCKET_PATH": socketPath, + "CMUX_WORKSPACE_ID": workspaceId, + "CMUX_SURFACE_ID": surfaceId, + "CMUX_AGENT_HOOK_STATE_DIR": root.path, + "CMUX_CLI_SENTRY_DISABLED": "1", + ], + standardInput: approvalInput, + timeout: 5 + ) + + #expect(!result.timedOut, Comment(rawValue: result.stderr)) + #expect(result.status == 0, Comment(rawValue: result.stderr)) + #expect(result.stdout == "{}\n") + #expect(waitForConditionBlocking(timeout: 1) { + commands.snapshot().contains { command in + guard let object = codexHookJSONObject(command), + object["method"] as? String == "feed.push", + let params = object["params"] as? [String: Any], + let event = params["event"] as? [String: Any] else { + return false + } + return event["hook_event_name"] as? String == "PreToolUse" + } + }) + #expect(waitForConditionToRemainTrueBlocking(duration: 0.3) { + let snapshot = commands.snapshot() + return !snapshot.contains { $0.hasPrefix("notify_target_async ") } + && !snapshot.contains { $0.hasPrefix("set_agent_lifecycle codex needsInput ") } + }) } @Test func codexInstalledHookReturnsBeforeSlowCmuxCommandFinishes() throws { @@ -400,11 +581,33 @@ struct CLICodexHookTimeoutRegressionTests { .trimmingCharacters(in: .whitespacesAndNewlines) let sleepPID = try #require(Int32(sleepPIDText)) defer { _ = Darwin.kill(sleepPID, SIGKILL) } + let processSnapshot: () -> String = { + let process = Process() + let output = Pipe() + process.executableURL = URL(fileURLWithPath: "/bin/ps") + process.arguments = ["-o", "pid=,ppid=,pgid=,state=,command=", "-p", sleepPIDText] + process.standardOutput = output + process.standardError = FileHandle.nullDevice + do { + try process.run() + process.waitUntilExit() + return String( + data: output.fileHandleForReading.readDataToEndOfFile(), + encoding: .utf8 + )?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + } catch { + return "" + } + } #expect( - waitForConditionBlocking(timeout: 1) { + // The detached hook and its supervisor are separate processes; + // hosted macOS can defer the final reaping turn under a cold app + // test launch. Keep this well below the 30-second watchdog + // deadline so a genuinely orphaned timer still fails promptly. + waitForConditionBlocking(timeout: 5) { Darwin.kill(sleepPID, 0) == -1 && errno == ESRCH }, - "The watchdog timer process \(sleepPID) outlived its completed hook invocation" + "The watchdog timer process \(sleepPID) outlived its completed hook invocation (ps: \(processSnapshot()))" ) } @@ -621,7 +824,9 @@ struct CLICodexHookTimeoutRegressionTests { let workspaceId = "11111111-1111-1111-1111-111111111111" let surfaceId = "22222222-2222-2222-2222-222222222222" let sessionId = "codex-installed-stale-stop-session" + let transcriptURL = root.appendingPathComponent("rollout.jsonl") try FileManager.default.createDirectory(at: codexHome, withIntermediateDirectories: true) + try Data().write(to: transcriptURL) defer { Darwin.close(listenerFD) unlink(socketPath) @@ -632,7 +837,11 @@ struct CLICodexHookTimeoutRegressionTests { listenerFD: listenerFD, commands: commands, surfaceId: surfaceId, - connectionLimit: 24 + // Each generated hook performs several bounded target probes and + // emits telemetry on its own connection. Keep the fixture's + // accept budget above the complete prompt/prompt/stop episode so + // a valid lifecycle cannot be truncated by the mock itself. + connectionLimit: 64 ) let install = runCodexHookProcess( @@ -647,9 +856,13 @@ struct CLICodexHookTimeoutRegressionTests { let promptCommand = try #require( codexHookEntries(in: codexHome).first { $0.eventName == "UserPromptSubmit" }?.command ) + let sessionStartCommand = try #require( + codexHookEntries(in: codexHome).first { $0.eventName == "SessionStart" }?.command + ) let stopCommand = try #require( codexHookEntries(in: codexHome).first { $0.eventName == "Stop" }?.command ) + let ledgerURL = root.appendingPathComponent("codex-turn-ledger.json", isDirectory: false) let environment = [ "HOME": root.path, "CODEX_HOME": codexHome.path, @@ -662,9 +875,25 @@ struct CLICodexHookTimeoutRegressionTests { "CMUX_AGENT_HOOK_STATE_DIR": root.path, "CMUX_CLI_SENTRY_DISABLED": "1", "CMUX_BUNDLED_CLI_PATH": cliPath, - "CMUX_CODEX_PID": "4242", + "CMUX_CODEX_PID": String(ProcessInfo.processInfo.processIdentifier), + "CMUX_CODEX_INVOCATION_ID": "installed-\(sessionId)", + "CMUX_CODEX_TURN_LEDGER_PATH": ledgerURL.path, ] + // Tokenized Codex hooks normally receive SessionStart before prompt + // callbacks. Seed that owner record here so the stale-stop assertions + // exercise the modern ledger path rather than the legacy fallback. + let sessionStart = runCodexHookProcess( + executablePath: "/bin/sh", + arguments: ["-c", sessionStartCommand], + environment: environment, + standardInput: #"{"session_id":"\#(sessionId)","cwd":"\#(root.path)","transcript_path":"\#(transcriptURL.path)","hook_event_name":"SessionStart"}"#, + timeout: 3 + ) + #expect(sessionStart.status == 0, Comment(rawValue: sessionStart.stderr)) + #expect(sessionStart.stdout == "{}\n") + #expect(waitForFile(ledgerURL, containing: sessionId, timeout: 2)) + let oldPrompt = runCodexHookProcess( executablePath: "/bin/sh", arguments: ["-c", promptCommand], @@ -678,6 +907,14 @@ struct CLICodexHookTimeoutRegressionTests { commands.snapshot().contains { $0.hasPrefix("set_status codex Running ") } }) + let rollout = """ + {"type":"event_msg","payload":{"type":"task_complete","turn_id":"old-turn"}} + {"type":"turn_context","payload":{"turn_id":"current-turn"}} + {"type":"event_msg","payload":{"type":"task_started","turn_id":"current-turn"}} + """ + try Data((rollout + "\n").utf8).write(to: transcriptURL, options: .atomic) + + let currentPromptStart = commands.snapshot().count let currentPrompt = runCodexHookProcess( executablePath: "/bin/sh", arguments: ["-c", promptCommand], @@ -687,11 +924,19 @@ struct CLICodexHookTimeoutRegressionTests { ) #expect(currentPrompt.status == 0, Comment(rawValue: currentPrompt.stderr)) #expect(currentPrompt.stdout == "{}\n") - #expect(waitForConditionBlocking(timeout: 2) { - let snapshot = commands.snapshot() - return snapshot.contains { $0.hasPrefix("clear_notifications ") } + let currentTurnBecameVisible = waitForConditionBlocking(timeout: 2) { + let snapshot = Array(commands.snapshot().dropFirst(currentPromptStart)) + return AgentJournalAppendCapture.captures(in: snapshot).contains { capture in + let attention = capture.draft["attention"] as? [String: Any] + return capture.kind == "agent.turn.started" + && capture.sessionId == sessionId + && capture.workspaceId == workspaceId + && capture.surfaceId == surfaceId + && attention?["turnIdentity"] as? String == "current-turn" + } && snapshot.contains { $0.hasPrefix("set_status codex Running ") } - }) + } + #expect(currentTurnBecameVisible, Comment(rawValue: commands.snapshot().joined(separator: "\n"))) let staleStopStart = commands.snapshot().count let staleStop = runCodexHookProcess( @@ -714,6 +959,9 @@ struct CLICodexHookTimeoutRegressionTests { }, "An installed async Stop from an older turn must not notify or mark a newer running turn idle, saw \(staleStopCommands)" ) + #expect(!AgentJournalAppendCapture.captures(in: staleStopCommands).contains { capture in + (capture.draft["attention"] as? [String: Any])?["notification"] != nil + }) } @Test func codexPromptSubmitDoesNotReviveStoppedTurn() throws { @@ -894,7 +1142,20 @@ struct CLICodexHookTimeoutRegressionTests { let surfaceId = "22222222-2222-2222-2222-222222222222" let sessionId = "codex-fresh-session" let stateURL = root.appendingPathComponent("codex-hook-sessions.json") + let codexHome = root.appendingPathComponent(".codex", isDirectory: true) + let rolloutURL = codexHome + .appendingPathComponent("sessions/2026/08/12/rollout-\(sessionId).jsonl", isDirectory: false) try FileManager.default.createDirectory(at: root, withIntermediateDirectories: true) + try FileManager.default.createDirectory( + at: rolloutURL.deletingLastPathComponent(), + withIntermediateDirectories: true + ) + let rolloutLine: [String: Any] = [ + "type": "session_meta", + "payload": ["id": sessionId, "cwd": root.path, "source": "cli"], + ] + try JSONSerialization.data(withJSONObject: rolloutLine, options: [.sortedKeys]) + .write(to: rolloutURL, options: .atomic) defer { Darwin.close(listenerFD) unlink(socketPath) @@ -941,8 +1202,13 @@ struct CLICodexHookTimeoutRegressionTests { "CMUX_SURFACE_ID": surfaceId, "CMUX_AGENT_HOOK_STATE_DIR": root.path, "CMUX_CLI_SENTRY_DISABLED": "1", + "CODEX_HOME": codexHome.path, + "CMUX_CODEX_TURN_LEDGER_PATH": root.appendingPathComponent("codex-turn-ledger.json").path, + "CMUX_AGENT_LAUNCH_KIND": "codex", + "CMUX_AGENT_LAUNCH_EXECUTABLE": "/usr/local/bin/codex", + "CMUX_AGENT_LAUNCH_CWD": root.path, ], - standardInput: #"{"session_id":"\#(sessionId)","cwd":"\#(root.path)","hook_event_name":"SessionStart"}"#, + standardInput: #"{"session_id":"\#(sessionId)","transcript_path":"\#(rolloutURL.path)","cwd":"\#(root.path)","hook_event_name":"SessionStart"}"#, timeout: 5 ) @@ -979,6 +1245,11 @@ struct CLICodexHookTimeoutRegressionTests { "CMUX_AGENT_HOOK_STATE_DIR": root.path, "CMUX_CLI_SENTRY_DISABLED": "1", "CMUX_CODEX_PID": "1", + "CODEX_HOME": codexHome.path, + "CMUX_CODEX_TURN_LEDGER_PATH": root.appendingPathComponent("codex-turn-ledger.json").path, + "CMUX_AGENT_LAUNCH_KIND": "codex", + "CMUX_AGENT_LAUNCH_EXECUTABLE": "/usr/local/bin/codex", + "CMUX_AGENT_LAUNCH_CWD": root.path, ], standardInput: #"{"session_id":"\#(sessionId)","turn_id":"turn-done","cwd":"\#(root.path)","hook_event_name":"UserPromptSubmit","prompt":"late"}"#, timeout: 5 diff --git a/cmuxTests/CLITestProcessRunner.swift b/cmuxTests/CLITestProcessRunner.swift new file mode 100644 index 000000000000..e6275b1e4499 --- /dev/null +++ b/cmuxTests/CLITestProcessRunner.swift @@ -0,0 +1,329 @@ +import Darwin +import Foundation + +/// The outcome of one child process run by ``CLITestProcessRunner``. +/// +/// `status` is the exit code for a normal exit and the signal number for a +/// child that died from a signal (the same convention `Process.terminationStatus` +/// uses, so call sites that compare against `SIGTERM`/`SIGKILL` keep working). +struct CLITestProcessOutcome: Sendable { + let status: Int32 + let stdout: String + let stderr: String + let timedOut: Bool +} + +/// Runs CLI children for tests without parking any blocking work on libdispatch. +/// +/// The app-host test bundle runs its Swift Testing suites in parallel. The old +/// per-file helpers blocked three libdispatch global-queue workers per child +/// (`waitUntilExit`, and one `readDataToEndOfFile` per output pipe) and every +/// mock socket server parked an `accept` loop plus one blocking reader per +/// client on the same pool. Under parallel execution that exhausts the global +/// worker pool, so the block that is supposed to observe the child's exit never +/// runs and the mock server never answers: the CLI child sits in a socket read, +/// the helper's 5s deadline fires, and the test records +/// `status: 15, timedOut: true` (or `status: 0, timedOut: true` when the child +/// had already exited but nobody could observe it). The app host then keeps +/// those orphaned children alive past the XCTest summary. +/// +/// This runner owns the whole child lifecycle on plain POSIX primitives and +/// dedicated threads: `posix_spawn` into its own process group, one reaper +/// thread per child that blocks in `waitpid`, one drain thread per output +/// pipe, and a semaphore the calling test waits on. Nothing here depends on a +/// libdispatch worker being available, so a saturated pool cannot delay exit +/// detection, and a timeout kills the whole process group so a grandchild +/// cannot outlive the test holding the capture pipes open. +enum CLITestProcessRunner { + /// Runs `executablePath` to completion or until `timeout` elapses. + /// + /// - Parameters: + /// - standardInput: Written to the child's stdin from a dedicated thread, + /// then the write end is closed. `nil` connects stdin to `/dev/null`. + /// - outputDrainGrace: How long to wait for the output pipes to reach EOF + /// after the child is gone. A grandchild that inherited a pipe can hold + /// it open; the captured prefix is returned instead of blocking. + static func run( + executablePath: String, + arguments: [String], + environment: [String: String], + standardInput: String? = nil, + timeout: TimeInterval, + outputDrainGrace: TimeInterval = 2 + ) -> CLITestProcessOutcome { + func launchFailure(_ detail: String) -> CLITestProcessOutcome { + CLITestProcessOutcome( + status: -1, + stdout: "", + stderr: "test runner could not spawn \(executablePath): \(detail)", + timedOut: false + ) + } + + var stdinFDs: [Int32] = [-1, -1] + var stdoutFDs: [Int32] = [-1, -1] + var stderrFDs: [Int32] = [-1, -1] + defer { + for descriptor in stdinFDs + stdoutFDs + stderrFDs where descriptor >= 0 { + close(descriptor) + } + } + guard pipe(&stdoutFDs) == 0, pipe(&stderrFDs) == 0 else { + return launchFailure(String(cString: strerror(errno))) + } + if standardInput != nil { + guard pipe(&stdinFDs) == 0 else { + return launchFailure(String(cString: strerror(errno))) + } + } + guard (stdinFDs + stdoutFDs + stderrFDs).allSatisfy({ $0 < 0 || $0 > STDERR_FILENO }) else { + return launchFailure("capture pipe collided with standard I/O") + } + + var fileActions: posix_spawn_file_actions_t? + var setupStatus = posix_spawn_file_actions_init(&fileActions) + guard setupStatus == 0 else { + return launchFailure(String(cString: strerror(setupStatus))) + } + defer { posix_spawn_file_actions_destroy(&fileActions) } + + if standardInput == nil { + setupStatus = "/dev/null".withCString { + posix_spawn_file_actions_addopen(&fileActions, STDIN_FILENO, $0, O_RDONLY, 0) + } + } else { + setupStatus = posix_spawn_file_actions_adddup2(&fileActions, stdinFDs[0], STDIN_FILENO) + } + if setupStatus == 0 { + setupStatus = posix_spawn_file_actions_adddup2(&fileActions, stdoutFDs[1], STDOUT_FILENO) + } + if setupStatus == 0 { + setupStatus = posix_spawn_file_actions_adddup2(&fileActions, stderrFDs[1], STDERR_FILENO) + } + for descriptor in stdinFDs + stdoutFDs + stderrFDs where setupStatus == 0 && descriptor >= 0 { + setupStatus = posix_spawn_file_actions_addclose(&fileActions, descriptor) + } + guard setupStatus == 0 else { + return launchFailure(String(cString: strerror(setupStatus))) + } + + var attributes: posix_spawnattr_t? + setupStatus = posix_spawnattr_init(&attributes) + guard setupStatus == 0 else { + return launchFailure(String(cString: strerror(setupStatus))) + } + defer { posix_spawnattr_destroy(&attributes) } + setupStatus = posix_spawnattr_setpgroup(&attributes, 0) + if setupStatus == 0 { + setupStatus = posix_spawnattr_setflags( + &attributes, + Int16(POSIX_SPAWN_CLOEXEC_DEFAULT | POSIX_SPAWN_SETPGROUP) + ) + } + guard setupStatus == 0 else { + return launchFailure(String(cString: strerror(setupStatus))) + } + + let argumentStrings = [executablePath] + arguments + let environmentStrings = environment.map { "\($0.key)=\($0.value)" }.sorted() + guard (argumentStrings + environmentStrings).allSatisfy({ !$0.utf8.contains(0) }) else { + return launchFailure("argument or environment contains NUL") + } + var argumentPointers = argumentStrings.map { strdup($0) } + var environmentPointers = environmentStrings.map { strdup($0) } + defer { + for pointer in argumentPointers where pointer != nil { free(pointer) } + for pointer in environmentPointers where pointer != nil { free(pointer) } + } + guard argumentPointers.allSatisfy({ $0 != nil }), + environmentPointers.allSatisfy({ $0 != nil }) else { + return launchFailure("could not allocate argv or environment") + } + argumentPointers.append(nil) + environmentPointers.append(nil) + + var processIdentifier: pid_t = 0 + let spawnStatus = executablePath.withCString { executablePointer in + argumentPointers.withUnsafeMutableBufferPointer { argumentBuffer in + environmentPointers.withUnsafeMutableBufferPointer { environmentBuffer in + guard let argumentBase = argumentBuffer.baseAddress, + let environmentBase = environmentBuffer.baseAddress else { + return Int32(EINVAL) + } + return posix_spawn( + &processIdentifier, + executablePointer, + &fileActions, + &attributes, + argumentBase, + environmentBase + ) + } + } + } + guard spawnStatus == 0, processIdentifier > 1 else { + return launchFailure(String(cString: strerror(spawnStatus == 0 ? ECHILD : spawnStatus))) + } + + // The child owns its copies now; close ours so EOF propagates. + close(stdoutFDs[1]); stdoutFDs[1] = -1 + close(stderrFDs[1]); stderrFDs[1] = -1 + if stdinFDs[0] >= 0 { + close(stdinFDs[0]); stdinFDs[0] = -1 + } + + let stdoutDrain = PipeDrain(descriptor: stdoutFDs[0], name: "cmux-test-stdout-drain") + stdoutFDs[0] = -1 + let stderrDrain = PipeDrain(descriptor: stderrFDs[0], name: "cmux-test-stderr-drain") + stderrFDs[0] = -1 + if let standardInput, stdinFDs[1] >= 0 { + let writeDescriptor = stdinFDs[1] + stdinFDs[1] = -1 + detachBlockingThread(name: "cmux-test-stdin-writer") { + writeAll(descriptor: writeDescriptor, data: Data(standardInput.utf8)) + close(writeDescriptor) + } + } + let waiter = ProcessWaiter(processIdentifier: processIdentifier) + + var timedOut = false + if !waiter.wait(timeout: timeout) { + timedOut = true + _ = kill(-processIdentifier, SIGTERM) + if !waiter.wait(timeout: 1) { + _ = kill(-processIdentifier, SIGKILL) + _ = waiter.wait(timeout: 5) + } + } + + let stdout = stdoutDrain.text(waitingUpTo: outputDrainGrace) + let stderr = stderrDrain.text(waitingUpTo: outputDrainGrace) + guard let status = waiter.status else { + return CLITestProcessOutcome( + status: SIGKILL, + stdout: stdout, + stderr: stderr.isEmpty + ? "test runner could not reap process group after SIGKILL" + : "\(stderr)\ntest runner could not reap process group after SIGKILL", + timedOut: true + ) + } + return CLITestProcessOutcome(status: status, stdout: stdout, stderr: stderr, timedOut: timedOut) + } + + /// Runs `body` on a dedicated thread instead of a libdispatch global-queue + /// worker. Use it for anything that blocks indefinitely (accept loops, + /// blocking socket reads) so parallel tests cannot exhaust the shared pool. + static func detachBlockingThread(name: String, _ body: @escaping @Sendable () -> Void) { + let thread = Thread(block: body) + thread.name = name + thread.stackSize = 1 << 20 + thread.start() + } + + private static func writeAll(descriptor: Int32, data: Data) { + data.withUnsafeBytes { rawBuffer in + guard let base = rawBuffer.bindMemory(to: UInt8.self).baseAddress else { return } + var remaining = rawBuffer.count + var cursor = base + while remaining > 0 { + let written = Darwin.write(descriptor, cursor, remaining) + if written > 0 { + remaining -= written + cursor = cursor.advanced(by: written) + } else if written < 0, errno == EINTR { + continue + } else { + return + } + } + } + } + + /// Reaps one child on a dedicated thread so exit detection never waits for + /// a libdispatch worker. + private final class ProcessWaiter: @unchecked Sendable { + private let processIdentifier: pid_t + private let lock = NSLock() + private let finished = DispatchSemaphore(value: 0) + private var storedStatus: Int32? + + init(processIdentifier: pid_t) { + self.processIdentifier = processIdentifier + CLITestProcessRunner.detachBlockingThread(name: "cmux-test-process-reaper") { [self] in + reap() + } + } + + var status: Int32? { + lock.lock() + defer { lock.unlock() } + return storedStatus + } + + func wait(timeout: TimeInterval) -> Bool { + if status != nil { return true } + if finished.wait(timeout: .now() + timeout) == .success { return true } + // Do not turn a completion racing the deadline into a timeout. + return status != nil + } + + private func reap() { + var rawStatus: Int32 = 0 + var waitResult: pid_t + repeat { + waitResult = waitpid(processIdentifier, &rawStatus, 0) + } while waitResult == -1 && errno == EINTR + + let status: Int32 + if waitResult == processIdentifier { + let terminatingSignal = rawStatus & 0x7f + status = terminatingSignal == 0 ? (rawStatus >> 8) & 0xff : terminatingSignal + } else { + status = -1 + } + lock.lock() + storedStatus = status + lock.unlock() + finished.signal() + } + } + + /// Reads one pipe to EOF on a dedicated thread so a child writing more than + /// a pipe buffer never blocks while the test waits for it to exit. + private final class PipeDrain: @unchecked Sendable { + private let lock = NSLock() + private var data = Data() + private let finished = DispatchSemaphore(value: 0) + + init(descriptor: Int32, name: String) { + CLITestProcessRunner.detachBlockingThread(name: name) { [self] in + var collected = Data() + var buffer = [UInt8](repeating: 0, count: 16384) + while true { + let count = Darwin.read(descriptor, &buffer, buffer.count) + if count > 0 { + collected.append(buffer, count: count) + } else if count < 0, errno == EINTR { + continue + } else { + break + } + } + close(descriptor) + lock.lock() + data = collected + lock.unlock() + finished.signal() + } + } + + func text(waitingUpTo timeout: TimeInterval) -> String { + _ = finished.wait(timeout: .now() + timeout) + lock.lock() + defer { lock.unlock() } + return String(data: data, encoding: .utf8) ?? "" + } + } +} + diff --git a/cmuxTests/FeedEventClassificationTests.swift b/cmuxTests/FeedEventClassificationTests.swift index 0463656538d2..363fe5ec0fa0 100644 --- a/cmuxTests/FeedEventClassificationTests.swift +++ b/cmuxTests/FeedEventClassificationTests.swift @@ -1,5 +1,5 @@ +import Foundation import Testing - // `FeedEventClassifier` lives in `CLI/FeedEventClassifier.swift`, which is // compiled into both the `cmux-cli` target and this test target — so the pure // classification decision can be unit-tested directly, without `@testable` @@ -164,10 +164,9 @@ struct FeedEventClassificationTests { /// A COMPLETED codex tool proves any pending native approval prompt /// resolved — execution strictly follows approval (by the user or by - /// Codex's own auto-review) — so PostToolUse clears the pane's stale - /// permission notification, mirroring the pane-wide clears Claude's - /// lifecycle hooks and Hermes' approval-response hook already perform. - @Test func codexToolCompletionClearsNativeApprovalPrompt() { + /// Codex's own auto-review) — so PostToolUse resolves the matching + /// approval notification without touching a newer request in the pane. + @Test func codexToolCompletionResolvesNativeApprovalPrompt() { #expect(classify("codex", "PostToolUse", tool: "shell").clearsNativeApprovalPrompt == true) #expect(classify("codex", "post_tool_use", tool: "shell").clearsNativeApprovalPrompt == true) } @@ -272,6 +271,10 @@ struct FeedEventClassificationTests { private static let workspaceUUID = "11111111-2222-3333-4444-555555555555" private static let surfaceUUID = "66666666-7777-8888-9999-AAAAAAAAAAAA" + private static let approvalIdentity = CodexApprovalNotificationIdentity( + scope: "111111111111111111111111", + approvalID: "111111111111111111111111.aaaaaaaaaaaaaaaaaaaaaaaa" + ) private func attentionCommand( _ source: String, @@ -279,14 +282,17 @@ struct FeedEventClassificationTests { tool: String, displayName: String = "Codex", workspaceId: String? = workspaceUUID, - surfaceId: String? = surfaceUUID + surfaceId: String? = surfaceUUID, + approvalIdentity: CodexApprovalNotificationIdentity? = nil ) -> String? { FeedEventClassifier.nativeApprovalPromptAttentionCommand( classification: FeedEventClassifier.classify(source: source, event: event, toolName: tool), displayName: displayName, toolName: tool, workspaceId: workspaceId, - surfaceId: surfaceId + surfaceId: surfaceId, + source: source, + approvalIdentity: approvalIdentity ) } @@ -297,8 +303,8 @@ struct FeedEventClassificationTests { /// is what silently regresses https://github.com/manaflow-ai/cmux/issues/9592. @Test func codexPermissionRequestBuildsGatedNotifyCommand() { #expect( - attentionCommand("codex", "PermissionRequest", tool: "shell") - == "notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) Codex|Permission|shell needs approval|c=needs-permission;p=0" + attentionCommand("codex", "PermissionRequest", tool: "shell", approvalIdentity: Self.approvalIdentity) + == "notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) Codex|Permission|shell needs approval|c=needs-permission;p=0;a=\(Self.approvalIdentity.approvalID)" ) } @@ -306,8 +312,8 @@ struct FeedEventClassificationTests { /// needed" string rather than an empty interpolation. @Test func codexPermissionRequestWithoutToolNameFallsBackToApprovalNeeded() { #expect( - attentionCommand("codex", "PermissionRequest", tool: "") - == "notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) Codex|Permission|Approval needed|c=needs-permission;p=0" + attentionCommand("codex", "PermissionRequest", tool: "", approvalIdentity: Self.approvalIdentity) + == "notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) Codex|Permission|Approval needed|c=needs-permission;p=0;a=\(Self.approvalIdentity.approvalID)" ) } @@ -316,12 +322,12 @@ struct FeedEventClassificationTests { /// so both must be neutralized. @Test func attentionCommandSanitizesPipeAndNewlineInToolName() { #expect( - attentionCommand("codex", "PermissionRequest", tool: "a|b") - == "notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) Codex|Permission|a¦b needs approval|c=needs-permission;p=0" + attentionCommand("codex", "PermissionRequest", tool: "a|b", approvalIdentity: Self.approvalIdentity) + == "notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) Codex|Permission|a¦b needs approval|c=needs-permission;p=0;a=\(Self.approvalIdentity.approvalID)" ) #expect( - attentionCommand("codex", "PermissionRequest", tool: "a\nb") - == "notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) Codex|Permission|a b needs approval|c=needs-permission;p=0" + attentionCommand("codex", "PermissionRequest", tool: "a\nb", approvalIdentity: Self.approvalIdentity) + == "notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) Codex|Permission|a b needs approval|c=needs-permission;p=0;a=\(Self.approvalIdentity.approvalID)" ) } @@ -329,8 +335,315 @@ struct FeedEventClassificationTests { /// `clear_notifications` line. @Test func codexToolCompletionBuildsPaneScopedClearCommand() { #expect( - attentionCommand("codex", "PostToolUse", tool: "shell") - == "clear_notifications --tab=\(Self.workspaceUUID) --panel=\(Self.surfaceUUID)" + attentionCommand("codex", "PostToolUse", tool: "shell", approvalIdentity: Self.approvalIdentity) + == "clear_notifications --tab=\(Self.workspaceUUID) --panel=\(Self.surfaceUUID) --approval-id=\(Self.approvalIdentity.approvalID)" + ) + } + + @Test func nativeApprovalAttentionRequiresCorrelationIdentity() { + #expect( + attentionCommand("codex", "PermissionRequest", tool: "shell", approvalIdentity: nil) + == "notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) Codex|Permission|shell needs approval|c=needs-permission;p=0" + ) + #expect(attentionCommand("codex", "PostToolUse", tool: "shell", approvalIdentity: nil) == nil) + } + + @Test func nonCodexNativeApprovalRetainsLegacyPaneScopedCommands() { + let prompt = FeedEventClassification( + hookEventName: "PreToolUse", + isActionable: false, + notifiesNativeApprovalPrompt: true, + clearsNativeApprovalPrompt: false + ) + let clear = FeedEventClassification( + hookEventName: "PostToolUse", + isActionable: false, + notifiesNativeApprovalPrompt: false, + clearsNativeApprovalPrompt: true + ) + #expect( + FeedEventClassifier.nativeApprovalPromptAttentionCommand( + classification: prompt, + displayName: "Claude", + toolName: "Bash", + workspaceId: Self.workspaceUUID, + surfaceId: Self.surfaceUUID, + source: "claude" + ) == "notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) Claude|Permission|Bash needs approval|c=needs-permission;p=0" + ) + #expect( + FeedEventClassifier.nativeApprovalPromptAttentionCommand( + classification: clear, + displayName: "Claude", + toolName: "Bash", + workspaceId: Self.workspaceUUID, + surfaceId: Self.surfaceUUID, + source: "claude" + ) == "clear_notifications --tab=\(Self.workspaceUUID) --panel=\(Self.surfaceUUID)" + ) + } + + @Test func codexRequestAndCompletionBuildCorrelatedSettleCommands() throws { + let request: [String: Any] = [ + "session_id": "codex-session", + "turn_id": "turn-1", + "tool_name": "shell", + "tool_input": ["command": "git status", "timeout_ms": 1_000], + ] + let completion: [String: Any] = [ + "session_id": "codex-session", + "turn_id": "turn-1", + "tool_name": "shell", + "tool_input": ["timeout_ms": 1_000, "command": "git status"], + "tool_response": ["exit_code": 0], + "tool_use_id": "available-only-after-review", + ] + let requestIdentity = try #require(CodexApprovalNotificationIdentity.make( + rawObject: request, + fallbackSessionID: nil + )) + let completionIdentity = try #require(CodexApprovalNotificationIdentity.make( + rawObject: completion, + fallbackSessionID: nil + )) + + #expect(requestIdentity == completionIdentity) + #expect( + attentionCommand( + "codex", + "PermissionRequest", + tool: "shell", + approvalIdentity: requestIdentity + ) + == "notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) Codex|Permission|shell needs approval|c=needs-permission;p=0;a=\(requestIdentity.approvalID);d=1;o=feed" + ) + #expect( + attentionCommand( + "codex", + "PostToolUse", + tool: "shell", + approvalIdentity: completionIdentity + ) + == "clear_notifications --tab=\(Self.workspaceUUID) --panel=\(Self.surfaceUUID) --approval-id=\(requestIdentity.approvalID)" + ) + } + + @Test func codexAutoReviewerIsReadFromTheMatchingTurnContext() { + let rawObject: [String: Any] = [ + "session_id": "codex-session", + "turn_id": "turn-current", + ] + let rolloutLines = [ + #"{"type":"turn_context","payload":{"turn_id":"turn-old","approvals_reviewer":"user"}}"#, + #"{"type":"turn_context","payload":{"turn_id":"turn-current","approvals_reviewer":"auto_review"}}"#, + ] + + #expect( + CodexApprovalNotificationPolicy().reviewRoute( + rawObject: rawObject, + rolloutLines: rolloutLines + ) == .autoReview + ) + } + + @Test func codexApprovalIdentityRequiresDeterministicToolInput() { + let rawObject: [String: Any] = [ + "session_id": "codex-session", + "turn_id": "turn-current", + "tool_name": "shell", + ] + #expect(CodexApprovalNotificationIdentity.makeScope( + rawObject: rawObject, + fallbackSessionID: nil + ) != nil) + #expect(CodexApprovalNotificationIdentity.make(rawObject: rawObject, fallbackSessionID: nil) == nil) + var unsupported = rawObject + unsupported["tool_input"] = Date() + #expect(CodexApprovalNotificationIdentity.make(rawObject: unsupported, fallbackSessionID: nil) == nil) + } + + @Test func codexApprovalIdentityUsesSharedRequestIdentifierWhenPresent() throws { + let base: [String: Any] = [ + "session_id": "codex-session", + "turn_id": "turn-current", + "tool_name": "shell", + "tool_input": ["command": "git status"], + ] + var first = base + first["request_id"] = "request-a" + var second = base + second["request_id"] = "request-b" + let firstIdentity = try #require(CodexApprovalNotificationIdentity.make( + rawObject: first, + fallbackSessionID: nil + )) + let secondIdentity = try #require(CodexApprovalNotificationIdentity.make( + rawObject: second, + fallbackSessionID: nil + )) + #expect(firstIdentity != secondIdentity) + } + + @Test(arguments: ["tool_input", "toolInput"]) + func codexApprovalIdentityDoesNotTrustToolArguments(container: String) throws { + let payload: [String: Any] = [ + "session_id": "session", "turn_id": "turn", "tool_name": "shell", + container: ["request_id": "model-controlled", "item_id": "also-untrusted", "command": "echo ok"], + ] + let identity = try #require(CodexApprovalNotificationIdentity.make(rawObject: payload, fallbackSessionID: nil)) + #expect(!identity.isAuthoritative) + var missingEnvelope = payload + missingEnvelope.removeValue(forKey: "turn_id") + missingEnvelope[container] = ["turn_id": "model-turn", "command": "echo ok"] + #expect(CodexApprovalNotificationIdentity.make(rawObject: missingEnvelope, fallbackSessionID: nil) == nil) + var providerEnvelope = payload + providerEnvelope["context"] = ["request_id": "provider-id"] + #expect(CodexApprovalNotificationIdentity.make(rawObject: providerEnvelope, fallbackSessionID: nil)?.isAuthoritative == true) + } + + @Test func codexCompletionOnlyCallIDRetainsTupleResolution() throws { + let request: [String: Any] = [ + "session_id": "session", "turn_id": "turn", "tool_name": "shell", + "tool_input": ["command": "echo ok"], + ] + var completion = request + completion["call_id"] = "completion-only" + let requestIdentity = try #require(CodexApprovalNotificationIdentity.make(rawObject: request, fallbackSessionID: nil)) + let completionIdentity = try #require(CodexApprovalNotificationIdentity.make(rawObject: completion, fallbackSessionID: nil)) + let command = try #require(attentionCommand("codex", "PostToolUse", tool: "shell", approvalIdentity: completionIdentity)) + #expect(command.contains("--approval-id=\(completionIdentity.approvalID)")) + #expect(command.contains("--approval-fallback-id=\(requestIdentity.approvalID)")) + } + + @Test(arguments: ["thread_settings", "threadSettings", "context"]) + func codexThreadDefaultCannotOverrideEffectiveTurnReviewer(container: String) { + let payload: [String: Any] = [ + "session_id": "session", "turn_id": "turn", "tool_name": "shell", + container: ["approvals_reviewer": "auto_review"], + ] + let policy = CodexApprovalNotificationPolicy() + #expect(policy.reviewRoute(rawObject: payload, rolloutLines: []) == nil) + #expect(policy.reviewRoute(rawObject: payload, rolloutLines: [ + #"{"type":"turn_context","payload":{"turn_id":"turn","approvals_reviewer":"user"}}"#, + ]) == .user) + #expect(!policy.isAutoReviewed(rawObject: payload, transcriptPath: "/rollout") { _, _ in ["{}"] }) + #expect(policy.reviewRoute(rawObject: payload, rolloutLines: [ + #"{"type":"turn_context","payload":{"turn_id":"turn","approvals_reviewer":"auto_review"}}"#, + ]) == .autoReview) + } + + @Test func codexApprovalIdentityBoundsNestedEnvelopeTraversal() { + var deeplyWrapped: [String: Any] = ["tool_input": ["command": "git status"]] + for _ in 0..<8 { + deeplyWrapped = ["notification": deeplyWrapped] + } + let rawObject: [String: Any] = [ + "session_id": "codex-session", + "turn_id": "turn-current", + "tool_name": "shell", + "notification": deeplyWrapped, + ] + + // The supported envelope depth is intentionally finite. A payload + // buried beyond it must fail closed instead of recursively walking an + // attacker-controlled object graph. + #expect(CodexApprovalNotificationIdentity.make( + rawObject: rawObject, + fallbackSessionID: nil + ) == nil) + } + + @Test func codexAutoReviewFailsClosedWithoutReadableRolloutTail() { + let policy = CodexApprovalNotificationPolicy() + let rawObject: [String: Any] = ["approvals_reviewer": "auto_review"] + #expect(!policy.isAutoReviewed(rawObject: rawObject, transcriptPath: nil) { _, _ in + [#"{"type":"turn_context","payload":{"approvals_reviewer":"auto_review"}}"#] + }) + #expect(!policy.isAutoReviewed(rawObject: rawObject, transcriptPath: "/missing") { _, _ in nil }) + #expect(!policy.isAutoReviewed(rawObject: rawObject, transcriptPath: "/empty") { _, _ in [] }) + } + + @Test func codexUserReviewSkipsRolloutRead() { + let policy = CodexApprovalNotificationPolicy() + var readCount = 0 + let isAutoReviewed = policy.isAutoReviewed( + rawObject: ["approvals_reviewer": "user"], + transcriptPath: "/should-not-be-opened" + ) { _, _ in + readCount += 1 + return [#"{"type":"turn_context","payload":{"approvals_reviewer":"auto_review"}}"#] + } + + #expect(!isAutoReviewed) + #expect(readCount == 0) + } + + @Test func codexReviewerDoesNotLeakFromAnOlderKnownTurn() { + let rawObject: [String: Any] = [ + "session_id": "codex-session", + "turn_id": "turn-current", + ] + let rolloutLines = [ + #"{"type":"turn_context","payload":{"turn_id":"turn-old","approvals_reviewer":"auto_review"}}"#, + #"{"type":"turn_context","payload":{"turn_id":"turn-newer","approvals_reviewer":"user"}}"#, + ] + + #expect( + CodexApprovalNotificationPolicy().reviewRoute( + rawObject: rawObject, + rolloutLines: rolloutLines + ) == nil + ) + } + + @Test func codexRolloutReviewerRequiresMatchingTurnID() { + let rolloutLines = [ + #"{"type":"turn_context","payload":{"turn_id":"another-turn","approvals_reviewer":"auto_review"}}"#, + ] + #expect(CodexApprovalNotificationPolicy().reviewRoute( + rawObject: ["session_id": "codex-session", "turn_id": "requested-turn"], + rolloutLines: rolloutLines + ) == nil) + } + + @Test func codexTurnReviewerDoesNotOverrideAnMCPRequest() { + let rolloutLines = [ + #"{"type":"turn_context","payload":{"turn_id":"turn-current","approvals_reviewer":"auto_review"}}"#, + ] + + for toolName in [ + "mcp__codex_apps__calendar_create_event", + "MCP__codex_apps__calendar_create_event", + ] { + let rawObject: [String: Any] = [ + "session_id": "codex-session", + "turn_id": "turn-current", + "tool_name": toolName, + "context": ["approvals_reviewer": "auto_review"], + ] + #expect( + CodexApprovalNotificationPolicy().reviewRoute( + rawObject: rawObject, + rolloutLines: rolloutLines + ) == nil + ) + } + + // App-server notifications may wrap the MCP tool name and turn under + // `notification.params`; the turn-wide reviewer must still not silence + // a connector whose effective reviewer is unknown. + #expect( + CodexApprovalNotificationPolicy().reviewRoute( + rawObject: [ + "notification": [ + "params": [ + "tool_name": "mcp__codex_apps__calendar_create_event", + "turn_id": "turn-current", + ], + ], + ], + rolloutLines: rolloutLines + ) == nil ) } @@ -342,7 +655,8 @@ struct FeedEventClassificationTests { "PermissionRequest", tool: "shell", workspaceId: Self.workspaceUUID.lowercased(), - surfaceId: Self.surfaceUUID.lowercased() + surfaceId: Self.surfaceUUID.lowercased(), + approvalIdentity: Self.approvalIdentity ) #expect(command?.contains("notify_target_async \(Self.workspaceUUID) \(Self.surfaceUUID) ") == true) } diff --git a/tests/test_codex_permission_prompt_notification.py b/tests/test_codex_permission_prompt_notification.py index a28bd11aba78..0a82463dcef3 100644 --- a/tests/test_codex_permission_prompt_notification.py +++ b/tests/test_codex_permission_prompt_notification.py @@ -2,7 +2,7 @@ """ Regression: a codex PermissionRequest feed hook raises the "Agent Needs Permission"-gated notification, acknowledged before the hook -returns, and codex tool completion clears it. +returns, and codex tool completion resolves the correlated request. https://github.com/manaflow-ai/cmux/issues/9592: the feed bridge normalized codex PermissionRequest to non-actionable PreToolUse telemetry and never @@ -14,6 +14,7 @@ from __future__ import annotations +import hashlib import json import os import subprocess @@ -54,6 +55,39 @@ def codex_payload(event: str) -> dict: } +def approval_id(payload: dict) -> str: + scope_seed = ( + f"session={payload['session_id']}\n" + f"turn={payload['turn_id']}" + ) + scope = hashlib.sha256(scope_seed.encode()).hexdigest()[:24] + canonical_input = json.dumps( + {"value": payload.get("tool_input")}, + ensure_ascii=False, + separators=(",", ":"), + sort_keys=True, + ) + request_seed = ( + f"{scope_seed}\n" + f"tool={payload.get('tool_name', '')}\n" + f"input={canonical_input}" + ) + request = hashlib.sha256(request_seed.encode()).hexdigest()[:24] + return f"{scope}.{request}" + + +EXPECTED_APPROVAL_ID = approval_id(codex_payload("PermissionRequest")) +EXPECTED_NOTIFY_COMMAND = ( + f"notify_target_async {FAKE_WORKSPACE_ID} {FAKE_SURFACE_ID} " + "Codex|Permission|shell needs approval|c=needs-permission;p=0" + f";a={EXPECTED_APPROVAL_ID};d=1;o=feed" +) +EXPECTED_CLEAR_COMMAND = ( + f"clear_notifications --tab={FAKE_WORKSPACE_ID} --panel={FAKE_SURFACE_ID} " + f"--approval-id={EXPECTED_APPROVAL_ID}" +) + + def strip_capability_prefix(raw: str) -> str: if raw.startswith("_cmux_capability_v1 "): parts = raw.split(" ", 2) @@ -71,6 +105,8 @@ def run_feed_hook_capture( method_delays: dict[str, float] | None = None, settle_seconds: float = 0, payload: dict | None = None, + environment: dict[str, str] | None = None, + generic_subcommand: str | None = None, ) -> tuple[dict, list, float]: """Runs `cmux hooks feed --source codex` and returns (stdout JSON, ordered received frames, elapsed seconds).""" @@ -79,6 +115,8 @@ def run_feed_hook_capture( env.pop(key, None) env["CMUX_SURFACE_ID"] = FAKE_SURFACE_ID env["CMUX_WORKSPACE_ID"] = FAKE_WORKSPACE_ID + if environment: + env.update(environment) if socket_password is not None: env["CMUX_SOCKET_PASSWORD"] = socket_password with FakeCmuxSocket( @@ -89,18 +127,11 @@ def run_feed_hook_capture( method_delays=method_delays, ) as fake: started = time.monotonic() + hook_arguments = ["feed", "--source", "codex", "--event", event] + if generic_subcommand is not None: + hook_arguments = ["codex", generic_subcommand] result = subprocess.run( - [ - cli_path, - "--socket", - str(socket_path), - "hooks", - "feed", - "--source", - "codex", - "--event", - event, - ], + [cli_path, "--socket", str(socket_path), "hooks", *hook_arguments], input=json.dumps(payload if payload is not None else codex_payload(event)), capture_output=True, text=True, @@ -333,6 +364,67 @@ def test_stalled_live_target_probe_does_not_starve_notification( ) +def test_shared_approval_lookup_does_not_quarantine_corrupt_state(cli_path: str, root: Path) -> None: + state_dir = root / "corrupt-hook-state" + state_dir.mkdir() + state_file = state_dir / "codex-hook-sessions.json" + state_file.write_text("{not valid JSON", encoding="utf-8") + backup = state_dir / ".codex-hook-sessions.json.quarantined.json" + backup.write_text("previous recovery backup", encoding="utf-8") + payload = codex_payload("PermissionRequest") + payload["approvals_reviewer"] = "auto_review" + run_feed_hook_capture( + cli_path, root / "cmux-read-only.sock", "PermissionRequest", payload=payload, + environment={"CMUX_AGENT_HOOK_STATE_DIR": str(state_dir)}, + ) + assert state_file.read_text(encoding="utf-8") == "{not valid JSON" + assert backup.read_text(encoding="utf-8") == "previous recovery backup" + + +def test_completion_only_tool_use_id_preserves_legacy_settling(cli_path: str, root: Path) -> None: + payload = codex_payload("PermissionRequest") + payload.pop("tool_use_id") + _, request_frames, _ = run_feed_hook_capture( + cli_path, root / "cmux-derived-request.sock", "PermissionRequest", payload=payload, + ) + assert EXPECTED_NOTIFY_COMMAND in raw_commands(request_frames), request_frames + assert notification_views(request_frames) == [], request_frames + _, completion_frames, _ = run_feed_hook_capture( + cli_path, root / "cmux-derived-completion.sock", "PostToolUse", + ) + assert EXPECTED_CLEAR_COMMAND in raw_commands(completion_frames), completion_frames + + +def test_completion_only_call_id_includes_derived_fallback(cli_path: str, root: Path) -> None: + payload = codex_payload("PostToolUse") + payload["call_id"] = "only-on-completion" + _, frames, _ = run_feed_hook_capture( + cli_path, root / "cmux-call-fallback.sock", "PostToolUse", payload=payload, + ) + assert any(f"--approval-fallback-id={EXPECTED_APPROVAL_ID}" in command for command in raw_commands(frames)), frames + + +def test_native_hook_and_feed_share_journal_identity(cli_path: str, root: Path) -> None: + state_dir = root / "native-hook-state" + state_dir.mkdir() + payload = codex_payload("PermissionRequest") + payload.pop("tool_use_id") + payload["tool_call_id"] = "shared-native-call" + _, hook_frames, _ = run_feed_hook_capture( + cli_path, root / "cmux-native-hook.sock", "PermissionRequest", payload=payload, + generic_subcommand="notification", environment={"CMUX_AGENT_HOOK_STATE_DIR": str(state_dir)}, + surface_delivery_target=(FAKE_WORKSPACE_ID, FAKE_SURFACE_ID), + ) + _, feed_frames, _ = run_feed_hook_capture( + cli_path, root / "cmux-native-feed.sock", "PermissionRequest", payload=payload, + surface_delivery_target=(FAKE_WORKSPACE_ID, FAKE_SURFACE_ID), + ) + for frames in (hook_frames, feed_frames): + views = notification_views(frames) + assert len(views) == 1 and views[0]["request_identity"] == "shared-native-call", frames + assert not any(command.startswith("notify_target_async ") for command in raw_commands(frames)), frames + + def main() -> int: try: cli_path = resolve_cmux_cli() @@ -353,11 +445,15 @@ def main() -> int: test_permission_notification_survives_slow_authentication(cli_path, root) test_permission_notification_targets_rehomed_pane(cli_path, root) test_stalled_live_target_probe_does_not_starve_notification(cli_path, root) + test_shared_approval_lookup_does_not_quarantine_corrupt_state(cli_path, root) + test_completion_only_tool_use_id_preserves_legacy_settling(cli_path, root) + test_completion_only_call_id_includes_derived_fallback(cli_path, root) + test_native_hook_and_feed_share_journal_identity(cli_path, root) except Exception as exc: print(f"FAIL: {exc}") return 1 - print("PASS: codex permission prompts notify, acknowledge, and clear") + print("PASS: codex permission prompts notify, acknowledge, and resolve") return 0