Self Review Connector

Self-Review the Connector

Goal

Mechanically audit the {{source_name}} connector and emit a markdown report at tests/unit/sources/{{source_name}}/SELF_REVIEW.md containing:

• Overall score (0–100) and verdict (READY / ALMOST / NEEDS WORK / NOT READY).
• Top 3–5 prioritized recommendations.
• Per-section findings with path:line citations so the developer can navigate to each spot in seconds.

The skill does not modify connector code or run agents. Output is the input to a fix loop, not part of one.

When to use

After connector-dev → connector-tester → connector-doc-writer → connector-spec-generator. Run before declaring the connector ready to ship, or any time the developer wants a checkpoint.

Inputs

• source_name (required).
• Optional: report path. Default tests/unit/sources/{{source_name}}/SELF_REVIEW.md. The default path is gitignored — same convention as dev_table_config.json.
• PR auto-detection. When run on a branch that has an open PR, the skill posts the report as a sticky comment and adds the connector-self-reviewed label on pass. The repo's CI workflow (.github/workflows/connector-self-review.yml) requires that label to merge — see Posting to GitHub below.

Severity & scoring

Every check resolves to pass / warn / fail. Apply weights:

Sum points; normalize to 0–100 against the max (all checks pass). Any BLOCKER fail caps the verdict at NOT READY regardless of point total — abstract methods missing or tests failing means the connector cannot ship even if everything else is perfect.

Bands:

• 90+ — READY: minor polish only.
• 75–89 — ALMOST: 1–2 MAJOR findings to address.
• 50–74 — NEEDS WORK: meaningful gaps.
• <50 or any BLOCKER fail — NOT READY.

Procedure

For each section below, run the listed checks in order. For each check, record one line in the report:

✅ A1. <one-line description> — `<file>:<line>`
⚠️ A7. 3 of 11 HTTP calls lack `timeout=` — `source.py:142, 178, 256`
❌ E2. Connection-parameter naming mismatch (see Top recommendations)

Cite a path on every finding except where genuinely none exists (e.g. "no record-mode run on file"). Citations are what make the report actionable.

Section A — Connector implementation (13 checks)

Source under audit: src/databricks/labs/community_connector/sources/{{source_name}}/{{source_name}}.py

Running pylint (A11)

Run pylint against the connector source with the exact flag set used by CI and the precommit hook (keep them in sync — if CI changes, update here too):

files=$(git ls-files \
  "src/databricks/labs/community_connector/sources/{{source_name}}/*.py" \
  | grep -v '_generated_')

.venv/bin/pylint \
  --max-line-length=100 \
  --max-locals=25 \
  --max-branches=20 \
  --max-module-lines=1500 \
  --disable=W,C0114,C0115,C0116,C0415,R0801,R0902,R0903,R0911,R0913,R0917,R1705 \
  --ignore-long-lines='^\s*(#|f?".*"|f?'"'"'.*'"'"')$' \
  $files

Fall back to pylint on PATH if .venv/bin/pylint is absent. If pylint is not installed at all, mark A11 = warn with the install hint (pip install -e ".[dev]") — do not fail the audit on a missing tool.

Pylint emits findings as path:line:col: message. Each non-zero exit line is one finding; cite the first 5 verbatim in the section, and roll the rest into a count (… and 7 more). Any finding = fail for A11.

Section B — Testing & simulator validation (14 checks)

Test file: tests/unit/sources/{{source_name}}/test_{{source_name}}_lakeflow_connect.py Spec dir: src/databricks/labs/community_connector/source_simulator/specs/{{source_name}}/

Detecting record-mode evidence (B11)

Live/record runs of the simulator emit (next to the test file):

tests/unit/sources/{{source_name}}/cassettes/Test{{Source}}Connector.json
tests/unit/sources/{{source_name}}/cassettes/Test{{Source}}Connector.json.coverage.json
tests/unit/sources/{{source_name}}/cassettes/Test{{Source}}Connector.json.validation.json

Gitignored — they exist only on the developer's machine after they ran record mode. They are the evidence.

For B11:

1. Look for any cassettes/*.json.validation.json. If none → B11 = fail: "No record-mode run on file. Run CONNECTOR_TEST_MODE=record CONNECTOR_TEST_CONFIG_PATH=… pytest …."
2. Parse the most recent validation report. If the report has a top-level summary block with status: clean | drift (the recommended enhancement to validator.py), use that directly. Otherwise fall back to counting per-endpoint drift entries.
3. If drift count > 0 → fail listing drifting endpoints.
4. Compare validation-report mtime against src/.../sources/{{source_name}}/{{source_name}}.py mtime and endpoints.yaml mtime. If validation report is older → warn: "Record run is stale relative to current connector / spec; re-run record mode."

For B12: same shape — read coverage.json, list endpoints in spec that weren't hit (coverage.endpoints_in_spec − coverage.endpoints_hit).

Section C — Artifacts (10 checks)

Section D — Security (10 checks)

Static-analysis pass for obvious smells. Does not replace a real security review for high-risk integrations.

Search scope: src/databricks/labs/community_connector/sources/{{source_name}}/

Section E — Cross-doc consistency (7 checks)

Three artifacts describe the connector at different levels: the API doc (<source>_api_doc.md), the public README (README.md), and the code (<source>.py + connector_spec.yaml). They must agree.

Output report format

Write the report to tests/unit/sources/{{source_name}}/SELF_REVIEW.md with this layout:

# Self-Review — {{source_name}}

**Overall: 87 / 100 — ALMOST**

Run at: 2026-05-06T15:32:00Z

## Top recommendations

1. **MAJOR** — `endpoints.yaml` is missing `synthesize_future_records`
   on `/repos/{owner}/{repo}/commits`. Without it,
   `test_read_terminates` doesn't actually verify the `_init_time`
   cap. *Fix: add `synthesize_future_records: {cursor_field:
   commit.author.date, count: 3}`.*
2. **MAJOR** — `__init__` reads `options.get("api_token")` but
   `connector_spec.yaml` lists the parameter as `token`. Real users
   following the README will fail.
   *Fix: align all three on one name.*
3. **MAJOR** — Last record-mode run was 23 days ago and connector
   source has been edited since. Re-run
   `CONNECTOR_TEST_MODE=record pytest …`.
4. **MINOR** — 3 `requests.get(...)` calls lack `timeout=` —
   `source.py:142, 178, 256`.

## A. Connector implementation — 29 / 33
- ✅ A1. Class extends LakeflowConnect — `source.py:34`
- ✅ A2. All abstract methods implemented
- ✅ A3. `read_table_deletes` implemented (1 cdc_with_deletes table)
- ✅ A4. Read pattern: sliding window (Strategy A) for `commits`,
       snapshot for `branches`
- ✅ A5. `_init_time` cap present — `source.py:67-72`
- ✅ A6. `max_records_per_batch` honored on all incremental tables
- ⚠️ A7. 3 of 11 HTTP calls lack `timeout=` — see Top recommendations
- ✅ A8–A10
- ❌ A11. Pylint: 1 finding — `source.py:142: R0912 too-many-branches
       (21/20)`. Same gate as CI; fix before merge.
- ✅ A12. No ingestion-agent surface implemented (experimental)
- ✅ A13. `GithubDataSource(LakeflowSource)` exposed — `__init__.py:5`

## B. Testing & simulator validation — 22 / 26
- ✅ B1–B7
- ✅ B8. All 8 connector URLs have spec entries
- ✅ B9–B10. 47 passed / 3 skipped in default mode
- ⚠️ B11. Last record run: 2026-04-13. Status: clean (0 drift).
       **Stale.**
- ✅ B12. Coverage 100%
- ❌ B13. `synthesize_future_records` directive missing on commits
       endpoint
- ✅ B14

## C. Artifacts — 19 / 20
- ✅ C1–C8
- ⚠️ C6. `_generated_<source>_python_source.py` is older than
       `<source>.py` — re-run
       `python tools/scripts/merge_python_source.py <source>`

## D. Security — 14 / 15
- ✅ D1–D7
- ⚠️ D8. `logger.debug(f"Sending request: {request.headers}")` at
       `source.py:201`
- ✅ D9–D10

## E. Cross-doc consistency — 5 / 9
- ✅ E1, E3, E4, E6, E7
- ❌ E2. Connection-parameter naming mismatch — see Top recommendations
- ⚠️ E5. `external_options_allowlist` is missing `lookback_days`,
       `window_seconds`

Posting to GitHub

The skill is the merge gate. It is the only thing that runs the audit — there is no CI fallback. The companion workflow (.github/workflows/connector-self-review.yml) just checks for the label this skill applies.

Phase 6 — Identify the PR

1. Detect the current branch:

   git rev-parse --abbrev-ref HEAD
   

2. Find the open PR via GitHub. The remote branch name may have a username prefix (e.g., user/branch-name), so search for PRs whose head branch contains the local branch name:

   gh pr list --repo databrickslabs/lakeflow-community-connectors \
     --state open --json number,url,headRefName \
     --jq '[.[] | select(.headRefName | contains("<branch>"))][0]'
   

3. If no PR is found, write the local report only and stop with: "No open PR found for current branch. Local report at ."

Phase 7 — Post a sticky comment

Post the full report as a single PR comment. Sticky: find an existing comment with the marker <!-- connector-self-review:<source_name> --> at the top and update in place; otherwise create a new comment. No spam on re-runs.

# find existing
EXISTING_ID=$(gh api repos/{owner}/{repo}/issues/{number}/comments --paginate \
  | jq '.[] | select(.body | contains("<!-- connector-self-review:<source_name> -->")) | .id' \
  | head -1)

BODY="<!-- connector-self-review:<source_name> -->
$(cat <report path>)"

if [ -n "$EXISTING_ID" ]; then
  gh api --method PATCH "repos/{owner}/{repo}/issues/comments/$EXISTING_ID" -f body="$BODY"
else
  gh api --method POST "repos/{owner}/{repo}/issues/{number}/comments" -f body="$BODY"
fi

Phase 8 — Label management (the merge gate)

This is the binary signal CI looks at. Two outcomes:

On pass (no BLOCKER fail; verdict ∈ {READY, ALMOST}):

gh pr edit <number> --add-label "connector-self-reviewed"

Then print:

Self-review passed (<verdict>). Label `connector-self-reviewed` added.
The CI gate will see the label and allow merge once other checks
pass. Sticky comment updated: <comment URL>.

On fail (any BLOCKER fail, or verdict ∈ {NEEDS WORK, NOT READY}):

1. Do not add the label.
2. Remove the label if it was previously present (this happens automatically on synchronize events too — see the workflow — but call it out so the developer isn't confused about a stale pass):

   gh pr edit <number> --remove-label "connector-self-reviewed" 2>/dev/null || true
   

3. Print the failure detail and which checks need fixing:

   Self-review FAILED with N BLOCKER fail(s) and M MAJOR finding(s).
   Top fixes:
     1. <first BLOCKER or MAJOR with citation>
     2. ...
   Fix the issues above and re-run /self-review-connector --source <name>.
   

Why a label rather than a status check or inline review

• Required status check would need the audit to run on CI. The skill is Claude-powered; running Claude on every PR push is expensive, slow, and non-deterministic. We want the developer to run it locally, on demand, against their committed work.
• Inline PR review with path:line comments is nice for human-style feedback but doesn't naturally gate merge. Comments aren't a check.
• A label is a binary signal that GitHub's branch-protection rules can require via a workflow that simply checks the label exists. The companion workflow does that — see the design doc for shape. New pushes automatically remove the label, forcing the developer to re-review.

This pattern is borrowed verbatim from plugin-marketplace's /plugin-self-review skill, where the plugin-self-reviewed label plays the same role.

If gh is not authenticated or the user lacks PR write permissions, surface the error verbatim and skip Phases 7–8 — do not attempt to retry or escalate. The local report is still useful.

Rules

• Read-mostly. Do not modify connector code, simulator spec, corpus, or test files. The only write is the report.
• No agents, no network. Every check is local: Read / Bash with grep, python -m py_compile, pytest, JSON / YAML / TOML parsing.
• Cite a path on every finding (file:line) unless the finding is "thing does not exist," in which case state the expected path and the run-command that produces it.
• Be concise. Each finding is one line in the report. The Top Recommendations block is 3-5 items max — not the full list.
• Run pytest synchronously. For B9 / B10, never background; block on completion. If pytest hangs, do not increase timeout — flag the hang as a finding under section A or B as appropriate and continue with the rest of the audit.
• Do not re-run record mode. B11 reads the existing artifact; if the artifact doesn't exist, that's the finding. The skill is not authorized to spend the user's API quota.

Scope boundaries

This skill audits the connector. It does not:

• Run any agent (no spawning).
• Modify code (read-mostly).
• Make network calls (no live API hits, no record mode).
• Replace a real security review for high-risk integrations.
• Gate CI — it's developer-invoked self-check.