feat(fidelity): per-operation fidelity manifest, plus the three defects it exposed - #126
Merged
Conversation
The parser walked only the service shape's `operations` list, so every operation a model binds through a `resource` shape was invisible to codegen: 285 operations across 5 models, including bedrock at 0 of 101. `internal/services/bedrock/provider.go` already carried the workaround comment "since bedrock router is empty". Collect operations from resources too — `operations`, `collectionOperations` and the lifecycle slots — walking nested resources and visiting each once so a cyclic resource graph terminates. Regenerating adds 109 operations to the CRUD registry (ecs 56, transfer 41, ssoadmin 12) and removes none. Those operations previously returned InvalidAction and are now engine-served, consistent with the other 46 JSON-protocol services; the fidelity manifest declares the tier either way. bedrock and lambda are rest-json, so the engine does not serve them. Recovered per model: bedrock 0->101, lambda 19->85, ecs 12->76, transfer 29->71, sso-admin 67->79.
…d manifest DevCloud serves 104 services three different ways — hand-written providers, the generic CRUD engine, and an honest InvalidAction — but a caller had no way to tell which applied to a given operation. The tiers were defined in prose in docs/crud-engine.md and nowhere else. Generate the manifest at codegen time by joining three mechanical inputs: the Smithy models (the operation universe), the CRUD registry (auto-crud), and each provider's dispatch-case literals (hand-verified), scanned with go/ast and intersected with the model so unrelated switch literals cannot leak in. Precedence is hand-verified > auto-crud > unimplemented, mirroring the runtime, where the engine is reached only when a provider's dispatch falls through. s3, lambda and bedrock route on HTTP method and path rather than an operation name, so they declare their operations explicitly in pathRoutedOps. The universe is model union hand-verified: bedrock serves InvokeModel, which AWS models under bedrock-runtime, and hiding it would understate what DevCloud does. Result: 7,249 operations across 104 services — 4,261 hand-verified, 957 auto-crud, 2,031 unimplemented. Exposed at GET /devcloud/api/fidelity[?service=] (admin API) and via fidelity.Lookup() in Go. TestFidelityManifestCoverage fails the build when an operation carries an undeclared tier, a registered service is missing, a CRUD-registered operation is marked unimplemented, or a service resolves zero hand-verified operations — the signal that a new path-routing provider needs a pathRoutedOps entry.
github-actions
Bot
added
documentation
…e it crud.Handle refuses any non-JSON protocol, so a CRUD registration for a provider that answers Query is dead: the runtime returns InvalidAction while the manifest advertised auto-crud. cloudwatch hit this for all 20 of its auto-crud operations — its Smithy model declares JSON, but the provider serves Query, and the provider is what the runtime follows. Scan the protocol each provider declares alongside its dispatch literals (ScanHandVerified becomes ScanProviders), and drop non-JSON services before generating the CRUD registry. The manifest consumes the filtered set, so registry and manifest stay consistent by construction. TestAutoCRUDIsReachable now fails the build if a service ever declares auto-crud operations while serving a protocol the engine refuses — it reproduced this bug before the fix. cloudwatch's 20 operations move auto-crud -> unimplemented, which is what the runtime already did. 53 lines of dead registration removed. sqs is also dropped from the registry with no tier change: all of its operations are hand-verified.
![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: 00c7b63dbb
ℹ️ 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".
64 services already implement TagResource/UntagResource/ListTagsForResource on the shared TagStore. These three were the stragglers: their tag calls fell through to the generic CRUD engine, which echoes input rather than storing it, so tag -> list did not round-trip. CloudWatch was worse still — it serves Query, which the engine refuses outright, so its tag calls returned InvalidAction. Wire all three onto shared.TagStore. KMS keys tags by the resolved key ARN, so tagging by id, ARN or alias and listing by any of them addresses the same resource. CloudWatch parses the Query form encoding and answers in XML through the existing cwResp helper. Each service gains a boto3 round-trip test (tag -> list -> untag -> list) plus a per-resource isolation test; KMS also rejects tagging an unknown key with NotFoundException. 9 operations move to hand-verified. The manifest picks that up on regeneration with no manual bookkeeping. Also documents the promotion policy: operations move up on request with a use case, not by guesswork. The remaining auto-crud surface is overwhelmingly operational (DynamoDB backups and global tables, KMS custom key stores, CloudWatch Insight Rules, ECR pull-through cache), which no local inner loop calls — while S3, SQS, Lambda, IAM and STS already carry zero auto-crud operations.
skyoo2003
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0c6d4af1b
ℹ️ 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".
…atch CRUD Review of #126 found the manifest describing a surface DevCloud does not have, in both directions. The scan intersected each provider's case literals with its Smithy model. That was doing two jobs at once, and got both wrong: it discarded the 226 operations providers serve beyond their model (dynamodbstreams listed 4 of the 22 it dispatches; acm's UpdateCertificate and bedrock's InvokeModelWithResponseStream were absent), while still admitting 5 strings that are not operations at all, because identitystore switches on "DisplayName" to patch an attribute and pipes switches on "POST" to resolve a path. The docs meanwhile promised a model-union universe, which the code did not implement. Scope the scan instead: read only the keyed switches inside each provider's HandleRequest, keyed by the receiver type Register binds to the service ID, following delegation (sqs branches on protocol, kafka wraps, appsync serves 37 operations from a default clause). Non-dispatch switches then never come into view, so no intersection is needed and iam still splits from sts. Drop the model intersection entirely. Restore the CloudWatch CRUD registrations 36a37b8 removed. That commit read the protocol off the provider, but gateway.DetectProtocol reads it off the request — CloudWatch answers Query for boto3 and falls through to the engine for X-Amz-Target callers, so the filter deleted 17 live operations. TestAutoCRUDIsReachable encoded the same wrong premise; replace it with one that asks the engine directly. Correct the unimplemented tier's documented error while here: it claimed InvalidAction for every service, which is false for the three path-routed providers (s3 405 MethodNotAllowed, lambda 404 ResourceNotFoundException, bedrock 400 UnsupportedOperation). Separately, EventBridge kept a bus's or rule's tags after deletion. ARNs are derived from the name, so recreating one inherited the old tags; delete them with the resource and pin it with two boto3 tests. Manifest: 4,496 hand-verified / 948 auto-crud / 2,031 unimplemented.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14d7d20ea5
ℹ️ 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".
…gs on delete Follow-up review of #126 found two more places where a name-derived ARN lets tags outlive or cross-wire the resource they describe. A rule name is unique per bus — the rules primary key is (name, bus_name, account_id) — but ruleARN ignored the bus, so two same-named rules on different custom buses resolved to one ARN. Tagging either changed the other's tags, and deleting one wiped the survivor's. Qualify custom-bus rule ARNs with the bus name, which is also the shape AWS uses, and leave the default bus bare. CWStore.DeleteAlarms removed the alarms row and left its resource_tags rows, so an alarm recreated under the same name inherited the deleted one's tags. Delete them with the alarm. DevCloud does not hand out AlarmArn yet, so the helper mirrors the ARN a caller constructs from the name; tags filed under another region or account stay out of reach, and the comment says so. Both are pinned by boto3 tests: same-named rules on two buses keep separate tag sets across a delete, and a recreated alarm starts untagged.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a generated fidelity manifest that declares, per operation, whether DevCloud serves it
hand-verified,auto-crud, orunimplemented— so a caller never has to guess whether a green response means an implementation or a plausible fabrication. Building it surfaced two real defects (a codegen parser gap hiding 285 operations, and CRUD registrations the runtime could never serve) and one inconsistency (three services still faking tag storage), all fixed here.Related Issue
Refs
docs/roadmap.md— Phase 1, "promote high-value auto-crud ops to hand-verified fidelity" follow-up. No tracking issue.Changes
Four commits, each self-contained:
fix(codegen): parse operations bound to Smithyresourceshapes. The parser walked onlyservice.operations, so operations bound through resources were invisible — 285 across 5 models: bedrock 0→101, lambda 19→85, ecs 12→76, transfer 29→71, sso-admin 67→79.internal/services/bedrock/provider.goalready carried the workaround comment "since bedrock router is empty".feat(fidelity): the generated manifest atinternal/generated/fidelity, joining Smithy models (universe), the CRUD registry (auto-crud), and each provider's dispatch-case literals scanned withgo/astand intersected with the model (hand-verified). Exposed atGET /devcloud/api/fidelity[?service=]and viafidelity.Lookup().fix(fidelity): stop declaringauto-crudon providers that cannot serve it.crud.Handlerefuses non-JSON protocols, so a CRUD registration for a Query provider is dead. CloudWatch hit this for all 20 of itsauto-crudoperations — its model declares JSON, its provider answers Query, and the runtime follows the provider. The manifest was advertising coverage the runtime answered withInvalidAction.feat(tags): persist tags for KMS, CloudWatch and EventBridge. 64 services already do this on the sharedTagStore; these three fell through to the CRUD engine, which echoes input rather than storing it, sotag → listdid not round-trip. CloudWatch's tag calls returnedInvalidActionoutright.Coverage: 7,249 operations across 104 services — 4,270
hand-verified, 931auto-crud, 2,048unimplemented.Two things worth a reviewer's attention
InvalidActionand are now engine-served — consistent with the other JSON-protocol services, and the manifest declares the tier either way. Happy to gate this differently if you'd rather they stayInvalidAction.s3,lambda,bedrock), not an operation name, so they declare their operations inpathRoutedOps. Each entry was cross-checked against the Smithy model; the sole exception isbedrock'sInvokeModel, which AWS models under bedrock-runtime. The universe is therefore model ∪ hand-verified so a served operation is never hidden.On promotion scope
docs/fidelity-manifest.mdnow states that promotion happens on request with a use case, not by maintainer guesswork. The manifest is what makes that defensible: reading the remainingauto-crudsurface shows it is overwhelmingly operational — DynamoDB backups and global tables, KMS custom key stores, CloudWatch Insight Rules, ECR pull-through cache — while the operations a local inner loop actually calls (S3, SQS, Lambda, IAM, STS) already carry zeroauto-crudoperations. The tag fix in commit 4 is the one gap the data did justify closing.Test Plan
go test ./...— green, including new tests ininternal/codegen(dispatch scanner, protocol scanner, parser resource walk, cycle termination) andcmd/devcloud(manifest coverage, auto-crud reachability).make test-compat— 771 passed (was 764 before the tag tests, 729 in stale docs;docs/services-matrix.mdcorrected).golangci-lint run— 0 issues.Tier("bogus")failsTestFidelityManifestCoverage;TestAutoCRUDIsReachablereproduced the CloudWatch defect before commit 3 fixed it.make codegentwice leaves the tree clean.admin.enabled: true: summary,?service=s3detail (PutObject: hand-verified,SelectObjectContent: unimplemented), 404 for unknown service.The manifest tests are the guard that matters: the build fails if an operation carries an undeclared tier, a registered service is missing, a CRUD-registered operation is marked
unimplemented, a service resolves zerohand-verifiedoperations (a new path-routing provider), or a service declaresauto-crudwhile serving a protocol the engine refuses.Checklist
golangci-lint run)changie new, see docs/release.md) — or N/A (docs/tests/chore only)