Skip to content

Generalize webhook persisted failures and align ProposalTools redaction - #2474

Merged
Chris0Jeky merged 3 commits into
mainfrom
issue-2351/webhook-proposaltools-residuals
Sep 4, 2026
Merged

Generalize webhook persisted failures and align ProposalTools redaction#2474
Chris0Jeky merged 3 commits into
mainfrom
issue-2351/webhook-proposaltools-residuals

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

Summary

Closes two residuals (R1, R2) recorded by the unknown-exception surface inventory in #2470. Both
bring a surface into line with the shipped boundary rule: an unknown exception contributes no
text to any user-visible or persisted value, while deliberate domain messages stay useful.

Per-residual disposition

R1 — persisted webhook delivery failure text was redacted, not generalized.
backend/src/Taskdeck.Api/Workers/OutboundWebhookDeliveryWorker.cs persisted
Redact($"Webhook delivery threw {ex.GetType().Name}: {ex.Message}") into
OutboundWebhookDelivery.LastErrorMessage. SensitiveDataRedactor.Redact is pattern-based, so any
exception text matching no rule (Windows paths, SQLite constraint text, provider URLs, stack detail)
was stored verbatim. The catch (Exception ...) arm that is still in Processing now passes
SensitiveDataRedactor.GenericUnexpectedFailureMessage directly to MarkFailure, and logs the
original exception exactly once through the existing _logger as a redacted SummarizeException
summary carrying the existing DeliveryId identifier — no new correlation scheme, no double log.
Retry/backoff and the dead-letter vs retry-scheduled transitions are unchanged (MarkFailure was
not touched). All deliberate arms keep their existing stable messages: endpoint HTTP status, invalid
URI, disallowed scheme, blocked host, inactive subscription, shutdown requeue.

The assignment uses the constant directly in the call expression, so the
scripts/check-unknown-exception-boundary.mjs guard from #2470 will accept this file when it is
added to PERSISTED_STATE_FILES — no allowlist entry needed. The guard script itself is not added
here; it lives on the other branch.

R2 — ProposalTools.Error(Result) diverged from the other MCP tools.
backend/src/Taskdeck.Api/Mcp/ProposalTools.cs handled UnexpectedError correctly but returned
result.ErrorMessage verbatim for every other code, unlike ReadTools.cs / WriteTools.cs, which
route through SensitiveDataRedactor.SanitizeLlmFailureMessage. It now calls the same helper, so
behavior is identical across the MCP tool surface: generic constant for UnexpectedError,
Redact() otherwise.

ProposalResources.cs was checked for the same asymmetry: its PublicFailureMessage(Result) helper
already uses SanitizeLlmFailureMessage, so no divergence exists there. (Its separate
failureReason = p.FailureReason replay at line 135 is inventory row R3 — legacy stored text — and
is deliberately out of scope for this PR.)

Part of #2351. This PR intentionally does not close the umbrella issue.

Verification

  • dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~OutboundWebhookDeliveryWorkerTests|FullyQualifiedName~ProposalToolsErrorSafetyTests|FullyQualifiedName~McpToolsTests"53 passed, 0 failed, 0 skipped.
  • dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --no-build2812 passed, 0 failed, 4 skipped (2816 total), 6 m 1 s. No flakes.
  • node scripts/check-docs-governance.mjs — passed.
  • git diff --check — clean.

New / changed regressions:

  • ProcessDueDeliveriesAsync_ShouldGeneralizePersistedFailureMessage_WhenDispatchThrowsDuringProcessing
    (rewritten from the old redaction assertion): a synthetic exception carrying a bearer token, a
    token= value, a capture payload, a Windows path, a SQLite UNIQUE constraint failed string and a
    provider URL. LastErrorMessage equals the generic constant and contains none of those markers,
    and exactly one Error log entry is emitted containing the exception type and the delivery ID.
  • ProcessDueDeliveriesAsync_ShouldKeepStableStatusMessage_WhenEndpointReturnsHttp500: a deliberate
    HTTP 500 keeps Webhook endpoint returned HTTP 500. and LastResponseStatusCode == 500.
  • ProcessDueDeliveriesAsync_ShouldGeneralizePersistedFailureMessage_WhenDispatchTimesOut: a client
    timeout persists the generic constant, leaks no provider host, and logs exactly once.
  • GetProposalStatus_KnownDomainFailureWithRedactablePattern_IsRedacted: a ValidationError message
    containing token=abc123 is returned with the token redacted and its stable prefix intact, while
    the existing NotFound test proves an ordinary domain message passes through unchanged.

Documentation

Two append-only bullets added to the Runtime Enforcement section of
docs/security/SECURITY_LOGGING_REDACTION.md (webhook persisted failure; ProposalTools alignment).
docs/STATUS.md untouched. The inventory doc rows R1/R2 live on
issue-2351/surface-inventory-guard (#2470) and are reconciled by the coordinator after both land.

Not verified

  • No end-to-end run against a live webhook endpoint or a real SQLite failure; the delivery path is
    exercised through the existing fake repository/unit-of-work harness only.
  • Other test projects (Application, Integration, Domain, Architecture, Cli) were not run — no files
    outside Taskdeck.Api and its test project were changed.
  • Frontend, E2E and the smart-CI lane were not run (no files in those regions changed).
  • The Record the unknown-exception surface inventory and guard new raw flows #2470 boundary guard could not be executed here (the script is on the other branch); guard
    compatibility is argued from its documented PERSISTED_STATE_FILES rule, not measured.
  • Historic LastErrorMessage rows written before this change are not backfilled.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Fresh-context independent review at exact head c9e8d31.

Verdict: no CRITICAL or HIGH correctness, security, or data-loss finding. Merge-blocking: none.

Checked: the whole webhook delivery loop (only the persisted errorMessage argument changed; MarkFailure retry/backoff, dead-letter transition, and SaveChanges are byte-identical; HTTP-status, invalid-URI, and shutdown-cancellation arms still return or match before the generic arm; a client timeout with an uncancelled token already fell into the generic arm before this PR); single logging (the loop-level LogError cannot fire for a swallowed exception and the sibling arms are mutually exclusive); the log passes SummarizeException(ex), consistent with every other worker and the SEC-14 policy; every known-domain message reachable from the three ProposalTools tools was enumerated and none collides with a redaction rule; the webhook test asserts marker absence on the persisted field and the ProposalTools test fails without the fix.

LOW, declined on this thread (no commit): the now-unused using Taskdeck.Domain.Exceptions in ProposalTools.cs; the generic constant's 'correlation ID' wording versus the DeliveryId the webhook log line carries (pre-existing to the constant, tracked implicitly under the #2351 residual about two generic strings).

Remaining gate: exact-head hosted ci-required green and the three-minute aging floor, then a merge commit. Part of #2351; the umbrella stays open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant