Skip to content

Address missed activity summary review feedback#114

Merged
haasonsaas merged 2 commits intomainfrom
codex/review-feedback-20260503-agentd
May 3, 2026
Merged

Address missed activity summary review feedback#114
haasonsaas merged 2 commits intomainfrom
codex/review-feedback-20260503-agentd

Conversation

@haasonsaas
Copy link
Copy Markdown
Contributor

Summary

  • accept already-extracted GitHub PR labels from activePullRequest metadata
  • remove the ineffective dictionary sorting helper from dropped-reason aggregation

Tests

  • swift test --filter ActivitySummary

@cursor
Copy link
Copy Markdown

cursor Bot commented May 3, 2026

PR Summary

Low Risk
Low risk: small changes to activity-summary parsing/serialization and a new test; main impact is how GitHub PR labels are recognized and the determinism of droppedReasonCounts ordering in JSON output.

Overview
Improves activity summary artifact extraction by allowing activePullRequest metadata to be either a full GitHub PR URL or an already-extracted org/repo#num label (with whitespace trimmed), via a new regex-based extractor.

Removes the unused/ineffective dictionary sorting helper and stops sorting droppedReasonCounts during summary construction (markdown rendering still sorts when displaying). Adds a test covering the label-only activePullRequest metadata case.

Reviewed by Cursor Bugbot for commit 2087fee. Bugbot is set up for automated code reviews on this repo. Configure here.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Unanchored regex falsely matches non-PR GitHub URLs
    • Anchored PR label extraction with wholeMatch and added a regression test so non-PR GitHub URLs with numeric fragments are rejected.
Preview (2c971252f8)
diff --git a/Sources/agentd/ActivitySummary.swift b/Sources/agentd/ActivitySummary.swift
--- a/Sources/agentd/ActivitySummary.swift
+++ b/Sources/agentd/ActivitySummary.swift
@@ -194,7 +194,7 @@
       sourceBatchIds: sourceBatchIds.sorted(),
       displayIds: displayIds.sorted(),
       droppedCounts: dropped,
-      droppedReasonCounts: droppedReasonCounts.sortedByKey(),
+      droppedReasonCounts: droppedReasonCounts,
       apps: appCounters.map { key, count in
         ActivityAppSummary(appName: key.appName, bundleId: key.bundleId, frameCount: count)
       }.sorted(),
@@ -261,7 +261,12 @@
   }
 
   private static func githubPullRequestLabel(_ raw: String) -> String? {
-    guard let components = URLComponents(string: raw), components.host == "github.com" else {
+    let trimmed = raw.trimmingCharacters(in: .whitespacesAndNewlines)
+    if let extractedLabel = githubPullRequestExtractedLabel(trimmed) {
+      return extractedLabel
+    }
+
+    guard let components = URLComponents(string: trimmed), components.host == "github.com" else {
       return nil
     }
     let parts = components.path.split(separator: "/").map(String.init)
@@ -269,6 +274,13 @@
     return "\(parts[0])/\(parts[1])#\(number)"
   }
 
+  private static func githubPullRequestExtractedLabel(_ raw: String) -> String? {
+    guard let match = raw.wholeMatch(of: #/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)#([0-9]+)/#) else {
+      return nil
+    }
+    return "\(match.1)/\(match.2)#\(match.3)"
+  }
+
   private static func isoDate(_ raw: String) -> Date? {
     ISO8601DateFormatter().date(from: raw)
   }
@@ -495,9 +507,3 @@
     )
   }
 }
-
-extension Dictionary where Key == String, Value == Int {
-  fileprivate func sortedByKey() -> [String: Int] {
-    Dictionary(uniqueKeysWithValues: sorted { $0.key < $1.key })
-  }
-}

diff --git a/Tests/agentdTests/DiagnosticCLITests.swift b/Tests/agentdTests/DiagnosticCLITests.swift
--- a/Tests/agentdTests/DiagnosticCLITests.swift
+++ b/Tests/agentdTests/DiagnosticCLITests.swift
@@ -241,6 +241,66 @@
     XCTAssertTrue(markdown.contains("secret.ocrText:openai: 1"))
   }
 
+  func testActivitySummaryExtractsActivePullRequestLabelMetadata() async throws {
+    let root = try temporaryDirectory()
+    defer { try? FileManager.default.removeItem(at: root) }
+    let now = Date(timeIntervalSince1970: 21_600)
+    try writeBatch(
+      ActivitySummaryTests.batch(
+        id: "batch_label_only",
+        startedAt: Date(timeIntervalSince1970: 7_000),
+        endedAt: Date(timeIntervalSince1970: 7_030),
+        frames: [],
+        metadata: [
+          "activePullRequest": "evalops/agentd#113",
+          "activePullRequest.firstSeenAt": "1970-01-01T01:56:40Z",
+          "activePullRequest.foregroundSeconds": "45",
+        ]
+      ),
+      to: root.appendingPathComponent("batch_label_only.json")
+    )
+
+    let summary = try await ActivitySummary.run(
+      options: ActivityOptions(sinceHours: 6, batchDirectory: root, windowLabel: "6h"),
+      now: now
+    )
+
+    XCTAssertEqual(summary.artifacts.map(\.label), ["evalops/agentd#113"])
+    XCTAssertEqual(summary.artifacts.first?.url, "evalops/agentd#113")
+    XCTAssertEqual(summary.artifacts.first?.foregroundSeconds, 45)
+  }
+
+  func testActivitySummaryIgnoresNonPullRequestGitHubDocumentPath() async throws {
+    let root = try temporaryDirectory()
+    defer { try? FileManager.default.removeItem(at: root) }
+    let now = Date(timeIntervalSince1970: 21_600)
+    try writeBatch(
+      ActivitySummaryTests.batch(
+        id: "batch_non_pr_url",
+        startedAt: Date(timeIntervalSince1970: 7_000),
+        endedAt: Date(timeIntervalSince1970: 7_030),
+        frames: [
+          ActivitySummaryTests.frame(
+            appName: "Google Chrome",
+            bundleId: "com.google.Chrome",
+            windowTitle: "cerebro",
+            documentPath: "https://github.com/evalops/cerebro#123",
+            capturedAt: Date(timeIntervalSince1970: 7_000),
+            displayId: 42
+          )
+        ]
+      ),
+      to: root.appendingPathComponent("batch_non_pr_url.json")
+    )
+
+    let summary = try await ActivitySummary.run(
+      options: ActivityOptions(sinceHours: 6, batchDirectory: root, windowLabel: "6h"),
+      now: now
+    )
+
+    XCTAssertTrue(summary.artifacts.isEmpty)
+  }
+
   func testActivitySummaryArtifactsWriteInstructionsAndResource() async throws {
     let batchRoot = try temporaryDirectory()
     let artifactRoot = try temporaryDirectory()

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 2087fee. Configure here.

return nil
}
return "\(match.1)/\(match.2)#\(match.3)"
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unanchored regex falsely matches non-PR GitHub URLs

Low Severity

githubPullRequestExtractedLabel uses an unanchored firstMatch regex, and it's tried before URL-based parsing in githubPullRequestLabel. Since githubPullRequestLabel is also called on documentPath values (not just pre-extracted metadata labels), a URL like https://github.com/owner/repo#123 (a repo page with a numeric fragment) would match the substring owner/repo#123 and be returned as a PR label. Previously, URL parsing would have correctly rejected this because the path doesn't contain /pull/.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2087fee. Configure here.

@haasonsaas haasonsaas merged commit 5c71596 into main May 3, 2026
3 checks passed
@haasonsaas haasonsaas deleted the codex/review-feedback-20260503-agentd branch May 3, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants