basecamp / basecamp/basecamp-sdk

check-wrapper-drift: a 4th tier for outgoing request wrappers must check threading, not declaration

Open
#531 2 comments 0 reactions 0 assignees View on GitHub
enhancement go
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.