Skip to content

fix: repair AWS service onboarding and measure its cost #136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
skyoo2003 merged 5 commits into from
Sep 5, 2026
Merged

fix: repair AWS service onboarding and measure its cost #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
56e1867
test: add reproducer for imports.go regeneration emitting an empty im…
skyoo2003 Sep 5, 2026
9a53664
fix: generate-imports.sh emitted an empty import block
skyoo2003 Sep 5, 2026
4881772
fix: scaffolded services served nothing while the manifest called the…
skyoo2003 Sep 5, 2026
113ac22
test: add generic service smoke coverage and measure onboarding cost
skyoo2003 Sep 5, 2026
bad2b42
docs: add changelog fragments and TDD evidence report
skyoo2003 Sep 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 192 additions & 0 deletions .claude/plans/aws-service-coverage-100.plan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
# Plan: Repeatable AWS Service Onboarding

**Source PRD**: `.claude/prds/aws-service-coverage-100.prd.md`
**Selected Milestone**: 1 — Onboarding cost is known
**Complexity**: Medium (small diffs; the measurement, not the code, is the deliverable)

## Summary

The service-onboarding pipeline already exists end to end — per-service model
download, a codegen scaffolder that writes a self-registering provider and
refuses to clobber a hand-written one, config that auto-enables every registered
service, and a main loop that needs no edit per service. Milestone 1 is therefore
a **repair-and-measure** job, not a build-a-tool job.

Two defects make the existing pipeline unusable and one makes its output dishonest:
the import generator gates on a file no service has, so it empties `imports.go`;
the scaffolded provider declines in a way that never reaches the CRUD engine, so
a new service serves zero operations; and the fidelity manifest would publish
those zero-op services as `auto-crud` anyway. Fixing all three is under 30 lines.
The remaining work is removing the one genuinely repetitive step (a hand-written
boto3 fixture per service — there are already 104) and then running the whole
path on one real service with a stopwatch.

## Findings That Shape the Plan

| Finding | Evidence | Consequence |
|---|---|---|
| **`generate-imports.sh` skips every service dir.** It requires `register.go`; zero of 103 services have one — they register from an `init()` inside `provider.go`. | `scripts/generate-imports.sh:19`; `ls internal/services/*/register.go` → no matches; 103 dirs vs 103 entries in the committed `cmd/devcloud/imports.go` | Running the script today writes an **empty import block**. The weekly sync workflow runs it (`.github/workflows/smithy-sync.yml:37`) before its test step, so that job has been failing every week rather than opening sync PRs. No new service can be reached until this is fixed. |
| **The scaffold declines the wrong way.** It returns `generated.ErrNotImplemented`; the CRUD fallback is reached only on `plugin.ErrUnhandledOp`. | `internal/codegen/templates/scaffold_provider.go.tmpl:31` vs `internal/gateway/router.go:61` | A freshly scaffolded service serves **zero** operations and cannot meet the PRD's quality floor as generated. One-line fix. |
| **The fidelity manifest does not check engine wiring.** An op is labelled `auto-crud` from the model-derived CRUD registry alone. | `internal/codegen/gen_fidelity.go:95-104` (`tierFor` consults only the `hand` and `crud` maps) | A scaffolded service would be published as `auto-crud` while returning an error at runtime — the manifest overstating coverage is the PRD's top-ranked risk, and this is the mechanism. |
| Services are auto-enabled and auto-started; no per-service config or `main.go` edit. | `internal/config/config.go:122` (no `services` block ⇒ all enabled); `cmd/devcloud/main.go:102` iterates `registry.RegisteredServices()` | `docs/contributing.md` step 7 ("Update main.go") is dead. Two of its ten documented steps do not apply. |
| **104 near-identical boto3 fixtures are hand-written**, each a one-line wrapper over a shared helper. | `tests/compatibility/conftest.py:126` (`_make_client`), 104 `*_client(devcloud_server)` definitions | This is the single largest per-service cost and the only step that genuinely scales with 330 services. One parameterized fixture removes it. |
| The scaffolder already refuses to overwrite a hand-written provider. | `internal/codegen/generator.go:65` (`os.Stat` guard, comment states the intent) | Re-running codegen over the whole tree is safe; no quarantine mechanism needs inventing. |
| **338 upstream models are missing locally** (431 upstream vs 93 in-tree). | `comm` over the `aws-sdk-go-v2/aws-models` listing vs `smithy-models/` | Confirms the PRD's ~330 figure. Upstream filenames are hyphenated (`api-gateway`, `app-mesh`) while package names are not — the download script takes the **upstream filename**, and the package name comes from the model's service id. They differ. |

## Patterns to Mirror

| Category | Source | Pattern |
|---|---|---|
| Generated file | `cmd/devcloud/imports.go:1` | `// Code generated by <script>. DO NOT EDIT.` header |
| Scaffold safety | `internal/codegen/generator.go:65` | `os.Stat` guard with a comment naming what it protects — never overwrite hand-written code |
| Script failure | `scripts/download-smithy-models.sh:79-88` | Exit non-zero rather than a silent green no-op, with a comment naming the silent failure it prevents |
| Conformance test | `cmd/devcloud/conformance_test.go:24` | Iterate the live registry, `t.Errorf` per violation (not `Fatal`), conservative floor constant with a comment explaining the number |
| Codegen classification | `internal/codegen/gen_crud_meta.go` | Skip rather than guess when confidence is low; sorted iteration for reproducible output |
| Compat test | `tests/compatibility/test_verifiedpermissions.py` | One call per operation, assert `resp["ResponseMetadata"]["HTTPStatusCode"] == 200` |
| Go test framework | 125 `_test.go` files | `stretchr/testify` `assert`/`require`, table-driven |
| Changelog | `changes/unreleased/*.yaml` | Changie entry per change |

## Files to Change

| File | Action | Why |
|---|---|---|
| `scripts/generate-imports.sh` | UPDATE | Gate on the package being a registering service, not on `register.go`. Fail non-zero if the generated list is implausibly short, mirroring the download script's anti-silent-no-op rule. |
| `cmd/devcloud/imports_test.go` | CREATE | Fail the build if `imports.go` drifts from the service directories — so the generator can never silently gut it again |
| `internal/codegen/templates/scaffold_provider.go.tmpl` | UPDATE | Return `plugin.ErrUnhandledOp` so a scaffolded service reaches the CRUD engine; carry `cfg.DataDir` in `Init` like hand-written providers |
| `internal/codegen/scan_handverified.go` | UPDATE | Additionally report, per service, whether its provider is engine-wired (returns `plugin.ErrUnhandledOp`) |
| `internal/codegen/gen_fidelity.go` | UPDATE | Label `auto-crud` only for engine-wired services; otherwise `unimplemented` |
| `internal/codegen/scan_handverified_test.go` + a `gen_fidelity` test | UPDATE / CREATE | Cases: wired service ⇒ `auto-crud`; unwired service ⇒ `unimplemented` |
| `tests/compatibility/conftest.py` | UPDATE | Add one parameterized client fixture over the existing `_make_client`; leave the 104 named fixtures untouched |
| `tests/compatibility/test_service_smoke.py` | CREATE | Per-service smoke test proving the service routes and serves ≥1 operation |
| `docs/contributing.md` | UPDATE | Rewrite "Adding a New AWS Service" to the real (now working) steps; delete the two dead ones |
| `changes/unreleased/Fixed-*.yaml` | CREATE | Changie entries for the import generator and scaffold fixes |

## Tasks

### Task 1: Repair the import generator
- **Action**: Replace the `register.go` existence check with a check that the
package actually registers a service (a `DefaultRegistry.Register(` occurrence
in any `.go` file in the directory). After generation, exit non-zero if fewer
services were written than a conservative floor — an empty or near-empty
`imports.go` must fail loudly, never regenerate green.
- **Mirror**: `scripts/download-smithy-models.sh:79-88` — the closing block that
exits non-zero with a comment naming the silent no-op it prevents.
- **Validate**: `bash scripts/generate-imports.sh && git diff --exit-code cmd/devcloud/imports.go`
— the regenerated file must be byte-identical to what is committed today.
- **Watch**: the current header does not emit the SPDX line the committed file
carries (`scripts/add-spdx-header.sh` adds it separately). Either emit it in
the header or run the SPDX script after, or the idempotency check fails on a
one-line diff.

### Task 2: Lock `imports.go` against drift
- **Action**: A Go test asserting every `internal/services/*` package that
registers a service appears in `imports.go`, and vice versa.
- **Mirror**: `cmd/devcloud/conformance_test.go:24` — per-violation `t.Errorf`,
a floor constant with a comment explaining the number.
- **Validate**: `go test ./cmd/devcloud/...`; delete one line from `imports.go`
and confirm the test fails.

### Task 3: Make the scaffold meet the quality floor
- **Action**: In the scaffold template, return `plugin.ErrUnhandledOp` from
`HandleRequest` instead of `generated.ErrNotImplemented`, and store
`cfg.DataDir` in `Init`. This is what makes a new JSON-protocol service serve
its CRUD-shaped operations the moment it is scaffolded.
- **Mirror**: `docs/crud-engine.md` — "a provider opts in by returning
`plugin.ErrUnhandledOp` from its dispatch `default:` case".
- **Validate**: scaffold one service into a scratch directory, build, start the
server, and confirm a `Create*` call returns 200 rather than an error.
- **Note (corrected during implementation)**: the engine's scope is narrower
than "JSON-family". `crud.JSONProtocol` (`internal/shared/crud/crud.go:71`)
matches only protocols starting with `json`, so **`rest-json` is out too**,
not just Query / EC2 Query / REST-XML. Locally only 39 of 93 models (42%) are
`awsJson1_0` / `awsJson1_1`; `rest-json` adds another 27 that a scaffold
cannot serve. This shrinks Milestone 4's addressable set and is PRD Open
Question 1 — **not** resolved here, see Open Decisions.

### Task 4: Stop the manifest overstating coverage
- **Action**: Extend the provider scan to record whether each service's dispatch
returns `plugin.ErrUnhandledOp`. In `tierFor`, an operation is `auto-crud`
only when its service is engine-wired; otherwise it stays `unimplemented`.
- **Mirror**: `gen_crud_meta.go` — classify only what can be established, skip
rather than guess.
- **Validate**: `make codegen && git diff --stat` — expect **no change** to
today's manifest, since all 46 JSON services are already wired. A non-empty
diff means the wiring detection is wrong, not that the manifest was wrong.
- **Why it matters**: without this, Task 3's fix is unverifiable and every
scaffolded service inflates the published coverage number.

### Task 5: Remove the per-service fixture cost
- **Action**: Add one parameterized boto3 client fixture built on the existing
`_make_client`, plus a smoke test that, for each service under test, issues one
request and asserts the service is routed and serves at least one operation.
Leave the 104 existing named fixtures alone — they back real tests, not
scaffolding, and rewriting them is out of scope.
- **Mirror**: `tests/compatibility/conftest.py:126` (`_make_client`) and the
assertion style of `test_verifiedpermissions.py`.
- **Validate**: `make test-compat` — the existing 775 tests stay green and the
new smoke test passes for at least one scaffolded service.
- **Watch**: the smoke test must fail for a service that routes but serves
nothing. Confirm by pointing it at a service scaffolded *before* Task 3.

### Task 6: Measure the baseline — the actual deliverable
- **Action**: Take one JSON-protocol AWS service that is not currently
registered, run the full repaired path with a stopwatch, and record each
stage: model download, `make codegen` with `-scaffold-output`, import
regeneration, build, smoke test, docs. Record wall-clock, lines of
hand-written code, and anything that required a human decision. Write the
number and the breakdown into the PRD's Success Metrics table.
- **Validate**: a second, different service reproduces the number within a
reasonable margin. One data point is an anecdote.
- **Output**: the per-service cost figure that Milestone 2 sizes against and
that Milestone 3 uses to decide whether 330 services is affordable.

## Validation

```bash
bash scripts/generate-imports.sh && git diff --exit-code cmd/devcloud/imports.go
make codegen && git diff --stat # expect no manifest change from Task 4
make build
CGO_ENABLED=0 go test ./...
make test-compat # 775 boto3 tests must stay green
make stats # service count unchanged until Task 6
make codegen && git diff --exit-code # generation is idempotent
```

## Risks

| Risk | Likelihood | Mitigation |
|---|---|---|
| Task 1 regenerates `imports.go` with a real diff (SPDX line, ordering), masking whether the fix is correct | High | Make byte-identical regeneration the explicit pass condition; resolve the SPDX gap before touching anything else |
| Task 3 makes 338 future services serve plausible-but-empty responses, accelerating the breadth-without-depth risk the PRD ranks first | High | This is the intended behaviour of the chosen quality floor, not a defect — Task 4 is the control that keeps the published number honest. Do not merge Task 3 without Task 4. |
| Task 4's wiring detection misclassifies a provider with an unusual dispatch shape, silently demoting live operations to `unimplemented` | Medium | The no-diff validation over today's 46 wired services is the check; any diff blocks the merge |
| The weekly sync workflow starts opening PRs again and immediately floods with scaffolded services | Medium | Verify what the repaired workflow produces before merging Task 1; the sync scaffolds from `smithy-models/`, which still holds only 93 models, so nothing new appears until models are added deliberately |
| The measured baseline is fast but unrepresentative — an easy service chosen first | Medium | Task 6 requires two services; prefer one small and one large rather than two small |
| Repairing the import generator surfaces that the weekly sync has been failing for a long time, with a backlog of upstream model drift | Medium | Expected. Land the repair first and review the resulting model diff as its own change, not inside this milestone |

## Acceptance

- [ ] All tasks complete
- [ ] `bash scripts/generate-imports.sh` reproduces the committed `imports.go` byte for byte, and fails loudly on an implausibly short result
- [ ] A test fails if `imports.go` drifts from the service directories
- [ ] A freshly scaffolded JSON-protocol service serves its CRUD operations with no hand-written code
- [ ] The fidelity manifest reports `auto-crud` only for engine-wired services; regenerating produces no diff against today's manifest
- [ ] `make test-compat` green; a service that routes but serves nothing fails the smoke test
- [ ] The per-service onboarding cost is a recorded number, reproduced on a second service, written into the PRD
- [ ] Patterns mirrored (generated-file header, loud-failure scripts, conformance-test style), not reinvented

## Open Decisions for the User

1. **Is the broken weekly sync in scope here, or its own fix?** Task 1 repairs
the cause. Whatever upstream model drift accumulated while the job was failing
is a separate, potentially large diff that should not ride along in this
milestone.
2. **Which service is the measurement subject?** The plan needs one small and one
larger JSON-protocol service not currently registered. `accessanalyzer` is a
reasonable first candidate; confirm its protocol after download.
3. **Non-JSON protocols stay unresolved.** The scaffold fix only helps
JSON-family services, so the quality floor remains unreachable for roughly 29%
of the surface. That is PRD Open Question 1 and Milestone 5 — flagged here so
it is not mistaken for something this milestone closes.
4. **Should the 104 hand-written fixtures be consolidated?** The plan says no —
they back real tests and the parameterized fixture only serves new services.
Say so if a consolidation pass is wanted instead.
Loading