Webhook reply hardening: dedup review fan-out, kill late thread tasks, instrument silent paths (closes #518, #520, #524)#525
Merged
Conversation
…ead tasks, instrument silent paths Three fixes for the recurring webhook misbehavior pattern: 1. **#518: dedup review fan-out.** GitHub fires both pull_request_review (with the inline comments embedded) AND pull_request_review_comment (one per inline comment). reply_to_review was iterating the inline comments and creating per-comment replies, while the per-comment webhook handler did the same in parallel. The per-comment lock didn't serialize them — both posted clarification replies on the same thread (observed twice on rhencke/orly PR #3). reply_to_review is now a no-op for inline comments; the per-comment webhook is the single source of truth. 2. **#520: kill late-arriving thread tasks for resolved threads.** When webhook triage queues a thread task for a comment whose thread fido has already auto-resolved (because an earlier in-thread task completed first), the worker re-does the work that's already shipped (#521). create_task now calls gh.is_thread_resolved_for_comment and drops the task on the floor when the thread is already closed. Strict 'is True' check so MagicMock test stubs don't trip the guard. 3. **#524: instrument the silent triage path + bump preempt-yield timeout to 30s.** reply_to_comment was silently swallowing comments after needs_more_context returned NO; the opus triage classifier logged nothing, no error fired, comment sat un-acked. Most likely cause: webhook session.prompt waiting >2s for the worker to yield the lock. wait_for_pending_preempt timeout 2s -> 30s; _triage now logs request + returned-chars and warns when category parse fails.
rhencke
approved these changes
Apr 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
reply_to_reviewno longer iterates inline comments — the per-comment webhook (pull_request_review_comment) handles each one. Both used to fire in parallel and post duplicate clarification replies on the same thread (observed twice on rhencke/orly PR Phase 2: Rewrite task-cli.sh as kennel/cli.py #3).create_taskcalls newGitHub.is_thread_resolved_for_commentand drops the task if the thread is already resolved. Stops fido from re-doing already-shipped work (Worker re-runs already-done work when triage queues a follow-up task after the work landed #521 race) when triage queues a follow-up after auto-resolve._triagenow logs request + return preview and warns on unparseable category.wait_for_pending_preempttimeout 2s → 30s so a webhook waiting on a slow yielding worker doesn't bail and lose the preempt.Test plan
thread for comment N already resolved on GitHub — skipping queueinstead of queuingwait_for_pending_preemptyield timing visible in log; if any timeout,session: preempter still pending after 30.0swarning surfacesCloses #518, #520, #524. Related to (but does not fix): #521 (which becomes a no-op once #520 lands).