fix: the weekly model sync discarded the only changes worth reviewing

.github/workflows/smithy-sync.yml

@@ -48,22 +48,65 @@ jobs:
             echo "changed=true" >> $GITHUB_OUTPUT
           fi
 
+      # continue-on-error, not a gate: this suite includes the published-figure
+      # gate in cmd/devcloud/coverage_test.go, which an upstream model that gains
+      # a single operation is *designed* to fail. Ending the job here would skip
+      # Create Pull Request and discard the refreshed models with the runner —
+      # so the only sync worth reviewing would be the only one nobody ever sees.
+      # The failure is not swallowed; it is re-raised below, after the PR exists.
       - name: Run tests
+        id: tests
         if: steps.changes.outputs.changed == 'true'
+        continue-on-error: true
         run: CGO_ENABLED=0 go test ./... -v
 
+      # 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
+      # reduces it to the question they actually have: which operations moved.
+      - name: Summarise the churn
+        if: steps.changes.outputs.changed == 'true'
+        run: |
+          {
+            echo "Automated weekly sync of AWS Smithy models from \`aws-sdk-go-v2\`."
+            echo
+            echo "**In-job test result: \`${{ steps.tests.outcome }}\`.**"
+            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
+            echo "This PR's own \`ci\`, \`compat\` and \`codegen-drift\` runs are the"
+            echo "gate on merging."
+            echo
+            echo "---"
+            echo
+            python3 scripts/model_churn.py --upstream
+            echo
+            echo "<sub>Summary from \`scripts/model_churn.py\`; re-derive with"
+            echo "\`python3 scripts/model_churn.py --upstream\`.</sub>"
+          } > /tmp/sync-pr-body.md
+          cat /tmp/sync-pr-body.md
+        env:
+          GITHUB_TOKEN: ${{ github.token }}
+
       - name: Create Pull Request
         if: steps.changes.outputs.changed == 'true'
         uses: peter-evans/create-pull-request@5f6978faf089d4d20b00c7766989d076bb2fc7f1 # v8
         with:
           commit-message: "chore: sync Smithy models and regenerate code"
           title: "chore: weekly Smithy model sync"
-          body: |
-            Automated weekly sync of AWS Smithy models from `aws-sdk-go-v2`.
-
-            This PR updates generated code in `internal/generated/` and scaffolds
-            in `internal/services/` based on the latest AWS service models.
-
-            Please review and merge if CI passes.
+          body-path: /tmp/sync-pr-body.md
           branch: smithy-sync/weekly
           delete-branch: true
+
+      # The PR now exists, so the failure can end the job. Without this the cron
+      # 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.
+      - name: Fail if the sync's tests failed
+        if: steps.tests.outcome == 'failure'
+        run: |
+          echo "::error::Smithy sync tests failed; see the PR opened by this run."
+          exit 1

changes/unreleased/Added-20260906-210100.yaml

@@ -0,0 +1,5 @@
+kind: Added
+body: '`scripts/model_churn.py` summarises what a Smithy model sync actually changed — which services gained or lost operations, which models moved only documentation, and how many upstream models are not vendored here. The weekly sync PR is built from it, so a reviewer reads a change rather than a whole-tree regeneration of 194 models. Run it by hand with `python3 scripts/model_churn.py --upstream`'
+time: 2026-09-06T21:01:00.000000+09:00
+custom:
+    Issue: "147"

changes/unreleased/Documentation-20260906-210200.yaml

@@ -0,0 +1,5 @@
+kind: Documentation
+body: 'docs/coverage.md now publishes what keeping up with upstream costs, measured rather than estimated: refreshing all 194 vendored models changed 93 of them, 32 moved an operation, and none was documentation-only. The reading states its own ceiling — the 93 had been vendored 141 days earlier, and the 101 vendored the day before did not move at all, so it is an accumulated backlog and not a weekly rate. docs/contributing.md adds what a reviewer of the weekly sync PR is actually being asked to check'
+time: 2026-09-06T21:02:00.000000+09:00
+custom:
+    Issue: "147"

changes/unreleased/Fixed-20260906-210000.yaml

@@ -0,0 +1,5 @@
+kind: Fixed
+body: 'The weekly Smithy model sync discarded the changes worth reviewing. Its test step ended the job before the pull request was opened, and an upstream model that gains an operation is designed to fail that step — it moves the fidelity manifest and trips the published-figure gate. So a cosmetic sync produced a PR and a semantic one produced a red cron job with nothing attached. The test result is now recorded rather than gating, the PR is always opened when models moved, and the failure is re-raised afterwards so the cron does not silently go green'
+time: 2026-09-06T21:00:00.000000+09:00
+custom:
+    Issue: "147"

cmd/devcloud/sync_test.go

@@ -0,0 +1,211 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gopkg.in/yaml.v3"
+)
+
+// The weekly Smithy sync is the only mechanism that surfaces upstream model
+// churn, and its output is a pull request. These tests gate the shape of that
+// workflow for the same reason coverage_test.go gates docs/coverage.md: the
+// asset is not Go, but a silent regression in it is invisible until the week it
+// matters.
+
+// syncStep is the subset of a GitHub Actions step these tests read. `with`
+// values are not all strings — `delete-branch: true` is a bool — so the map is
+// typed loosely and read as text only where it is read at all.
+type syncStep struct {
+	Name            string         `yaml:"name"`
+	ID              string         `yaml:"id"`
+	Uses            string         `yaml:"uses"`
+	Run             string         `yaml:"run"`
+	If              string         `yaml:"if"`
+	ContinueOnError bool           `yaml:"continue-on-error"`
+	With            map[string]any `yaml:"with"`
+}
+
+type syncWorkflow struct {
+	Jobs map[string]struct {
+		Steps []syncStep `yaml:"steps"`
+	} `yaml:"jobs"`
+}
+
+// syncSteps returns the steps of the sync job in file order.
+func syncSteps(t *testing.T) []syncStep {
+	t.Helper()
+
+	path := filepath.Join(repoRoot(t), ".github", "workflows", "smithy-sync.yml")
+	raw, err := os.ReadFile(path)
+	require.NoError(t, err)
+
+	var wf syncWorkflow
+	require.NoError(t, yaml.Unmarshal(raw, &wf))
+	require.Len(t, wf.Jobs, 1, "smithy-sync.yml is expected to hold exactly one job")
+
+	for _, job := range wf.Jobs {
+		require.NotEmpty(t, job.Steps, "the sync job has no steps")
+		return job.Steps
+	}
+	return nil
+}
+
+// findStep returns the index of the first step matching pred, or -1.
+func findStep(steps []syncStep, pred func(syncStep) bool) int {
+	for i, s := range steps {
+		if pred(s) {
+			return i
+		}
+	}
+	return -1
+}
+
+func runsGoTest(s syncStep) bool { return strings.Contains(s.Run, "go test") }
+
+func opensPullRequest(s syncStep) bool { return strings.Contains(s.Uses, "create-pull-request") }
+
+// prBodyText returns the text that becomes the pull request body.
+//
+// create-pull-request takes the body either inline as `body` or from a file as
+// `body-path`. Both are legitimate, and which one the workflow uses is not a
+// guarantee worth pinning — what the body *says* is. So a `body-path` resolves
+// to the shell of every step that writes to that path, which is where the
+// content actually comes from.
+func prBodyText(t *testing.T, steps []syncStep) string {
+	t.Helper()
+
+	prIdx := findStep(steps, opensPullRequest)
+	require.NotEqual(t, -1, prIdx, "no step opens a pull request")
+	with := steps[prIdx].With
+
+	if body, ok := with["body"].(string); ok {
+		return body
+	}
+
+	path, ok := with["body-path"].(string)
+	require.True(t, ok, "the create-pull-request step supplies neither body nor body-path")
+
+	var b strings.Builder
+	for _, s := range steps[:prIdx] {
+		if strings.Contains(s.Run, path) {
+			b.WriteString(s.Run)
+		}
+	}
+	require.NotEmpty(t, b.String(),
+		"body-path is %q but no earlier step writes to it, so the PR body is empty", path)
+	return b.String()
+}
+
+// TestSyncOpensAPullRequestEvenWhenTestsFail is the reproducer for the defect
+// that made the sustaining cost unmeasurable.
+//
+// A step that fails ends the job, and every step after it is skipped, unless
+// either the failing step is marked continue-on-error or the later step's `if`
+// re-enables it with always(). The sync runs the full Go suite — which includes
+// the published-figure gate in coverage_test.go — before it opens the PR. An
+// upstream model that gains a single operation moves the manifest, fails that
+// gate, and takes the PR with it. The refreshed models are then discarded with
+// the runner, so the one change worth reviewing is the one nobody ever sees.
+//
+// The rule asserted here is GitHub Actions' own: the PR step must be reachable
+// from a failed test step. How that is arranged — continue-on-error on the test
+// or always() on the PR — is left to the workflow.
+func TestSyncOpensAPullRequestEvenWhenTestsFail(t *testing.T) {
+	steps := syncSteps(t)
+
+	testIdx := findStep(steps, runsGoTest)
+	require.NotEqual(t, -1, testIdx,
+		"no step runs 'go test'; if the sync stopped testing, this gate is reading the wrong thing")
+
+	prIdx := findStep(steps, opensPullRequest)
+	require.NotEqual(t, -1, prIdx, "no step opens a pull request")
+	require.Less(t, testIdx, prIdx,
+		"the test step is expected to run before the PR step; reordering them changes what this gate means")
+
+	reachable := steps[testIdx].ContinueOnError || strings.Contains(steps[prIdx].If, "always()")
+	assert.True(t, reachable,
+		"a failing test step ends the job and the PR is never opened, so the refreshed models "+
+			"are discarded with the runner. Mark the test step continue-on-error, or gate the "+
+			"PR step with always(), so a red sync still leaves a reviewable PR.")
+}
+
+// TestSyncStillFailsWhenTestsFail is the other half of the fix, and the reason
+// continue-on-error is not sufficient on its own.
+//
+// Marking the test step continue-on-error makes the PR reachable and makes the
+// job green — a weekly cron that reports success no matter what upstream did.
+// That is the same silent no-op download-smithy-models.sh was fixed to stop
+// doing, traded for the one this milestone removes. So a workflow that swallows
+// the test failure must re-raise it after the PR exists.
+func TestSyncStillFailsWhenTestsFail(t *testing.T) {
+	steps := syncSteps(t)
+
+	testIdx := findStep(steps, runsGoTest)
+	require.NotEqual(t, -1, testIdx)
+
+	if !steps[testIdx].ContinueOnError {
+		t.Skip("the test step is not continue-on-error, so its failure already fails the job")
+	}
+	require.NotEmpty(t, steps[testIdx].ID,
+		"a swallowed failure cannot be re-raised without an id to read it from")
+
+	prIdx := findStep(steps, opensPullRequest)
+	require.NotEqual(t, -1, prIdx, "no step opens a pull request")
+
+	outcome := "steps." + steps[testIdx].ID + ".outcome"
+	reraised := findStep(steps[prIdx+1:], func(s syncStep) bool {
+		return strings.Contains(s.If, outcome) && strings.Contains(s.Run, "exit 1")
+	})
+	assert.NotEqual(t, -1, reraised,
+		"the test failure is swallowed by continue-on-error and never re-raised, so the weekly "+
+			"cron reports success regardless of what upstream changed. Add a step after the PR "+
+			"that reads "+outcome+" and exits non-zero.")
+}
+
+// TestSyncPullRequestBodyReportsTheTestResult keeps the previous guarantee
+// honest. Opening a PR whose tests failed is only an improvement if the body
+// says so; a red PR that reads like a green one invites a merge rather than a
+// review.
+func TestSyncPullRequestBodyReportsTheTestResult(t *testing.T) {
+	steps := syncSteps(t)
+
+	testIdx := findStep(steps, runsGoTest)
+	require.NotEqual(t, -1, testIdx)
+	require.NotEmpty(t, steps[testIdx].ID,
+		"the test step needs an id before its result can be quoted in the PR body")
+
+	assert.Contains(t, prBodyText(t, steps), "steps."+steps[testIdx].ID,
+		"the PR body must state the test result, so a reviewer sees a red sync as red")
+}
+
+// TestSyncPullRequestBodySummarisesTheChurn is the guarantee that makes the PR
+// reviewable rather than merely present.
+//
+// The sync regenerates from all 194 models at once, so its diff is whole-tree
+// whether upstream moved one model or ninety — measured on 2026-09-06, a real
+// refresh changed 93 models and 134 generated files. "Please review" over that
+// is not an action anyone performs. scripts/model_churn.py reduces it to the
+// question a reviewer has, which operations moved, and the body must carry that
+// answer or the reviewer is back to reading the regeneration.
+func TestSyncPullRequestBodySummarisesTheChurn(t *testing.T) {
+	steps := syncSteps(t)
+
+	churnIdx := findStep(steps, func(s syncStep) bool {
+		return strings.Contains(s.Run, "model_churn.py")
+	})
+	require.NotEqual(t, -1, churnIdx,
+		"no step runs scripts/model_churn.py, so the PR cannot say which operations moved")
+
+	prIdx := findStep(steps, opensPullRequest)
+	require.Less(t, churnIdx, prIdx, "the churn summary must be produced before the PR is opened")
+
+	assert.Contains(t, prBodyText(t, steps), "model_churn",
+		"the churn summary is produced but never reaches the PR body")
+}

docs/contributing.md

@@ -65,6 +65,29 @@ make codegen-s3
 - **Generator:** `internal/codegen/generator.go` — produces Go files using templates in `internal/codegen/templates/`
 - **Output:** `internal/generated/{service}/` — types, interface, serializer, deserializer, router, errors, base_provider
 
+### Reviewing the weekly model sync
+
+[`smithy-sync.yml`](../.github/workflows/smithy-sync.yml) refreshes all 194
+vendored models every Monday and opens a pull request. The diff is whole-tree —
+a measured refresh moved 93 models and 134 generated files — so do not try to
+read it. Read the PR body instead: it is generated by `scripts/model_churn.py`
+and lists which services gained or lost operations.
+
+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.
+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.
+
 ## Adding a New AWS Service
 
 1. **Add the Smithy model** — place the JSON model file in `smithy-models/`

docs/coverage.md

@@ -314,6 +314,41 @@ taken on a different day and are not a controlled comparison, so read this as
 readings agree on is the shape: memory is dominated by the runtime and the
 store, not by how many services are registered.
 
+## Keeping up with upstream
+
+The 205 services are vendored from 194 Smithy models, and AWS keeps changing
+them. A [weekly workflow](../.github/workflows/smithy-sync.yml) refreshes all of
+them and opens a pull request. What that review costs was measured once, on
+**2026-09-06**:
+
+| Reading | Value |
+|---|---|
+| Vendored models refreshed | 194 |
+| Models that changed | 93 |
+| Of those, models that added or removed an operation | 32 |
+| Of those, models that changed only documentation | 0 |
+| Net change in known operations | 12,407 to 12,660 |
+| Generated files that moved | 134 |
+| Wall-clock to download 194 models | 1 min 53 s |
+
+**This is one sample, and it is not one week of churn.** The 93 models that
+changed were all vendored on 2026-04-18 — 141 days earlier. The other 101 were
+vendored on 2026-09-05, and not one of them changed. So the reading above is an
+accumulated backlog, and the only measurement at weekly scale is the second
+cohort's: 101 models, one day, zero changes. The weekly rate is still unknown,
+and a figure derived from a single 141-day sample should not be quoted as one.
+
+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 below fail on
+purpose. That failure *is* the review: the numbers on this page have to be
+re-derived, by a person, before the sync can merge.
+
+The sync PR states which operations moved, so that review reads a change rather
+than a regeneration. Re-derive it with
+`python3 scripts/model_churn.py --upstream`.
+
 ## Reproducing these numbers
 
 ```bash