diff --git a/.claude/skills/connector-review/SKILL.md b/.claude/skills/connector-review/SKILL.md new file mode 100644 index 0000000000..41284567b2 --- /dev/null +++ b/.claude/skills/connector-review/SKILL.md @@ -0,0 +1,246 @@ +--- +name: connector-review +description: | + Adversarial review of a connectors PR, branch, or ref range, with clean-room validation of every finding. Experts + work alone and use the focused connector skills as their source of subsystem rules. +argument-hint: "[PR number | branch | ref range]" +disable-model-invocation: true +--- + +# Apache Iggy Connector Review + +`` = `$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/`. + +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 deep. You write plain. Keep the two apart. +> +> **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, do not guess. Cite `path::Symbol`, plus +> `file:line` as a 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, simple english** When you write technical text (documentation, READMEs, runbooks, procedures, error +> messages, release notes, reports), write plain English in the spirit of ASD-STE100 Simplified Technical English, so +> that a smart reader outside the field understands it on one read. Obey these rules: +> +> CLASSIFY FIRST. Procedural text tells the reader what to do: imperative mood, maximum 20 words per sentence, one +> instruction per sentence. Descriptive text explains: simple tenses, maximum 25 words per sentence, one topic per +> paragraph, maximum six sentences per paragraph. Never mix the two in one passage. +> +> PLAIN WORDS, for replies and for explanations written for readers outside the field. Use the common word when one +> exists ("use", not "utilize"). Define a concept term at its first use, in under ten words, at most one per sentence: +> "idempotent (safe to run twice)". Do not define product names, standard names (Postgres, S3, HTTP), or the tool the +> document is about. Address the reader as "you". Lead with the point. Procedures and reference documents follow the +> rules above alone. +> +> VERBS. Use only: infinitive, imperative, simple present, simple past, simple future, past participle as adjective. No +> present perfect ("has completed" -> "completed"). No "-ing" verb forms ("making it easy" -> new sentence). Active +> voice; passive only in descriptions when the agent is unknown. Approved modals: can, will, must. Banned: should, +> would, may, might, could. For "should": write "must" if required, delete if optional. +> +> SENTENCES. Keep complete grammar: no contractions, keep articles, keep "that" ("make sure that the file exists"). Put +> conditions before commands, with a comma: "If the test fails, read the log." No semicolons: write two sentences. No +> em-dashes: an em-dash hides the logic between two statements. Name the relation ("because", "but", "for example") +> or write two sentences. Use a vertical list for more than two items or steps. +> +> WORDS. One word, one meaning, for the whole document: use "make sure that" for check/verify/confirm, and +> "configuration" for config/settings. Noun chains of maximum three words. Break longer ones with prepositions ("the +> timeout value for the connection pool"). Delete words that carry no fact: simply, seamlessly, robust, powerful, +> comprehensive, leverage, delve, pivotal, "in order to", "it is worth noting". Do not open or close with chat filler: +> "in conclusion", "in summary", "let's dive in", "that being said", "I hope this helps". +> +> AVOID THE AI DRIFTS. Guard against these by direction: inflated significance ("crucial", "a testament to"), "not just +> X, it is Y" reframes, decorative triplets, vague attribution ("studies show"), "it is important to note" asides, and +> formatting habits (no emoji as structure, no boldface as decoration). State the fact. The fact carries itself. +> Replace: utilize -> use, prior to -> before, in the event that -> if, e.g. -> for example. American spelling. +> +> WARNINGS. Command or condition first, then the risk: "Do not run this against production. The command deletes rows." +> +> NEVER TOUCH. Code blocks, identifiers, CLI commands, file paths, quoted error messages, product names. Each counts as +> one word toward sentence limits. Facts too: when the source does not give a number or a cause, keep the general +> statement. Do not invent specifics. +> +> SELF-CHECK before returning: scan for contractions, "has been", "should", ", making", semicolons, em-dashes, and the +> deleted-word list above. Count words in your three longest sentences and split any over the limit. Collapse synonym +> rotation. +> +> REPLIES TO THE USER. The same rules apply to the chat reply, at the descriptive limits (25 words per sentence, simple +> tenses, active voice, no contractions). Start with the answer or the result. If a concept term is necessary, define it +> in a few words. Do not restate the request. Keep the whole reply to 5 sentences or fewer, code and lists excluded. Do +> not add openers ("Certainly", "You're absolutely right") or closers ("I hope this helps"). Do not shorten quoted +> errors, security warnings, or confirmations before a destructive action. +> +> Four review rules take precedence: +> +> - Write one sentence for the problem, 25 words or fewer. Write one sentence for the fix, imperative, 20 words or +> fewer. +> - Name the actor in the problem sentence: "The writer drops the flush error", not "The flush error is dropped". +> - NEVER TOUCH covers these too: `path::Symbol`, `file:line`, code, identifiers, file paths, quoted error messages, +> technical terms, severity labels, confidence labels. Keep them EXACT. +> - These writing rules govern your own output only. Never review the code, the comments, or the commit messages against +> them. +> +> **Finding format.** +> +> - 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`. +> +> Deep analysis, plain words. Dig deep. Write short and clear. + +## Step 1: Identify the target (no reading) + +- Classify ``: matches `^#?(pr)?[0-9]+$` case-insensitively -> PR, the digits are ``. Anything else -> ref + range or bare branch. Empty -> review `origin/master...HEAD`. +- ``: `` 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`. +- `` = `/review-`. `mkdir -p` it. +- PR: `gh pr view --json title,body,headRefOid > /pr.json`, `gh pr diff > /diff.patch`, + `gh pr diff --name-only > /files.txt`. `` = first 8 of `headRefOid`. +- Empty target: `git diff origin/master...HEAD > /diff.patch`, same with `--name-only`, and `` = + `git rev-parse --short=8 HEAD`. +- Ref range (`A..B` / `A...B`): `git diff > /diff.patch`, same with `--name-only`, and `` = + `git rev-parse --short=8 `. +- Bare branch ``: `git diff $(git merge-base origin/master ).. > /diff.patch`, same with + `--name-only`, and `` = `git rev-parse --short=8 `. Do not create `pr.json` for these three cases. +- Guard: `git rev-parse HEAD` must equal the reviewed head (`headRefOid` for a PR, `HEAD` for an empty target, + `` for a range, or `` for a branch). Experts read the local checkout. If it differs, stop and ask + the user to check out the reviewed head. +- ``: 1-3 word `snake_case` summary, `[a-z0-9_]`, <= 24 chars. From the PR title; no PR -> from + `git log -1 --format=%s`. +- Report path: `/report.md`. + +Do not `cat` any of the files you just wrote. `wc -l /diff.patch` is the only look you take. + +## Step 2: Round 1, connector experts (one message, parallel) + +Read `/files.txt` only to select roles. For a plugin-only diff with no path under +`core/connectors/runtime/src/` or `core/connectors/sdk/src/`, spawn `plugin`, `contracts`, and `testing`. Otherwise +spawn `plugin`, `runtime`, `sdk`, and `testing`. + +Spawn all selected `Agent` calls in one message: `subagent_type: general-purpose`, `name: -` (bare role +names collide with concurrent sessions: one shared agent namespace), no `model` (inherits). Prompt = role block + +Charter + this brief, with ``, ``, `` filled in: + +> Before you inspect the diff, load `connectors-overview` and every focused connector skill named in your role block +> with the Skill tool. Treat those skills as the source of connector rules. Do not substitute remembered rules. +> Target: `` at ``. Diff: `/diff.patch`. Changed files: `/files.txt`. PR title and body: +> `/pr.json` (drop this sentence when there is no PR). Classify each finding's origin. Check existing codebase +> conventions before you call a deviation `intro`. Name the closest exemplar plugin that you compared when plugin code +> is in scope. +> Deliverable = the file `/.md`, written with the Write tool BEFORE you end your turn: findings in Charter +> format, then `Simplifications: ...`, then `Verdict: APPROVE | REQUEST CHANGES - reason`. A previous worker finished +> reading and then idled without delivering. The Write call IS the delivery. Your final message is only the path. Budget +> 3/4 reading and 1/4 writing. Partial work beats no file. +> You work alone: no teammates, no SendMessage, no questions back. + +Role blocks: + +- **plugin**: Senior connector-plugin engineer. Load `connector-sink`, `connector-source`, and `connector-transform` for + the plugin types present in the changed-file list. Own sink, source, and transform implementations. Focus on lifecycle, + delivery semantics, state, retry behavior, payload handling, configuration, logging, and the closest exemplar. +- **runtime**: Connectors runtime and FFI host engineer. Load `connector-runtime`. Own `runtime/src/`. Focus on lifecycle, + FFI boundaries, task and library ownership, source and sink loops, state storage, configuration, metrics, and errors. +- **sdk**: SDK contract guardian. Load `connector-sdk`. Own `sdk/src/`. Focus on public traits, FFI layouts and macros, + schema and payload conversions, state and retry contracts, serialization boundaries, and compatibility. +- **contracts**: Plugin boundary reviewer for a plugin-only diff. Load `connector-runtime` and `connector-sdk`. Focus on + how the changed plugins use existing runtime and SDK contracts. Do not review untouched runtime or SDK code as owned + code. +- **testing**: Connector test and documentation lead. Load `connector-testing` plus each focused implementation skill + that matches the changed files. Own connector tests, fixtures, examples, configuration files, and documentation. + Focus on coverage of reachable branches, consistency within each file, real-infrastructure boundaries, and sync with + each plugin's root `config.toml`. + +Collect: wait for the completion notifications, then `ls /*.md`. A role with no file gets one `SendMessage` nudge +to `-` ("Write `/.md` now, then stop."). If the file is still missing, respawn the role once with +the same prompt. Never open a subagent transcript via `TaskOutput` because it is the whole JSONL. + +## Step 3: Merge into neutral claims (moderator) + +Read every selected role file. Write `/claims.md`, one line per claim: +`C [sev] path::Symbol (file:line) - claim. Fix: action. (origin)`. Strip role names, confidence, and argument. The +same symbol and defect from several roles form one claim at the highest severity. Keep a private raised-by map for the +report. Simplification items are claims too. + +If no claims exist, skip Steps 4 and 5. Go to Step 6 with empty sections and `Verdict: APPROVE`. Write the report file. + +## Step 4: Clean-room validation (one message, parallel) + +Shard claims about five per validator. Spawn one `Agent` per shard plus one sweep validator, all in one message: +`subagent_type: general-purpose`, `model: opus`, `name: validator--` / `sweep-`. Each gets ONLY its +claims verbatim, `/files.txt`, `/diff.patch`, the target identity, and the Charter. Do not provide the role +files, raised-by map, or your reasoning. The missing context removes the anchoring bias. + +Before validators inspect the diff, they load `connectors-overview` and the focused connector skills relevant to their +claims with the Skill tool. The sweep validator loads every focused connector skill relevant to the changed-file list. + +Validator mandate (adversarial): for each claim, open the cited symbol and trace call sites. Then rate it +`C: PASS | FIX: | REMOVE: `. Judge the +severity and recheck the anchor. Deliver `/validate-.md` via Write, with the same idle rule as Step 2. + +Sweep mandate: inspect all claims and the diff. Answer only which real defects the list misses and which listed items +wrongly clear a bug. Deliver `/sweep.md`. Write additions in Charter format with `(sweep)`. + +Apply: drop REMOVE, apply FIX (wording, symbol, line, severity), and fold sweep additions in as `(sweep, unvalidated)`. +A `critical` sweep addition gets one extra validator before it can block the verdict. + +## Step 5: Contested items (only when triggered) + +Contested = a validator REMOVEs or downgrades a `critical` or `warning`, or a sweep addition contradicts a PASS. For +each item, spawn one `Agent` (`model: opus`) with the claim, the validator's verdict text, the expert's original line, +and the paths. Instruct it to load `connectors-overview` and the focused connector skill relevant to the claim before +it reads the code. It writes `UPHELD | OVERTURNED - reason (cite symbol)` to `/contested-.md`. Cap this at five +per run. Past the cap, adjudicate and mark `(moderator call)`. + +## Step 6: Synthesize, write, done + +Output in the simple English of the Charter. The Charter binds you too: + +```text +## Review: [change description] + +### Confirmed (expert + clean-room validator) +- [sev] path::Symbol (file:line) - problem. Fix: action. (raised: role[, role]; validated: PASS|FIX) + +### Contested +- path::Symbol (file:line) - problem. + Expert: position. Validator: counter. **Tiebreak**: UPHELD|OVERTURNED - why. + +### Retracted (validator REMOVE) +- finding - why. + +### Pre-existing (origin pre-*, not blocking) +- path::Symbol (file:line) - follows pattern in [ref]. + +### Simplification opportunities (non-blocking) +- path::Symbol (file:line) - current shape. Simpler: alternative. Saves: ~N lines / removes indirection. + +### Verdict: APPROVE | REQUEST CHANGES +Confirmed critical + warning only. Simplifications informational. Reason: one line. + +Counts: critical N, warning N, nit N, simplify N (Confirmed + Simplification sections) +``` + +Then write `/report.md` with: + +1. H1 `# Connector Review - ()`. +2. Metadata, one line each: target ``, reviewed commit, ISO timestamp, roles, validator count, contested count. +3. The report above, verbatim. +4. Appendix `## Raw findings per expert`: each role file verbatim in a fenced block. +5. `## Validation record`: counts of PASS / FIX / REMOVE, sweep additions, contested outcomes. + +Last user-facing line: `Findings written: /report.md`. Do not clean up. One-shot agents end themselves, and +`` stays in the scratchpad. diff --git a/AGENTS.md b/AGENTS.md index b2da04c8bf..7d68104aaf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -147,6 +147,7 @@ Repo-wide, user-invoked only. `disable-model-invocation: true` keeps it out of the agent's context; do not replicate its steps. When a non-trivial change passes verification, suggest `/team-review `. +- [connector-review](.claude/skills/connector-review/SKILL.md) - adversarial connectors PR/branch review with validation - [team-review](.claude/skills/team-review/SKILL.md) - adversarial 4-expert PR/branch review, ~10 subagents per run ## Repo-wide principles