docs: publish the v1.0 compatibility policy, and lock the surfaces it promises - #129
Conversation
… promises The only stability policy in the repo guaranteed `ServicePlugin`, `PluginConfig`, `Response`, `Resource` and the `ProtocolType` constants across v1.x. All five live in `internal/plugin`, which Go forbids importing from another module — so the sole documented v1.0 guarantee applied to nobody. docs/compatibility-policy.md states the promise for the surfaces a user is actually exposed to: config keys, environment variables, the CLI, admin API response keys, fidelity tier names, and wire behaviour. The wire guarantee is scoped to hand-verified operations covered by tests/compatibility/ — that suite, 775 tests run in CI and again against the tagged commit, is the enforcement rather than prose. Everything else is listed as explicitly not guaranteed, including auto-crud response content and all of `internal/`, which keeps the planned IR and ModelSource refactor unblocked. Two surface-lock tests keep the document honest. The admin ones matter most: the existing tests decode responses into the internal structs, so renaming a JSON tag renames both sides of the assertion and they stay green while every consumer breaks. Decoding into map[string]any and asserting key presence catches it — verified by renaming `resourceCount` and watching only the new test fail. Asserting presence rather than payload equality keeps the additive change the policy permits from failing the build. Also scopes the plugin-api.md guarantee to in-tree contributors, cross-links the policy from the release pre-flight checklist, and corrects the docs index, which still claimed 101 services.
github-actions
Bot
added
documentation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64908017f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review on #129 found two cases where the published compatibility policy states a guarantee the code does not keep. Both are in Config.Service. An empty services block started every registered service. YAML decodes it to a non-nil empty map, and the len() > 0 check read that as "no block at all". An operator who writes an empty block gets 104 services instead of zero. A block is a block; only what it lists runs. DEVCLOUD_SERVICES could only narrow the YAML block, never widen it. With DEVCLOUD_SERVICES=sqs and a block listing only s3, the two filters intersect to the empty set and nothing starts at all. docs/configuration.md has said since before this PR that the variable disables everything it does not name "regardless of their enabled setting in YAML" — so the code was the bug, not the prose. It now names the running set outright: it starts a service the block omits, overrides a block entry the file disabled, and still takes that service's data_dir from the block. The existing TestGuaranteedEnvSurface masked the second case by naming only s3, which the fixture's YAML already listed — an intersecting filter passes that. It now names lambda too, which the YAML omits. All three new assertions fail against the previous Service() and pass against this one. Full Go suite green, 775 boto3 compatibility tests green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d2e1a15af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The remaining Codex findings on #129. Each is the policy promising more than the code delivers, and each is fixed by shrinking the promise rather than by pretending the gap is not there. Wire behaviour. "A hand-verified operation covered by a test keeps its response shape" reads as the whole response. The suite does not check whole responses — test_lambda.py::test_create_function asserts FunctionName and the presence of FunctionArn, so dropping Runtime, Handler or MemorySize from CreateFunction stays green. The promise is now the asserted fields, which is exactly what a failing build defends, with that example spelled out. Unimplemented errors. The manifest claimed JSON and Query services answer InvalidAction (400). Only the 46 providers that return ErrUnhandledOp reach that fallback in gateway/router.go; 32 answer NotImplemented (501) from their own dispatch default, iot / iotwireless / apigatewayv2 / backup / bedrock / s3 use their own vocabulary, and sqs differs by protocol — Query 501, JSON 400. Verified against a running server: IAM AcceptDelegationRequest returns 501 NotImplemented today, which the published policy would have made a bug. Guaranteed now is only that the call fails and never fabricates a success; the code and status are documented, not promised, so normalizing them later is a minor release. The tier comment is generated, so the fix is in the codegen template with internal/generated regenerated from it. Plugin API. plugin-api.md required a major bump for a breaking ServicePlugin change while the policy reserved the right to restructure all of internal/ freely, leaving no answer for a maintainer holding both documents. Nothing outside this module can import internal/plugin, so the in-tree contract binds contributors, not release versioning; it now says so, and the policy's internal/ exclusion names it rather than silently contradicting it. Full Go suite green, lint clean, 775 boto3 compatibility tests green, codegen drift-free after rm -rf internal/generated && make codegen.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35fe3da4ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three more Codex findings on #129, and the first one is a real bug rather than an over-broad promise. CloudFront's dispatch fallback returned HTTP 200 with `<?xml version="1.0"?>` and nothing else. boto3 parses that as a successful empty result, so GetPublicKey — which the fidelity manifest classifies as unimplemented — looked like it worked and returned nothing. That is true of all 122 CloudFront operations in that tier. A sweep of every provider's dispatch default found this was the only one; the other 32 already fail, and it now joins them with NotImplemented (501). Verified against a running server: GetPublicKey answers 501 where it answered 200, and ListDistributions still answers 200. TestDefaultOperations asserted the old behaviour in as many words — "Unimplemented operations should return 200 with empty XML" — so the test was holding the bug in place. It now asserts the failure, for a modelled operation and an unmatched path both. DEVCLOUD_SERVICES=all. expandTiers returns nil for "all", which Config.Service reads as "no filter", so a YAML block still restricts the running set. Here the code is right and the policy wording was wrong: configuration.md has always defined "all" as switching the filter off. The policy now carves it out instead of claiming the variable always names the set outright. Manifest coverage. The policy said an unclassified operation fails the build. TestFidelityManifestCoverage cannot see an operation that never reached the manifest — it iterates what is there and checks aggregate floors with 1,475 operations of slack. The claim is now what the tests actually enforce, plus the bound that makes the gap small: 93 of 104 services have an in-tree Smithy model, so their operation universe comes from the model and an operation losing its implementation reclassifies rather than vanishes. The 11 without one are reported by modelBacked on the fidelity endpoint. No new test — TestFidelityManifestCoversCRUDRegistry already cross-checks the engine-served half against the runtime registry; its comment now says so, and drops its reference to the InvalidAction claim this PR corrected. Full Go suite green, lint clean, 775 boto3 compatibility tests green, codegen drift-free.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0846de20d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CloudFront TagResource and UntagResource were implemented, classified
hand-verified, and unreachable. Both live at POST /2020-05-31/tagging and are
told apart only by ?Operation=Tag or ?Operation=Untag, but matchURI split the
entire pattern into path segments — so the final segment was the literal
"tagging?Operation=Tag", which no real path segment can equal. Any route
pattern carrying a query string was dead code.
The previous commit turned that from a silent 200 with an empty XML body into
a 501, which made it visible. This makes it work.
matchURI now cuts pattern and request URI at "?" and requires the pattern's
constraint to be satisfied: a bare key must be present ("?WithTags"), a
key=value must be present with that value ("?Operation=Tag"). Because a
constrained pattern is more specific than a bare one and OperationRoutes is
ordered by operation name, MatchOperation makes two passes and tries
constrained routes first — otherwise ".../distribution" would keep shadowing
".../distribution?WithTags". A request without the query still cannot match a
constrained route, so no previously-dead pattern can hijack a working one.
The fix is in the shared template, so all 92 generated routers carry it.
MatchOperation's three callers — cloudfront, efs, route53 — now pass
req.URL.RequestURI() rather than req.URL.Path; for efs and route53 that is a
no-op, since neither service has a query-constrained route.
TestTags passed "TagResource" straight to HandleRequest, so it exercised the
handler and never the routing, which is why an unreachable operation looked
covered. It now goes through doRequest, and
TestMatchOperationHonoursQueryConstraints pins all four CloudFront routes plus
the shadowing case and a non-matching query. Both fail against the old router.
Also narrows the wire guarantee once more: an assertion that only checks a
field is present cannot promise its type or meaning, so the policy now states
the promise per assertion rather than per field, using CreateFunction's two
assertions as the worked example.
Verified end to end against a running server: TagResource returns 204, the tag
reads back, UntagResource returns 204, and GetPublicKey still returns 501.
Full Go suite green, lint clean, 775 boto3 compatibility tests green, codegen
drift-free.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ffee430b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **Error codes, HTTP status and message wording.** What *is* guaranteed for an `unimplemented` | ||
| operation is that it **fails** — an AWS-shaped error, never a fabricated success. Which error |
There was a problem hiding this comment.
This guarantee is already false for Auto Scaling: internal/generated/fidelity/manifest_gen.go:528 classifies DeleteNotificationConfiguration as TierUnimplemented, but an invocation reaches the default branch in internal/services/autoscaling/provider.go:176-180, which returns HTTP 200 with a generic XML response. Fresh evidence beyond the previously identified CloudFront case is that Auto Scaling independently still fabricates success, so either make this provider reject its 18 manifest-unimplemented operations or narrow the guarantee.
Useful? React with 👍 / 👎.
| if c.Services != nil { | ||
| return ServiceConfig{} | ||
| } |
There was a problem hiding this comment.
When the file contains services: {}, this new branch intentionally leaves every service disabled, but cmd/devcloud/main.go:97-101 then unconditionally warns that serving zero services is “never what an operator wanted” and advises enabling entries. Since the same change explicitly documents an empty block as a supported way to run nothing, every deliberate use now produces a misleading operational warning; distinguish an explicit empty map from an accidental empty selection or make the warning neutral.
Useful? React with 👍 / 👎.
1 participant
Summary
The only stability policy in the repo guarantees
ServicePlugin,PluginConfig,Response,Resourceand theProtocolTypeconstants across v1.x — all five live ininternal/plugin, which Go forbids importing from another module, so the sole documented v1.0 guarantee applies to nobody. This PR writes the promise for the surfaces a user is actually exposed to, and adds tests that fail the build when one of them drifts.Milestone 2 of the AWS v1 stabilization work. With this, the only thing left before tagging v1.0 is the tag itself.
Related Issue
Refs #129 — no prior issue; the changelog fragment uses this PR's number, per the convention in #126–#128.
Changes
docs/compatibility-policy.md(new) — what v1.0 guarantees across 1.x, per surface: config keys, environment variables, the CLI, admin API response keys, fidelity tier names, and wire behaviour. Plus what is explicitly not guaranteed, and the deprecation procedure that must precede any removal.tests/compatibility/. That suite — 775 tests, run in CI and again against the tagged commit before a release publishes — is the enforcement, rather than prose backed by review discipline.auto-crudresponse content (948 operations), hand-verified operations with no compat test, store durability, and all ofinternal/are listed as not guaranteed. The last one keeps the planned IR /ModelSourcerefactor unblocked.internal/config/config_test.go—TestGuaranteedConfigSurfaceandTestGuaranteedEnvSurfaceassert every guaranteed key and env var lands on the expected field.internal/admin/api_test.go—TestGuaranteedAdminSurface_{Collections,Fidelity}assert the wire keys of all four routes.map[string]anyon purpose. The existing tests decode into the internal structs (serviceInfo,RequestLog), so renaming a JSON tag renames both sides of the assertion and they stay green while every consumer breaks.docs/plugin-api.md— scopes its existing guarantee to in-tree contributors and points users at the new policy.docs/release.md— resolves the cross-link the pre-flight checklist from ci: enforce the checks a v1.0 tag depends on #127 was written expecting; adds a compatibility-review step.README.md/docs/README.md— index the new doc; corrects the docs index, which still claimed 101 services.Test Plan
CGO_ENABLED=0 go test ./...— all packages pass.golangci-lint run ./internal/config/... ./internal/admin/...— 0 issues.json:"resourceCount"→json:"resource_count"ininternal/admin/api.go→TestGuaranteedAdminSurface_Collectionsfails naming the missing key.TestAPI_Servicesstays green, which is exactly the gap this closes.yaml:"format"→yaml:"fmt"ininternal/config/config.go→TestGuaranteedConfigSurfacefails onlogging.format.changie batch v9.9.9 --dry-run— 29 entries, 0 dead issue links, so the release gate atrelease.yml:225passes.Checklist
golangci-lint run)