fix(workflows): approval integrity — deny denies, timeouts fire, sends fail loud, toolsAllow enforced#462
Merged
Conversation
…s fail loud, toolsAllow enforced Four root-cause fixes from the 2026-07-02 approval-noise investigation (143 self-replicated MSP workflow copies, 70 forever-pending approvals, approved runs delivering email to nobody): 1. Telegram Deny on a durable run RESUMED it as approved. Both the inline-button handler and the text-reply surface routed denials through resumeWorkflowRunAfterApproval, which unconditionally writes approval_status='approved' and re-executes the pipeline. New shared denyWorkflowRunAfterApproval (run→failed, step→denied) is now the one deny path; gateway workflows.deny refactored onto it. The button handler also re-edits the Telegram message when a resume fails, so a dead resume no longer reads "✅ Approved". 2. Approval timeouts were never enforced for durable runs. The setTimeout enforcement only exists on the non-durable path; every gateway/cron run pauses durably, so the "auto-deny at <time>" in the operator message was a label (live: 70 pending, 61 past timeout_at, 0 ever timed_out). resumeDueWorkflowWaits now sweeps expired pending approvals with an atomic pending→timed_out claim, then applies timeout_action (deny → shared deny path, approve → resume). 3. send_email swallowed every failure. The email tool's catch-all returned errors as plain text and sendWorkflowEmail returned ok:true unconditionally — an email with to:"" logged DELIVERED and completed the run. Tool failures now return details.ok=false, sendWorkflowEmail inspects the result, rejects empty recipients, and both the action executor and email output node throw so the step fails visibly. 4. Workflow agent toolsAllow was prompt prose, not policy. The dispatcher rendered the node's 8-tool allowlist as text and the embedded session got the full 121-tool set — which is how the MSP podcast workflow saved new active, cron-scheduled copies of itself every morning. seedWorkflowSessionToolPolicy now writes the grants onto the per-step session entry before dispatch (same structural seam as Worker Runtime v2 ephemeral sessions). Also carries the keychain VITEST shell-out guard (identical hunk to PR #461) so test runs on this branch can't raise the macOS "Keychain Not Found" modal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T3dkQzz2mSrSNtGBE9cmXr
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…losed everywhere Follow-ups from the 24-finding adversarial review of e4288af: - sendWorkflowEmail validates provider (resend/mailgun/sendgrid, lowercased) — an unknown provider fell through the email tool as a detail-less "Unknown action" text result and read as DELIVERED. - Retry-exhaustion check was unsatisfiable (`length===0 && items[0]...`): retry-exhausted steps recorded as completed. Now `||`, so errorItemSet AND empty non-retryable results both mark the step failed. - Workflow agent session keys get a random suffix — same-millisecond parallel dispatch collided keys and cross-contaminated seeded tool policy. - Approval-timeout sweep fails CLOSED for timeout_action='approve': the flag is workflow-author-controlled (agents author workflows), honoring it would auto-execute gated side effects unattended and re-run pipelines inside the locked cron tick. Unarmed until gated on operator sign-off (0 live rows). - Sweep compensates the pending→timed_out claim if the deny fails, so a DB error can't strand a run in waiting_approval with a terminal approval row. - Orphan sweep: pending approvals whose run already reached a terminal state are cancelled instead of polluting the pending list forever. - denyWorkflowRunAfterApproval only broadcasts run-completed when it actually denied; the Telegram button reports when the run had already left the waiting state, and resume-failure re-edits truncate the error. - sessions.toolsAllow is now fail-closed in pi-tools: an allowlist whose entries match no registered tool used to be silently stripped (full 121-tool set — reopening the self-replication vector); unknown entries now simply match nothing. Live-data remediation done alongside (not in this diff): the two ACTIVE workflows with send_email to:"" (Daily SaaS Radar, Forward Observer) were backfilled with a real recipient so this deploy flips no green cron red. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T3dkQzz2mSrSNtGBE9cmXr
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
1 participant
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.
Root cause: the 2026-07-02 approval-noise investigation
Live state found this morning: 143 self-replicated copies of the "MSP Morning Podcast 2" workflow (11 active, all cron-firing 7 AM), 70 approvals stuck pending forever, and operator-approved runs "delivering" email to an empty recipient. Four production defects, all fixed here:
1. Telegram Deny resumed the run as APPROVED (safety)
Both the inline-button handler and the text-reply surface routed denials through
resumeWorkflowRunAfterApproval, which unconditionally writesapproval_status='approved'and re-executes the pipeline. Only gatewayworkflows.denywas correct.denyWorkflowRunAfterApproval(run→failed, step→denied) inworkflow-execution-service.ts; all three deny surfaces now use it (ApprovalTextCommandDepsgainsdenyRun).2. Approval timeouts never fired for scheduled runs
Timeout enforcement was a
setTimeoutin the non-durable path only; every gateway/cron run pauses durably. The "auto-deny at HH:MM" in the operator's Telegram message was a label — live DB had 70 pending / 61 pasttimeout_at/ zero rows evertimed_out.resumeDueWorkflowWaitsnow sweeps expired pending approvals: atomicpending→timed_outclaim (race-safe vs operator taps), then appliestimeout_action(deny → shared deny path; approve → resume). This also makestimeoutAction:'approve'a working notify-then-proceed mode.3. send_email swallowed every failure
The email tool's catch-all returned errors as plain text;
sendWorkflowEmailreturnedok:trueunconditionally — a send withto:""loggedsend_email: DELIVEREDand completed the run (observed on both of the operator's 06-27 approvals).details.ok=false;sendWorkflowEmailrejects empty recipients and inspects the result; the action executor and the email output node throw so the step fails visibly into retry/error handling.4. Workflow agent toolsAllow was prose, not policy
CoreAgentDispatcherrendered the node'stoolsAllowas text in the prompt; the embedded session got the full 121-tool set. That's how the podcast workflow's draft agent (prompt: "Create and save a workflow-board draft only") saved new active, cron-scheduled copies of itself every morning.seedWorkflowSessionToolPolicywrites the node's grants onto the per-step session entry before dispatch — the same structural seam Worker Runtime v2 ephemeral sessions use (pi-tools.policy.tssessions.toolsAllow), proven live since 6/14.Also carries the keychain VITEST shell-out guard (identical hunk to #461) so test runs on this branch can't raise the macOS "Keychain Not Found" modal.
Verification
workflow-execution-service.approval-timeout.test.ts,workflow-runner.session-policy.test.ts(proves the model-visible tool registry shrinks to the grants),email-delivery-tool.test.ts, and the corrected text-command tests (the old suite asserted the deny-resumes bug as expected behavior).tsgo --noEmit: 189 = baseline, zero net-new.oxfmt --checkclean on all touched files.🤖 Generated with Claude Code
https://claude.ai/code/session_01T3dkQzz2mSrSNtGBE9cmXr