diff --git a/.github/workflows/smithy-sync.yml b/.github/workflows/smithy-sync.yml index f1db281..b660743 100644 --- a/.github/workflows/smithy-sync.yml +++ b/.github/workflows/smithy-sync.yml @@ -60,6 +60,39 @@ jobs: continue-on-error: true run: CGO_ENABLED=0 go test ./... -v + # The gate above fails on purpose when upstream moves an operation, and + # until now the fix was a person transcribing nine numbers out of four + # failure messages into three files. This does that arithmetic and leaves + # it in the PR, so the review is "should ec2 have gained 46 operations?" + # rather than "is 19,247 the right total?". + # + # It does not relax the gate: this runs before the PR's own ci, which + # still asserts docs/coverage.md against the binary in both directions. A + # change that needs a sentence rather than a number — a service that has + # newly stopped serving anything, which the page must name — makes this + # step exit non-zero, and the body says so. + # + # It must stay *after* Run tests. cmd/devcloud/sync_test.go finds the test + # step by the first `run` containing "go test", and this step's does too, + # so moving it earlier would silently repoint three existing sync gates. + - name: Re-derive the published figures + id: figures + if: steps.changes.outputs.changed == 'true' + continue-on-error: true + env: + DEVCLOUD_UPDATE_DOCS: "1" + run: | + # pipefail, or sed's exit code is the step's: the updater refusing to + # invent a sentence would then report success and the PR would read as + # a clean sync. The runner's default shell is `bash -e`, not `bash -eo + # pipefail`, so this has to be said. + set -o pipefail + CGO_ENABLED=0 go test ./cmd/devcloud/ \ + -run 'TestUpdatePublishedFigures' -v \ + | sed -n '/^\*\*[0-9]* of /,/^$/p;/^| Figure/,/^$/p;/^PROSE REQUIRED/,$p' \ + > /tmp/figures.md + cat /tmp/figures.md + # The diff is whole-tree whether upstream moved one model or ninety — a # real refresh measured on 2026-09-06 changed 93 models and 134 generated # files. Asking a reviewer to "review" that is asking for nothing. This @@ -74,14 +107,24 @@ jobs: echo echo "A \`failure\` is expected when upstream added or removed an" echo "operation: the published-figure gate over \`docs/coverage.md\` fails" - echo "on purpose so a human looks at a coverage change. Re-derive the" - echo "figures and correct the doc in this PR — do not silence the gate." + echo "on purpose so a human looks at a coverage change. The figures" + echo "below are already re-derived and committed to this PR." echo echo "This PR's own \`ci\`, \`compat\` and \`codegen-drift\` runs are the" echo "gate on merging." echo echo "---" echo + echo "**Published figures: \`${{ steps.figures.outcome }}\`.**" + echo + echo "A \`failure\` means a change needs a sentence rather than a" + echo "number — read the PROSE REQUIRED block below and write it" + echo "before merging." + echo + cat /tmp/figures.md + echo + echo "---" + echo python3 scripts/model_churn.py --upstream echo echo "Summary from \`scripts/model_churn.py\`; re-derive with" @@ -105,8 +148,13 @@ jobs: # would report success every week no matter what upstream did — the same # silent no-op scripts/download-smithy-models.sh was fixed to stop doing, # traded for the one continue-on-error just removed. + # + # Both continue-on-error steps are read, not just the tests. A PROSE + # REQUIRED result means docs/coverage.md is wrong until someone writes a + # sentence, and reading only the test outcome would end the job green on + # the one failure mode this automation cannot fix for itself. - name: Fail if the sync's tests failed - if: steps.tests.outcome == 'failure' + if: steps.tests.outcome == 'failure' || steps.figures.outcome == 'failure' run: | - echo "::error::Smithy sync tests failed; see the PR opened by this run." + echo "::error::Smithy sync needs a look (tests=${{ steps.tests.outcome }}, figures=${{ steps.figures.outcome }}); see the PR opened by this run." exit 1 diff --git a/changes/unreleased/Changed-20260913-145759.yaml b/changes/unreleased/Changed-20260913-145759.yaml new file mode 100644 index 0000000..4ea13df --- /dev/null +++ b/changes/unreleased/Changed-20260913-145759.yaml @@ -0,0 +1,5 @@ +kind: Changed +body: The weekly Smithy sync re-derives the published coverage figures itself and commits them into its own pull request, so reviewing it is confirming which operations moved rather than transcribing the figures out of failing tests by hand +time: 2026-09-13T14:57:59.733064+09:00 +custom: + Issue: "165" diff --git a/cmd/devcloud/coverage_test.go b/cmd/devcloud/coverage_test.go index 795faca..3b92007 100644 --- a/cmd/devcloud/coverage_test.go +++ b/cmd/devcloud/coverage_test.go @@ -3,8 +3,10 @@ package main import ( + "encoding/json" "fmt" "os" + "path/filepath" "regexp" "sort" "strconv" @@ -20,18 +22,25 @@ import ( // doc would still be free to drift from it. const coveragePath = "../../docs/coverage.md" -// coverageRow matches one row of the summary table at the top of +// coverageRowPattern matches one row of the summary table at the top of // docs/coverage.md: // // | **Registered** | The gateway routes the service. … | **205** | // // The label is anchored to the row start so a number quoted in prose elsewhere -// on the page cannot be mistaken for the published figure. +// on the page cannot be mistaken for the published figure. Capture group 1 is +// the published figure, and is the only span figures_update_test.go rewrites — +// the gate that reads a row and the tool that writes it share one pattern, so a +// restructured page cannot leave one of them silently reading the wrong cell. +func coverageRowPattern(label string) *regexp.Regexp { + return regexp.MustCompile(`(?m)^\|\s*\*\*` + regexp.QuoteMeta(label) + `\*\*\s*\|[^|]*\|\s*\*\*(\d+)\*\*\s*\|`) +} + +// coverageRow returns the figure the summary table publishes for label. func coverageRow(t *testing.T, doc, label string) int { t.Helper() - pattern := regexp.MustCompile(`(?m)^\|\s*\*\*` + regexp.QuoteMeta(label) + `\*\*\s*\|[^|]*\|\s*\*\*(\d+)\*\*\s*\|`) - matches := pattern.FindAllStringSubmatch(doc, -1) + matches := coverageRowPattern(label).FindAllStringSubmatch(doc, -1) if len(matches) != 1 { t.Fatalf("docs/coverage.md: found %d rows for %q, want exactly 1. "+ "The summary table was restructured; this gate reads it, so update the "+ @@ -46,17 +55,25 @@ func coverageRow(t *testing.T, doc, label string) int { return n } -// tierRow matches one row of the per-operation table in docs/coverage.md: +// tierRowPattern matches one row of the per-operation table in docs/coverage.md: // // | `hand-verified` | 4,496 | // -// Thousands separators are stripped: the doc is written for a reader, and the -// gate reads what the reader sees rather than asking the doc to be machine-shaped. +// Thousands separators are inside capture group 1: the doc is written for a +// reader, and the gate reads what the reader sees rather than asking the doc to +// be machine-shaped. tierRow strips them, and the updater writes them back. +func tierRowPattern(label string) *regexp.Regexp { + return regexp.MustCompile("(?m)^\\|\\s*`" + regexp.QuoteMeta(label) + "`\\s*\\|\\s*([\\d,]+)\\s*\\|") +} + +// totalKnownPattern matches the summed row of that same table. +var totalKnownPattern = regexp.MustCompile(`(?m)^\|\s*\*\*total known\*\*\s*\|\s*\*\*([\d,]+)\*\*\s*\|`) + +// tierRow returns the operation count the manifest table publishes for a tier. func tierRow(t *testing.T, doc, label string) int { t.Helper() - pattern := regexp.MustCompile("(?m)^\\|\\s*`" + regexp.QuoteMeta(label) + "`\\s*\\|\\s*([\\d,]+)\\s*\\|") - matches := pattern.FindAllStringSubmatch(doc, -1) + matches := tierRowPattern(label).FindAllStringSubmatch(doc, -1) if len(matches) != 1 { t.Fatalf("docs/coverage.md: found %d rows for tier %q, want exactly 1", len(matches), label) } @@ -87,6 +104,83 @@ func servedCounts() (serving int, registeredOnly []string) { return serving, registeredOnly } +// figures is every published number that is derived rather than decided. +// +// The gates below and the updater in figures_update_test.go both read this, so +// a figure has exactly one derivation. A number that lives in two places is the +// defect docs/coverage.md exists to prevent, and a second copy inside the tool +// that maintains it would be the worst place to keep one. +type figures struct { + registered int // gateway-routed services + serving int // services with >= 1 non-unimplemented operation + registeredOnly []string // the rest, sorted + compatTested int // registered minus the pinned boto3 exclusions + tiers map[fidelity.Tier]int + totalKnown int +} + +// derivedFigures reads every derivable published number out of the binary. +// +// It reads the registry and the fidelity manifest rather than docs/coverage.md +// because the page is the claim under test: deriving from it would make every +// gate below agree with whatever the page happened to say. +func derivedFigures(t *testing.T) figures { + t.Helper() + + serving, registeredOnly := servedCounts() + f := figures{ + registered: len(plugin.DefaultRegistry.RegisteredServices()), + serving: serving, + registeredOnly: registeredOnly, + tiers: map[fidelity.Tier]int{}, + } + for _, svc := range fidelity.Services { + for _, tier := range svc.Operations { + f.tiers[tier]++ + f.totalKnown++ + } + } + f.compatTested = f.registered - len(loadCompatExclusions(t)) + return f +} + +// loadCompatExclusions returns the registered services no boto3 test can reach. +// +// docs/coverage.md's fourth number is the fleet minus these. The set is a fact +// about botocore, so tests/compatibility owns it — this reads that file rather +// than keeping a Go copy, because a Go copy would drift the week botocore +// publishes a client and only the Python side noticed. +func loadCompatExclusions(t *testing.T) []string { + t.Helper() + + path := filepath.Join(repoRoot(t), "tests", "compatibility", "exclusions.json") + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read the compatibility exclusions: %v", err) + } + + var doc struct { + NoBoto3Client map[string]string `json:"noBoto3Client"` + UnreachableFromBoto3 map[string]string `json:"unreachableFromBoto3"` + } + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("tests/compatibility/exclusions.json is not valid JSON: %v. "+ + "Both the compatibility suite and the published Compatibility-tested "+ + "figure are derived from it, so an unreadable file stops both rather "+ + "than quietly excluding nothing.", err) + } + + var ids []string + for id := range doc.NoBoto3Client { + ids = append(ids, id) + } + for id := range doc.UnreachableFromBoto3 { + ids = append(ids, id) + } + sort.Strings(ids) + return ids +} + // TestPublishedCoverageMatchesTheBinary is Milestone 6's gate: CI fails if a // registered service drops below the floor or the count regresses. // @@ -106,15 +200,18 @@ func TestPublishedCoverageMatchesTheBinary(t *testing.T) { } doc := string(raw) - registered := plugin.DefaultRegistry.RegisteredServices() - serving, registeredOnly := servedCounts() + f := derivedFigures(t) + serving, registeredOnly := f.serving, f.registeredOnly - if got, want := len(registered), len(fidelity.Services); got != want { + // Deliberately asserted here and not inside derivedFigures: this is a + // guarantee about codegen being current, and a helper that checked it would + // make the updater depend on it silently rather than fail on it. + if got, want := f.registered, len(fidelity.Services); got != want { t.Errorf("the registry holds %d services and the fidelity manifest %d; "+ "run `make codegen`", got, want) } - if got, want := coverageRow(t, doc, "Registered"), len(registered); got != want { + if got, want := coverageRow(t, doc, "Registered"), f.registered; got != want { t.Errorf("docs/coverage.md publishes %d registered services, the binary registers %d. "+ "If a service was added or removed on purpose, the published figure moves in the "+ "same commit — that is what this gate is for.", got, want) @@ -129,6 +226,18 @@ func TestPublishedCoverageMatchesTheBinary(t *testing.T) { t.Errorf("docs/coverage.md publishes %d registered-only services, the manifest reports "+ "%d: %v", got, want, registeredOnly) } + + // The fourth number was the one figures_update_test.go wrote and nothing read + // back. A figure the updater maintains and no gate asserts is maintained only + // on the weeks the sync happens to run: an exclusion added on the Python side + // would move the fleet, leave this page stating the old number, and every + // check would stay green. It is asserted here so the round trip closes. + if got, want := coverageRow(t, doc, "Compatibility-tested"), f.compatTested; got != want { + t.Errorf("docs/coverage.md publishes %d compatibility-tested services; %d registered "+ + "minus the %d pinned in tests/compatibility/exclusions.json is %d. Adding an "+ + "exclusion lowers the published figure in the same commit — that is what this "+ + "gate is for.", got, f.registered, f.registered-f.compatTested, want) + } } // TestPublishedOperationTiersMatchTheManifest gates the depth half of the claim. @@ -144,14 +253,8 @@ func TestPublishedOperationTiersMatchTheManifest(t *testing.T) { } doc := string(raw) - counts := map[fidelity.Tier]int{} - total := 0 - for _, svc := range fidelity.Services { - for _, tier := range svc.Operations { - counts[tier]++ - total++ - } - } + f := derivedFigures(t) + counts, total := f.tiers, f.totalKnown for _, tier := range []fidelity.Tier{ fidelity.TierHandVerified, @@ -164,8 +267,7 @@ func TestPublishedOperationTiersMatchTheManifest(t *testing.T) { } } - published := regexp.MustCompile(`(?m)^\|\s*\*\*total known\*\*\s*\|\s*\*\*([\d,]+)\*\*\s*\|`). - FindStringSubmatch(doc) + published := totalKnownPattern.FindStringSubmatch(doc) if published == nil { t.Fatal("docs/coverage.md: the 'total known' row did not parse") } @@ -186,8 +288,7 @@ func TestRegisteredOnlyServicesAreNamedInTheDocs(t *testing.T) { } doc := string(raw) - _, registeredOnly := servedCounts() - for _, id := range registeredOnly { + for _, id := range derivedFigures(t).registeredOnly { if !strings.Contains(doc, id) && !strings.Contains(doc, hyphenate(id)) { t.Errorf("%s serves nothing but docs/coverage.md never names it. "+ "The page states which services serve nothing and why; a service that "+ @@ -196,6 +297,23 @@ func TestRegisteredOnlyServicesAreNamedInTheDocs(t *testing.T) { } } +// quotedFigurePattern matches the registered/serving pair where the front pages +// state it in prose. Loose enough for both phrasings in use — "205 AWS services +// registered, 201 serving at least one operation" and "205 registered / 201 +// serving" — because these are sentences, and pinning their wording would make +// every edit a test failure. Capture groups 1 and 2 are the two figures, and +// they are the only spans the updater moves. +// +// Every gap excludes digits and is bounded, which is the difference between a +// loose reader and a loose writer. A gate that mismatches fails a test; the +// updater writes through this same pattern unattended every Monday, and an +// unbounded gap let "3 of the 431 registered services need 2 serving tiers" +// match with the 3 and the 2 as its figures. Bounding the gaps costs nothing +// real — both live phrasings separate the two numbers by ", " or " / " — and a +// rewording that stops matching fails loudly, because +// TestOtherDocsQuoteTheSameFigure treats zero matches as a failure. +var quotedFigurePattern = regexp.MustCompile(`(?m)(\d+)[^.\n\d]{0,24}?registered\b[^.\n\d]{0,4}?(\d+)[^.\n\d]{0,4}?serving`) + // TestOtherDocsQuoteTheSameFigure catches the drift that actually happened. // // README.md and docs/README.md both quoted "148 registered / 117 serving" three @@ -207,12 +325,8 @@ func TestRegisteredOnlyServicesAreNamedInTheDocs(t *testing.T) { // numbers: these are prose, and pinning their phrasing would make every edit a // test failure. func TestOtherDocsQuoteTheSameFigure(t *testing.T) { - registered := len(plugin.DefaultRegistry.RegisteredServices()) - serving, _ := servedCounts() - - // Loose enough for both phrasings in use — "205 AWS services registered, 201 - // serving at least one operation" and "205 registered / 201 serving". - quoted := regexp.MustCompile(`(?m)(\d+)[^.\n]{0,24}?registered\b[^.\n]*?(\d+)[^.\n]{0,4}?serving`) + f := derivedFigures(t) + registered, serving := f.registered, f.serving for _, path := range []string{"../../README.md", "../../docs/README.md"} { raw, err := os.ReadFile(path) @@ -221,7 +335,7 @@ func TestOtherDocsQuoteTheSameFigure(t *testing.T) { continue } - matches := quoted.FindAllStringSubmatch(string(raw), -1) + matches := quotedFigurePattern.FindAllStringSubmatch(string(raw), -1) if len(matches) == 0 { // Zero matches is a failure, not a pass. A rewording that stops // matching would otherwise disable this check in silence, which is @@ -241,7 +355,7 @@ func TestOtherDocsQuoteTheSameFigure(t *testing.T) { } } -// targetTableRow matches one row of the two-axis table in +// targetRowPattern matches one row of the two-axis table in // docs/coverage.md#the-target: // // | **Routing target** | **431 / 431 — met** | leak-zero; … | @@ -249,13 +363,18 @@ func TestOtherDocsQuoteTheSameFigure(t *testing.T) { // Only the first number in the value cell is captured. "431 / 431 — met" and // "205 — met" are written for a reader; the gate reads the figure the reader // sees rather than asking the table to be machine-shaped, which is the same -// trade tierRow makes. +// trade tierRow makes. The rest of the cell is prose, so the updater rewrites +// group 1 and reports the remainder as a sentence a person owes. +func targetRowPattern(label string) *regexp.Regexp { + return regexp.MustCompile(`(?m)^\|\s*\*?\*?` + regexp.QuoteMeta(label) + + `\*?\*?\s*\|\s*\*?\*?([\d,]+)`) +} + +// targetTableRow returns the figure the two-axis table publishes for label. func targetTableRow(t *testing.T, doc, label string) int { t.Helper() - pattern := regexp.MustCompile(`(?m)^\|\s*\*?\*?` + regexp.QuoteMeta(label) + - `\*?\*?\s*\|\s*\*?\*?([\d,]+)`) - matches := pattern.FindAllStringSubmatch(doc, -1) + matches := targetRowPattern(label).FindAllStringSubmatch(doc, -1) if len(matches) != 1 { t.Fatalf("docs/coverage.md: found %d target rows for %q, want exactly 1. "+ "The two-axis table is what this gate reads; if it was restructured, "+ @@ -290,7 +409,7 @@ func TestPublishedTargetTableMatchesTheBinary(t *testing.T) { } doc := string(raw) - registered := len(plugin.DefaultRegistry.RegisteredServices()) + registered := derivedFigures(t).registered if got := targetTableRow(t, doc, "Routing target"); got != registered { t.Errorf("docs/coverage.md publishes a routing target of %d services, the binary "+ diff --git a/cmd/devcloud/figures_update_test.go b/cmd/devcloud/figures_update_test.go new file mode 100644 index 0000000..2e516dd --- /dev/null +++ b/cmd/devcloud/figures_update_test.go @@ -0,0 +1,645 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +// This file is a tool, not a gate. The gates in coverage_test.go fail when +// docs/coverage.md disagrees with the binary; this writes the agreement they +// demand, so the weekly Smithy sync arrives with its arithmetic already done and +// a reviewer spends the review on whether the operations that moved should have. +// +// It lives beside the gates, in the same package, for one reason: the figures +// have one derivation (derivedFigures) and rewriting them from a separate +// binary would need its own copy of cmd/devcloud/imports.go's 431 blank imports +// to see the registry at all. A second copy of that list is a worse defect than +// anything this saves. +// +// It is switched by an environment variable rather than a test flag because +// `go test ./... -update-docs` fails in every other package with "flag provided +// but not defined". DEVCLOUD_STARTUP_BUDGET in budget_test.go sets the +// precedent. + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" + + "github.com/skyoo2003/devcloud/internal/generated/fidelity" +) + +// figureValue is one number this tool will rewrite, and what the PR body calls it. +type figureValue struct { + label string + want int +} + +// figureEdit is one published span this tool will rewrite, and where. +// +// pattern is always the same pattern the corresponding gate reads with, so a +// page restructure cannot leave the gate checking one cell and the tool writing +// another. figures maps one-to-one onto the pattern's capture groups, in order. +type figureEdit struct { + path string // repo-relative + pattern *regexp.Regexp + figures []figureValue + + // separated writes thousands separators, as docs/coverage.md does for + // operation counts and not for service counts. Which of the two applies is + // decided by the gate's own pattern: one accepts commas, the other does not. + separated bool + + // everyMatch rewrites all matches rather than insisting on exactly one. The + // front pages state the figure in a sentence, and a page is free to state it + // twice; a table row that appears twice is a restructured table, and writing + // into an ambiguous match is worse than failing. + everyMatch bool +} + +// figureChange is one span's before and after, for the table the PR body carries. +type figureChange struct { + label string + before string + after string +} + +// servingTarget reads the decided depth target off docs/coverage.md. +// +// It is the one row in the two-axis table the updater must not write: +// TestPublishedTargetTableMatchesTheBinary calls it "a decision, not a +// measurement" and uses it as the arithmetic the third number must satisfy, so +// writing it would make that gate tautological. It is read here for the same +// subtraction the gate performs, and for no other reason. +func servingTarget(t *testing.T, root string) int { + t.Helper() + + raw, err := os.ReadFile(filepath.Join(root, "docs", "coverage.md")) + if err != nil { + t.Fatalf("read the published coverage claim: %v", err) + } + return targetTableRow(t, string(raw), "Serving target") +} + +// publishedFigureEdits is the whole published surface this tool owns. +// +// What is absent is as deliberate as what is present. The serving target is a +// decision rather than a measurement, the protocol and runtime-cost tables are +// not derivable from the manifest at all, and docs/demand.md answers a different +// question. This writes numbers the binary already knows and nothing else. +func publishedFigureEdits(f figures, servingTarget int) []figureEdit { + const ( + coverage = "docs/coverage.md" + readme = "README.md" + docsIndex = "docs/README.md" + ) + + // The front pages restate the same two figures, so their rows carry the file + // name: three rows all labelled "Registered" would read as a table that had + // repeated itself rather than as three pages that have to agree. + pair := func(path string) []figureValue { + return []figureValue{ + {fmt.Sprintf("Registered (%s)", path), f.registered}, + {fmt.Sprintf("Serving ≥1 operation (%s)", path), f.serving}, + } + } + + return []figureEdit{ + {path: coverage, pattern: coverageRowPattern("Registered"), + figures: []figureValue{{"Registered", f.registered}}}, + {path: coverage, pattern: coverageRowPattern("Serving ≥1 operation"), + figures: []figureValue{{"Serving ≥1 operation", f.serving}}}, + {path: coverage, pattern: coverageRowPattern("Registered-only"), + figures: []figureValue{{"Registered-only", len(f.registeredOnly)}}}, + {path: coverage, pattern: coverageRowPattern("Compatibility-tested"), + figures: []figureValue{{"Compatibility-tested", f.compatTested}}}, + + {path: coverage, pattern: tierRowPattern(string(fidelity.TierHandVerified)), separated: true, + figures: []figureValue{{"`hand-verified` operations", f.tiers[fidelity.TierHandVerified]}}}, + {path: coverage, pattern: tierRowPattern(string(fidelity.TierAutoCRUD)), separated: true, + figures: []figureValue{{"`auto-crud` operations", f.tiers[fidelity.TierAutoCRUD]}}}, + {path: coverage, pattern: tierRowPattern(string(fidelity.TierUnimplemented)), separated: true, + figures: []figureValue{{"`unimplemented` operations", f.tiers[fidelity.TierUnimplemented]}}}, + {path: coverage, pattern: totalKnownPattern, separated: true, + figures: []figureValue{{"total known operations", f.totalKnown}}}, + + {path: coverage, pattern: targetRowPattern("Routing target"), separated: true, + figures: []figureValue{{"Routing target", f.registered}}}, + {path: coverage, pattern: targetRowPattern("Registered and engine-served, outside the serving target"), separated: true, + figures: []figureValue{{"Outside the serving target", f.registered - servingTarget}}}, + + {path: readme, pattern: quotedFigurePattern, everyMatch: true, figures: pair(readme)}, + {path: docsIndex, pattern: quotedFigurePattern, everyMatch: true, figures: pair(docsIndex)}, + } +} + +// applyFigureEdits returns doc with every edit's figures rewritten, and what +// moved. File I/O stays at the edges so the round-trip self-check below can run +// without writing to the tree it is checking. +// +// Only the captured spans move. The surrounding `|`, `**` and prose are left +// exactly as they are, because the gates are deliberately loose about wording +// and rewriting a whole match would pin phrasing nothing asked to pin. +func applyFigureEdits(doc string, edits []figureEdit) (string, []figureChange, error) { + var changes []figureChange + + for _, e := range edits { + locs := e.pattern.FindAllStringSubmatchIndex(doc, -1) + switch { + case len(locs) == 0: + return "", nil, fmt.Errorf("%s: nothing states %q in a form this tool can read. "+ + "The page was restructured; the gate that reads it uses the same pattern, so "+ + "move both deliberately rather than letting the figure stop being maintained", + e.path, e.figures[0].label) + case !e.everyMatch && len(locs) != 1: + return "", nil, fmt.Errorf("%s: found %d rows for %q, want exactly 1. "+ + "An ambiguous match is not written into: writing the wrong cell is worse "+ + "than failing, so restructure the table deliberately or fix the pattern", + e.path, len(locs), e.figures[0].label) + } + + for _, m := range locs { + if got, want := len(m)/2-1, len(e.figures); got != want { + return "", nil, fmt.Errorf("%s: the pattern for %q captures %d spans and %d "+ + "figures were supplied. They are declared together and must stay in step", + e.path, e.figures[0].label, got, want) + } + for i, fv := range e.figures { + changes = append(changes, figureChange{ + label: fv.label, + before: doc[m[2*(i+1)]:m[2*(i+1)+1]], + after: formatFigure(fv.want, e.separated), + }) + } + } + + // Splice the last span first: every index above came from the document as + // it is now, and replacing from the front would invalidate the rest. + for i := len(locs) - 1; i >= 0; i-- { + m := locs[i] + for g := len(e.figures) - 1; g >= 0; g-- { + start, end := m[2*(g+1)], m[2*(g+1)+1] + doc = doc[:start] + formatFigure(e.figures[g].want, e.separated) + doc[end:] + } + } + } + + return doc, changes, nil +} + +// formatFigure renders a figure the way docs/coverage.md writes it. +// +// Operation counts are thousands-separated because the page is written for a +// reader, and tierRow strips the separators back out — so the two agree by +// construction. Service counts are not separated, because the summary table's +// own gate pattern accepts no comma. +func formatFigure(n int, separated bool) string { + s := strconv.Itoa(n) + if !separated { + return s + } + // A depth target may be set above the fleet, and "outside the serving target" + // is then negative. The sign is held aside rather than grouped with the + // digits, which would otherwise render -123 as "-,123". + sign := "" + if strings.HasPrefix(s, "-") { + sign, s = "-", s[1:] + } + for i := len(s) - 3; i > 0; i -= 3 { + s = s[:i] + "," + s[i:] + } + return sign + s +} + +// renderFigureTable is what a reviewer reads instead of re-deriving. +// +// Every figure is listed, not only the ones that moved: "these four did not +// move" is half the answer a sync PR raises, and a table that silently omits +// them cannot give it. Shaped like scripts/model_churn.py's render — lead +// sentence in bold, blank line, table, em-dash for an empty cell — so the two +// halves of the body read as one. +func renderFigureTable(changes []figureChange) string { + var b strings.Builder + + moved := 0 + for _, c := range changes { + if c.before != c.after { + moved++ + } + } + + fmt.Fprintf(&b, "**%d of %d published figures moved.**\n\n", moved, len(changes)) + b.WriteString("| Figure | Before | After |\n") + b.WriteString("|---|---|---|\n") + for _, c := range changes { + after := c.after + if c.before == c.after { + after = "—" + } + fmt.Fprintf(&b, "| %s | %s | %s |\n", c.label, c.before, after) + } + b.WriteString("\n") + return b.String() +} + +// proseRequired returns the changes a number cannot carry, one block each. +// +// The tool writes counts. It will not invent a sentence, and the two cases below +// are sentences: a service that has newly stopped serving anything, which +// docs/coverage.md must name and explain, and a registered count restated in +// prose no gate reads. Reporting them and exiting non-zero is the whole +// difference between automating the arithmetic and automating the judgement. +func proseRequired(f figures, doc string, changes []figureChange) []string { + var blocks []string + + var unnamed []string + for _, id := range f.registeredOnly { + if !strings.Contains(doc, id) && !strings.Contains(doc, hyphenate(id)) { + unnamed = append(unnamed, id) + } + } + if len(unnamed) > 0 { + var b strings.Builder + b.WriteString("PROSE REQUIRED — the updater wrote the counts but cannot write the sentence:\n\n") + for _, id := range unnamed { + fmt.Fprintf(&b, " `%s` now serves no operation. docs/coverage.md states which\n"+ + " services serve nothing and why, and names each one. Add it under\n"+ + " \"Why a registered service can serve nothing\" before merging.\n", id) + } + blocks = append(blocks, b.String()) + } + + for _, c := range changes { + if c.label != "Registered" || c.before == c.after { + continue + } + blocks = append(blocks, fmt.Sprintf( + "PROSE REQUIRED — the registered count moved from %s to %s, and docs/coverage.md\n"+ + "restates it in two places no gate reads and this tool will not rewrite:\n\n"+ + " - the blockquote under the summary table (\"Two targets, not one: routing is\n"+ + " %s of %s, depth is …\")\n"+ + " - the denominator in the Routing target cell, which now reads \"%s / %s\"\n\n"+ + " Both are sentences about what the number means. Move them before merging.\n", + c.before, c.after, c.before, c.before, c.after, c.before)) + break + } + + return blocks +} + +// TestUpdatePublishedFigures rewrites every derivable figure the docs publish. +// +// Inert unless DEVCLOUD_UPDATE_DOCS=1. A test suite that rewrites tracked files +// by default is a trap: `go test ./...` on any branch must read the docs and +// never write them, or a green run stops meaning the docs were right and starts +// meaning they were overwritten. +func TestUpdatePublishedFigures(t *testing.T) { + if os.Getenv("DEVCLOUD_UPDATE_DOCS") != "1" { + t.Skip("set DEVCLOUD_UPDATE_DOCS=1 to rewrite the published figures") + } + + root := repoRoot(t) + f := derivedFigures(t) + + var order []string + byPath := map[string][]figureEdit{} + for _, e := range publishedFigureEdits(f, servingTarget(t, root)) { + if _, seen := byPath[e.path]; !seen { + order = append(order, e.path) + } + byPath[e.path] = append(byPath[e.path], e) + } + + var all []figureChange + docs := map[string]string{} + for _, rel := range order { + full := filepath.Join(root, rel) + raw, err := os.ReadFile(full) + if err != nil { + t.Fatalf("read %s: %v", rel, err) + } + + out, changes, err := applyFigureEdits(string(raw), byPath[rel]) + if err != nil { + t.Fatalf("%v", err) + } + all = append(all, changes...) + docs[rel] = out + + // Written only when it differs: an unchanged sync must leave a clean + // `git status`, or create-pull-request opens an empty PR every week. + if out != string(raw) { + if err := os.WriteFile(full, []byte(out), 0o644); err != nil { + t.Fatalf("write %s: %v", rel, err) + } + } + } + + // Printed rather than logged: the sync lifts this out of the step's stdout + // into the PR body, and t.Logf would indent every line out of Markdown. + fmt.Print(renderFigureTable(all)) + + blocks := proseRequired(f, docs["docs/coverage.md"], all) + for _, b := range blocks { + fmt.Println(b) + } + if len(blocks) > 0 { + t.Fatalf("%d published change(s) need a sentence this tool will not invent. "+ + "The counts are written; the prose above is not, and docs/coverage.md is "+ + "wrong until someone writes it. Do not relax the gate to merge past this.", + len(blocks)) + } +} + +// mangleFigure overwrites a pattern's captures with values, for the round trip below. +// +// It rewrites the whole match with strings.Replace rather than splicing by +// submatch index, so it shares no code with applyFigureEdits — a bug in the +// splice cannot cancel itself out across the round trip. +func mangleFigure(t *testing.T, doc string, pattern *regexp.Regexp, values ...string) string { + t.Helper() + + m := pattern.FindStringSubmatch(doc) + if m == nil { + t.Fatalf("the pattern under test matches nothing in the live document") + } + + mangled := m[0] + for i, v := range values { + mangled = strings.Replace(mangled, m[i+1], v, 1) + } + return strings.Replace(doc, m[0], mangled, 1) +} + +// TestUpdaterRestoresAMangledFigure is the updater's own self-check, and it is +// always on. +// +// The published documents are the golden file, and they update themselves: a +// corrupted figure restored to what the binary says must reproduce the committed +// page byte for byte. Asserting on the whole file rather than on the three cells +// is the point — a rewrite that corrupts an unrelated byte is exactly the +// failure a cell-level assertion misses. +func TestUpdaterRestoresAMangledFigure(t *testing.T) { + root := repoRoot(t) + edits := publishedFigureEdits(derivedFigures(t), servingTarget(t, root)) + + editsFor := func(rel string) []figureEdit { + var out []figureEdit + for _, e := range edits { + if e.path == rel { + out = append(out, e) + } + } + return out + } + read := func(rel string) string { + raw, err := os.ReadFile(filepath.Join(root, rel)) + if err != nil { + t.Fatalf("read %s: %v", rel, err) + } + return string(raw) + } + restore := func(rel, doc string) string { + out, _, err := applyFigureEdits(doc, editsFor(rel)) + if err != nil { + t.Fatalf("%v", err) + } + return out + } + + // The baseline is the page as the binary says it should read, not the page as + // committed. They are the same on a clean tree and differ on exactly the sync + // this tool exists for — and what is under test here is the round trip, not + // whether the figures are current. TestPublishedCoverageMatchesTheBinary owns + // that question; owning it twice made a stale page report "the thousands + // separator does not survive the rewrite" about a row nothing had touched. + coverage := restore("docs/coverage.md", read("docs/coverage.md")) + + // 1. A summary-table cell: the plain-integer shape, with prose either side. + if got := restore("docs/coverage.md", + mangleFigure(t, coverage, coverageRowPattern("Registered"), "999")); got != coverage { + t.Error("a mangled **Registered** row was not restored to the committed page") + } + + // 2. A tier row: the thousands-separated shape. Restoring "7" to "10,871" + // proves the separator survives the round trip rather than being dropped. + if got := restore("docs/coverage.md", + mangleFigure(t, coverage, tierRowPattern(string(fidelity.TierAutoCRUD)), "7")); got != coverage { + t.Error("a mangled `auto-crud` tier row was not restored, so the thousands " + + "separator does not survive the rewrite") + } + + // 3. A target row: a partial-cell rewrite. Only the first number is captured, + // so "/ 431 — met" must come back untouched rather than be swallowed. + if got := restore("docs/coverage.md", + mangleFigure(t, coverage, targetRowPattern("Routing target"), "12")); got != coverage { + t.Error("a mangled Routing target row was not restored, or the rewrite consumed " + + "the rest of the cell") + } + + // 4. Idempotence: a page the updater has already written must come back + // untouched and report no movement. An updater that rewrites a tree it just + // wrote opens an empty PR every week. + out, changes, err := applyFigureEdits(coverage, editsFor("docs/coverage.md")) + if err != nil { + t.Fatalf("%v", err) + } + if out != coverage { + t.Error("applyFigureEdits is not idempotent: a second pass moved a figure the " + + "first pass had just written, so an unchanged sync would not leave a clean " + + "git status") + } + for _, c := range changes { + if c.before != c.after { + t.Errorf("%s reported as moving from %s to %s on a page already holding "+ + "the derived figures", c.label, c.before, c.after) + } + } + + // 5. The README pair: two captures in one loose prose match. Both spans move + // and the sentence around them does not. + for _, rel := range []string{"README.md", "docs/README.md"} { + doc := restore(rel, read(rel)) + if got := restore(rel, + mangleFigure(t, doc, quotedFigurePattern, "1", "2")); got != doc { + t.Errorf("%s: a mangled figure pair was not restored, or the wording around "+ + "it moved with the numbers", rel) + } + } +} + +// TestQuotedFigurePatternReadsThePhrasingsInUse pins what the loose pattern is +// allowed to be loose about. +// +// It reads sentences, so it must survive rewording — and it is also written +// through, so it must not reach into a sentence that merely mentions the words. +// Both halves are asserted here rather than only on the live pages: the live +// pages happen to be unambiguous today, and the guarantee is about tomorrow's. +func TestQuotedFigurePatternReadsThePhrasingsInUse(t *testing.T) { + for _, tc := range []struct { + name string + text string + want []string // nil means "must not match" + whyItMustMatch string + }{ + { + name: "README prose", + text: "- **431 AWS services registered, 426 serving at least one operation** — every", + want: []string{"431", "426"}, + whyItMustMatch: "this is the sentence README.md states today", + }, + { + name: "docs index table cell", + text: "| [Coverage](coverage.md) | 431 registered / 426 serving — the routing and depth targets |", + want: []string{"431", "426"}, + whyItMustMatch: "this is the cell docs/README.md states today", + }, + { + name: "a sentence that only mentions the figures", + text: "Only 3 of the 431 registered services need 2 serving tiers.", + want: nil, + }, + { + name: "a sentence whose second number is unrelated", + text: "All 431 services are registered, and the 3 named below serve nothing, so 12 serving tiers exist.", + want: nil, + }, + } { + t.Run(tc.name, func(t *testing.T) { + m := quotedFigurePattern.FindStringSubmatch(tc.text) + if tc.want == nil { + if m != nil { + t.Errorf("matched %q in a sentence the updater must not write into. "+ + "A gate may mismatch and only fail a test; the updater writes through "+ + "this pattern every Monday, unattended.", m[0]) + } + return + } + if m == nil { + t.Fatalf("matched nothing, but %s. Zero matches disables "+ + "TestOtherDocsQuoteTheSameFigure in silence.", tc.whyItMustMatch) + } + if got := []string{m[1], m[2]}; got[0] != tc.want[0] || got[1] != tc.want[1] { + t.Errorf("captured %v, want %v", got, tc.want) + } + }) + } +} + +// TestUpdaterLeavesUnrelatedSentencesAlone is the same guarantee one layer up. +// +// Refusing to match and refusing to write are both acceptable answers; producing +// a document whose prose moved is not. +func TestUpdaterLeavesUnrelatedSentencesAlone(t *testing.T) { + const doc = "Only 3 of the 431 registered services need 2 serving tiers.\n" + + out, _, err := applyFigureEdits(doc, []figureEdit{{ + path: "README.md", + pattern: quotedFigurePattern, + everyMatch: true, + figures: []figureValue{{"Registered", 431}, {"Serving ≥1 operation", 426}}, + }}) + if err != nil { + return // refusing an unreadable page is the documented, correct outcome + } + if out != doc { + t.Errorf("the updater rewrote a sentence that only mentions the figures:\n"+ + " before: %s after: %s", doc, out) + } +} + +func TestFormatFigureWritesTheFigureTheDocsShow(t *testing.T) { + for _, tc := range []struct { + n int + separated bool + want string + }{ + {431, false, "431"}, + {10871, true, "10,871"}, + {19201, true, "19,201"}, + {999, true, "999"}, + {1000, true, "1,000"}, + {0, true, "0"}, + // A depth target may be set above the fleet, and "outside the serving + // target" is then negative. The separator must not splice into the sign. + {-123, true, "-123"}, + {-1234, true, "-1,234"}, + } { + if got := formatFigure(tc.n, tc.separated); got != tc.want { + t.Errorf("formatFigure(%d, %v) = %q, want %q", tc.n, tc.separated, got, tc.want) + } + } +} + +// TestProseRequiredNamesWhatANumberCannotCarry covers the branch that is the +// whole difference between automating the arithmetic and automating the +// judgement — and that TestUpdatePublishedFigures, being env-gated, never runs +// under `go test ./...`. +func TestProseRequiredNamesWhatANumberCannotCarry(t *testing.T) { + t.Run("a newly silent service the page does not name", func(t *testing.T) { + f := figures{registeredOnly: []string{"examplesvc"}} + blocks := proseRequired(f, "a page that names nothing", nil) + if len(blocks) != 1 { + t.Fatalf("got %d blocks, want 1: a service that serves nothing and is not "+ + "named is exactly the sentence the tool refuses to invent", len(blocks)) + } + if !strings.Contains(blocks[0], "examplesvc") { + t.Errorf("the block never names the service:\n%s", blocks[0]) + } + }) + + t.Run("a service the page already names", func(t *testing.T) { + f := figures{registeredOnly: []string{"examplesvc"}} + if blocks := proseRequired(f, "…the examplesvc service serves nothing because…", nil); len(blocks) != 0 { + t.Errorf("got %d blocks, want 0", len(blocks)) + } + }) + + t.Run("a service named in the page's hyphenated spelling", func(t *testing.T) { + f := figures{registeredOnly: []string{"cloudfrontkeyvaluestore"}} + doc := "…`" + hyphenate("cloudfrontkeyvaluestore") + "` serves nothing because…" + if blocks := proseRequired(f, doc, nil); len(blocks) != 0 { + t.Errorf("got %d blocks, want 0; the docs write IDs hyphenated:\n%s", len(blocks), doc) + } + }) + + t.Run("a registered count that moved", func(t *testing.T) { + changes := []figureChange{{label: "Registered", before: "431", after: "432"}} + blocks := proseRequired(figures{}, "", changes) + if len(blocks) != 1 { + t.Fatalf("got %d blocks, want 1", len(blocks)) + } + for _, want := range []string{"431", "432", "blockquote", "Routing target"} { + if !strings.Contains(blocks[0], want) { + t.Errorf("the block never mentions %q:\n%s", want, blocks[0]) + } + } + }) + + t.Run("a registered count that did not move", func(t *testing.T) { + changes := []figureChange{{label: "Registered", before: "431", after: "431"}} + if blocks := proseRequired(figures{}, "", changes); len(blocks) != 0 { + t.Errorf("got %d blocks, want 0: an unchanged figure owes no sentence", len(blocks)) + } + }) +} + +// TestRenderFigureTableListsEveryFigure pins the half of the answer a table that +// only showed movement could not give: "these four did not move". +func TestRenderFigureTableListsEveryFigure(t *testing.T) { + out := renderFigureTable([]figureChange{ + {label: "Registered", before: "431", after: "432"}, + {label: "Serving ≥1 operation", before: "426", after: "426"}, + }) + + if !strings.Contains(out, "**1 of 2 published figures moved.**") { + t.Errorf("the lead sentence does not count the movement:\n%s", out) + } + if !strings.Contains(out, "| Registered | 431 | 432 |") { + t.Errorf("a figure that moved is not shown moving:\n%s", out) + } + if !strings.Contains(out, "| Serving ≥1 operation | 426 | — |") { + t.Errorf("a figure that held is not shown holding with an em-dash:\n%s", out) + } +} diff --git a/cmd/devcloud/sync_test.go b/cmd/devcloud/sync_test.go index c4a6bda..0573b60 100644 --- a/cmd/devcloud/sync_test.go +++ b/cmd/devcloud/sync_test.go @@ -210,3 +210,69 @@ func TestSyncPullRequestBodySummarisesTheChurn(t *testing.T) { assert.Contains(t, prBodyText(t, steps), "model_churn", "the churn summary is produced but never reaches the PR body") } + +// TestSyncPullRequestBodyCarriesTheRederivedFigures is what keeps the cost +// reduction from evaporating in silence. +// +// The published-figure gate fails by design on any sync that moves an operation, +// and the fix used to be a person transcribing numbers. A step now does it — but +// a step whose output never reaches the PR body is a step nobody knows ran, and +// the reviewer is back to re-deriving by hand without being told they need not. +func TestSyncPullRequestBodyCarriesTheRederivedFigures(t *testing.T) { + steps := syncSteps(t) + + figuresIdx := findStep(steps, func(s syncStep) bool { + return strings.Contains(s.Run, "TestUpdatePublishedFigures") + }) + require.NotEqual(t, -1, figuresIdx, + "no step re-derives the published figures, so every sync that moves an "+ + "operation is a hand-transcription again") + + testIdx := findStep(steps, runsGoTest) + require.Less(t, testIdx, figuresIdx, + "the updater's own `run` contains 'go test', so placing it before the test "+ + "step silently repoints runsGoTest — and with it every other gate in this file") + + prIdx := findStep(steps, opensPullRequest) + require.Less(t, figuresIdx, prIdx, + "the figures must be re-derived before the PR is opened, or the PR carries "+ + "the stale ones") + + require.NotEmpty(t, steps[figuresIdx].ID, + "the updater step needs an id before its result can be quoted in the PR body") + assert.Contains(t, prBodyText(t, steps), "steps."+steps[figuresIdx].ID, + "the PR body must state whether re-derivation succeeded, so a PROSE REQUIRED "+ + "result reads as work outstanding rather than as a green sync") +} + +// TestSyncFailsTheJobWhenTheFiguresNeedProse is the other half of that signal. +// +// The updater runs under continue-on-error so the PR still gets opened, which +// means its failure is invisible to the workflow unless something reads the +// outcome back. Writing it into the PR body tells whoever opens the PR; ending +// the job non-zero is what tells the people who never open it, which on a Monday +// cron is everyone. +func TestSyncFailsTheJobWhenTheFiguresNeedProse(t *testing.T) { + steps := syncSteps(t) + + figuresIdx := findStep(steps, func(s syncStep) bool { + return strings.Contains(s.Run, "TestUpdatePublishedFigures") + }) + require.NotEqual(t, -1, figuresIdx, "no step re-derives the published figures") + require.True(t, steps[figuresIdx].ContinueOnError, + "this test exists because the step is continue-on-error; if it no longer is, "+ + "the job fails on its own and this gate should be reconsidered rather than kept") + + failIdx := findStep(steps, func(s syncStep) bool { + return strings.Contains(s.Run, "::error::") + }) + require.NotEqual(t, -1, failIdx, + "no step re-raises a swallowed failure, so the cron reports success every week "+ + "no matter what upstream did") + require.Greater(t, failIdx, figuresIdx, + "the re-raise must come after the step whose outcome it reads") + + assert.Contains(t, steps[failIdx].If, "steps."+steps[figuresIdx].ID+".outcome", + "a PROSE REQUIRED result means docs/coverage.md is wrong until someone writes "+ + "a sentence; if only the test step's outcome is read, that ends the job green") +} diff --git a/docs/contributing.md b/docs/contributing.md index 23f0f81..76397db 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -100,16 +100,28 @@ Three things to check, in order: 1. **Operations added or removed.** These are the only changes that alter what DevCloud serves. Everything else is upstream reshaping traits or docs. -2. **A red `ci` run on the published-figure gate.** Expected, not a defect: new - operations move the fidelity manifest, and `cmd/devcloud/coverage_test.go` - fails until [`docs/coverage.md`](coverage.md) is re-derived. Correct the - figures in the sync PR — never relax the gate to make it pass. +2. **The published figures, already re-derived.** New operations move the + fidelity manifest, so [`docs/coverage.md`](coverage.md) must move with them — + the sync does that arithmetic itself and commits it. Check the before→after + table at the top of the PR body against item 1: the figures should move for + the same reason the operations did. A `Published figures: failure` means the + change needs a sentence the tool will not invent — write it before merging, + and never relax the gate to make it pass. 3. **`codegen-drift` and `compat`.** These must be green on their own. A red `codegen-drift` means the committed output does not match the models; a red `compat` means a real behavioural regression. -The in-job test result is printed at the top of the PR body. A `failure` there -with a clean `codegen-drift` almost always means item 2. +The in-job test result is printed at the top of the PR body, and a `failure` +there with a clean `codegen-drift` almost always means the published-figure gate +fired on an operation that moved — which is the signal to look, not work to do. +The PR's own `ci` runs that gate again against the corrected figures, so it is +green unless something is genuinely wrong. + +Re-derive by hand, for a coverage change that did not come from a sync: + +```bash +DEVCLOUD_UPDATE_DOCS=1 go test ./cmd/devcloud/ -run TestUpdatePublishedFigures +``` ## Adding a New AWS Service diff --git a/docs/coverage.md b/docs/coverage.md index 93d88d5..8c9bb44 100644 --- a/docs/coverage.md +++ b/docs/coverage.md @@ -337,9 +337,13 @@ What the sample does settle is the *shape* of the work. None of the 93 was documentation-only, so no sync can be waved through on the assumption that AWS only reworded things. Thirty-two services gained operations — `ec2` alone gained 46 — which moves the manifest and makes the published-figure gate fail on -purpose. That failure *is* the review: the numbers here have to be re-derived, by -a person, before the sync can merge. The PR body states which operations moved; -re-derive it with `python3 scripts/model_churn.py --upstream`. +purpose. That failure is still what triggers the review; what it no longer costs +is the arithmetic. The sync re-derives every figure on this page and commits the +result into its own pull request, and the body states which ones moved. What a +person still owes is the judgement — whether those operations should have moved — +and any sentence a number cannot carry. Re-derive by hand with +`DEVCLOUD_UPDATE_DOCS=1 go test ./cmd/devcloud/ -run TestUpdatePublishedFigures`, +and read which operations moved with `python3 scripts/model_churn.py --upstream`. ## Reproducing these numbers diff --git a/scripts/model_churn.py b/scripts/model_churn.py index 890df6f..81aba18 100644 --- a/scripts/model_churn.py +++ b/scripts/model_churn.py @@ -264,7 +264,8 @@ def render( lines.append( "An operation added or removed moves the fidelity manifest, so the " "published-figure gate over `docs/coverage.md` is expected to fail. " - "Re-derive the figures in this PR rather than silencing the gate." + "The sync re-derives them into this PR; confirm they moved for the " + "reason the operations did, and never silence the gate." ) else: lines.append("No operation was added or removed by any changed model.") diff --git a/tests/compatibility/_coverage.py b/tests/compatibility/_coverage.py index 9c72fa8..40891d4 100644 --- a/tests/compatibility/_coverage.py +++ b/tests/compatibility/_coverage.py @@ -61,10 +61,20 @@ # registered: a registered service answers locally in AWS's error vocabulary, # which is the whole reason registering something DevCloud cannot serve beats # leaving the call to reach a billed AWS account. -NO_BOTO3_CLIENT = { - "sagemakerruntimehttp2": "botocore has no HTTP/2 SageMaker Runtime client", - "transcribestreaming": "botocore has no streaming Transcribe client", -} +# +# The membership below lives in exclusions.json rather than here, because +# cmd/devcloud/coverage_test.go derives the published Compatibility-tested figure +# from the same subtraction and cannot import botocore to find it out. One file, +# two readers, no second list that someone remembers to update. +EXCLUSIONS_PATH = pathlib.Path(__file__).resolve().parent / "exclusions.json" + + +def _load_exclusions(): + """Return the two exclusion maps, shared with the Go published-figure gate.""" + with EXCLUSIONS_PATH.open() as fh: + data = json.load(fh) + return data["noBoto3Client"], data["unreachableFromBoto3"] + # Registered, counted as serving operations, and reachable by no boto3 caller. # @@ -78,28 +88,19 @@ # the sibling whose route table models its method and path. The one operation # two siblings both model (DELETE /bots/{id}) is still refused rather than # guessed at. -UNREACHABLE_FROM_BOTO3: dict[str, str] = { - # botocore refuses to build or sign the request, so DevCloud is never asked. - # These are not fidelity gaps — no answer DevCloud could give would change - # the outcome — but they are honest subtractions from the published - # compatibility-tested figure, because nothing here exercises the service. - "codecatalyst": ( - "codecatalyst authenticates with a bearer token rather than SigV4, and " - "botocore raises NoAuthTokenError before the request is built" - ), - "cloudfrontkeyvaluestore": ( - "the client resolves its endpoint from a KVS ARN, so botocore raises " - "EndpointResolutionError instead of honouring endpoint_url" - ), - # Reachable in the sense that the request is sent and answered, and - # unreachable in the sense that matters: botocore decodes the reply with - # RpcV2CBORParser, and DevCloud has no CBOR encoder, so even a clean - # decline is read as a corrupt CBOR frame. See docs/coverage.md. - "partnercentralrevenuemeasurement": ( - "smithy.protocols#rpcv2Cbor — botocore parses every answer as CBOR and " - "DevCloud speaks none, so no reply it can send is intelligible" - ), -} +# In codecatalyst and cloudfront-keyvaluestore botocore refuses to build or sign +# the request, so DevCloud is never asked. These are not fidelity gaps — no +# answer DevCloud could give would change the outcome — but they are honest +# subtractions from the published compatibility-tested figure, because nothing +# here exercises the service. +# +# partnercentral-revenue-measurement is reachable in the sense that the request +# is sent and answered, and unreachable in the sense that matters: botocore +# decodes the reply with RpcV2CBORParser, and DevCloud has no CBOR encoder, so +# even a clean decline is read as a corrupt CBOR frame. See docs/coverage.md. +# +# An empty map stays legal: the category outlived its last member once already. +NO_BOTO3_CLIENT, UNREACHABLE_FROM_BOTO3 = _load_exclusions() # Probes botocore will not put on the wire, pinned rather than counted. diff --git a/tests/compatibility/exclusions.json b/tests/compatibility/exclusions.json new file mode 100644 index 0000000..d91a322 --- /dev/null +++ b/tests/compatibility/exclusions.json @@ -0,0 +1,12 @@ +{ + "_comment": "The registered services no boto3 test can exercise, and why. Read by tests/compatibility/_coverage.py and by cmd/devcloud/coverage_test.go, which derives the Compatibility-tested figure docs/coverage.md publishes. Adding an entry lowers that figure; it is a deliberate edit, and two gates fail until the doc moves with it.", + "noBoto3Client": { + "sagemakerruntimehttp2": "botocore has no HTTP/2 SageMaker Runtime client", + "transcribestreaming": "botocore has no streaming Transcribe client" + }, + "unreachableFromBoto3": { + "codecatalyst": "codecatalyst authenticates with a bearer token rather than SigV4, and botocore raises NoAuthTokenError before the request is built", + "cloudfrontkeyvaluestore": "the client resolves its endpoint from a KVS ARN, so botocore raises EndpointResolutionError instead of honouring endpoint_url", + "partnercentralrevenuemeasurement": "smithy.protocols#rpcv2Cbor — botocore parses every answer as CBOR and DevCloud speaks none, so no reply it can send is intelligible" + } +}