Skip to content

Commit a46418a

Browse files
bakeyclaude
andcommitted
docs(skills): rework the source-pack skill for the YAML pack era
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>
1 parent 2e39a0c commit a46418a

4 files changed

Lines changed: 75 additions & 33 deletions

File tree

docs/superpowers/skills/source-pack/SKILL.md

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,9 @@ before deciding anything. Summary of the rules that have survived review:
9191
## Phase 3 — Implement
9292

9393
Follow [references/implementation.md](references/implementation.md)
94-
§Implementation for the full checklist: pack file, registry entry, the
94+
§Implementation for the full checklist: the pack's YAML asset (packs are
95+
declarative embedded YAML validated by `packs/loader.rs` — authoring is
96+
data, not Rust) plus its accessor module and registry entry, the
9597
six fixture categories (including schema-mismatch), fingerprint pinning
9698
(capture → pin → sync test → contract-serving mocks → drift-refusal
9799
e2e), per-declaration end-to-end tests through `MockGateway`, and the
@@ -104,9 +106,9 @@ added `Fidelity` and list plucking; 5.2 added `total_pages_path`,
104106
(optional fields, `None` defaults) and test them at both the engine and
105107
the pack level. Before relying on any engine invariant this skill
106108
names, verify it exists on your base branch — see the implementation
107-
reference's **Engine baseline** section; a base predating milestone 5.2
108-
lacks the cursor/total safety guardrails entirely, and adding them is
109-
then prerequisite work.
109+
reference's **Engine baseline** section; `main` carries the full
110+
baseline today, but an older base may lack part of it, and adding the
111+
missing invariant (with regression tests) is then prerequisite work.
110112

111113
## Phase 4 — Self-review before any PR
112114

docs/superpowers/skills/source-pack/references/contract-reconciliation.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,17 @@ executors. This is the only reliable answer to:
9595
filter pushdown fidelity.
9696
- **Emitted output construction** — where the row array, totals, and
9797
cursors really live.
98+
- **Pagination soundness** — specifically whether the executor filters
99+
rows AFTER paginating (`.filter(...)` on the fetched page). If it does,
100+
the filtered page length carries no termination information and NO
101+
pack-side termination rule is sound (a short page may be mid-collection;
102+
an all-filtered page is empty but non-final). Check whether the
103+
response carries a raw-length or total signal (`pageInfo.fetched`,
104+
`paging.pages`) and declare it; if the gateway destroys the signal
105+
entirely, the fix belongs UPSTREAM — contribute it (precedent:
106+
oomol-lab/open-connector#228 added `pageInfo.fetched` to
107+
`list_repository_issues`) or defer the table per the admission gate.
108+
Never ship a table whose termination is known-unsound.
98109

99110
## 5. Validate generated inputs without provider credentials
100111

docs/superpowers/skills/source-pack/references/implementation.md

Lines changed: 45 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2,29 +2,25 @@
22

33
## Engine baseline — verify before relying
44

5-
This guide references engine capabilities that landed with milestone 5.2
6-
(PR #172); a base branch that predates it lacks ALL of them. Before
7-
designing anything, verify your baseline:
5+
Everything this guide references now ships on `main`: embedded-YAML packs
6+
with the validating loader, fingerprint pinning, `total_pages_path` and
7+
`raw_page_size_path` on `PageNumber`, `PaginationCursorInvalid` /
8+
`PaginationRawPageSizeInvalid`, per-mapping `ValueFormat` (incl.
9+
`Verbatim`), `FieldType::TimestampSecondsUtc`, `FixedValue::StrList`,
10+
`SourcePackTable::error_path`, and `testutil::EnvVarGuard` +
11+
`fingerprint_uncovered_columns`. Still verify rather than assume — every
12+
safety invariant this guide names is a claim about code, and your base
13+
branch may predate one of them:
814

915
```bash
10-
git grep -l "PaginationCursorInvalid\|total_pages_path\|ValueFormat\|TimestampSecondsUtc\|EnvVarGuard" \
16+
git grep -l "raw_page_size_path\|PaginationCursorInvalid\|fingerprint_uncovered_columns" \
1117
crates/skardi/src/sources/providers/open_connector/
1218
```
1319

14-
Zero hits means the base still has the pre-5.2 engine, where — most
15-
dangerously — a non-string cursor and every row-path failure read as
16-
"scan complete": a drifted gateway truncates results silently. In that
17-
case, bringing the engine up to this baseline (with its regression
18-
tests) is a PREREQUISITE step of your milestone, not an assumption to
19-
inherit. More generally: every safety invariant this guide names is a
20-
claim about code — verify it exists (and its failure-mode test passes)
21-
on YOUR branch before leaning on it. Documentation snapshots go stale;
22-
`git grep` does not.
23-
24-
The 5.2 baseline features referenced below: `total_pages_path` on
25-
`PageNumber`, `PaginationCursorInvalid`, per-mapping `ValueFormat`,
26-
`FieldType::TimestampSecondsUtc`, `FixedValue::StrList`,
27-
`SourcePackTable::error_path`, `testutil::EnvVarGuard`.
20+
Zero hits for any of these means your base predates part of the baseline;
21+
bringing the engine up to it (with its regression tests) is a
22+
PREREQUISITE step of your milestone, not an assumption to inherit.
23+
Documentation snapshots go stale; `git grep` does not.
2824

2925
## Design
3026

@@ -56,6 +52,13 @@ Match the strategy to what the executor actually emits (phase 1):
5652
fails as `PaginationCursorInvalid`; a repeated cursor fails as
5753
`PaginationLoop`. Use the page size the provider recommends as its
5854
ceiling.
55+
- `raw_page_size_path` (mutually exclusive with `total_pages_path`): for
56+
gateways that filter rows AFTER paginating and report the raw page
57+
length (e.g. `$.pageInfo.fetched` on `github.list_repository_issues`,
58+
upstream oomol-lab/open-connector#228). The scan continues while the
59+
RAW page was full, no matter how short or empty the filtered rows are —
60+
the filtered count carries no termination information for such actions,
61+
and the heuristic would silently truncate.
5962
- The request page size doubles as the limit-pushdown ceiling — use the
6063
provider's maximum.
6164

@@ -120,10 +123,23 @@ the gateway-failure path.
120123

121124
Work module by module; the reference packs are the style guide.
122125

123-
1. **`packs/<provider>.rs`** — table statics, a
124-
`pub(crate) static <PROVIDER>_PACK: SourcePack`, registry entry in
125-
`source_pack.rs` builtins (short-name uniqueness test will catch
126-
collisions). Module doc records every design decision with rationale.
126+
1. **`packs/<provider>.yaml` + `packs/<provider>.rs`** — packs are
127+
embedded YAML assets. Author the declaration in the YAML (`kind: pack`,
128+
`pack:`, `version:`, `tables:` keyed by bare short names — the id is
129+
derived as `<pack>.<table>`; per-table `action`, `row_path`,
130+
`fingerprint`, `pagination`, `resources`, `fixed_inputs`, `columns`,
131+
`filters`, `error_path`; design rationale as YAML comments). The `.rs`
132+
module is a small accessor (`OnceLock` + `loader::builtin` +
133+
`include_str!`) plus the module doc and the test suite; add the
134+
registry entry in `source_pack.rs` builtins and a `mod` line in
135+
`packs/mod.rs`. The loader validates structure FOR you at parse time —
136+
unknown keys, duplicate columns, filters on undeclared columns,
137+
duplicate mappings or shared filter inputs, resource/fixed-input/
138+
pagination key collisions, zero page sizes, non-finite floats, and
139+
path validity all fail as `SourcePackAssetInvalid` diagnostics — so
140+
your authoring attention goes to the SEMANTIC choices the loader
141+
cannot check: which action, which columns, which fidelity, which
142+
termination signal.
127143
2. **Fixtures** (`packs/fixtures/<provider>/*.json`) — redacted,
128144
provider-shaped pages covering ALL SIX admission-gate categories:
129145
null-bearing, null-parent, empty-list/empty-page, nested,
@@ -146,7 +162,13 @@ Work module by module; the reference packs are the style guide.
146162
`<provider>_discovery(path)` helper), so every e2e registration
147163
exercises the gate's pass side;
148164
- one drift-refusal e2e: a stub serving a different schema must fail
149-
registration with `ActionContractMismatch` naming table and action.
165+
registration with `ActionContractMismatch` naming table and action;
166+
- a coverage-gap pin: `testutil::fingerprint_uncovered_columns` walks
167+
every mapped path through the captured row-item schema, and the pack
168+
test asserts the exact uncovered set per table (columns riding
169+
`additionalProperties` passthrough sit outside the fingerprint gate;
170+
their drift surfaces at scan time). An empty set is the goal;
171+
pinning a non-empty one makes the gap a reviewed fact.
150172
4. **End-to-end tests** via `MockGateway` (`testutil.rs` — use
151173
`envelope_ok` / `envelope_err` / `discovery_ok`; the mocks speak the
152174
real protocol: uniform envelope, `POST /v1/actions/:id`, camelCase).

docs/superpowers/skills/source-pack/references/review-checklist.md

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,16 @@ The worst failure class: wrong results with a green status.
1515
end-of-collection spellings. Anything else that stops a scan —
1616
wrong-typed continuation state, structural traversal failure —
1717
fails loudly (`PaginationCursorInvalid`, `PaginationTotalInvalid`,
18-
propagated row-path errors), never reads as "done". These
19-
guardrails are the 5.2 engine baseline — VERIFY they exist on your
20-
branch (implementation.md §Engine baseline); on a pre-5.2 base,
21-
adding them (with regression tests) is prerequisite work, not a
22-
checkbox to assume.
18+
`PaginationRawPageSizeInvalid`, propagated row-path errors), never
19+
reads as "done". VERIFY these guardrails exist on your branch
20+
(implementation.md §Engine baseline); on an older base, adding them
21+
(with regression tests) is prerequisite work, not a checkbox to
22+
assume.
23+
- [ ] Post-pagination filtering checked: if the executor filters the
24+
fetched page, the table declares `raw_page_size_path` (or an
25+
authoritative total) — a filtered count is never a termination
26+
signal, and a missing signal means upstream contribution or
27+
deferral, not a heuristic.
2328
- [ ] Short/empty non-final pages cannot truncate: if the envelope has
2429
an authoritative total, the strategy declares `total_pages_path`;
2530
if not, the heuristic's limits are documented.
@@ -42,7 +47,9 @@ The worst failure class: wrong results with a green status.
4247
domain; every string-enum push is `Inexact`.
4348
- [ ] Fingerprints pinned from the live capture; sync test locks pin ↔
4449
contract fixture; drift-refusal e2e exists; mocks serve the
45-
captured contracts so the gate's pass side actually runs.
50+
captured contracts so the gate's pass side actually runs; the
51+
fingerprint coverage gap (uncovered mapped columns) is pinned per
52+
table.
4653
- [ ] Deliberate gaps (unmapped filters, deferred tables, `error_path:
4754
None`) each have a module-doc rationale AND a guard test.
4855

0 commit comments

Comments
 (0)