fix: restore closed-issue email notifications - #280
Conversation
Closes #279 Generated-with: OpenAI Codex (GPT-5)
|
Claude finished @kolatts's task in 2m 25s —— View job PR Review #280 — fix: restore closed-issue email notifications
SummaryRoot cause diagnosis is accurate and the fix is clean. The triage workflow was removing whatever label triggered it — including
Both layers are correct. The step-level condition is the reliable hard guard that doesn't depend on AI behavior.
One minor note: There are no C# unit tests for Overall: minimal, correct, and well-reasoned. LGTM. |
There was a problem hiding this comment.
Root cause diagnosis is accurate; the fix is clean and minimal. Belt-and-suspenders in the workflow (step-level condition plus prompt instruction) and the switch from a mutable label gate to the durable IssueEmailStore mapping are both correct calls. LGTM.
|
Claude finished @kolatts's task in 4m 5s —— View job PR Review #280 — fix: restore closed-issue email notifications
Root cause diagnosis is accurate and the fix is clean.
One follow-up worth filing: |
There was a problem hiding this comment.
Root cause diagnosis is accurate and the fix is clean.
.github/workflows/claude-triage.yml — belt-and-suspenders:
-
Prompt instruction (step 5, line 147) now explicitly tells Claude to only remove
claude-triage, notfrom-website. This is what caused the original bug — the old prompt said "remove the triggering label" without qualification, so whenfrom-websitetriggered the run Claude would callgh issue edit --remove-label "from-website"via its tool calls. -
Step-level condition (
if: always() && github.event.label.name == 'claude-triage') is a hard guard preventing the "Remove triggering label" step from firing onfrom-websitetriggers. Therun:already hardcodes--remove-label "claude-triage", so the condition is technically redundant — but it makes intent explicit and is the reliable layer that doesn't depend on AI behavior.
functions/feedback/IssueWebhookFunction.cs — the more significant fix:
- Dropping the mutable label gate in favour of
IssueEmailStore.GetAsync()is correct. The store record is write-once (set inProcessSubmissionsFunction.cs:81only whensubmission.Email != ""); it is never touched by label workflows, making it a strictly more reliable signal. - Handles the retroactive case: older submissions where
from-websitewas already stripped still have their table mapping and will now receive closure emails. PartitionKeydefaults to"issue"inIssueEmailEntity— matches the hardcoded partition key inIssueEmailStore.GetAsync(). No bug.GITHUB_ISSUE_LABELremains consumed byProcessSubmissionsFunction.cs:49for labelling newly-created issues; removing it fromIssueWebhookFunctionorphans nothing.- HMAC signature verification unchanged; no new security gaps.
One follow-up worth filing: IssueWebhookFunction has no unit tests. Validation lists dotnet build but not dotnet test. Risk is low given the removed guard's simplicity, but a test covering the store-lookup path (found and not-found) would close the gap.
LGTM.
Summary
from-websiteprovenance label during automated Claude triageIssueEmailstable mapping as the authoritative signal for close notificationsRoot cause
The triage workflow removed whichever label triggered it, including
from-website. The close webhook then required that mutable label before looking up the submitter’s stored email mapping, so valid GitHub webhook deliveries returned HTTP 200 while silently skipping notification.Impact
Website feedback submitters will receive the existing resolved/not-planned email when their mapped GitHub issue closes. Non-website issues remain excluded because they have no
IssueEmailsmapping.Validation
dotnet build functions/feedback/Feedback.csproj -c Releasenpm run typechecknpm run lintnpm test -- --run(393 tests)Closes #279
Generated-with: OpenAI Codex (GPT-5)