Repository navigation
refactor(workers): move security alert and attack-simulation handlers into apps/workers - #4408
aflordelis wants to merge 7 commits into
Conversation
… into apps/workers Rename only; imports and infra handler paths are repointed in the next commit.
…tries The attack-simulation types stay in apps/client (the ingest route owns the payload contract), so the runner and probes import them through the @server bridge. The triage prompt now names the probes' new home.
🟠 Preview is 25 commits behind
Preview URL is in the internal deploys channel. |
|
🚀 Preview is live at
|
There was a problem hiding this comment.
Bot review. No blocking findings. Clean mechanical move, well-fenced.
What I checked
@server/*resolution.apps/workers/tsconfig.jsonmaps@server/*to../client/server/*, so@server/security/attackSimulation/typesresolves toapps/client/server/security/attackSimulation/types.ts, which still exists. All seven probes andrunner.tswere correctly repointed to this absolute alias; no../typesleftovers.- SST handler update. The admin "Run Now" endpoint invokes the Lambda by resource name (
Resource.lambdaFunctionNames.attackSimulationatrun-attack-simulation.ts#L45), not by handler path, so the retargeted handlers ininfra/cron.ts#L451andinfra/securityAlerts.ts#L20are the only wiring touched. Unchanged construct IDs mean SST should update in place; the preview deploy at head went green, consistent with that. - Guard coverage.
checkWorkersMovedDirs.test.ts:MOVEDempties the wholeapps/client/server/security/attackSimulation/probessubtree;MOVED_MODULESblocksalertProcessoratsecurity/andfingerprint+runneratattackSimulation/by file-stem, correctly leavingtypes.tsto stay. The infra-handler scan at L148-149 anchors both new handler strings, so a stale handler insecurityAlerts.tsorcron.tswould fail the test. The siblingevery apps/workers infra handler resolves to a real module filetest also covers the two new handlers. - Stale references. No remaining imports of
apps/client/server/security/{alertProcessor,attackSimulation}anywhere in the tree. The surviving references toapps/client/server/security/*are only to files the PR explicitly leaves behind (cloudScan,wafScan,wafHeaderRedaction,wafLogsInsights,resolveStage, etc.). - ASCII rule on added lines. The one em-dash replaced in
secopsTriageService.ts:416is on an added line and now uses a hyphen; the CLAUDE.md and workers-README additions are ASCII-only.
Notes
- The runner and probes now reach their types through the
@server/*bridge (absolute path) rather than a../typesrelative path. The PR body calls this out as the deliberate tradeoff for leavingtypes.tswith the ingest route it is a payload contract for; agreed, no action. - The PR body's own "Unverified: that SST updates both functions in place rather than replacing them" was reduced-but-not-eliminated by the green preview deploy, since preview only exercises one stage. Worth watching the staging/prod deploy that follows merge for a replacement rather than an update.
Nothing requested from the author.
Test Report: PR #4408 refactor: move security alert and attack-simulation handlers into apps/workers
Overall result:
|
| File | Shows | Image |
|---|---|---|
| B4M-4408-00-login-admin.png | Signed in as QA Admin on /new | |
| B4M-4408-02-active-defense-before-run.png | Active Defense tab before Run Now, with one prior failed run | |
| B4M-4408-03-run-now-queued.png | "Attack simulation queued" alert after Run Now, no error toast | |
| B4M-4408-04-run-finished-failed.png | New 8:25:49 PM row: failed, 0 probes, 1 error, cooldown banner | |
| B4M-4408-05-cloud-tab.png | Cloud tab empty state (regression check) | |
| B4M-4408-06-waf-tab.png | Firewall / WAF tab empty state (regression check) |
julsanchez
left a comment
There was a problem hiding this comment.
Review pass 1 — approved
A clean move: the diff is renames plus import-path and handler-string edits, and the guard, bundle check and full typecheck all pass. Breaking things on purpose shows the guard and the bundle check both fail when they should.
Approving. I diffed the change at 27ecc18 with git diff -M. It is renames plus four kinds of edit: seven import type paths, two infra handler strings, one prompt path, and the guard and docs additions. No probe logic changed. assertTargetUrlIsSafe and the runner's domain guard are byte-identical. One P3 about a comment, below.
This review builds on the claude bot review and the QA pass at the same head, and does not redo them. QA's step-3 failed run comes from an environment gap: SECOPS_ATTACK_SIMULATION_INGEST_TOKEN is not set on pr4408. It does not gate this PR, and I agree. The relocated runner Lambda did load and write its run row. QA's failRun observation is pre-existing and is listed below.
Not blocking
-
F1 (P3)
packages/scripts/src/checkWorkersMovedDirs.test.ts:147— theinfra/securityAlerts.tscomment also sits over the runner anchor, which lives ininfra/cron.tsThe added comment at L147 says
infra/securityAlerts.ts, which no other moved handler lives in.It sits above two anchors: L148alertProcessor.handler(which is ininfra/securityAlerts.ts:20) and L149attackSimulation/runner.handler(which is ininfra/cron.ts:451). A reader would take the runner anchor to be asecurityAlerts.tshandler. The comment pattern also says each anchor exists because its file is otherwise unscanned, which is not true ofcron.ts: L134 already proves that file is read.Not wrong: the assertion itself is correct, and it fails when either handler is reverted. I reverted
infra/securityAlerts.ts:20to the old path and the test failed (1 failed | 47 passed).Fix. Put the runner anchor on its own line with
// infra/cron.tsas its comment, or drop that anchor, since the L134cron/anchor already provescron.tsis scanned. I would take the first: it keeps the explicit anchor the author added in the last commit.
Pre-existing, not introduced by this PR
- From QA (carried forward, not my finding): when the ingest token is missing,
failRundiscardsprobesRun/probeErrors, and the UI never shows theerrorcause. Therunner.tshunk is the import line only, so this predates the PR. runner.ts:4says "Executes 7 HTTP probes" but there are 6 (QA observation; the comment moved unchanged).
What the PR claims to do
| # | Claim | Holds? | Evidence |
|---|---|---|---|
| C1 | Move-only, no behavior change | met | git diff -M $(merge-base) 27ecc18: alertProcessor.ts, fingerprint.ts and probeUtils.ts are 100% renames. runner.ts (99%) and the 6 probes (95-98%) differ only in the import type ... types line. |
| C2 | attackSimulation/types.ts stays in the client; the runner and probes import it through @server/* |
met | apps/client/server/security/attackSimulation/types.ts still exists, and pages/api/admin/security-dashboard/attack-simulation-ingest.ts:17 imports it. It is type-only, so nothing from it is bundled at runtime. |
| C3 | Handler paths retargeted; construct IDs unchanged | met | infra/cron.ts:450-451 (attackSimulationFunction) and infra/securityAlerts.ts:17-20 (SecurityAlertsSchedule): only handler changed. infra/web.ts:169 still links attackSimulationFunction.name. |
| C4 | Probes path in the SecOps triage prompt updated | met | secopsTriageService.ts:416 now names apps/workers/src/security/attackSimulation/probes, which exists. |
| C5 | The guard stops moved files from being recreated and anchors both handler strings | met | vitest run checkWorkersMovedDirs: 48/48. I reverted the securityAlerts.ts handler and the guard failed (no infra handler points into a moved directory). |
| C6 | check-lambda-bundle.mjs: OK, 111 handlers bundled |
met | Reproduced: OK: 111 handlers bundled, 10283 modules resolved. I broke a probe's ../fingerprint import and got Could not resolve "../nope", so the moved files really are inside the scan. |
| C7 | Workers and client typecheck pass | met | Full pnpm turbo:typecheck: 41/41 tasks pass. CI Type Check is also green. |
| C8 | Workers tests 1234 pass; client node project 12772 pass; lint pass | untested | Not re-run by me. At this head CI Run Tests (all shards), Run Tests (workers-integration) and Lint are green. No tests were added or moved. |
| C9 | SST updates both functions in place (the PR's own checkbox marks this unverified) | partial | Reasoned, not run against AWS. On aws:lambda/Function, handler updates in place, and the logical names are unchanged. The preview deploy is green, and QA's Run Now ran the relocated runner. The alert processor's schedule is enabled only on production/dev (infra/securityAlerts.ts:30), so no preview has ever run it. |
| C10 | After this PR, cloudScan is the only Lambda handler still pointing into apps/client/server/security |
met | grep handler: 'apps/client/server/security' infra: the only hit is infra/cron.ts:308 cloudScan.handler. |
Docs & changelog — complete
User-visible change: no — Internal relocation; nothing a user can see changes.
| Expected | Touched? |
|---|---|
apps/workers/README.md |
yes |
CLAUDE.md (apps/workers line) |
yes |
Now-wrong: none. selfhost/, docs-site/ and docs/ have no references to the old paths. The only remaining reference into apps/client/server/security/attackSimulation is the ingest route's types import, which is intended.
Notes
Gates observed on 27ecc18, run by me in an isolated worktree. pnpm turbo:core:build OK. node scripts/check-lambda-bundle.mjs: 111 handlers bundled. packages/scripts vitest run checkWorkersMovedDirs: 48/48. pnpm turbo:typecheck: 41/41. CI at this head: every check is green and none failed. The preview gate FAILURE that Intake saw is now pass.
The bundle guard did not run in CI on this PR. lambda-bundle-guard.yml only triggers on package.json, the lockfile and apps/workers/tsconfig.json changes, so this PR's bundling evidence is the green preview deploy plus my local run above. That is not a defect in this PR. It means a move-only PR gets no CI bundle signal unless it also touches infra/ and so triggers a preview.
The first real run of alertProcessor from its new path will be on dev after merge. SecurityAlertsSchedule is enabled only for production/dev. Watch that stage's first 5-minute run, and check that the dev deploy updates the function rather than replacing it.
Ruled out. The moved files' remaining relative imports (./probes/*, ../fingerprint, ./probeUtils) all moved with them. The cross-tree imports (@server/utils/config, @server/security/resolveStage, @server/integrations/github/githubWorkflowTrigger, @server/utils/logger, @server/utils/eventBus) resolve through apps/workers/tsconfig.json's interim alias, the same way the other moved handlers do. git blame on both handler lines goes back only to the initial open-core import, so the old paths had no recorded reason to stay where they were. The PR merges cleanly onto origin/main (git merge-tree), and the head is 22 commits behind it.
Not reviewed. AWS-side state, which needs CloudWatch and deploy-plan access. The runner's success path ("finished with probe counts") is blocked by the missing preview secret, per QA.
Independently verified on 27ecc18: lambda bundle 111/111, checkWorkersMovedDirs 48/48 plus two deliberate-break checks, pnpm turbo:typecheck 41/41.
Reviewed origin/main…27ecc182 · 16 files, +33/-14 · 0 P0 · 0 P1 · 0 P2 · 1 P3
The runner handler lives in infra/cron.ts, not infra/securityAlerts.ts, so give its anchor its own comment instead of sharing the securityAlerts.ts one.
|
Review follow-up: F1 (P3) fixed in d250acc. The The branch also merges Not changed: the pre-existing |
QA delta since last run@julsanchez your QA pass ran on Delta: 1 fix commit + a merge of main. No re-test needed.
The open item from the last pass is unchanged: step 3 (Run Now) still needs |
julsanchez
left a comment
There was a problem hiding this comment.
Re-approving at d250acc after the review follow-up push: F1 fixed (test comment only) plus a clean merge of main. Pass-1 review stands; see the follow-up comment above.
Description
Refs #73
Moves the security alert processor and the Active Defense attack-simulation runner Lambdas from
apps/client/server/securityintoapps/workers/src/security. Move-only, no behavior change.Refs, notCloses: this is one slice of #73, andcloudScan/wafScanstay in the client because admin API routes call them directly.Customer note
none
Changes
git mvintoapps/workers/src/security/:alertProcessor.ts,attackSimulation/runner.ts,attackSimulation/fingerprint.ts, and the 7attackSimulation/probes/*files (commit 1 is 100% renames)attackSimulation/types.tsstays in the client (it is the ingest route's payload contract); the runner and probes now import it through@server/*infra/securityAlerts.tsandinfra/cron.ts; construct IDs are unchangedsecopsTriageService.ts)checkWorkersMovedDirs.test.tsso the moved files can't be recreated at the old path, and anchored both new handler strings in its infra scansrc/securityinapps/workers/README.mdandCLAUDE.mdGuide for Testers
Needs a preview deploy (the change only touches where two Lambdas are bundled from).
Regression Checks
What NOT to test
Additional Information
cloudScan(infra/cron.ts) is the only Lambda handler still pointing intoapps/client/server/security.Verification and open items
pnpm --filter @bike4mind/workers typecheck/ client typecheckpnpm --filter @bike4mind/workers testvitest run --project nodecheckWorkersMovedDirs.test.tspnpm lint:checkpnpm turbo:core:build && node scripts/check-lambda-bundle.mjshandlerchanges; the preview deploy confirms it.apps/workers/README.mdandCLAUDE.md, fix(credits): release the voice credit hold when a call never connects #4333 and feat(data-lakes): show last-synced time and sync schedule on the Drive lake panel #4384 elsewhere ininfra/cron.ts.Checklist