fix(breeze): only monitor explicit mentions and review requests#320
fix(breeze): only monitor explicit mentions and review requests#320bingran-you merged 2 commits intomainfrom
Conversation
397939a to
e256d4d
Compare
yuezengwu
left a comment
There was a problem hiding this comment.
Thanks for tightening the signal. Overall the change is coherent: shouldProcessReason is now the single gate, it's applied at both entry points (parseNotifications in commands/poll.ts and buildNotificationCandidate via recentNotifications), and the SKILL/README/CLI help are all updated to match. Tests cover both the accepted and dropped reasons.
A few smaller things worth considering before merge:
1. Dead code in src/products/breeze/engine/daemon/gh-client.ts. requiredReviewBacklog (lines 226–321) and assignedItems (lines 173–217) are no longer called from collectCandidates or anywhere else in production. They still compile and still have tests (describe("GhClient.reviewRequests / assignedItems", ...), plus the requiredReviewBacklog test at line 298). Per the repo-level guidance to avoid keeping unused shims, I'd prefer deleting both methods, their helper buildAssignedCandidate / buildRequiredReviewCandidate builders if they have no other callers, and the associated tests — so there's one obvious way to produce candidates (notifications + explicit review requests) and nothing drifts. If you'd rather keep them for a planned re-introduction, a // @deprecated — reintroduction planned by … header would make that intent explicit.
2. Stale comment / empty scope in collectCandidates (gh-client.ts ~454–466). After the two try blocks are removed, if (options.includeSearch) now wraps only the reviewRequests call plus a trailing blank line. At that point the branch is doing one thing — it might read more clearly inlined with the single call, or the blank line at least removed. Minor, but it's the kind of leftover that tends to confuse readers.
3. Behavioral note for the release. Previously breeze would auto-pick up (a) PRs with review:required where the user hadn't been explicitly pinged, and (b) issues/PRs assigned to the user without a review request or mention. After this change, neither will appear in the inbox. That matches the PR's intent, but worth calling out in the release notes / changelog so users who were relying on the broader sweep understand the scope shift.
4. Minor: breeze-daemon-poller.test.ts:264 now uses reason: "mention" on a Discussion subject. That's fine for the "falls back to repo base when no subject URL" path, but the previous comment block ("Canonical shape ... reason: author") is not accurate in-tree any more since author would be dropped. Consider updating the docstring to explain that the canonical shape is being exercised against a review_requested entry now.
Nothing here is blocking — the core logic and test coverage look right. LGTM once the dead code is either removed or annotated.
This reply was drafted by breeze, an autonomous agent running on behalf of the account owner.
|
Addressed the dead-code cleanup follow-up:
The paired tree contract update is merged in agent-team-foundation/first-tree-context#191. |
yuezengwu
left a comment
There was a problem hiding this comment.
Thanks for the fast follow-up — cleanup looks great.
Verified:
assignedItems,requiredReviewBacklog, and theirbuildAssignedCandidate/buildRequiredReviewCandidatebuilders are gone fromsrc/products/breeze/engine/daemon/gh-client.tsandsrc/products/breeze/engine/runtime/task.ts(no remaining references anywhere insrc/ortests/).collectCandidatesnow has a single-purposeincludeSearchbranch with the trailing blank line removed (src/products/breeze/engine/daemon/gh-client.ts:301–312).pnpm typecheck✅ and the targeted breeze suite (breeze-daemon-gh-client,breeze-task,breeze-daemon-poller) 38/38 ✅ locally at0c62ed6.
Items #3 (release-note callout for the scope shift) and #4 (minor docstring polish in breeze-daemon-poller.test.ts) were non-blocking observations, not required changes. #3 is a release-time concern and #4 is tiny — happy to leave both as-is.
LGTM — approving.
This reply was drafted by breeze, an autonomous agent running on behalf of the account owner.
Summary
Testing