basecamp / basecamp/basecamp-sdk
check-wrapper-drift: a 4th tier for outgoing request wrappers must check threading, not declaration
- Dominant language
- Go
- Stars
- 49
- Forks
- 12
- Avg merge
- 20h 47m
- Merged PRs (30d)
- 89
Description
## What this issue is now
A fourth tier of `scripts/check-wrapper-drift` for **outgoing request wrappers**, whose deliverable is a **threading** check: for each field on the hand-written `CreateXRequest`, does a value the caller sets actually reach the generated body construction?
This is a rewrite of the original scope, which asked for tag-presence in the wrapper — a **declaration** check. That framing was disproven by evidence carried on this issue itself (below), so anyone picking this up from the old body would have built a gate that is green against the exact regression that motivated it. Nothing about the issue is stale; it was self-contradictory.
**Do not start this without reading "Why declaration is the wrong check" first.** Threading is dataflow, materially harder than tiers 1-3, and the size of the real job is the main reason this has not been done.
## Why the tier is needed
The three existing tiers cover the response direction — `*FromGenerated` converters (tier 1), direct-decode wrappers (tier 2), inline-converted composite literals (tier 3). Outgoing request wrappers are excluded by design, at `scripts/check-wrapper-drift/main.go:437-444` (as of `270ea44d0`):
```
// - Outgoing request wrappers whose name happens to match a
// `generated.CreateXRequest` / etc. (e.g. CreatePersonRequest,
// ScheduleAttributes): data flows wrapper→generated, not generated→
// wrapper. The tag-presence check still works in principle, but the
// semantics (caller-driven vs server-driven payloads) and the failure mode
// (caller cannot supply a new field vs wire data silently dropped) differ
// enough to warrant a separate tier with its own documentation, deferred
// to a follow-up.
```
The failure mode is direction-specific, which is why it wants its own tier and not a widened existing one:
- **Covered tiers (response):** a new generated field means wire data is silently dropped from the wrapper.
- **This tier (request):** a new generated request field means **a caller cannot supply it at all**. The spec grows a capability, the hand-written Go surface never exposes it, nothing fails.
It is a class, not a straggler. Go is the only SDK whose public create-request structs are hand-written; in TypeScript, Ruby, Python, Kotlin and Swift `createEntry` *is* the generated method, so a spec change reaches them for free.
## Why declaration is the wrong check
Migrated from #654 and confirmed against the current tree. #641 added three members to `CreateScheduleEntry`; five SDKs picked them up and Go's public `CreateScheduleEntryRequest` did not.
The red proof is the operative constraint here: with **all three fields still declared on the struct** and only the three body-construction lines removed, the request went out carrying just `summary` / `starts_at` / `ends_at`, and the test failed with `expected url=… got `.
> Any gate that checks declaration alone would have passed that.
A field present on the struct but absent from the body builder is a **silent no-op** — worse than a missing field, because the API looks correct at the call site. So:
1. **Declaration** — every field on `generated.CreateXRequestContent` has a counterpart on the hand-written `CreateXRequest`. Necessary, cheap, and **not sufficient**.
2. **Threading** — each declared field reaches the generated body construction. This is the check that would have caught #648, and it is the deliverable.
The #648 regression test is `TestSchedulesService_CreateEntryJoinLinkHighlightAndStatus`, at `go/pkg/basecamp/schedules_test.go:1817` (as of `270ea44d0`), and it asserts the observed request body rather than the marshalled struct — which is the shape a threading gate has to generalise. It is the best available fixture for testing the gate. (Earlier revisions of this issue cited `schedules_test.go:1683-1729`; that range is unrelated #779 error-classification code today.)
## Scope
- Enumerate the wrapper→generated request pairs following the derivation recipe at `scripts/check-wrapper-drift/main.go:403-424`. Two known examples, both current as of `270ea44d0`: `CreatePersonRequest` (`go/pkg/basecamp/people.go:23` / `go/pkg/generated/client.gen.go:776`) and `ScheduleAttributes` (`go/pkg/basecamp/projects.go:111` / `client.gen.go:2802`).
- Add them as a fourth tier whose primary assertion is **threading**, with declaration as the cheap precondition. Tier 3's population walk is the nearest existing machinery — it already classifies assignment shapes and refuses what it cannot recognise — but it runs generated→wrapper and this tier runs the other way, so what counts as "populated" has to be re-derived, not reused verbatim.
- Whatever the walk cannot classify must be **reported**, never credited. That invariant is the one thing #721 established across twenty rounds and it applies here unchanged.
- Document the tier in the file header alongside tiers 1-3 and remove the exclusion bullet.
## Sizing, honestly
Threading is dataflow over hand-written service methods, and the shapes that defeat an AST-only walk are inventoried in #741 for the sibling analysis. Expect the same families — assignment-shape classification and identity — to show up here. Before building it, decide whether the answer is a gate at all: a conformance-style assertion on the observed request body (what `schedules_test.go:1817` does) proves threading for the call sites it executes, while a static tier holds a syntactic invariant across call sites nobody has written yet. Neither subsumes the other, and the cheaper one may be enough.
## References
- #654 — closed as a duplicate; its evidence lives in the comment on this issue
- #648 / #641 — the concrete miss and the spec change that caused it
- #721 / #741 — the promotion-walk precedent and its known-wrong-shape inventory
Contributor guide
Research direction
Read the exclusion and derivation sections in scripts/check-wrapper-drift/main.go:403-444, then inspect Tier 3's population walk. Use go/pkg/basecamp/schedules_test.go:1817 and the CreatePersonRequest and ScheduleAttributes pairs as fixtures; done means the fourth tier checks wrapper-to-generated threading, reports unclassified shapes, and documents the tier.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- testing, tooling
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100