docs(23): gap-closure plan for AUTOGATE-03 idempotency defect (CR-01)
This commit is contained in:
parent
cc87607a58
commit
a918b72d04
3 changed files with 247 additions and 8 deletions
|
|
@ -0,0 +1,233 @@
|
|||
---
|
||||
phase: 23-classification-disposition-per-client-automation-gate
|
||||
plan: 06
|
||||
type: execute
|
||||
wave: 3
|
||||
depends_on: [23-05]
|
||||
files_modified:
|
||||
- lib/services/remediation-service.ts
|
||||
- lib/services/remediation-service.test.ts
|
||||
- lib/services/webhook-service.ts
|
||||
autonomous: true
|
||||
gap_closure: true
|
||||
requirements: [AUTOGATE-03]
|
||||
must_haves:
|
||||
truths:
|
||||
- "A repeat ticket-create webhook joining an already-acknowledged phishing campaign does NOT re-post the acknowledge_user customer-visible note (closes Truth #18 / AUTOGATE-03 to fully SATISFIED)"
|
||||
- "The first auto_report pass for a USER_AWARENESS campaign still posts the acknowledge_user note exactly once"
|
||||
- "The auto-post path persists a remediation_actions row (status='completed', approved_by='system:auto_report') and a remediation_completed audit_events row, so the idempotency check has a record to read AND the Action Area / Timeline UI reflect the auto-sent note (closes WR-01)"
|
||||
artifacts:
|
||||
- path: "lib/services/remediation-service.ts"
|
||||
provides: "autoPostAcknowledgment(campaignId, actor) — idempotent, persisting auto-post orchestrator"
|
||||
exports: ["autoPostAcknowledgment"]
|
||||
- path: "lib/services/remediation-service.test.ts"
|
||||
provides: "idempotency + persistence tests for autoPostAcknowledgment"
|
||||
contains: "autoPostAcknowledgment"
|
||||
- path: "lib/services/webhook-service.ts"
|
||||
provides: "auto_report branch calls autoPostAcknowledgment instead of generateAndPostAcknowledgment directly"
|
||||
contains: "autoPostAcknowledgment"
|
||||
key_links:
|
||||
- from: "lib/services/webhook-service.ts"
|
||||
to: "autoPostAcknowledgment"
|
||||
via: "runGatedPhishingStages auto_report branch"
|
||||
pattern: "autoPostAcknowledgment\\(campaignId"
|
||||
- from: "lib/services/remediation-service.ts autoPostAcknowledgment"
|
||||
to: "generateAndPostAcknowledgment"
|
||||
via: "post-commit call, gated by prior remediation_actions acknowledge_user existence check"
|
||||
pattern: "generateAndPostAcknowledgment\\(campaignId"
|
||||
---
|
||||
|
||||
<objective>
|
||||
Close the single unresolved Phase 23 gap: the auto_report webhook path
|
||||
(`runGatedPhishingStages` in `lib/services/webhook-service.ts`) calls
|
||||
`generateAndPostAcknowledgment(campaignId)` on every ticket-create webhook that
|
||||
lands in a USER_AWARENESS campaign, with no idempotency guard and no persisted
|
||||
record. Because a phishing "campaign" is normally multiple employees reporting
|
||||
the same email — and `groupReportIntoCampaign` returns the same `campaignId` for
|
||||
merged-into-existing cases — a company with `auto_classify`+`auto_report` on
|
||||
re-sends the customer-visible "thank you for reporting" note to every
|
||||
already-notified ticket on every additional report. This is CR-01 (23-REVIEW.md),
|
||||
confirmed unresolved by direct code inspection in 23-VERIFICATION.md (Truth #18).
|
||||
|
||||
Fix strategy (mirrors the already-VERIFIED, idempotent manual path in
|
||||
`remediateApprovedActions`): introduce a new idempotent, audit-persisting
|
||||
`autoPostAcknowledgment(campaignId, actor)` in `remediation-service.ts` (which
|
||||
has a test file and already owns the acknowledge-post + audit pattern), then wire
|
||||
`runGatedPhishingStages` to call it instead of posting directly. The persisted
|
||||
`remediation_actions` row is BOTH the idempotency record the next pass reads AND
|
||||
the row the Action Area / Timeline UI already render from (closes WR-01, the root
|
||||
cause of CR-01).
|
||||
|
||||
Purpose: A company that opts into auto_report gets the acknowledgment note posted
|
||||
exactly once per campaign, with a full audit/remediation trail — matching every
|
||||
other phishing state-change in this codebase.
|
||||
Output: `autoPostAcknowledgment` export + unit tests + rewired webhook branch.
|
||||
No new migration (reuses the existing `remediation_actions` / `audit_events`
|
||||
tables — deliberately avoids the "migrations don't re-run on live volumes"
|
||||
caveat).
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
|
||||
@$HOME/.claude/get-shit-done/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@.planning/PROJECT.md
|
||||
@.planning/ROADMAP.md
|
||||
@.planning/STATE.md
|
||||
@.planning/phases/23-classification-disposition-per-client-automation-gate/23-VERIFICATION.md
|
||||
@.planning/phases/23-classification-disposition-per-client-automation-gate/23-REVIEW.md
|
||||
|
||||
<interfaces>
|
||||
<!-- Contracts the executor needs. Extracted from the codebase — no exploration needed. -->
|
||||
|
||||
From lib/services/phishing-audit.ts (the ONLY audit_events writer — reuse it, do not INSERT into audit_events directly):
|
||||
```typescript
|
||||
export interface AuditEventInput {
|
||||
campaignId: string;
|
||||
actor: string | null;
|
||||
eventType: string; // canonical strings include 'remediation_completed'
|
||||
payload: Record<string, unknown>;
|
||||
}
|
||||
// Pass the transaction client to make the audit row commit/rollback atomically with the state write.
|
||||
export async function writeAuditEvent(input: AuditEventInput, client?: AuditQueryClient): Promise<string>;
|
||||
```
|
||||
|
||||
From lib/services/triage-note-service.ts (posts the customer-visible note; unchanged by this plan):
|
||||
```typescript
|
||||
export async function generateAndPostAcknowledgment(campaignId: string): Promise<TriageNoteResult>;
|
||||
```
|
||||
|
||||
remediation_actions table (migrations/097_phishing_triage_schema.sql) — reuse, no schema change:
|
||||
```sql
|
||||
-- id UUID PK default gen_random_uuid(), campaign_id UUID FK, action_type TEXT,
|
||||
-- status TEXT NOT NULL DEFAULT 'proposed', params JSONB, approved_by TEXT,
|
||||
-- approved_at TIMESTAMPTZ, created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
|
||||
```
|
||||
|
||||
Existing idempotent analog to replicate — remediateApprovedActions (lib/services/remediation-service.ts:182-236):
|
||||
- Runs state write inside postgresClient.transaction(...)
|
||||
- Writes exactly one writeAuditEvent(..., client) per real transition, INSIDE the transaction
|
||||
- Calls generateAndPostAcknowledgment(campaignId) AFTER the transaction commits, in try/catch, only when a transition actually happened (never on the idempotent re-run), never propagating the note-post error
|
||||
</interfaces>
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 1: Add idempotent, audit-persisting autoPostAcknowledgment to remediation-service.ts</name>
|
||||
<files>lib/services/remediation-service.ts, lib/services/remediation-service.test.ts</files>
|
||||
<read_first>
|
||||
- lib/services/remediation-service.ts (read fully — replicate the exact shape of `remediateApprovedActions`, lines 182-236: transaction for state, writeAuditEvent inside it, generateAndPostAcknowledgment AFTER commit in try/catch, error caught not propagated. This is the closest existing idempotency analog — the manual path is already VERIFIED and must NOT be touched, only mirrored.)
|
||||
- lib/services/phishing-audit.ts (writeAuditEvent signature + AuditEventInput; this is the ONLY place that inserts into audit_events)
|
||||
- lib/services/remediation-service.test.ts lines 1-100 (mock setup: queryMock/transactionMock, writeAuditEventMock, generateAndPostAcknowledgmentMock, makeClient() SQL-branch dispatcher, MockRows interface, stage() helper — extend these, match the style exactly)
|
||||
- migrations/097_phishing_triage_schema.sql lines 133-157 (remediation_actions + audit_events columns — confirm no schema change is needed)
|
||||
</read_first>
|
||||
<behavior>
|
||||
- Test A (first pass posts once): no existing acknowledge_user remediation_actions row for the campaign → autoPostAcknowledgment INSERTs exactly one remediation_actions row (action_type='acknowledge_user', status='completed', approved_by='system:auto_report'), calls writeAuditEvent exactly once with eventType 'remediation_completed', calls generateAndPostAcknowledgment exactly once, returns { posted: true }.
|
||||
- Test B (idempotency — THE CR-01 FIX): an acknowledge_user remediation_actions row already exists for the campaign → autoPostAcknowledgment does NOT INSERT a remediation_actions row, does NOT call writeAuditEvent, does NOT call generateAndPostAcknowledgment, returns { posted: false }.
|
||||
- Test C (note-post failure is non-fatal): generateAndPostAcknowledgment rejects → autoPostAcknowledgment still resolves (does not throw) and returns { posted: true } (the DB state/audit already committed; mirrors remediateApprovedActions' post-commit try/catch).
|
||||
</behavior>
|
||||
<action>
|
||||
Add a new exported async function `autoPostAcknowledgment(campaignId: string, actor: string | null): Promise<{ posted: boolean }>` to lib/services/remediation-service.ts. Reuse the existing imports (`postgresClient`, `writeAuditEvent`, `generateAndPostAcknowledgment`) — add no new imports.
|
||||
|
||||
Implementation, mirroring remediateApprovedActions:
|
||||
1. Open `postgresClient.transaction(async (client) => { ... })`.
|
||||
2. Serialize concurrent webhooks for the same campaign: `SELECT id FROM campaigns WHERE id = $1 FOR UPDATE` with `[campaignId]`. (Locks the campaign row so two simultaneous ticket-create webhooks for the same campaign cannot both pass the existence check.)
|
||||
3. Idempotency check: `SELECT id FROM remediation_actions WHERE campaign_id = $1 AND action_type = 'acknowledge_user' LIMIT 1`. If `rows.length > 0`, return a sentinel from the transaction indicating already-posted (e.g. `{ inserted: false }`) — do NOT insert, do NOT write audit.
|
||||
4. Otherwise INSERT the completed row: `INSERT INTO remediation_actions (campaign_id, action_type, status, approved_by, approved_at) VALUES ($1, 'acknowledge_user', 'completed', $2, NOW())` with `[campaignId, actor]`, then `await writeAuditEvent({ campaignId, actor, eventType: 'remediation_completed', payload: { actionType: 'acknowledge_user', auto: true } }, client)` INSIDE the transaction. Return `{ inserted: true }`.
|
||||
5. AFTER the transaction commits: if `inserted === true`, `try { await generateAndPostAcknowledgment(campaignId); } catch (err) { console.error('[AUTO-REMEDIATE] acknowledge_user note post failed', campaignId, err); }` — never rethrow (the persisted record already committed; the manual path accepts the identical trade-off).
|
||||
6. Return `{ posted: inserted }`.
|
||||
|
||||
Do NOT modify remediateApprovedActions, approveRemediationActions, or markCampaignFalsePositive — they are VERIFIED (truths #5, #16 etc.) and out of scope.
|
||||
|
||||
In remediation-service.test.ts: import `autoPostAcknowledgment`, add an `existingAckRows?: unknown[]` field to the MockRows interface, and extend makeClient()'s query dispatcher with two new branches (place them BEFORE the existing generic `FROM remediation_actions` / `SELECT status FROM campaigns` branches so they match first):
|
||||
- `sql.includes('SELECT id FROM campaigns') && sql.includes('FOR UPDATE')` → return `{ rows: [{ id: 'campaign-1' }], rowCount: 1 }`
|
||||
- `sql.includes("action_type = 'acknowledge_user'")` → return `{ rows: rows.existingAckRows ?? [], rowCount: rows.existingAckRows?.length ?? 0 }`
|
||||
The existing `INSERT INTO remediation_actions` branch already returns an id and is reused as-is. Add a `describe('autoPostAcknowledgment', ...)` block with Tests A, B, C from the behavior section, using the existing stage()/makeClient()/callsContaining() helpers and asserting on writeAuditEventMock / generateAndPostAcknowledgmentMock call counts.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>npx vitest run lib/services/remediation-service.test.ts</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- `lib/services/remediation-service.ts` exports `autoPostAcknowledgment` (grep: `grep -c "export async function autoPostAcknowledgment" lib/services/remediation-service.ts` returns 1).
|
||||
- The idempotency SELECT is present: `grep -c "action_type = 'acknowledge_user'" lib/services/remediation-service.ts` returns >= 1.
|
||||
- The campaign lock is present: `grep -c "FROM campaigns WHERE id = \$1 FOR UPDATE" lib/services/remediation-service.ts` returns >= 1.
|
||||
- The auto path writes an audit row: source contains a writeAuditEvent call with eventType 'remediation_completed' inside autoPostAcknowledgment.
|
||||
- `npx vitest run lib/services/remediation-service.test.ts` passes, and the new autoPostAcknowledgment describe block contains the idempotency test (Test B) asserting generateAndPostAcknowledgmentMock was NOT called when an acknowledge_user row already exists.
|
||||
- `npx tsc --noEmit --pretty` produces no output.
|
||||
- remediateApprovedActions / approveRemediationActions / markCampaignFalsePositive bodies are byte-for-byte unchanged (git diff shows additions only, no edits to the existing functions).
|
||||
</acceptance_criteria>
|
||||
<done>autoPostAcknowledgment exists, is idempotent per campaign, persists a remediation_actions + remediation_completed audit_events row on first post, and is covered by a passing test proving the second call does not re-post.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 2: Wire runGatedPhishingStages auto_report branch to autoPostAcknowledgment</name>
|
||||
<files>lib/services/webhook-service.ts</files>
|
||||
<read_first>
|
||||
- lib/services/webhook-service.ts lines 1-23 (import block) and lines 509-568 (runGatedPhishingStages — the auto_report branch at lines 550-566 that currently calls generateAndPostAcknowledgment(campaignId) with no guard; this is the exact defect site from Truth #18)
|
||||
- lib/services/remediation-service.ts (confirm the autoPostAcknowledgment signature added in Task 1)
|
||||
</read_first>
|
||||
<action>
|
||||
In lib/services/webhook-service.ts:
|
||||
1. Replace the import `import { generateAndPostAcknowledgment } from './triage-note-service';` (line 22) with `import { autoPostAcknowledgment } from './remediation-service';`. Confirm no other reference to `generateAndPostAcknowledgment` remains in this file (it was only used in the auto_report branch); if none remain, the import removal leaves no dangling reference.
|
||||
2. In the auto_report branch, replace `await generateAndPostAcknowledgment(campaignId);` (line ~561) with `await autoPostAcknowledgment(campaignId, 'system:auto_report');`. Leave the surrounding `if (verdict === 'USER_AWARENESS')` guard, the verdict-backfill query, and the stage's try/catch exactly as they are — only the single post call changes.
|
||||
|
||||
The `'system:auto_report'` actor sentinel is what stamps `remediation_actions.approved_by` and `audit_events.actor`, distinguishing the auto path from a human approver in the Action Area / Timeline UI (WR-01).
|
||||
|
||||
Do NOT change detectPhishingTicket, groupReportIntoCampaign, the gate lookup, the auto_parse or auto_classify stages, or any other webhook behavior — truth #17 (gated pipeline wiring) is VERIFIED and must remain intact.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>grep -c "autoPostAcknowledgment(campaignId, 'system:auto_report')" lib/services/webhook-service.ts</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- The auto_report branch calls the new function: `grep -c "autoPostAcknowledgment(campaignId, 'system:auto_report')" lib/services/webhook-service.ts` returns 1.
|
||||
- The direct unguarded post is gone: `grep -c "generateAndPostAcknowledgment" lib/services/webhook-service.ts` returns 0.
|
||||
- The new import is present: `grep -c "import { autoPostAcknowledgment } from './remediation-service'" lib/services/webhook-service.ts` returns 1.
|
||||
- The `if (verdict === 'USER_AWARENESS')` guard is still present (grep: `grep -c "verdict === 'USER_AWARENESS'" lib/services/webhook-service.ts` returns >= 1).
|
||||
- `npx tsc --noEmit --pretty` produces no output.
|
||||
- `npx vitest run lib/services/remediation-service.test.ts lib/services/phishing-automation-gate.test.ts` passes (no regression in the phase's tested surfaces).
|
||||
</acceptance_criteria>
|
||||
<done>The webhook auto_report path posts the acknowledge_user note via the idempotent autoPostAcknowledgment; a repeat webhook for the same campaign finds the persisted remediation_actions row and skips the re-post.</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<threat_model>
|
||||
## Trust Boundaries
|
||||
|
||||
| Boundary | Description |
|
||||
|----------|-------------|
|
||||
| Autotask webhook -> pipeline | Untrusted external trigger; fires once per ticket-create, repeatable/redeliverable |
|
||||
| pipeline -> Autotask TicketNotes (noteType 18) | Customer-visible external side effect — the asset being over-triggered |
|
||||
|
||||
## STRIDE Threat Register
|
||||
|
||||
| Threat ID | Category | Component | Disposition | Mitigation Plan |
|
||||
|-----------|----------|-----------|-------------|-----------------|
|
||||
| T-23-06-01 | Denial of Service / Repudiation | `runGatedPhishingStages` auto_report -> `autoPostAcknowledgment` | mitigate | Idempotency via a persisted `remediation_actions` acknowledge_user row (checked under `SELECT ... campaigns FOR UPDATE`); every auto-post writes exactly one `remediation_completed` audit_events row stamped `actor='system:auto_report'` — no un-audited customer-visible side effect, no N-fold re-post (closes CR-01/WR-01) |
|
||||
| T-23-06-02 | Tampering (race) | concurrent same-campaign webhooks | mitigate | `SELECT id FROM campaigns WHERE id = $1 FOR UPDATE` serializes the check-then-insert so two simultaneous webhooks cannot both pass the existence check and double-post |
|
||||
| T-23-06-03 | (accepted) redelivered Autotask webhook | `logWebhookEvent` dedup gap (IN/CR pre-existing) | accept | Out of this gap's scope (webhook-event dedup is a separate pre-existing concern, IN in 23-REVIEW.md); the per-campaign acknowledge idempotency added here already prevents the customer-visible duplicate even on redelivery |
|
||||
|
||||
No package-manager installs in this plan — no package legitimacy gate required.
|
||||
</threat_model>
|
||||
|
||||
<verification>
|
||||
- `npx tsc --noEmit --pretty` — clean (no output).
|
||||
- `npx vitest run lib/services/remediation-service.test.ts` — passes, including the new autoPostAcknowledgment idempotency test (Test B) proving the second call does not re-post.
|
||||
- `grep -c "generateAndPostAcknowledgment" lib/services/webhook-service.ts` — returns 0 (the unguarded direct call is removed).
|
||||
- `grep -c "autoPostAcknowledgment(campaignId, 'system:auto_report')" lib/services/webhook-service.ts` — returns 1.
|
||||
- Manual path untouched: `git diff` shows no edits to remediateApprovedActions / approveRemediationActions / markCampaignFalsePositive bodies.
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
Truth #18 / AUTOGATE-03 moves from PARTIALLY SATISFIED to fully SATISFIED:
|
||||
- First auto_report pass for a USER_AWARENESS campaign posts the acknowledge_user note exactly once and persists a remediation_actions (status='completed', approved_by='system:auto_report') + remediation_completed audit_events row.
|
||||
- Every subsequent ticket-create webhook joining the same campaign reads the persisted acknowledge_user row and skips the post — zero duplicate customer-visible notes.
|
||||
- No VERIFIED truth (#1-17) or its files/behavior is modified.
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
Create `.planning/phases/23-classification-disposition-per-client-automation-gate/23-06-SUMMARY.md` when done.
|
||||
</output>
|
||||
Loading…
Add table
Add a link
Reference in a new issue