|
| 1 | +--- |
| 2 | +name: source-pack |
| 3 | +description: >- |
| 4 | + Develop a new Open Connector source pack for Skardi end-to-end: research the |
| 5 | + provider's live gateway contract, implement the pack (tables, fixtures, |
| 6 | + fingerprints, tests, docs), self-review against this repo's accumulated |
| 7 | + review standards, and submit a PR. Use this skill whenever the user asks to |
| 8 | + add, support, integrate, or onboard a data source or SaaS provider (Notion, |
| 9 | + Jira, Gmail, HubSpot, Discord, Feishu, …) as SQL tables, mentions "source |
| 10 | + pack", a "milestone 5.x" task, or wants any provider reachable through Open |
| 11 | + Connector — even if they never say the words "source pack". |
| 12 | +--- |
| 13 | + |
| 14 | +# Developing an Open Connector source pack |
| 15 | + |
| 16 | +You are implementing one milestone of the Open Connector integration: a |
| 17 | +**source pack** — static, Skardi-reviewed relational contracts over a |
| 18 | +provider's read actions, exposed as SQL tables. The GitHub pack (raw |
| 19 | +passthrough rows) and Slack pack (normalized rows) are the two reference |
| 20 | +implementations; read them before writing anything. |
| 21 | + |
| 22 | +The single most important lesson baked into this repo, learned the hard |
| 23 | +way: **the wire contract is Open Connector's, not the provider's raw |
| 24 | +API.** The GitHub pack originally shipped with `per_page`, `issue_number`, |
| 25 | +a nonexistent action ID, the wrong execute endpoint, and the wrong |
| 26 | +response envelope — all plausible from GitHub's own docs, all wrong |
| 27 | +against the real gateway, and all invisible to CI because the mocks |
| 28 | +encoded the same wrong assumptions. Every phase below exists to prevent |
| 29 | +that class of failure. |
| 30 | + |
| 31 | +## Required reading (before phase 1) |
| 32 | + |
| 33 | +1. `docs/superpowers/specs/2026-07-11-open-connector-integration-design.md` |
| 34 | + — especially the **source-pack admission gate**: complete terminating |
| 35 | + pagination, deterministic schema, read-only allowlist, documented |
| 36 | + authz/rate limits, bounded safety defaults, null/empty/nested/ |
| 37 | + schema-mismatch fixtures, docs. The gate is the definition of done. |
| 38 | +2. `docs/superpowers/specs/2026-07-11-open-connector-integration-tasks.md` |
| 39 | + — the milestone map; entries 5.1 (GitHub) and 5.2 (Slack) are the |
| 40 | + template for what your milestone entry must eventually say. |
| 41 | +3. `crates/skardi/src/sources/providers/open_connector/packs/github.rs` |
| 42 | + and `packs/slack.rs` — read the module docs top to bottom; every design |
| 43 | + decision a pack makes is recorded there with its rationale, and yours |
| 44 | + must be too. |
| 45 | +4. `docs/open-connector.md`, `docs/open-connector-github.md`, |
| 46 | + `docs/open-connector-slack.md` — the documentation shape you will add |
| 47 | + to. |
| 48 | + |
| 49 | +## Phase 1 — Reconcile the contract against a live gateway |
| 50 | + |
| 51 | +Do this FIRST, before designing tables. Read |
| 52 | +[references/contract-reconciliation.md](references/contract-reconciliation.md) |
| 53 | +for the concrete steps: starting the local gateway, probing the real API, |
| 54 | +reading the provider's executor source in the Open Connector repo, and |
| 55 | +validating generated inputs without provider credentials. |
| 56 | + |
| 57 | +Non-negotiable outputs of this phase: |
| 58 | + |
| 59 | +- The exact action IDs that exist (never assume a name; `github. |
| 60 | + list_repositories` did not exist). |
| 61 | +- Every input key, verbatim from `inputSchema` (camelCase, and |
| 62 | + `additionalProperties: false` means a wrong key is a hard 400). |
| 63 | +- The row shape: does the executor pass provider rows through raw |
| 64 | + (GitHub-style) or rebuild them normalized (Slack-style)? Only the |
| 65 | + executor source answers this — declared output schemas can be lax |
| 66 | + (`additionalProperties: true`) while the executor passes through fields |
| 67 | + the schema never mentions. |
| 68 | +- How pagination is emitted (top-level `nextCursor`? sibling |
| 69 | + `total_count`? authoritative `paging.pages`?) and how in-band provider |
| 70 | + errors are handled (most OC executors consume them and return a failure |
| 71 | + envelope; `error_path` is only for gateways that forward them). |
| 72 | +- Captured output schemas for fingerprint pinning (phase 3). |
| 73 | + |
| 74 | +## Phase 2 — Design the tables |
| 75 | + |
| 76 | +Read [references/implementation.md](references/implementation.md) §Design |
| 77 | +before deciding anything. Summary of the rules that have survived review: |
| 78 | + |
| 79 | +- Choose read-only list actions with **complete terminating pagination** |
| 80 | + only. An action whose pagination cannot be completed (Slack message |
| 81 | + history, at the time of 5.2) is deferred and documented as absent, not |
| 82 | + shipped incomplete. |
| 83 | +- Every design decision (a pinned input, an unmapped filter, an excluded |
| 84 | + table, a nullability choice) is written into the module doc with its |
| 85 | + why. Reviewers here read module docs as claims to be verified. |
| 86 | +- If the user named specific resources ("I want Notion pages and |
| 87 | + databases"), scope to those; otherwise propose the natural first wave |
| 88 | + (list-shaped, high-value, gate-passing) the way 5.1 chose 8 tables and |
| 89 | + 5.2 chose 3. |
| 90 | + |
| 91 | +## Phase 3 — Implement |
| 92 | + |
| 93 | +Follow [references/implementation.md](references/implementation.md) |
| 94 | +§Implementation for the full checklist: pack file, registry entry, the |
| 95 | +six fixture categories (including schema-mismatch), fingerprint pinning |
| 96 | +(capture → pin → sync test → contract-serving mocks → drift-refusal |
| 97 | +e2e), per-declaration end-to-end tests through `MockGateway`, and the |
| 98 | +three documentation targets (pack doc, spec entry with counted |
| 99 | +verification, `docs/open-connector.md` status). |
| 100 | + |
| 101 | +Engine extensions are allowed when the pack genuinely needs them (5.1 |
| 102 | +added `Fidelity` and list plucking; 5.2 added `total_pages_path`, |
| 103 | +`ValueFormat`, `TimestampSecondsUtc`) — keep them backward-compatible |
| 104 | +(optional fields, `None` defaults) and test them at both the engine and |
| 105 | +the pack level. |
| 106 | + |
| 107 | +## Phase 4 — Self-review before any PR |
| 108 | + |
| 109 | +This phase is why the submitted code is good. Work through |
| 110 | +[references/review-checklist.md](references/review-checklist.md) — it is |
| 111 | +the distillation of every review round the existing packs went through. |
| 112 | +Treat it the way you would treat a human reviewer's findings: verify each |
| 113 | +item against the actual code, fix what fails, and be honest about |
| 114 | +severity. Then: |
| 115 | + |
| 116 | +1. `cargo fmt` and `cargo clippy` clean. |
| 117 | +2. `cargo test -p skardi --lib` — the FULL library suite, not just the |
| 118 | + pack filter (engine changes ripple). |
| 119 | +3. Count tests with the documented methodology |
| 120 | + (`cargo test -p skardi --lib sources::providers::open_connector` and |
| 121 | + the pack-scoped filter) and make every count in docs/spec match. |
| 122 | +4. Run the repo's code review on your own diff (the `review` / |
| 123 | + `/code-review` skill if available) and fix or consciously rebut every |
| 124 | + finding. A finding you disagree with gets a verified technical |
| 125 | + rebuttal, not silence — reviews here have been wrong before (cited |
| 126 | + line numbers stale, counts miscounted), and verifying against the |
| 127 | + code before acting is part of the standard. |
| 128 | + |
| 129 | +## Phase 5 — Submit the PR |
| 130 | + |
| 131 | +- Branch `feature/open-connector-<provider>-pack` off latest `main` |
| 132 | + (fetch first). If the work must stack on an unmerged PR's branch, |
| 133 | + stack — and recommend Draft until the base merges. |
| 134 | +- Commits: conventional style (`feat(sources): …`), detailed bodies that |
| 135 | + explain *why* (look at `git log` for the house voice), ending with the |
| 136 | + repo's standard co-author trailer. |
| 137 | +- Tick the milestone entry in the tasks spec with a verification blurb |
| 138 | + matching 5.1/5.2's density (decisions, live-reconciliation status, |
| 139 | + counted tests with the counting command). |
| 140 | +- PR body modeled on the merged pack PRs (#168-level per-module detail): |
| 141 | + what shipped per module, design decisions with rationale, engine |
| 142 | + extensions, verification section with test counts, live-reconciliation |
| 143 | + status, and any deliberate deferrals (e.g. fingerprint pins pending, |
| 144 | + tables gated on upstream support). |
| 145 | +- `gh pr create` with that body; use Draft when stacked or when the user |
| 146 | + asked for in-progress visibility. |
| 147 | + |
| 148 | +## Working style |
| 149 | + |
| 150 | +- Evaluate before you fix: when review feedback arrives (from the user or |
| 151 | + your own phase-4 pass), first verify the claim against the code — |
| 152 | + some findings are already fixed, stale, or wrong, and saying so with |
| 153 | + evidence is as valuable as a fix. |
| 154 | +- No credentials in Skardi, ever. Provider credentials live in the |
| 155 | + gateway; tests use `EnvVarGuard` with per-test-unique variable names; |
| 156 | + tokens never appear in YAML, logs, `Debug`, or errors. |
| 157 | +- Errors carry identity (action, table, page, row, column) and JSON |
| 158 | + *kinds*, never values; snippets stay bounded. |
| 159 | +- When you and the user have live-gateway access, prefer one real probe |
| 160 | + over an hour of speculation. |
0 commit comments