slbotbm commented on code in PR #4121: URL: https://github.com/apache/iggy/pull/4121#discussion_r4007453472
########## .claude/skills/connector-review/SKILL.md: ########## @@ -0,0 +1,151 @@ +--- +name: connector-review +description: Adversarial 4-expert review of a connectors PR, branch, or ref range (sinks, sources, runtime, SDK, transforms) with clean-room validation of every finding. Experts work alone, no peer debate. Expensive, one run spawns ~10 subagents. Load connectors-overview first. +argument-hint: "[PR number | branch | ref range]" +disable-model-invocation: true +--- + +# Apache Iggy Connector Review + +`<TARGET>` = `$ARGUMENTS`: a PR number, a branch, or a ref range. Empty means `origin/master..HEAD`. Scope: anything under `core/connectors/` plus connector integration tests under `core/integration/tests/connectors/`. + +Prereq: load `connectors-overview` first as router (sibling skill in this directory; repo-wide rules in `<repo>/AGENTS.md`). This file owns **review mechanics only**. + +You = **moderator**. You never open the diff or a source file: you route paths, merge claims, synthesize. Every token you load rides along every later turn. Reviewers and validators are one-shot agents that deliver by writing a file; nobody chats. + +## Charter (paste VERBATIM into every expert, validator, and tiebreak prompt) + +> You think big brain. You speak caveman. Separate things. +> +> **Thinking, unchanged.** Read the diff, then every changed file in full from the local checkout, then whatever call sites you need. Trace call chains. Verify invariants. Prove findings, don't guess. Cite `path::Symbol`, plus `file:line` as secondary anchor. Running tests or builds needs a stated justification: reading and tracing settles most claims, and parallel cargo runs block on one target-dir lock. +> +> **Output style.** Drop articles, filler, pleasantries, hedging. Fragments OK. Keep EXACT: symbol, `file:line`, error quotes, code, technical terms, severity and confidence labels. +> +> - Finding, one line each: `[sev] path::Symbol (file:line) - problem. Fix: action. (origin, conf:H|M|L)` +> - `sev`: `critical` = correctness/safety/data-loss/security, blocks merge; `warning` = real defect, perf hit, API issue; `nit` = style/naming; `simplify` = complexity/dead-code reduction, format `[simplify] path::Symbol (file:line) - what's complex. Simpler: alternative. Saves: ~N lines / removes indirection. (origin, conf)`. +> - `origin`: `intro` (PR introduced), `pre-surfaced` (existed, exposed by PR), `pre-untouched` (existed, not touched). +> - Never flag em dashes or other punctuation style as a finding. +> - Simplification mandate: less code > more code. Per changed file ask whether ~30% smaller keeps correctness: dead fields/params/branches/imports, duplication of an existing helper (cite it), single-impl traits, premature generics, checks for impossible states. Do not propose simplifications that change semantics or break public API. If nothing qualifies, write `Simplifications: none`. +> +> **Connector mandatory checks (every expert, where in scope).** Verify, don't assume: +> `SecretString` on credential fields, no `Serialize` on plugin config (or redacted serializer with justification); FFI return codes honored (`0` ok, `-1` invalid, `1` open failure; duplicate-ID guard intact); sink redelivery claim vs `AutoCommit::When(PollingMessages)` + FFI swallow path; idempotency key on retry (message `id` dedup, `ON CONFLICT`, composite `_id`, deterministic run id + 409 handling). +> `Permanent*` vs transient classification vs retry strategy agreement; drop accounting (`errors` vs `messages_filtered`, flushed once per batch via pre-built labels); every error lands at its level (fatal exits process, per-plugin sets status, per-message skips, per-batch bumps metric or logs). +> Config forward-compat (`Option`/`#[serde(default)]`); `consume`/`poll` take `&self`; no `tokio::spawn`/`block_on` in plugins; no lock held across `.await`; `BTreeMap` headers; `try_to_bytes(&self)` no-clone for JSON; `simd_json::OwnedValue` mutated in place, never clone-then-replace; `Vec::with_capacity`; `[lib] crate-type = ["cdylib", "lib"]`; benchmark/verbose flags wired; closest exemplar named. +> STOP tripwires are critical: `#[repr(C)]` change without SDK bump; `Schema` variant rename; transient↔`Permanent*` promotion; consumer-group rename; plugin path resolution change; trait change without changelog + all-plugins-same-PR + bump + old-`.so` note. +> `state.rs` protocol change is critical: 0o600 mode, tmp `sync_data`, post-rename parent-dir `sync_all`, save/load failures surface (`CannotWriteStateFile`/`CannotOpenStateFile`), never silent fresh-start. Scope creep (unrelated refactor in same PR) gets flagged. +> Logging shape: connector ID + name on every line, literal API field labels, `debug!` default upgraded to `info!` on verbose only, never eager `format!()` around tracing args, never log secrets. Control API returns plugin config verbatim (#3802): never demand plugin-side fixes for control-plane exposure. +> +> Caveman = output compression, not analysis compression. Dig deep. Write short. + +## Step 1: Identify the target (no reading) + +- Classify `<TARGET>`: matches `^#?(pr)?[0-9]+$` case-insensitively -> PR, the digits are `<PR>`. Anything else -> ref range or bare branch. Empty -> review `origin/master..HEAD`. +- `<TOPIC>`: `<TARGET>` lowercased, chars outside `[a-z0-9-]` replaced by `-`, repeats collapsed, trimmed, max 40 chars (`PR3123` -> `pr3123`, `origin/master..HEAD` -> `origin-master-head`). Empty -> `date +%s`. +- `<DIR>` = `<session scratchpad dir from your system prompt>/review-<TOPIC>`. `mkdir -p` it. +- PR: `gh pr view <PR> --json title,body,headRefOid > <DIR>/pr.json`, `gh pr diff <PR> > <DIR>/diff.patch`, `gh pr diff <PR> --name-only > <DIR>/files.txt`. `<SHORTCOMMIT>` = first 8 of `headRefOid`. +- Ref range or bare branch: `git diff $(git merge-base origin/master HEAD)..HEAD > <DIR>/diff.patch`, same with `--name-only`, `<SHORTCOMMIT>` = `git rev-parse --short=8 HEAD`. No `pr.json` on this path. Review Comment: fixed -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
