feat(skills): add the source-pack skill for AI-driven pack development - #174
Conversation
A shared Claude Code skill that lets an AI session take a provider request (Notion, Jira, …) to a review-ready PR by encoding what the GitHub and Slack packs taught us. Lives under docs/superpowers/skills/ — .claude/ is personal workspace configuration and stays untracked; each developer installs shared skills with a one-line symlink (docs/superpowers/skills/README.md). - SKILL.md: the five-phase workflow — live contract reconciliation FIRST (the wire contract is Open Connector's, not the provider's raw API), table design under the admission gate, implementation, self-review, then PR submission in the house style. - references/contract-reconciliation.md: running the local gateway, the verified /v1 surface (uniform envelope, no /execute suffix, camelCase strict schemas, alias header, no read/write classification), probing actions, reading executor source as the row-shape authority (passthrough vs normalized), the 400-vs-credential-wall input validation trick, and contract capture for fingerprint pinning. - references/implementation.md: pagination/filter/field design rules (total_pages_path, Inexact string-enums, boundary-row protections, ValueFormat), the six fixture categories, the fingerprint pinning recipe (capture -> pin -> sync test -> contract-serving mocks -> drift e2e), the per-declaration e2e test floor, and the three doc targets. - references/review-checklist.md: the distilled review standards from every round both packs went through — silent-truncation checks, contract honesty, structural assertions and row identity, both sides of every gate, information discipline, docs/spec sync, and the final self-review pass required before any PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
30a63b2 to
eacee17
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Review blocked on a missing dependencyThe skill content is strong and I verified its concrete claims against the code ( But the skill is not yet self-adequate on this branch. It names two reference implementations as required reading ("read them before writing anything") and leans on both repeatedly — GitHub (passthrough rows) and Slack (normalized rows). On this branch:
So the skill's entire normalized-row path and its entire fingerprint-pinning recipe (phase 3's capture → pin → sync test → contract-serving mocks → drift-refusal e2e) have no working exemplar on this branch — only Slack demonstrates both. An agent onboarding a cursor-paginated, normalized provider (e.g. Notion) would be sent to a file that isn't there. This branch was cut from the merged GitHub PR (#168), so merge order is the real issue: if this skill reaches Request: land the Slack pack (#172) first, then I'll re-review this skill with all of its referenced artifacts actually present. |
…preconditions A review argued the 400-vs-credential-wall step is invalid because action-runner.ts resolves the connection before executing. The code citation is accurate but the conclusion is not: for the DEFAULT connection, connection-service.ts#resolveForExecution is non-throwing when nothing is configured (credential resolution is lazy), so the runner reaches core/execution.ts#executeAction whose first step is validateActionInput — invalid input fails as invalid_input before any credential exists, valid input proceeds until provider-runtime.ts raises the credential wall. Verified live (v1.3.1, zero connections: per_page -> 400 naming the property; perPage -> 403 authorization failure) and now explained in the doc instead of asserted. The review did expose two real preconditions, now documented: probe the DEFAULT connection only (a missing NAMED connection fails as connection_not_found BEFORE validation, collapsing the distinction), ensure no action policy blocks the action, and calibrate the two distinct responses once per session before trusting either as schema evidence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The source-pack skill instructs pack contract tests to pin expected_fingerprint through action_registry::fingerprint_schema so pin and registration can never disagree on the canonicalization — but the function was module-private, so the documented sync test could not compile from a sibling packs module, and re-deriving the hash elsewhere is exactly what the guidance forbids. Now pub(crate) with a doc note naming pack sync tests as the intended caller, and the skill's implementation reference spells out the import path so the recipe is copy-ready. No behavior change; zero new clippy warnings; 203 open_connector tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The skill cited PaginationCursorInvalid, total_pages_path, ValueFormat, TimestampSecondsUtc, FixedValue::StrList, error_path, and EnvVarGuard as existing invariants — all seven land with milestone 5.2 (PR #172) and none exist on main today, so a pack built from this guide on a pre-5.2 base would inherit the old cursor arm where a non-string cursor and every row-path failure read as end-of-collection (silent truncation). The implementation reference now opens with an Engine baseline section: a one-command git grep to verify the baseline, the rule that a missing invariant is PREREQUISITE work (engine fix + regression tests) rather than an assumption, and the general principle that every named safety invariant is a claim about code to verify on the actual branch. SKILL.md phase 3 and the checklist's termination item point at it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Non-timestamp mappings declare Verbatim (which also keeps a timestamp literal local rather than pushing a guessed spelling); Rfc3339 and EpochSeconds are for genuine timestamp inputs only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # crates/skardi/src/sources/providers/open_connector/action_registry.rs
The skill predated three structural changes that alter how a pack is actually authored; the workflow now matches the shipped reality: - Packs are embedded YAML assets: the implementation checklist authors packs/<provider>.yaml (bare table keys, derived ids, per-table pagination/columns/filters blocks, rationale as YAML comments) with a small OnceLock accessor module and registry entry — and enumerates what the validating loader already rejects (unknown keys, duplicate columns/mappings, undeclared filter columns, input-namespace collisions, zero page sizes, non-finite floats) so authoring attention goes to the semantic choices the loader cannot check. - Fingerprint pinning gained the coverage-gap pin: the recipe now ends with fingerprint_uncovered_columns asserting each table's exact uncovered set, and the review checklist carries the item. - Pagination soundness is a first-class phase-1 check: executors that filter rows AFTER paginating destroy the termination signal — the skill now teaches raw_page_size_path (with the upstream-contribution precedent, oomol-lab/open-connector#228) and the rule that a missing signal means upstream fix or deferral, never a heuristic. The engine-baseline section is reframed (main carries the full baseline today; verify-by-grep stays, with a refreshed marker list), and the branch absorbed origin/main (one trivial doc-comment conflict on fingerprint_schema, resolved keeping the richer rationale). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pack The Notion pack (PR #177) proved that contract reconciliation alone is insufficient: it passed every contract-level check and still mapped columns that were always-NULL on the real wire, because declared schemas under-declare, misname (archived vs is_archived), or anyOf-hide the fields passthrough executors emit — and passthrough columns raise no error at registration or scan time. - New Phase 4 (references/live-verification.md): user-configured real credentials (never handled by the agent), per-action live probes, both-direction column-vs-wire diffs, end-to-end scans of every table through skardi-server, fixture re-derivation as redacted live captures with a mechanical redaction audit, provider API version recording, and evidence requirements for the PR. - Self-review and PR become phases 5/6; checklist gains real-data items (always-NULL detection, pins-return-rows, live multi-page pagination, live-capture fixtures, credential-rotation reminder). - contract-reconciliation.md now states the captured contract is fingerprint input, not column truth. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Adds a shared Claude Code skill —
docs/superpowers/skills/source-pack/— that lets an AI session take "add as a data source" all the way to a review-ready PR, by encoding the methodology the GitHub (#168) and Slack (#172) packs were built and reviewed with..claude/is personal workspace configuration and stays untracked. Shared skills live in the repo underdocs/superpowers/skills/; each developer installs one with a one-line symlink (seedocs/superpowers/skills/README.md):After that the skill auto-triggers on "onboard/integrate " requests, or explicitly via
/source-pack.What the skill encodes
SKILL.md— the six-phase workflowper_page,issue_number, a nonexistent action ID, the wrong execute endpoint, and the wrong response envelope — all plausible from GitHub's docs, all wrong live, all invisible to CI because the mocks encoded the same assumptions.archivedvsis_archived), oranyOf-hide the fields passthrough executors emit. No pack is submission-ready until every table has scanned real rows end to end.references/contract-reconciliation.md— running the local gateway; the verified/v1surface (uniform{success, message, data, meta}envelope,POST /v1/actions/:idwith no/executesuffix, camelCase strict schemas,x-oo-connector-alias, no read/write classification); enumerating and probing actions; reading the OC provider executor source as the row-shape authority (GitHub-style passthrough vs Slack-style normalization); the 400-vs-credential-wall trick that validates every generated input against the strict schema without provider credentials; capturing output schemas for fingerprint pinning.references/implementation.md— the design rules that survived review (authoritativetotal_pages_pathover the short-page heuristic, every string-enum push isInexact, boundary-row protections,ValueFormat, fixed-input pins, conservative nullability,error_pathonly for forwarding gateways); the six fixture categories including schema-mismatch; the fingerprint recipe (capture → pin → sync test → contract-serving mocks → drift-refusal e2e); the per-declaration e2e floor (tables sharing a strategy constant still get their own wire pins); the three documentation targets.references/live-verification.md— the phase-4 recipe distilled from #177's review cycle: real credentials configured by the USER in the gateway (PUT /api/connections/<service>; the agent never touches the secret, and a pasted secret triggers a rotation reminder); per-action live probes with the pack's exact inputs (a pinned enum that returns zero rows looks healthy — Notion's version-coupleddata_sourcevsdatabase); both-direction diffs of real row keys vs mapped columns; end-to-end scans of every table through skardi-server (fingerprint gate against LIVE discovery, every mapped column non-NULL somewhere, real multi-page pagination); fixture re-derivation as redacted live captures with a mechanical redaction audit; recording the gateway's pinned provider API version.references/review-checklist.md— the distilled record of every review round both packs went through, organized by failure class: silent-truncation/termination correctness, contract honesty, test quality (structural JSON assertions, row identity over cardinality, both sides of every gate, negative-space guards), information discipline (kinds never values, bounded snippets), docs/spec sync (counts with the counting command), and code hygiene. Each item traces to a real defect caught in this repo.Verification
.gitignoretouched — nothing under.claude/is tracked..claude/skills/source-pack→docs/superpowers/skills/source-pack.Dependencies
The skill references the milestone-5.2 engine baseline (
PaginationCursorInvalid,total_pages_path,ValueFormat,TimestampSecondsUtc,FixedValue::StrList,error_path,testutil::EnvVarGuard) — none of which exist onmainuntil #172 merges. The implementation reference now opens with an Engine baseline section (verify-by-git grep, and the rule that a missing invariant is prerequisite engine work, not an assumption), so the skill is safe to merge in either order — but merging #172 first is the intended sequence, and the first real exercise (5.3 via/source-pack notion) should start only after both land.🤖 Generated with Claude Code