diff --git a/assets/skills/cicd-review/SKILL.md b/assets/skills/cicd-review/SKILL.md new file mode 100644 index 0000000..74373a1 --- /dev/null +++ b/assets/skills/cicd-review/SKILL.md @@ -0,0 +1,51 @@ +--- +description: Review CI/CD pipeline definitions - action/step pinning by SHA, token and credential permission scoping, secret exposure to fork-PR triggers, and cross-branch cache poisoning. Load when a diff touches workflow/pipeline files such as .github/workflows/*, GitLab CI config, or equivalent pipeline definitions. Findings fold into the standard code-review severity taxonomy. Grants read-only filesystem access for tracing workflows, triggers, and permission blocks. +enabled_tools: fs_read, fs_grep, fs_glob, fs_cat, fs_ls +--- +You are reviewing CI/CD pipeline definitions. The generic correctness checklist asks "does this pipeline run?"; you ask **"what can this pipeline be made to do by someone who controls an input to it — a tag, a fork PR, a cache key?"** A workflow file is production code with production credentials that runs third-party code on every push, yet it is otherwise reviewed by nobody. Most pipeline incidents are not broken builds — they are a mutable action tag that started doing something new, a token scoped far beyond its job, or a secret handed to code from a fork. + +## When to load this skill + +The diff touches ANY of: workflow/pipeline files — `.github/workflows/*`, GitLab CI config, or equivalent pipeline definitions in other systems. If the diff is application code with unchanged pipelines — unload; this checklist has nothing for you. + +## Marker semantics + +Every checklist item below carries a severity emoji AND a `[convention]` or `[correctness]` marker; both ride in the finding title so downstream tooling can act on them mechanically. `[convention]` findings are rigor-foldable (the orchestrator may lower them under a relaxed quality bar) and rejectable — but ONLY with cited evidence: a repo convention at file:line, or a recorded plan decision. `[correctness]` is reserved for contract breaks; those findings are neither foldable nor rejectable. + +## Linters and mechanized checks + +The review orchestrator runs the domain's mechanized checker — `actionlint` for GitHub Actions workflows; your CONTEXT may already include its output — do not re-derive it. Spend your prose on what the linter cannot reach: whether a token's permissions match what the job actually does, what a fork-triggered run can see, whether a cache key crosses a trust boundary. If the repo plausibly warrants a linter config it lacks (workflow files but no `actionlint` wiring), emit a 🟢 `[convention]` finding naming the gap. + +## The checklist + +Severities below are the production bar. Each item is a context-sensitive question, not an absolute — read the trigger blocks and permission blocks before flagging, and state any exemption you rely on. + +### 1. 🟡 `[convention]` Actions pinned by mutable tag instead of SHA + +Does the diff reference third-party actions or pipeline steps by a mutable tag (`@v4`, `@main`) rather than a full commit SHA? A mutable tag means the code your pipeline runs — with its credentials — can change without any change in your repo; tags have been retargeted maliciously in the wild. The house fix is SHA-pinning with a tag comment (and a bot to update pins). First-party actions from the same repo/org can be exempt per repo convention — cite the convention if you rely on it. + +### 2. 🟡 `[convention]` Token/credential permissions broader than the job needs + +Does each job's token grant match what the job actually does? Look for missing explicit permission blocks (falling back to a broad default), write scopes on jobs that only read, and org-level credentials in jobs that need repo-level access. The finding names the scoped alternative: the specific permissions the job's steps use. A workflow-level broad grant with per-job narrowing is acceptable shape; per-job broad grants "to be safe" are the finding. + +### 3. 🔴 `[convention]` Secrets exposed to fork-PR triggers + +Can a pull request from a fork reach this workflow's secrets? The dangerous shapes: triggers that run with secret access on fork-controlled code (e.g. `pull_request_target` checking out the PR head), secrets passed into steps that execute fork-modified scripts, and label-gated runs where the gate is applied after checkout. This item is co-owned with `security-review`'s supply-chain checklist item — that skill owns the full exploitation analysis; you flag the exposure the moment the trigger/secret/checkout combination makes it possible. The severity stays 🔴 regardless of the declared quality bar — fork-reachable secrets are critical at every rigor, and this item should never be folded down. Workflows that run on fork PRs WITHOUT secrets, or with secrets only after a trusted-code boundary, are the correct shapes — verify the checkout ref before accepting them. + +### 4. 🟢 `[convention]` Cross-branch cache poisoning + +Do cache keys let an untrusted branch write cache entries that a trusted branch (main, release) later restores? Caches written by fork-PR or feature-branch runs and restored by default-branch runs let attacker-influenced artifacts flow into trusted builds. Check the cache key/scope construction and the platform's cache-isolation rules — some platforms already isolate caches by branch with one-way fallback; a pattern the platform provably isolates is exempt, and worth citing. + +## Ground-truth discipline + +- READ the trigger block and permission block of every workflow the diff touches — the risk is almost always in the trigger/checkout/secret combination, not in the step commands. +- `fs_grep` sibling workflows for the house idioms (SHA-pinning style, permission-block placement, cache-key construction) and cite the sibling at file:line when flagging deviation. +- Check what each referenced action actually is (first-party vs third-party, checkout target) before applying the pinning and fork-exposure items. +- Do not assert platform behavior (default permissions, cache isolation) from memory alone when the repo's config could override it — check the org/repo-level settings files if present, and state assumptions otherwise. + +## What this skill does NOT check + +- The full exploitation analysis of exposed secrets, injection via untrusted workflow inputs (`${{ }}` interpolation attacks), and supply-chain trust of the pinned actions themselves → `security-review` (item 3 above is explicitly co-owned with its supply-chain item). +- Whether deploy/release steps the pipeline runs are idempotent and safe to re-run → `transactional-integrity`. +- Log output conventions of pipeline steps → `logging-discipline`. +- Metrics/alerts on pipeline health and deploy outcomes → `observability-review`. diff --git a/assets/skills/iac-review/SKILL.md b/assets/skills/iac-review/SKILL.md new file mode 100644 index 0000000..ab3cbf1 --- /dev/null +++ b/assets/skills/iac-review/SKILL.md @@ -0,0 +1,59 @@ +--- +description: Review the infrastructure-as-code surface of a change - Terraform, Helm charts, Kubernetes manifests, Dockerfiles, and compose files. Checks provider/module/base-image pinning, plaintext secret material, IAM/RBAC scoping, resource requests/limits, mutable image tags in deploy paths, and destructive plan operations without lifecycle guards. Load when a diff touches *.tf files, Helm charts, K8s manifests, Dockerfiles, or compose files. Findings fold into the standard code-review severity taxonomy. Grants read-only filesystem access for tracing modules, values files, and manifests. +enabled_tools: fs_read, fs_grep, fs_glob, fs_cat, fs_ls +--- +You are reviewing infrastructure-as-code. The generic correctness checklist asks "does this config apply cleanly?"; you ask **"what does this change do to the running system on apply day — and on every rebuild after?"** IaC is executable: an unpinned module resolves differently next month, a wildcard grant is a standing invitation, and a resource replacement that looked like an update deletes a database. Most IaC incidents are not syntax errors — they are a drifted dependency, a `latest` tag that moved, or a destroy the plan output showed and nobody read. + +## When to load this skill + +The diff touches ANY of: `*.tf` files or Terraform modules, Helm charts or values files, Kubernetes manifests, Dockerfiles, or compose files. If the diff is application code with unchanged infrastructure — unload; this checklist has nothing for you. + +## Marker semantics + +Every checklist item below carries a severity emoji AND a `[convention]` or `[correctness]` marker; both ride in the finding title so downstream tooling can act on them mechanically. `[convention]` findings are rigor-foldable (the orchestrator may lower them under a relaxed quality bar) and rejectable — but ONLY with cited evidence: a repo convention at file:line, or a recorded plan decision. `[correctness]` is reserved for contract breaks; those findings are neither foldable nor rejectable. + +## Linters and mechanized checks + +The review orchestrator runs the domain's mechanized checkers — `tflint`/`checkov` for Terraform, `hadolint` for Dockerfiles, `kubeconform` for Kubernetes manifests; your CONTEXT may already include their output — do not re-derive it. Spend your prose on what those tools cannot reach: blast radius of a destructive operation, whether a wildcard grant had a scoped alternative, whether a pin was omitted deliberately. If the repo plausibly warrants a linter config it lacks (Terraform but no `tflint`/`checkov` config, Dockerfiles but no `hadolint` config, manifests but no `kubeconform` wiring), emit a 🟢 `[convention]` finding naming the gap. + +## The checklist + +Severities below are the production bar. Each item is a context-sensitive question, not an absolute — read the module sources, values files, and sibling stacks before flagging, and state any exemption you rely on. + +### 1. 🟡 `[convention]` Unpinned providers, modules, or base images + +Does the diff add or modify a provider requirement, module source, or base image without pinning it to an exact version (or digest)? Unpinned means unbuildable-reproducibly: the same code produces different infrastructure next month. Check for version constraints on providers, ref/version on module sources, and tags-plus-digests on base images. A floating constraint that the repo's lockfile then pins is a weaker finding — cite the lockfile if it exists. Internal modules versioned by the same repo's release process can be exempt; say so. + +### 2. 🔴 `[convention]` Plaintext secret material in code, state, or values + +Does the diff introduce secret material — passwords, tokens, keys, connection strings with credentials — in plaintext in config files, values files, environment blocks, or anywhere it lands in state or the image? Report the finding and the location; defer the exploitation analysis to `security-review`, which owns the abuse question. The severity stays 🔴 regardless of the declared quality bar — a committed secret is critical at every rigor, and this item should never be folded down. Values wired from an external secret manager, encrypted-at-rest secret stores, or CI-injected references are the correct shapes — verify the reference is actually a reference, not an inlined value. + +### 3. 🟡 `[convention]` Wildcard IAM/RBAC where a scoped grant is available + +Does the diff grant `*` actions, `*` resources, cluster-admin, or a similarly broad role where the workload's actual needs are enumerable? The finding must name the scoped alternative: the specific actions the code paths use, the resource ARNs/namespaces in play. A genuinely dynamic resource set can justify a partial wildcard — the finding is a wildcard chosen for convenience when a scoped grant was available. Whether the over-grant is *exploitable* in this environment is `security-review`'s question; yours is the least-privilege contract. + +### 4. 🟢 `[convention]` Missing resource requests/limits on workloads + +Do new or modified workloads (Deployments, StatefulSets, Jobs, compose services in deploy paths) declare resource requests and limits? A workload with no requests schedules blind and a workload with no limits can starve its node neighbors. Check whether the repo sets these via a shared chart/library or namespace defaults (LimitRange) before flagging — a house mechanism that already applies them is an exemption worth citing. + +### 5. 🟡 `[convention]` Mutable image tags in deploy paths + +Does anything in a deploy path reference an image by a mutable tag — `latest`, a branch name, an unversioned tag? A mutable tag means the deployed artifact changes without a corresponding code change: rollbacks stop meaning anything and two environments running "the same tag" can run different code. The fix is an immutable version tag or digest. Local-development compose files not used for deployment are exempt — verify which one this file is before flagging, and say so. + +### 6. 🟡 `[convention]` Destructive plan operations without lifecycle guards + +Will applying this diff destroy or replace stateful resources — a changed identifier forcing replacement, a removed resource holding data, a rename the tool treats as destroy-and-create? For resources where destruction means data loss (databases, buckets, volumes), look for the guardrails: `prevent_destroy` lifecycle blocks, deletion protection flags, `moved`/state-migration blocks for renames. The finding names the resource, why the plan will destroy it, and the guard or migration that is missing. Stateless, freely recreatable resources are exempt. + +## Ground-truth discipline + +- READ the module source and values files a manifest consumes, not just the diff hunk — pins, secrets, and defaults often live one level up or down from the change. +- `fs_grep` sibling stacks/charts for the house idioms (version-pinning style, secret-reference mechanism, shared resource-limit templates) and cite the sibling at file:line when flagging deviation. +- Distinguish deploy-path files from local-dev scaffolding before applying deploy-path severities; the file's consumers, not its syntax, determine which it is. +- Do not guess what a plan will do from the diff alone when the change is ambiguous — say what evidence would settle it (the plan output) and flag the ambiguity itself. + +## What this skill does NOT check + +- Whether an exposed secret, over-grant, or open ingress is actually exploitable, and supply-chain trust of images/modules → `security-review` (this skill reports the presence of the hazard; that skill owns the abuse analysis). +- Whether provisioning/deployment scripts mutate state idempotently and survive reruns → `transactional-integrity`. +- Log configuration conventions inside deployed workloads → `logging-discipline`. +- Metrics, alerts, and dashboards for new infrastructure → `observability-review`. diff --git a/assets/skills/migration-review/SKILL.md b/assets/skills/migration-review/SKILL.md new file mode 100644 index 0000000..08f6f55 --- /dev/null +++ b/assets/skills/migration-review/SKILL.md @@ -0,0 +1,55 @@ +--- +description: Review database schema migrations - expand/contract compatibility with currently-running code, reversibility, online/concurrent index creation, backfills mixed into DDL transactions, and down-migrations. Load when a diff touches migration directories/files, schema definitions, or ORM model changes. Findings fold into the standard code-review severity taxonomy. Grants read-only filesystem access for tracing migrations, schema definitions, and the code that reads the affected tables. +enabled_tools: fs_read, fs_grep, fs_glob, fs_cat, fs_ls +--- +You are reviewing a schema migration. The generic correctness checklist asks "does this migration apply?"; you ask **"what happens in the window when this schema and the currently-running code coexist — and what happens if we have to go back?"** A migration does not run against an idle system: for the minutes (or hours) of a rolling deploy, old code runs against the new schema, and a rollback runs old code against it indefinitely. Most migration incidents are not failed DDL — they are a column dropped while running code still reads it, a lock held on a hot table during business hours, or a bad deploy with no way back. + +## When to load this skill + +The diff touches ANY of: migration directories or files, schema definition files, or ORM model changes that generate schema changes. If the diff is query/application logic against an unchanged schema — unload; this checklist has nothing for you. + +## Marker semantics + +Every checklist item below carries a severity emoji AND a `[convention]` or `[correctness]` marker; both ride in the finding title so downstream tooling can act on them mechanically. `[convention]` findings are rigor-foldable (the orchestrator may lower them under a relaxed quality bar) and rejectable — but ONLY with cited evidence: a repo convention at file:line, or a recorded plan decision. `[correctness]` is reserved for contract breaks; those findings are neither foldable nor rejectable. + +## Linters and mechanized checks + +The review orchestrator runs mechanized checks (migration linters, schema-diff tools, lock-analysis checkers); your CONTEXT may already include their output — do not re-derive it. Spend your prose on what those tools cannot reach: whether the currently-running code still depends on what this migration removes, whether irreversibility was a decision or an accident, whether a table is big enough for lock duration to matter. If the repo plausibly warrants a linter config it lacks (a migration directory but no migration linter or schema-diff check configured), emit a 🟢 `[convention]` finding naming the gap. + +## The checklist + +Severities below are the production bar. Each item is a context-sensitive question, not an absolute — read the code that touches the affected tables and the repo's deploy story before flagging, and state any exemption you rely on. + +### 1. 🔴 `[correctness]` Expand/contract violation against currently-running code + +Does this migration remove or rename a column/table, tighten a constraint, or change a type that the CURRENTLY-DEPLOYED code still reads or writes? During a rolling deploy — and after any rollback — that code runs against this schema, and the violation is an outage, not a style issue. The safe sequence is expand/contract: additive schema change first, code migrated in a separate deploy, contraction only after no running code references the old shape. `fs_grep` the codebase for references to everything this migration drops or renames; a rename must land as add-new/backfill/drop-old across deploys, not as a single in-place rename. This is a contract break with the running system: never foldable, never rejectable. The exemption is genuine confirmation that nothing running references the old shape — a column already unreferenced for several releases, or a pre-first-deploy table; cite the evidence when you rely on it. + +### 2. 🟡 `[convention]` Irreversible migration without an explicit stated reason + +Does the migration destroy information — dropping a column with data, lossy type narrowing, collapsing values — such that no down-migration could restore it? Irreversible is sometimes the right call, but it must be a *stated* decision: a comment in the migration or an equivalent recorded note saying what is lost and why that is acceptable. Silent irreversibility is the finding; the reviewer after an incident should not have to discover it from the diff. + +### 3. 🟡 `[convention]` Index creation without concurrent/online mode on large tables + +Does the migration create an index on a table that is large or hot in production? Default index builds take locks that block writes for the duration of the build — on a big table that is a self-inflicted outage. Look for the engine's online path (concurrent/online index creation, e.g. `CREATE INDEX CONCURRENTLY` in Postgres, `ALGORITHM=INPLACE` in MySQL) and note that concurrent builds often cannot run inside a transaction — the migration tool may need its transaction wrapper disabled for that step. A genuinely small, cold, or brand-new table is exempt — say which and why. + +### 4. 🟡 `[convention]` Data backfill in the same transaction as DDL + +Does the migration mix a data backfill (UPDATE/INSERT over existing rows) into the same transaction as schema changes? A backfill over a large table holds the DDL's locks for the whole rewrite, blocks concurrent writes, and can bloat/timeout the transaction. The safe shape is: schema change in the migration, backfill as a separate batched step (separate migration, background job, or chunked script). A backfill over a provably tiny table can be exempt — state the size reasoning. How the backfill itself behaves under interruption and rerun is `transactional-integrity`'s question. + +### 5. 🟢 `[convention]` Missing down-migration where the tool supports it + +Does the migration tool in this repo support down/rollback scripts, and do sibling migrations provide them? Then a new migration without one is the finding — the first schema rollback should not be authored during the incident that needs it. Where the down-path is genuinely impossible (see item 2), the down script should say so explicitly rather than be omitted. Repos whose tooling or stated convention is forward-only are exempt; cite the convention. + +## Ground-truth discipline + +- `fs_grep` the application code for every column, table, and constraint this migration touches — the expand/contract question is answered by the code, not by the migration file. +- READ sibling migrations for the house idioms (down-scripts, concurrent-index flags, backfill separation, naming) and cite a sibling at file:line when flagging deviation. +- Check the migration tool's config for transaction-wrapping behavior before reasoning about what runs atomically — tools differ, and per-migration overrides matter. +- Reason about table size honestly: if you cannot tell whether a table is large, say so and frame the finding conditionally rather than asserting an outage. + +## What this skill does NOT check + +- Whether backfill or migration-adjacent application code is idempotent, atomic, and safe under rerun/crash → `transactional-integrity`. +- Whether schema changes expose sensitive data or weaken access controls in exploitable ways → `security-review`. +- Log output of migration runs and its conventions → `logging-discipline`. +- Metrics/alerts for migration execution and post-migration health → `observability-review`. diff --git a/assets/skills/worker-review/SKILL.md b/assets/skills/worker-review/SKILL.md new file mode 100644 index 0000000..326e116 --- /dev/null +++ b/assets/skills/worker-review/SKILL.md @@ -0,0 +1,55 @@ +--- +description: Review the background-work surface contract of a change - queue/stream consumers, scheduled jobs, and cron handlers. Checks retry/backoff policy, DLQ/poison-message routing, graceful-shutdown drain, concurrency/prefetch bounds, and visibility-timeout vs processing-time reasoning. Load when a diff touches queue/job/cron consumer registration, handler wiring, or schedule definitions. Findings fold into the standard code-review severity taxonomy. Grants read-only filesystem access for tracing consumer registration, transport config, and shutdown paths. +enabled_tools: fs_read, fs_grep, fs_glob, fs_cat, fs_ls +--- +You are reviewing background-work wiring. The generic correctness checklist asks "does this handler process a message?"; you ask **"what happens to this consumer on the bad days — a poison message, a deploy mid-batch, a downstream outage?"** A worker's contract is with the queue, the scheduler, and the deploy pipeline, not with a single happy-path message. Most worker incidents are not broken handler logic — they are a queue wedged behind one malformed message, in-flight work silently dropped by a rolling restart, or a retry storm hammering a struggling dependency. + +## When to load this skill + +The diff touches ANY of: queue/stream consumer registration, job/worker handler wiring, cron or schedule definitions, or the transport configuration behind them (retry counts, prefetch, visibility timeouts, shutdown hooks). If the diff is handler business logic behind unchanged wiring — unload; this checklist has nothing for you. Note: a diff that wires a consumer AND changes state is two reviews — this skill covers the consumer contract, and `transactional-integrity` covers the state changes; state-changing worker diffs load BOTH. + +## Marker semantics + +Every checklist item below carries a severity emoji AND a `[convention]` or `[correctness]` marker; both ride in the finding title so downstream tooling can act on them mechanically. `[convention]` findings are rigor-foldable (the orchestrator may lower them under a relaxed quality bar) and rejectable — but ONLY with cited evidence: a repo convention at file:line, or a recorded plan decision. `[correctness]` is reserved for contract breaks; those findings are neither foldable nor rejectable. + +## Linters and mechanized checks + +The review orchestrator runs mechanized checks (config validators, schema checks for queue/schedule definitions); your CONTEXT may already include their output — do not re-derive it. Spend your prose on what linters cannot reach: whether a retry policy exists at all, where a poison message goes, what a deploy does to in-flight work. If the repo plausibly warrants a linter config it lacks, emit a 🟢 `[convention]` finding naming the gap. + +## The checklist + +Severities below are the production bar. Each item is a context-sensitive question, not an absolute — read the transport's own guarantees and the deployment story before flagging, and when you rely on an exemption, state it so the next reviewer sees it was considered. + +### 1. 🟡 `[convention]` No retry/backoff policy where the transport provides none + +For each new or rewired consumer: when the handler fails, who retries, how many times, with what backoff? Some transports provide redelivery with backoff out of the box — READ the transport config before claiming they do here. If neither the transport nor the code establishes a policy, a transient downstream blip becomes permanent message loss (or an immediate hot-loop of retries). Name the consumer, the transport, and what a single failure currently does. A consumer whose transport is configured with sane redelivery is exempt — cite the config at file:line. + +### 2. 🟡 `[convention]` No DLQ/poison-message route — one bad message wedges the queue + +A message that fails every retry must go SOMEWHERE terminal: a dead-letter queue, a parked table, a quarantine topic. Trace the exhausted-retries path for each consumer the diff adds: if the message returns to the head of the queue forever, one malformed payload halts all processing behind it. Ordered/single-partition consumers are the highest-blast-radius case. Where the message *lands* is your question; whether the handler's error path also acknowledges correctly under at-least-once delivery is `transactional-integrity`'s. + +### 3. 🟡 `[convention]` No graceful-shutdown drain — in-flight work lost on deploy + +Every deploy sends this worker a termination signal mid-message. Does the diff's worker stop taking new work, finish (or cleanly nack) what is in flight, and exit within the platform's grace period? Look for a shutdown hook, drain loop, or the framework's built-in drain — and check the termination grace configured for the deployment. Exemptions are real: a stateless cron job reading a read-only source loses nothing on interruption, and an at-least-once transport redelivers whatever was in flight (making drain an efficiency concern, not a loss) — say explicitly which exemption you are relying on. The item bites hardest for at-least-once consumers with long-running in-flight work and for anything ack-early. + +### 4. 🟢 `[convention]` Unbounded concurrency/prefetch + +Does the new consumer bound how many messages it processes at once — worker-pool size, prefetch/fetch-count, max in-flight? An unbounded consumer amplifies a queue backlog into a self-inflicted outage: memory blowup, connection-pool exhaustion, a thundering herd against the downstream the handler calls. Check the transport defaults before flagging — some default to a sane prefetch; some default to unlimited. A low-volume schedule-driven job with structurally bounded input is exempt. + +### 5. 🟢 `[convention]` Visibility-timeout vs processing-time reasoning absent + +Where the transport uses a visibility timeout, lease, or lock (with redelivery when it expires): is there any evidence — a comment, a config value derived from measurements, a heartbeat/extension call — that the timeout exceeds the handler's realistic worst-case processing time? A timeout shorter than processing time means the message redelivers WHILE the first attempt is still running: duplicate concurrent processing by design. You flag the absent reasoning; whether the handler survives that concurrent duplicate is `transactional-integrity`'s question. Transports with no visibility/lease mechanism are exempt. + +## Ground-truth discipline + +- READ the transport/framework configuration, not just the handler — retry counts, DLQ wiring, prefetch, and drain behavior live in config and registration code, not in the handler body. +- `fs_grep` sibling consumers for the established idioms (DLQ naming, shutdown hooks, backoff helpers) — a new consumer skipping the house pattern is the strongest form of evidence; cite the sibling at file:line. +- Check the deployment manifests for termination grace periods when reasoning about drain — the code's drain loop is only as good as the time the platform gives it. +- Do not flag a missing guard the transport demonstrably provides; cite the config that provides it instead. + +## What this skill does NOT check + +- Whether the handler is idempotent under redelivery, atomic across writes, or safe against dual-writes to a DB plus an external system → `transactional-integrity` (state-changing worker diffs load both skills). +- Whether job payloads, queue names, or worker logs expose secrets or abusable data → `security-review` and `logging-discipline`. +- Metrics, alerts, and dashboards for the new worker (queue depth, processing lag, failure rate) → `observability-review`. +- Log lines, levels, and message conventions inside the handler → `logging-discipline`.