diff --git a/handoff.md b/handoff.md index ac70319..92bc54b 100644 --- a/handoff.md +++ b/handoff.md @@ -1,257 +1,266 @@ # Handoff -Where the work stands, what it actually achieved, and what is left. +## The job, and the only measure of it -Every number here is reproducible — see [Verifying these numbers](#verifying-these-numbers). -Counts of what the toolkit emits and refuses are not repeated here; -`docs/emittance_tracker.md` is the one place they live. +Make the 35 generated ThousandEyes resource acceptance tests pass. A test +passes when, and only when, this says so: + +```sh +sh '/Users/dafyddwatkins/localtesting/thousandeyes/tf_acceptance_tests.sh' +``` + +Nothing else counts. Audit observation counts, correction counts, attribute +counts, `postcheck`, `make check` — all of these have moved a great deal while +the benchmark stayed at zero. Run the benchmark, quote its number, and treat a +change that does not move it as a change that did nothing. + +**Current score: 1 passed, 34 failed.** It was 0/35 at the start of this work. --- -## Read this first +## The test harness, in full -The distinction this document turns on: **coverage** is how much of a document -generates at all. **Correctness** is whether what generates matches how the API -really behaves — what `docs/mapping.md` specifies and what `README.md` calls -"the work in progress". They are not the same thing. +### Where everything lives -**Correctness work has now started, and the offline half is largely done.** -Constraint keywords parse, bounds and patterns become validators, `format: -password` and `writeOnly` become `Sensitive`, `deprecated` becomes a -`DeprecationMessage`, and a documented default fills the response. Every one of -those was zero when this document was first written. +| What | Path | +|---|---| +| Toolkit (this repo) | `/Users/dafyddwatkins/GitHub/terraform/terraform-plugin-framework-codegen` | +| Generated provider under test | `/Users/dafyddwatkins/GitHub/terraform/scratch/gen/thousandeyes` | +| Built CLI | `/Users/dafyddwatkins/GitHub/terraform/scratch/bin/tfpfgen` | +| Test scripts | `/Users/dafyddwatkins/localtesting/thousandeyes/` | -**The half that needs a live API has not started, and cannot yet.** No audit has -ever run against any of the three pilots: all three revised specs contain zero -`x-tfpfgen-*` extensions, and no scratch tree has an `audit/` or -`spec/corrections/` directory. Every shape that depends on an observation — -server defaults, immutability, normalisation, conditional validity — is -unexercised, and the generated config validators along with them. +Other pilot trees sit beside the ThousandEyes one: `github`, `jamfpro`, +`credentials`. Only ThousandEyes has a live token and an audit. -### The presence census +### The scripts -The measure of the remaining gap. `docs/contract.md` states the intent: +`tf_set_env_var.sh` — the one place the lab credential lives. Every other +script sources it. It exports `THOUSANDEYES_API_TOKEN` (what the generated +provider reads), `TFPFGEN_AUTH_TOKEN` (what `tfpfgen audit run` reads — the +toolkit's fixed secret contract) and `TE_PROVIDER_DIR`. -> `Optional` alone is the rare one. Most APIs answer with a value for every -> field they accept … emitting it as `Optional` alone gives the practitioner a -> perpetual diff. +`tf_acceptance_tests.sh [filter]` — **the benchmark**. Discovers every package +under `internal/services` holding a `TestAcc`, runs them with `TF_ACC=1`, and +prints a summary ending `N passed, M failed, K skipped`. The optional argument +is a case-insensitive substring matched against package paths: -Counting `computed_optional_required` across every attribute tree of all three -pilots, at `30b23ac` on 2026-08-25: +```sh +sh tf_acceptance_tests.sh # all 35 +sh tf_acceptance_tests.sh tags/v1/tag # one resource, ~7s +sh tf_acceptance_tests.sh tests/v1 # a group +``` -| | optional | computed_optional | ratio | -|---|---|---|---| -| when first measured | 2,221 | 64 | 35× | -| after the coverage work | 3,216 | 65 | 49× | -| now | 10,574 | 670 | **15.8×** | +Knobs: `TE_PARALLEL` (default 1 — both `-p` and `-parallel`, so nothing runs +concurrently against the live tenant), `TE_TIMEOUT` (default 120m, the whole +run's `go test` timeout, not per test). -The absolute figures are not comparable across rows — union variants brought -each branch's own fields into the tree, so there are far more attributes to -count than there were. The ratio is the figure that means something, and it -moved the right way for the first time: `x-tfpfgen-server-default` is still the -thing that would close it, and only an audit produces one. +Logs land at `/Users/dafyddwatkins/localtesting/thousandeyes/acceptance-.log`. ---- +`tf_remediation_loop.sh [filter]` — the full loop: build tfpfgen → probe the +live API → compile and accept corrections → regenerate → run the benchmark, +repeated until the pass count stops moving. Knobs: `TE_LOOP_MAX` (default 5), +`TE_LOOP_AUDIT` (`0` reuses the last probe findings — much faster when only +generator code changed), `TE_LOOP_ACCEPT` (`0` stops and leaves proposals for +review). It ends by ranking the distinct `Error:` reasons from the last run, +which is where the next change comes from. Per-phase logs go to a timestamped +`remediation-/` directory. -## What merged +**`TE_LOOP_ACCEPT=1` crosses the correction gate automatically.** That gate is +deliberate — accepting a correction is a human decision the pipeline exists to +enforce. The loop crosses it on purpose; run with `TE_LOOP_ACCEPT=0` first if +you want to read what the probe wants to change. -`#72`–`#86` are the coverage work, described in the git history. Since then: +### What each generated resource carries -| PR | What | +For `` under `internal/services/resources//v1//`: + +| File | Purpose | |---|---| -| #87 | `docs/mapping.md` committed — the specification for all correctness work | -| #88 | Integer path parameters parsed with a diagnostic | -| #89 | List-resource test content type | -| #90 | List-resource addressing schema | -| #91 | Schema constraint keywords parse (`maxLength`, `uniqueItems`, `minimum`, …) | -| #92 | Update the ThousandEyes test document | -| #93 | `Sensitive` and `DeprecationMessage` emitted | -| #94 | Constraint validators emitted from declared bounds | -| #95 | Point the GitHub test document at an immutable ref | -| #96 | A documented default fills the response | -| #97 | `prune.go` decomposed | -| #98 | Comment sweep | -| #99 | A list resource requires a resource to match | -| #100 | Resource identity schema | -| #101 | Fixtures respect `format` | -| #102 | Generated tests assert what the provider actually produces | -| #103 | `docs/mapping.md` gains the entity operation sets | -| #104 | A lone write is an invocation, whichever method spells it | -| #105 | A datasource filters on the fields of the objects it lists | -| #106 | A filter survives only where the field it selects on does | -| #107 | `docs/emittance_tracker.md` — counts live in one file | -| #108 | A list result names the object by the key the resource is addressed by | -| #109 | A union becomes one attribute per variant where nothing writes it | -| #110 | `CLAUDE.md` and `README.md` restated against the tree they describe | -| #112 | Update the ThousandEyes test document again | -| #113 | The test documents named `vendor_openapi_specs` rather than *corpus* | -| #114 | Those documents committed and embedded, retiring the fetch-and-pin scheme | +| `resource_acceptance_test.go` | the live lifecycle: **step 1** apply minimal, **step 2** import + `ImportStateVerify`, **step 3** apply maximal | +| `tests/terraform/acceptance/resource_{minimal,maximal}.tf` | what the live steps apply — **built from recorded bodies** | +| `tests/terraform/unit/resource_{minimal,maximal}.tf` | what the unit tests apply — **still derived from the document** | +| `tests/responses/resource_{minimal,maximal}.json` | wire JSON the mocks answer with | +| `mocks/responders.go` | httpmock responders built from those | +| `crud.go`, `state.go`, `construct.go`, `model.go`, `resource.go` | the provider itself | ---- +The acceptance and unit fixtures diverge on purpose. Unit configs must keep +matching their mocks, which are built from the same derivation; acceptance +configs replay what the API accepted and carry a per-run random suffix. -## Where correctness stands - -`docs/mapping.md` lists thirteen API behaviours and the shape each demands. -**Detection** is whether the audit can observe the behaviour; **expression** is -whether the generator can emit the shape. Measured in the generated trees, not -in this repo's source — an emitter builds most of what it emits. - -| # | Behaviour | Detect | Express | -|---|---|---|---| -| 1 | Accepted on write, never returned | ✗ no observation kind | ✗ `WriteOnly` never emitted | -| 2 | Never accepted, always returned | ~ `writable=false`, `serverForced`, `volatile` | ~ Computed yes; `UseStateForUnknown` still only on `id` | -| 3 | Optional in, always returned | ✓ `serverDefault` | ~ a *documented* default now fills the response; an observed one needs the audit | -| 4 | Returned obfuscated (`****`) | ✗ misreads as `serverForced` | ✓ `Sensitive` emitted from `format: password` / `writeOnly` | -| 5 | Valid only when a sibling equals a value | ✓ `validWhen` | ~ emitter exists; **no tree contains one**, because no audit has run | -| 6 | Settable at create, refused after | ✓ `immutable` | ~ `RequiresReplace` yes; `…IfConfigured` no | -| 7 | Rejected at create, settable on update | ✗ | ✗ `construct.go` still discards `isCreate` | -| 8 | Echoed back semantically equivalent | ✓ `normalisation` | ✗ no custom type, no `SemanticEquals` | -| 9 | Silently clamped or truncated | ~ enums only | ✓ bounds, lengths and patterns become validators | -| 10 | Omitted → returns `""`/`[]`/`0` | ~ lands as `serverDefault` | ✗ no policy either way | -| 11 | Collection returned in arbitrary order | ✗ | ✗ `uniqueItems` now parses, but `SetAttribute` is never emitted | -| 12 | Collection returns server-injected members | ✗ | ✗ | -| 13 | Field carries one of several object shapes | n/a structural | ~ one attribute per variant where nothing writes it; a writable union is refused | - -Emitted symbol counts across the three trees at `30b23ac`, 2026-08-25, every -one of which was zero when this document was first written: -`Sensitive` 53, `DeprecationMessage` 171, `int64validator.Between` 180, -`UTF8LengthBetween` 42, `LengthAtLeast`/`LengthAtMost` 138, `RegexMatches` 21, -`Default` 18. - -Still zero, and each is a row above: `WriteOnly`, `SetAttribute`, -`SetNestedAttribute`, `SemanticEquals`, `RequiresReplaceIfConfigured`, and any -config validator at all. - -`specmodel.Schema` now parses `writeOnly`, `deprecated`, `uniqueItems`, -`maxLength`, `minLength`, `maxItems` and `minItems`. Only `nullable` remains -unparsed. +### The audit artifacts ---- +Under the provider tree: -## Outstanding coverage work - -Refusals grouped by what the reason says, across all three pilots. The stage -split and the totals are in `docs/emittance_tracker.md`. - -| Family | Share | Note | -|---|---|---| -| SDK model lacks the accessor | 729 | The biggest by far, and the one to characterise next. Mostly fields the generated model genuinely lacks rather than a naming bug. | -| Object with no declared shape | 139 | `additionalProperties: true`, or neither properties nor `additionalProperties`. The vendor documented nothing — arguably a vendor-facing report rather than codegen work. | -| Singleton at a fixed path with no operation set | 125 | One object at a fixed path that fits no operation set. | -| Collection shape unsupported | 105 | Arrays of arrays, maps of objects. | -| Read/write type mismatch | 89 | What survives after #85. | -| Nothing survives to read back or send | 88 | Every field of the entity was refused, so the entity goes too. | -| Terraform reserved name at a schema root | 17 | | - -Two families from the previous handoff have all but closed: the path-parameter -type mismatch is down from 250 refusals to 1 (#88), and union refusals from 90 -to 13 (#109) — eleven of those thirteen a branch referencing no component. - -### Gap 2b — file transfer - -`multipart/form-data`, untouched. Agreed shape is `source` and -`content_base64`, mutually exclusive via `resourcevalidator.Conflicting`, with a -computed `content_base64` for downloads. Needs five things, which is why it was -deferred: `multipart/form-data` parsing in `specmodel`; an IR flag for "this -operation takes a file"; a construct idiom that is `AddOrReplacePart(name, -contentType, content)` rather than field setters; the two attributes and their -validator; and **a request adapter reachable from the resource** — -`MultipartBody.SetRequestAdapter` needs one and generated services receive -`*sdk.APIClient`, not the adapter. +| Path | What | +|---|---| +| `audit/observations/.observations.json` | one fact per property | +| `audit/bodies/.bodies.json` | **the accepted create bodies** — request, response, status | +| `audit/inputs.json` | operator-supplied values the probe cannot invent (authored) | +| `spec/corrections/*.correction.json` | accepted corrections (authored) | +| `spec/corrections/proposed/` | awaiting a human decision | +| `spec/revised.yaml` | generated — never hand-edit | --- -## Owed by the repository owner before work can start +## The architecture -`CLAUDE.md` makes every domain term owner-approved. These block their gaps: +The document is a hypothesis; the API is the authority. -- **The undeclared-response-schema observation** — its kind, its `x-tfpfgen-*` - key, and whether it is eligible for `audit.auto_accept`. The vendor declares a - 200 with no schema, so there is nothing to map into state; the agreed approach - is an observation recording the shape the API actually returned. Also needs - quirkserver ground truth: a shape whose document declares no response schema - while the server returns one. -- **`mapping.md` row 1** — `WriteOnly` plus an `..._version` Int64 trigger would - be the first generated attribute with no wire counterpart. Name and suffix. -- **`mapping.md` row 8** — `x-tfpfgen-normalisation` and its value set. - `internal/spec/revise/compile.go:116` still refuses for want of it. +``` +OpenAPI doc ──> specmodel ──> IR ──> sdkbind ──> emit ──> provider tree + ^ | + | v + corrections <── revise <── observations <── audit (live probe) + + bodies +``` -Settled since this list was written: variant sub-attribute naming (#109, now in -the glossary as **variant attribute**), and `docs/mapping.md` is committed. +Presence (`required` / `optional` / `computed`) is corrected into the document +and re-derived. **Values are not.** Values come from `audit/bodies/` and are +replayed directly into acceptance fixtures. That split is the point: deriving +values again from the document is what produced a year of one-field-at-a-time +failures (`icon`, `match_type`, `filters` were all the same bug). + +### Key code + +| Concern | Where | +|---|---| +| Additive minimal search (add a field until 2xx) | `internal/audit/run/steps_create.go: searchMinimal` | +| Subtractive maximal reduction (drop until 2xx) | `internal/audit/run/steps_create.go: reduceMaximal` | +| Refusal grammar (what a 4xx names) | `internal/audit/run/adjust.go: classifyRefusal` | +| Recorded bodies artifact | `internal/audit/observe/bodies.go` | +| Replaying a body into a fixture | `internal/fixtures/fixtures.go: FromAcceptedBody` | +| Per-run unique names | `internal/fixtures/fixtures.go: WithRunSuffix`, `RunSuffixBlock` | +| Acceptance vs unit split | `internal/emit/render_fixtures.go: resourceFixtures` | +| Probe step ordering | `internal/audit/strategy/program.go: buildProgram` | +| Presence rule | `internal/intermediate_representation/attributes.go` (~line 490) | --- -## Lessons that change how to work on this +## What has merged -**Check what the SDK already decided before designing from the document.** -Three premise failures in one session, all the same shape: +| PR | What | +|---|---| +| #115 | a nested attribute is held as a value that can be unknown | +| #116 | the audit reads the refusals it is given | +| #117 | a created resource answers with what names it (identity, create-response id, examples, safe literals) | +| #118 | an entity names the property that identifies it (`x-tfpfgen-identifier-property`) | +| #119 | an acceptance test replays a request the API took | -- *Typed maps* — the plan assumed the SDK carries a Go map. kiota emits no - `map[string]string` at all; it generates a model whose only field is - `additionalData map[string]any`. -- *Discriminated unions* — the plan assumed documents declare discriminators. - GitHub, which owned almost every union refusal, declares none. -- *Merging object unions* — argued for on the grounds that branches need naming - and reads are ambiguous. Both false: kiota names every branch and exactly one - accessor is non-nil. Building it made GitHub's refusals sharply worse. +**Open PR** — the branch this handoff is on: the maximal create ordering fix, +plus the quirkserver's `NamesRefusedFieldInProse`. -In each case a five-minute `grep` of the generated SDK would have prevented -hours. Measure the pilots before designing, not after. +--- -**Measure at the layer the claim is about.** An earlier revision of this file -reported ten framework symbols as never emitted. Four of them were being emitted -at the time. The census had been taken by grepping this repo for the symbol, and -`internal/emit/render_constraints.go` spells a pair of bounds as -`fmt.Sprintf("%sBetween(%v, %v)", …)` — so the string never appears here and -always appears in the output. Grep the generated tree. +## Where the 34 failures actually are -**`postcheck` catches what unit tests do not.** Two bugs in #85 produced -generated Go that did not compile, and neither would have surfaced in the -toolkit's own suite: construction typed a slice from the *getter* while filling -it with the *constructor's* values; and a nested block with no writable children -rendered a loop declaring an index nothing read. +``` +33 fail at Step 1/3 (create) + 1 fail at Step 3/3 -**Refusal counts going *up* can be correct.** One misleading entity-level -refusal becoming several accurate field-level ones raises the total and improves -the toolkit. Read the reasons, not just the total. +30 Error: Create failed (HTTP 4xx) + 4 Error: Provider produced inconsistent result after apply + 1 Error: Invalid id + 1 Error: Create failed: the request never completed +``` -**Rebase rather than merge `main` into these branches.** #83 and #85 were merged -the other way and both broke `main`. +**The score tracks recorded-body coverage.** Only four entities have a body +recorded — `tag`, `credential`, `dashboard`, `connectors_generic` — because +only four got through the probe. The other 31 still have document-derived +configs, which is why they fail at step 1 with a 4xx. + +So the single highest-value work is **getting more entities through the +probe**. Everything else is downstream of that. + +### Next steps, ranked + +1. **Unblock the probe.** 30 entities blocked in the last run. Read the reasons + in the audit output — they group into: parent objects that cannot be + re-created, creates the search could not heal, and a tenant ceiling on + `/templates`. Each group unblocked is several tests. +2. **Populate `audit/inputs.json`.** Some entities need real tenant values no + generator can invent: a reachable stream endpoint URL, an agent id + (`tests_ftp_server`), a dashboard id (`dashboard_snapshot`), a discriminator + `type` for `connectors_generic` / `operations_webhook`. +3. **The four inconsistent-result failures.** A value sent and echoed back + differently. `FromAcceptedBody` already drops what is never returned; this is + the narrower case of a value the API rewrites. +4. **`Invalid id` / `request never completed`** — one each, `templates_sharing_setting` + (a singleton) and `stream` (`lastSuccess`/`lastFailure` declared `int64`, the + API answers RFC 3339). --- -## Verifying these numbers +## Blockers that are not code -Five local gates, all of which CI also runs. `make check` is the first four: +**`account_group` cannot pass.** A leaked object permanently holds the fixture +name and the API refuses to delete it: -```sh -make check # fmt, build, vet, coverage, hygiene -golangci-lint run # make check leaves this to CI +``` +DELETE /v7/account-groups/281474976717041 +400 "Unable to delete accounts outside your organization." ``` -Then the loop that matters, into `/Users/dafyddwatkins/GitHub/terraform/scratch/gen`: +It needs org-admin or ThousandEyes support. The per-run random suffix (#119) +means new leaks will not recur, but this one predates it. -```sh -go build -o …/scratch/bin/tfpfgen ./cmd/tfpfgen -cd …/scratch/gen/ -tfpfgen provider generate # postcheck: go mod tidy, go build, go vet -tfpfgen provider verify # must report no drift -``` +**Import is flaky.** `tag` failed once at step 2 with `Read failed (HTTP 403)` +and passed on a re-run with no change — read-after-write lag, with ThousandEyes +answering 403 where 404 is expected. Any full-suite number is unreliable until +the retry predicate treats that as lag. Re-run a single failure before +believing it. + +**A full suite run is slow.** Serial by design (`TE_PARALLEL=1`). One earlier +run had a single resource burn 1802s on a create timeout. Prefer filtered runs +while iterating. + +--- -The emitted-symbol census, which is a fact about the generated tree: +## Lessons that would have saved hours + +**Run the benchmark.** Every proxy metric moved while it sat at zero. If a +change cannot be shown to move the pass count, say so plainly rather than +reporting the proxy. + +**Verify a diagnosis before acting on it.** Three claims in this work were +asserted confidently and were wrong: that a live minimal object was racing the +maximal create (it was — but only under the *strategy* program, not the plan +path that was read first); that the recipe carrying a stale body caused the +maximal failures (it did not — two tests written to prove it passed without the +fix); and that a regression was caused by a code change when it was a local DNS +failure. Instrument and look. One `fmt.Fprintf` to stderr settled in one run +what an hour of reading did not. + +**Fix a fact in the layer that owns it.** Two workarounds were written into the +emitter for facts that belong to the probe — sending `x-tfpfgen-server-default` +as an input value, and stopping the state mapper nulling any optional +attribute. Both were reverted in #119. If a fact about the API has nowhere to +live, that is a missing observation kind, not a licence to put policy in the +generator. + +**Naming is owner-owned.** A new observation kind or `x-tfpfgen-*` key needs +the repository owner to approve the name before it is coined, and the decision +recorded in `docs/glossary.md`. `identifierProperty` went through that; the +"accepted on write, never returned" fact (`docs/mapping.md` row 1) still needs +it. + +--- + +## Verifying the toolkit itself ```sh -grep -rc 'Sensitive:' /internal/services | grep -v ':0$' +make check # fmt, build, vet, coverage gate, hygiene gate +golangci-lint run # make check leaves this to CI ``` -The presence census: +Coverage gate: 90% total, 80% per package under `internal/`. Currently 90.7%. + +Regenerating a pilot tree: ```sh -tfpfgen provider generate --print-ir > ir.json -# count computed_optional_required across every attribute tree +go build -o ../scratch/bin/tfpfgen ./cmd/tfpfgen +cd ../scratch/gen/thousandeyes +tfpfgen provider generate # postcheck: go mod tidy, go build, go vet ``` -**The report is the acceptance test.** Each piece of work should name the count -it expects to move, and the before/after `unsupported.json` totals should show -it moved by that much and nothing else changed. That is a stronger gate than any -unit test here, because it measures three real documents. +Claims about generated output are verified against a generated tree, never by +grepping this repo for a symbol — an emitter builds most of what it emits. diff --git a/internal/audit/run/adaptive_test.go b/internal/audit/run/adaptive_test.go index 7695b78..e812784 100644 --- a/internal/audit/run/adaptive_test.go +++ b/internal/audit/run/adaptive_test.go @@ -492,3 +492,55 @@ func TestUnit_Adaptive_RedactionHoldsEndToEnd(t *testing.T) { // The run's activity ledger must not carry it either. _ = time.Now } + +// TestUnit_Adaptive_TheReductionKeepsWhatTheCreateNeeded: a field the API +// demanded at createMinimal is not something the maximal reduction may drop. +// +// The reduction protects the body the entity creates by. That body is the one +// the run got accepted, not the one the plan started from — the plan's is the +// document's guess, and the guess is what needed healing. Protecting the guess +// leaves the healed field droppable, and a reduction that drops it can never +// reach an accepted create at all. +func TestUnit_Adaptive_TheReductionKeepsWhatTheCreateNeeded(t *testing.T) { + t.Parallel() + s := quirkserver.New(t, quirkserver.Quirks{ + // Demanded by the API, absent from the document, and named in a + // sentence the loop can act on. + RequiredButUndeclared: []string{"serial"}, + NamesRefusedFieldInProse: true, + // One optional field the maximal carries and the API will not take, + // so the maximal is refused and has to be reduced. + RejectsDocumentedValue: map[string]string{"colour": "bad-colour"}, + }) + + p := &plan.Plan{ + Entities: []plan.EntityPlan{{ + Entity: "thing", Role: "resource", Budget: plan.Budget{Requests: 60}, + Steps: []plan.Step{ + {Kind: plan.StepCreateMinimal, Method: "POST", Path: "/things", + Body: map[string]any{"name": "tfpfgen--thing-name"}}, + {Kind: plan.StepCreateMaximal, Method: "POST", Path: "/things", + Body: map[string]any{"name": "tfpfgen--thing-name", "colour": "bad-colour"}}, + }, + }}, + Budget: plan.RunBudget{Requests: 300, Objects: 10, Duration: "1m"}, + } + + _, sum := mustRun(t, testOptions(t, s, p, testEnv(), nil)) + + var recorded *observe.AcceptedBody + for _, b := range sum.Bodies { + if b.Entity == "thing" { + recorded = b.Maximal + } + } + if recorded == nil { + t.Fatal("no maximal create was ever accepted: the reduction dropped what the create needed") + } + if _, kept := recorded.Request["serial"]; !kept { + t.Errorf("the reduction dropped the field the API demanded: %#v", recorded.Request) + } + if _, dropped := recorded.Request["colour"]; dropped { + t.Errorf("the reduction kept the field the API refused: %#v", recorded.Request) + } +} diff --git a/internal/audit/run/steps_create.go b/internal/audit/run/steps_create.go index 63bd982..98ca180 100644 --- a/internal/audit/run/steps_create.go +++ b/internal/audit/run/steps_create.go @@ -39,6 +39,12 @@ func (r *runner) runCreateMinimal(ctx context.Context, ent *entityState, step *p if err != nil { return err } + // The recipe learns the body that worked. Everything downstream + // replays it — re-creating this entity as another's parent, narrowing + // a refused maximal, cleaning up at the end — and the body the plan + // started from is the document's guess, which is what needed healing + // in the first place. + ent.recipe.minimalBody = cloneAnyMap(rr.body) r.registry[ent.plan.Entity] = rr.obj ent.createdAt = time.Now() ent.ev.sent = sent diff --git a/internal/audit/strategy/program.go b/internal/audit/strategy/program.go index d53164e..453b566 100644 --- a/internal/audit/strategy/program.go +++ b/internal/audit/strategy/program.go @@ -135,10 +135,6 @@ func buildProgram(createBody *specmodel.Schema, gates []Gate, variants []Variant gatedStep(stepReadConsecutive, v), ) } - // The widest valid body, per variant. - for _, v := range variants { - prog = append(prog, gatedStep(stepCreateMaximal, v)) - } // One update per writable field, capped. for i, name := range fieldNames(fields) { if i == maxUpdateFields { @@ -166,11 +162,18 @@ func buildProgram(createBody *specmodel.Schema, gates []Gate, variants []Variant // prose hypotheses the gate loop did not already cover. prog = append(prog, perValueSteps(gates, hyps)...) - // Teardown. - prog = append(prog, - Step{Kind: stepDeleteWithConfirmation}, - Step{Kind: stepCleanupDelete}, - ) + // Teardown, and the widest valid body inside it. + // + // The maximal create makes a second object and deletes it again, so it + // runs once the first is gone: an API that keys an object on fields the + // two bodies share — both are synthesised from the same document — + // answers the second create with a conflict, and a conflict says nothing + // about how wide a valid body is. + prog = append(prog, Step{Kind: stepDeleteWithConfirmation}) + for _, v := range variants { + prog = append(prog, gatedStep(stepCreateMaximal, v)) + } + prog = append(prog, Step{Kind: stepCleanupDelete}) return prog } diff --git a/internal/audit/strategy/strategy_test.go b/internal/audit/strategy/strategy_test.go index 176f263..52ee43b 100644 --- a/internal/audit/strategy/strategy_test.go +++ b/internal/audit/strategy/strategy_test.go @@ -749,3 +749,35 @@ func TestCompileErrors(t *testing.T) { t.Fatal("resource with no create body should error") } } + +// TestUnit_Strategy_TheMaximalCreateFollowsTheDelete pins the one ordering the +// maximal create depends on. +// +// It makes a second object and deletes it again, so the first has to be gone +// first: an API that keys an object on fields both bodies carry — and both are +// synthesised from the same document — answers the second create with a +// conflict, and a conflict says nothing about how wide a valid body is. +func TestUnit_Strategy_TheMaximalCreateFollowsTheDelete(t *testing.T) { + s := compile(t, flatSpec, "widget", defaultCfg()) + + posOf := func(kind string) int { + for i, st := range s.Program { + if string(st.Kind) == kind { + return i + } + } + t.Fatalf("the program has no %s step: %+v", kind, s.Program) + return -1 + } + + create := posOf("createMinimal") + del := posOf("deleteWithConfirmation") + maximal := posOf("createMaximal") + cleanup := posOf("cleanupDelete") + + if !(create < del && del < maximal && maximal < cleanup) { + t.Errorf("program order is createMinimal=%d delete=%d createMaximal=%d cleanup=%d; "+ + "the maximal create must sit between the delete and the cleanup", + create, del, maximal, cleanup) + } +} diff --git a/internal/quirkserver/behaviour.go b/internal/quirkserver/behaviour.go index 650098d..fd1de45 100644 --- a/internal/quirkserver/behaviour.go +++ b/internal/quirkserver/behaviour.go @@ -66,6 +66,15 @@ func (s *Server) normalise(field string, v any) any { return v } +// refusedFieldDetail renders the missing field as the quirk asks: a bare name, +// or a sentence an adjustment loop can read. +func (s *Server) refusedFieldDetail(field string) string { + if s.quirks.NamesRefusedFieldInProse { + return "field " + field + " is required" + } + return field +} + func (s *Server) missingRequired(body map[string]any) string { for _, field := range s.quirks.RequiredButUndeclared { if _, ok := body[field]; !ok { diff --git a/internal/quirkserver/exhibit_write_test.go b/internal/quirkserver/exhibit_write_test.go index 4be50b8..2264832 100644 --- a/internal/quirkserver/exhibit_write_test.go +++ b/internal/quirkserver/exhibit_write_test.go @@ -199,6 +199,32 @@ var writeExhibits = map[string]func(*testing.T){ } }, + "NamesRefusedFieldInProse": func(t *testing.T) { + t.Parallel() + + // The same missing field, said two ways. Only the sentence is one an + // adjustment loop can act on; the bare name beside a generic title + // could be about anything, which is why it stays unhealable. + bare := New(t, Quirks{RequiredButUndeclared: []string{"key"}}) + _, body := post(t, bare.CollectionURL(), map[string]any{"value": "v"}) + if detail, _ := body["detail"].(string); detail != "key" { + t.Errorf("without the quirk the detail should be the bare name, got %v", body) + } + + prose := New(t, Quirks{RequiredButUndeclared: []string{"key"}, NamesRefusedFieldInProse: true}) + status, body := post(t, prose.CollectionURL(), map[string]any{"value": "v"}) + if status != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", status) + } + if detail, _ := body["detail"].(string); detail != "field key is required" { + t.Errorf("the refusal should spell the field into a sentence, got %v", body) + } + + if status, _ := post(t, prose.CollectionURL(), map[string]any{"key": "k"}); status != http.StatusCreated { + t.Errorf("supplying it should succeed, got %d", status) + } + }, + "ConditionallyRequired": func(t *testing.T) { t.Parallel() diff --git a/internal/quirkserver/handlers.go b/internal/quirkserver/handlers.go index 0082e09..09885e9 100644 --- a/internal/quirkserver/handlers.go +++ b/internal/quirkserver/handlers.go @@ -48,7 +48,7 @@ func (s *Server) create(w http.ResponseWriter, r *http.Request) { // Naming the field is what lets the auditor write the requirement down // as observed rather than guessed: an error that does not name it // could have been about anything. - s.fail(w, http.StatusBadRequest, "missing required field", missing) + s.fail(w, http.StatusBadRequest, "missing required field", s.refusedFieldDetail(missing)) return } diff --git a/internal/quirkserver/quirks.go b/internal/quirkserver/quirks.go index 48b31ae..1b91ba4 100644 --- a/internal/quirkserver/quirks.go +++ b/internal/quirkserver/quirks.go @@ -117,6 +117,16 @@ type Quirks struct { // EventuallyConsistentReads makes the first N reads of a new object 404. EventuallyConsistentReads int + // NamesRefusedFieldInProse makes a refusal spell the missing field into a + // sentence — "field serial is required" — rather than leaving the bare + // name in the detail beside a generic title. + // + // Both are real. The bare form is deliberately unhealable, because a name + // alone beside a generic title could be about anything; this is the form + // an adjustment loop can act on, and an API that writes it is one whose + // refusals teach the auditor what the create needs. + NamesRefusedFieldInProse bool + // ErrorEnvelope selects which shape errors take. Defaults to problem+json. ErrorEnvelope Envelope