Address missed activity summary review feedback#114
Conversation
PR SummaryLow Risk Overview Removes the unused/ineffective dictionary sorting helper and stops sorting Reviewed by Cursor Bugbot for commit 2087fee. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
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
wholeMatchand added a regression test so non-PR GitHub URLs with numeric fragments are rejected.
- Anchored PR label extraction with
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)" | ||
| } |
There was a problem hiding this comment.
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)
Reviewed by Cursor Bugbot for commit 2087fee. Configure here.


Summary
Tests