Skip to content

fix(security-agent): recover abandoned launches and clarify dismissal failures - #8928

Open
eshurakov wants to merge 3 commits into
mainfrom
kilo/brisk-atlas-cxs
Open

eshurakov wants to merge 3 commits into
mainfrom
kilo/brisk-atlas-cxs

Conversation

@eshurakov

Copy link
Copy Markdown
Contributor

Problem

Remediation attempts are claimed before finding/configuration/actor reads, but those reads were outside the launch failure handler. A transient exception could leave an attempt in launching permanently. Queue redelivery then skipped it, while the stale claim blocked subsequent work for the same owner or repository. There was no stale-launch recovery.

Dependabot dismissal treated every GitHub HTTP rejection as a retryable generic error. Permanent authentication, permission, or unavailable-alert failures exhausted queue delivery attempts without an actionable explanation, and diagnostics discarded the HTTP status.

Changes

  • Protect post-claim reads and preserve bounded retry/failure outcomes.
  • Recover expired launching claims in the existing minute dispatcher, using the configured acceptance timeout and existing retry limits/backoff. Honor cancellation and isolate individual recovery failures.
  • Fence recovery, launch handoff, cancellation, and failure writes by claim ownership and state; update parent/audit/ledger outcomes only when the transition wins. An old worker cannot initiate after losing its claim.
  • Bound session preparation to 60 seconds.
  • Return specific dismissal failures for 400/401, non-rate-limited 403, and 404/410. Keep network, rate-limit, server, conflict, and ambiguous 422 failures retryable. Never mark findings dismissed locally after rejected GitHub writeback.
  • Preserve safe HTTP-status/request-correlation diagnostics without logging tokens, comments, or response bodies.

Verification

  • pnpm --filter cloudflare-security-auto-analysis test: 210 tests passed.
  • pnpm --filter cloudflare-security-sync test: 120 tests passed.
  • Both worker package typechecks passed.
  • Targeted oxlint, formatting, and git diff --check passed.
  • Race regressions cover recovery versus running handoff/cancellation, stale-worker ownership, retry rows with retained markers, and terminal audit/ledger outcomes.
  • PostgreSQL integration tests were not run because the sandbox has no available PostgreSQL/Docker test infrastructure.

Scope And Evidence

Production logs show an active dispatcher repeatedly enqueueing remediation work and dismissal deliveries failing at GitHub writeback. They do not expose the exact queued-attempt blocker or upstream GitHub status, so this PR fixes independently reproduced code defects rather than claiming a fully attributed production incident.

Already-running executions are not automatically expired or cancelled. The pre-existing missing-callback reconciliation gap and initiation retry ambiguity remain outside this change. No production state was modified or deployment performed.

Comment thread services/security-sync/src/dismiss.ts Outdated
Comment thread services/security-auto-analysis/src/dispatcher.ts Outdated
@kilo-code-bot

kilo-code-bot Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental follow-up fixes both prior findings: the 403 abuse-detection matcher keeps such responses retryable, and a dedicated five-minute remediation launch reclaim window is decoupled from SECURITY_AGENT_COMMAND_ACCEPTED_TIMEOUT_MS, with regression tests for both. I found no new defects in the changed lines.

Assumptions: [INFERENCE] Test suites and typechecks were not executed in this read-only session; verification is based on reading the changed code, full-file context, and the new regression tests. [INFERENCE] The five-minute reclaim window comfortably exceeds the 60s prepareSession bound plus prelaunch reads; no per-stage timeout conflict was found.

Files Reviewed (4 files)
  • services/security-auto-analysis/src/dispatcher.ts - no issues
  • services/security-auto-analysis/src/dispatcher.test.ts - no issues
  • services/security-sync/src/dismiss.ts - no issues
  • services/security-sync/src/dismiss.test.ts - no issues
Previous Review Summary (commit dba63f7)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit dba63f7)

Status: 2 Issues Found | Recommendation: Address before merge

Executive Summary

The claim-fencing and stale-launch recovery logic is sound and well covered by the new race tests; the only substantive defect is a GitHub 403 abuse-detection response being classified as a terminal permission failure, plus one hardening gap on the reclaim timeout.

Assumptions: [INFERENCE] Tests and typechecks were not re-run in this read-only session (no installed dependencies); I relied on the diff and full-file reading. [INFERENCE] The new PRELAUNCH_READ_FAILED/PRELAUNCH_TIMEOUT paths persist raw error.message into last_error_redacted, which the web remediation API returns to the owner; this follows the existing pattern in the launch-failure path, so I did not raise it as a finding, but the value is not actually redacted.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 1
Issue Details (click to expand)

WARNING

File Line Issue
services/security-sync/src/dismiss.ts 47 GitHub abuse-detection 403 ("You have triggered an abuse detection mechanism") is not matched as a rate limit and becomes a terminal GITHUB_DISMISSAL_PERMISSION_DENIED instead of a retryable failure.

SUGGESTION

File Line Issue
services/security-auto-analysis/src/dispatcher.ts 150 Stale-launch reclaim reuses the command accepted timeout, which can be configured below the new 60s prepareSession bound, reclaiming healthy in-flight launches and orphaning prepared sessions.
Files Reviewed (8 files)
  • services/security-auto-analysis/src/remediation.ts - 1 suggestion
  • services/security-auto-analysis/src/dispatcher.ts - 1 suggestion
  • services/security-auto-analysis/src/logger.ts - no issues
  • services/security-auto-analysis/src/remediation-prelaunch.test.ts - no issues
  • services/security-auto-analysis/src/remediation-settled-outcomes.test.ts - no issues
  • services/security-auto-analysis/src/dispatcher.test.ts - no issues
  • services/security-sync/src/dismiss.ts - 1 warning
  • services/security-sync/src/dismiss.test.ts - no issues

Verified positives: cancellation, handoff, recovery, and failure writes are fenced by both claim token and expected status; parent remediation and ledger/audit outcomes are only updated when the guarded transition wins; the stale sweep tolerates per-row failures; the dismissal path returns terminal results without mutating local finding state or leaking response bodies, tokens, comments, or headers.

Fix these issues in Kilo Cloud


Reviewed by deepseek-flash-latest · Input: 49K · Output: 8.8K · Cached: 636.7K

Review guidance: REVIEW.md from base branch main

This branch has not been deployed

No deployments
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.

1 participant