Skip to content

fix: restore closed-issue email notifications - #280

Merged
kolatts merged 1 commit into
mainfrom
kolatts/279-fix-close-email-notifications
Jul 29, 2026
Merged

fix: restore closed-issue email notifications#280
kolatts merged 1 commit into
mainfrom
kolatts/279-fix-close-email-notifications

Conversation

@kolatts

@kolatts kolatts commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Summary

  • preserve the from-website provenance label during automated Claude triage
  • use the durable IssueEmails table mapping as the authoritative signal for close notifications
  • allow older website submissions whose label was already removed to receive closure emails

Root 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 IssueEmails mapping.

Validation

  • dotnet build functions/feedback/Feedback.csproj -c Release
  • npm run typecheck
  • npm run lint
  • npm test -- --run (393 tests)

Closes #279

Generated-with: OpenAI Codex (GPT-5)

Closes #279

Generated-with: OpenAI Codex (GPT-5)
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kolatts's task in 2m 25s —— View job


PR Review #280 — fix: restore closed-issue email notifications

  • Gather context and read changed files
  • Clean up previous Claude comments (none to clean up)
  • Review .github/workflows/claude-triage.yml
  • Review functions/feedback/IssueWebhookFunction.cs
  • Submit formal review

Summary

Root cause diagnosis is accurate and the fix is clean. The triage workflow was removing whatever label triggered it — including from-website — via both the Claude prompt instructions and the explicit step runner. The close webhook then required that label to be present before looking up the stored email mapping, so deliveries silently dropped.

claude-triage.yml — belt-and-suspenders fix:

  1. The Claude prompt for step 5 now explicitly tells the agent to only remove claude-triage, not from-website.
  2. The "Remove triggering label" step has a condition added (github.event.label.name == 'claude-triage') so it won't fire when from-website triggered the run. The hardcoded label string in the run: command reinforces this.

Both layers are correct. The step-level condition is the reliable hard guard that doesn't depend on AI behavior.

IssueWebhookFunction.cs — the more significant fix:

  • Removes the label-based gate entirely and uses IssueEmailStore.GetAsync() as the authoritative source. This is the right design: the store is populated only during website submission and is not affected by label mutations, so it correctly handles both new submissions (where from-website is now preserved) and older ones (where the label was already stripped).
  • GITHUB_ISSUE_LABEL env var is still used by ProcessQueueFunction.cs (labelling newly-created issues), so removing it from IssueWebhookFunction.cs doesn't orphan any config.

One minor note: There are no C# unit tests for IssueWebhookFunction. The PR body lists dotnet build but not dotnet test. Since the change is a straightforward guard removal (not new logic), this isn't blocking — but coverage would be worth adding in a follow-up.

Overall: minimal, correct, and well-reasoned. LGTM.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@kolatts
kolatts marked this pull request as ready for review July 29, 2026 17:19
@kolatts
kolatts merged commit 22c6263 into main Jul 29, 2026
4 checks passed
@kolatts
kolatts deleted the kolatts/279-fix-close-email-notifications branch July 29, 2026 17:19
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kolatts's task in 4m 5s —— View job


PR Review #280 — fix: restore closed-issue email notifications

  • Gather context from PR
  • Read changed files (.github/workflows/claude-triage.yml, functions/feedback/IssueWebhookFunction.cs, IssueEmailStore.cs, ProcessSubmissionsFunction.cs, IssueEmailEntity.cs)
  • Review .github/workflows/claude-triage.yml
  • Review functions/feedback/IssueWebhookFunction.cs
  • Submit formal review (APPROVED)
  • Clean up previous Claude comments — blocked: gh api and GitHub MCP tools require approval not yet granted

Root cause diagnosis is accurate and the fix is clean.

.github/workflows/claude-triage.yml — belt-and-suspenders:

  1. Prompt instruction (step 5, line 147) now explicitly tells Claude to only remove claude-triage, not from-website. This is what caused the original bug — the old prompt said "remove the triggering label" without qualification, so when from-website triggered the run, Claude would call gh issue edit --remove-label "from-website" via its tool calls.

  2. Step-level condition (if: always() && github.event.label.name == 'claude-triage') is a hard guard preventing the "Remove triggering label" step from firing on from-website triggers. The run: command 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 the right architectural call. The store record is write-once (set in ProcessSubmissionsFunction.cs:81 only when submission.Email != ""); it is never touched by label workflows, making it a strictly more reliable signal than a label.
  • Handles the retroactive case correctly: older submissions where from-website was already stripped by the old triage workflow still have their table mapping and will now receive closure emails.
  • PartitionKey defaults to "issue" in IssueEmailEntity — matches the hardcoded partition key in IssueEmailStore.GetAsync(). No bug.
  • GITHUB_ISSUE_LABEL remains consumed by ProcessSubmissionsFunction.cs:49 for labelling newly-created issues; removing it from IssueWebhookFunction orphans nothing.
  • HMAC signature verification unchanged; no new security gaps introduced.

One follow-up worth filing: IssueWebhookFunction has no unit tests. Validation in the PR body lists dotnet build but not dotnet test. Risk is low given the simplicity of the removed guard, but a test covering the store-lookup path (mapping found vs. not found) would close the gap.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Root cause diagnosis is accurate and the fix is clean.

.github/workflows/claude-triage.yml — belt-and-suspenders:

  1. Prompt instruction (step 5, line 147) now explicitly tells Claude to only remove claude-triage, not from-website. This is what caused the original bug — the old prompt said "remove the triggering label" without qualification, so when from-website triggered the run Claude would call gh issue edit --remove-label "from-website" via its tool calls.

  2. Step-level condition (if: always() && github.event.label.name == 'claude-triage') is a hard guard preventing the "Remove triggering label" step from firing on from-website triggers. The run: 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 in ProcessSubmissionsFunction.cs:81 only when submission.Email != ""); it is never touched by label workflows, making it a strictly more reliable signal.
  • Handles the retroactive case: older submissions where from-website was already stripped still have their table mapping and will now receive closure emails.
  • PartitionKey defaults to "issue" in IssueEmailEntity — matches the hardcoded partition key in IssueEmailStore.GetAsync(). No bug.
  • GITHUB_ISSUE_LABEL remains consumed by ProcessSubmissionsFunction.cs:49 for labelling newly-created issues; removing it from IssueWebhookFunction orphans 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.

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.

Closed website feedback issues do not send notification emails

1 participant