Queue: snapshot data-loss revert, three accepted-then-ignored params, and a diff-scoped lint gate - #2417
Draft
agbishop wants to merge 48 commits into
Draft
Queue: snapshot data-loss revert, three accepted-then-ignored params, and a diff-scoped lint gate#2417agbishop wants to merge 48 commits into
agbishop wants to merge 48 commits into
Conversation
…y persisted snapshot apigatewaySnapshotVersion went 1 -> 2 in d39bf33 alongside a purely additive `Tags *tags.Tags json:"tags,omitempty"` on the nested stageSnapshot. An older snapshot still decodes as the current shape with Tags zero-valued, so the bump bought nothing — but Restore discards on any version mismatch, resetting the registry and all nine dirty tables. Every instance with a persisted apigateway snapshot would lose its state on the first start after that commit. TestSnapshotVersionGuard did not catch it. The guard compared versions only inside branches keyed on the field list changing, so a version-only drift fell through silently — and the drift was real: the source said 2 while the golden still said 1. Split the comparison into a pure diffSnapshots function and give it a default branch, so "version bumped, fields unchanged" is a violation that must be confirmed rather than absorbed by the next -update run. The two apigateway restore fixtures pinned "version":2 literally; they now pin 1 and once again exercise the real restore path instead of the discard path. Closes gopherstack-qviw Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ly launching fewer InMemoryBackend.RunInstances clamped count to 1000 and carried on, so a direct backend caller — services/cloudformation/resources_ec2.go and tests, which do not pass through the handler's rejection — asked for more instances than it got and was told the call succeeded. That is the "parameter accepted then quietly ignored" class. It now returns an error, matching what an HTTP caller already saw. The pre-existing count < 1 -> 1 default stays: absent MinCount really does default to 1. The bound was also reported under the wrong error. It is gopherstack's own allocation-safety cap for CodeQL go/uncontrolled-allocation-size (alert #253), not an AWS quota — real EC2 has no flat per-request instance limit. Returning InvalidParameterValue framed it as a malformed request. AWS documents ResourceCountExceeded for exactly this situation: "You have exceeded the number of resources allowed for this request; for example, if you try to launch more instances than AWS allows in a single request. This limit is separate from your individual resource limit." EC2 models no typed exceptions in the SDK, so the code is verified against the API error-code reference and cited in errors.go, following the ErrOutpostArnNotFound precedent. Renamed the constant to maxInstancesPerRunInstancesRequest so it stops reading as an AWS quota. The outpost path still reserves the full constant rather than the requested count. A guard-then-use of count was empirically not recognised by CodeQL in this codebase (gopherstack-17sl), and reopening the alert is worse than a fixed ~16KB reservation, so count is kept out of the make() size argument entirely. Refs gopherstack-x6r7 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…going unlinted Every per-change gate in this repo has been scoped to a fixed directory — agents run "golangci-lint run ./services/<svc>/...", the orchestrator ran the same, and "go vet ." only covers the root. Nothing in that set covers test/, so a govet shadow in test/integration/datasync_test.go reached a commit and was only caught by CI's repo-wide run at merge time. scripts/lint-changed.sh resolves the actual diff to package directories and lints exactly those: the working tree (staged, unstaged and untracked) unioned with commits on this branch since it diverged from origin/main. Either half alone misses a real case — verifying before committing needs the working-tree diff, verifying at commit time needs the branch diff. It prints the package list it checked and names anything it skipped. Silent truncation is the exact failure this gate exists to prevent, so a large diff is batched rather than dropped, and every batch folds into the exit status. Closes gopherstack-a8b5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
updateLocationNfsInput did not declare ServerHostname at all, so a client changing an NFS location's hostname was told the update succeeded while LocationUri kept pointing at the old server. UpdateLocationNfsInput models the member (aws-sdk-go-v2/service/datasync@v1.61.4 api_op_UpdateLocationNfs.go:48). The URI is now rebuilt in the same shape CreateLocationNfs produces (locations_nfs.go:30, nfs://host/subdir with the leading slash trimmed), using the stored subdirectory when the hostname changes alone — so a hostname-only update cannot blank the path. Refs gopherstack-pz2v Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…that does not exist CreateJob checked only for an empty name and a duplicate, then stored whatever DatasetName, ProjectName and RecipeReference it was given — so a job could be created pointing at nothing and the call reported success. CreateProfileJob and CreateRecipeJob both document ResourceNotFoundException (aws-sdk-go-v2/service/databrew@v1.42.4 deserializers.go:465 and :960). Each reference is checked only when non-empty, because CreateRecipeJobInput accepts ProjectName as an alternative to DatasetName plus RecipeReference, so an unset reference is legal. CreateProject is deliberately untouched: its error switch (deserializers.go:626-638) has no ResourceNotFoundException case, so its unvalidated behaviour is correct. Validation runs before anything is written, so a rejected call leaves no job behind. 29 existing tests created jobs against never-created datasets, recipes and projects — behaviour the real service rejects. They now create the referenced resource first and exercise the valid path rather than asserting the gap. Closes gopherstack-gvdm Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e against IDs that do not exist Both operations accepted a reference to a resource that was never created and reported success, leaving a bundle or image pointing at nothing. Both document ResourceNotFoundException (aws-sdk-go-v2/service/workspaces@v1.73.1, awsAwsjson11_deserializeOpErrorCreateWorkspaceBundle and ...CreateWorkspaceImage), and the validator pattern and error values were already established in this service by d0b7241. CreateWorkspaceImage was worse than unvalidated: it took workspaceId as `_ /*workspaceId*/` and discarded it outright, though the handler had been threading it through all along. The parameter is now named and checked. CreateWorkspaceImageOutput and the WorkspaceImage type carry no source-workspace field, so an existence check is the whole correct scope — there is nothing to derive from the workspace. Both checks run before nextID, so a rejected call consumes no identifier and writes nothing. The tests prove that directly rather than by inspection: they create a resource, attempt a rejected create, create a second resource, and assert the second ID's counter is exactly one past the first. Nine existing tests created bundles and images against IDs like wsi-00000001 that were never created. They now create the referenced resource first. Closes gopherstack-e5pd Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ually does The guardduty pass (ca27323) changed several operations' status but left PARITY.md untouched, so the audit record understated the service. Each claim was re-verified against current code before being written down, not copied from the commit message: the malware-scan filters are genuinely applied (malware_scan_filter.go:37, called from both DescribeMalwareScans and ListMalwareScans), ListMembers genuinely filters on onlyAssociated (members.go:172), and all eight member operations genuinely check the detector. GetRemainingFreeTrialDays stays graded partial rather than ok. It now computes a real value under the shape the SDK models — AccountFreeTrialInfo has no top-level freeTrialDaysRemaining, only features[].freeTrialDaysRemaining (types.go:1817) — but features[] can only ever report the three always-on base sources, because no per-member feature-enablement state exists to read. Three implemented operations had no ops-table row at all (ListMalwareScans, GetMemberDetectors, UpdateMemberDetectors); that is the +3 in the operations badge, not new work. The pagination gap is now stated precisely, naming the ten plain-GET List operations that accept MaxResults/NextToken and emit neither. ListCoverage's filter is recorded as a gap and deliberately not implemented: nothing holds coverage-resource state, so a filter over a permanently-empty list would read as working while doing nothing. Regenerates the READMEs for this and the PARITY.md edits in the preceding commits. Closes gopherstack-8up3 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…pi and ReimportApi basepath=prepend was implemented in d39bf33 but nothing asserted what it does to the resulting route keys, so the behaviour could regress silently — and its absence from the tests is why a later review misread the mode as accepted-then- ignored. Covers ignore, prepend, split and the empty default against a spec declaring a /v1 base path and a spec declaring none, for both operations, asserting the resulting route key rather than the status code. split is asserted to behave as ignore, which is its documented state: the SDK models only the enum values and defers the semantics to prose, so implementing it would mean guessing at client-observable routing. Refs gopherstack-jni0 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… dropping ServerHostname The two siblings of the NFS fix in 6098648. Neither update-input struct declared ServerHostname, so a hostname change reported success while LocationUri kept pointing at the old server. Both members exist in the SDK (aws-sdk-go-v2/service/datasync@v1.61.4 api_op_UpdateLocationSmb.go:117 and api_op_UpdateLocationObjectStorage.go:100); AWS shipped the capability on all three location types at once (CHANGELOG.md:268), NFS was just filed first. Each URI is rebuilt in the shape its own Create produces, which differ: smb://host/subdir (locations_smb.go:32) against object-storage://host/bucket/subdir (locations_objectstorage.go:31). Bucket is preserved from stored state rather than re-derived, since UpdateLocationObjectStorageInput has no BucketName member. A hostname-only update leaves subdirectory and bucket intact. PARITY.md recorded both operations as "wire: fixed ... FIXED this sweep" while this member was missing — a doc asserting a parity that did not hold. Both rows now say what is actually true. UpdateLocationObjectStorage crossed cyclop's limit at 16 once the hostname branch was added; split into updateObjectStorageFields and updateObjectStorageSecretConfig rather than annotated. Closes gopherstack-2xhy Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
📊 Code Coverage Report
Tip This project maintains a minimum coverage threshold of 85%. Maintain or improve coverage on new code to ensure long-term stability. Last updated: Tue, 11 Aug 2026 21:26:21 GMT |
…Image against images that do not exist The siblings left out of 973aa01. Both take a SourceImageId that was never checked, and both document ResourceNotFoundException (aws-sdk-go-v2/service/workspaces@v1.73.1 deserializers.go:772 and :1636). CreateUpdatedWorkspaceImage is validated unconditionally — same account and region, no complication. CopyWorkspaceImage is validated only when SourceRegion is empty or matches this backend's own region. This service instantiates one InMemoryBackend per (account, region) (provider.go:26-28), b.images is a flat table, and storedImage carries no region field — so a genuine cross-region copy's source image lives in a backend instance this one cannot see. Rejecting it would make gopherstack more restrictive than real AWS, which is the worse bug. The cross-region path stays deliberately unvalidated and a test pins that as a choice rather than an oversight. sourceRegion had been discarded as `_ /*sourceRegion*/` despite the interface naming it; it is now threaded through and used. Both checks run before createImageLocked, so a rejected call consumes no identifier — asserted via the shared nextID counter advancing by exactly one across a rejected attempt. One existing test was passing for the wrong reason: TestDescribeImageAssociations_Validation asserts a missing AssociatedResourceTypes is rejected, but its ImageId came from an unvalidated copy that would now fail, so the assertion could have held on an empty ImageId instead. It creates a real source image first. Closes gopherstack-plmb Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ot a bare identifier
entryLineRe required `[A-Za-z0-9_]+` for the key, but real family keys name
several operations at once or carry a parenthetical — AddPermission/RemovePermission,
Database/TableMetadata (Get/List), Create/UpdateConfigurationTemplate response
shape. Every one of those was skipped without a word, so README family and
operation totals undercounted. The operations badge moves 6111 -> 6163; none of
that is new work, it is documentation that was already written and not being
read.
The key class now also accepts '/', '()', '-' and space, while keeping the
`:\s*\{` anchor that does the real disambiguating. Widening was checked against
every `<prefix>: {` occurrence in services/*/PARITY.md: 165 additional distinct
keys match, all of them legitimate names, and nothing that previously failed to
match as an entry now matches spuriously.
The silence was the actual defect, so a looser possibleEntryRe now detects
lines that look like entries but do not parse, and gendocs logs each with its
file and line. Sixteen such lines exist today — keys using commas, '*' or '->'
that are deliberately outside the parsing charset. They were invisible before
and are now reported on every run.
Warnings are non-fatal on purpose. ParseParityFile's contract is to degrade
gracefully rather than error, and CI's docs job already fails on any generated
diff, so a hard exit here would turn prose formatting in a PARITY.md note into
a blocking gate.
Closes gopherstack-udc7
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
29d3136 made these visible: fifteen entry keys used commas, '*' or '->', which the parser deliberately excludes because accepting them would let it match wrapped note prose containing ": {" and invent entries. They logged a warning on every run but were still missing from the ops and family totals. Fixed by renaming the keys rather than loosening the parser — commas become slashes, '->' becomes "to", 'Describe*DetectionJob' becomes 'DescribeDetectionJob-family'. Where the key was carrying an enumeration, it moves into the note: iam's five-operation list is now 'tag-cleanup-on-delete (5 resource kinds)' with the operations named in the note text, so nothing a reader relies on is lost. No status token changed — the added and removed wire/errors/state/persist/status values are identical. This is a naming change only. Fourteen of the sixteen warnings are gone. The remaining one is a false positive and is left alone: services/rds/PARITY.md's 'leaks' family entry is well-formed, but 'leaks' is also a reserved top-level key (parser.go:57), so matchEntry rejects it and warnUnparsedEntry reports it. Filed separately. Closes gopherstack-42va Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… was run The 2026-07-25 audit diffed against v1.51.11 while go.mod pins v1.56.4, so "every wired field diffed" was true of the wrong SDK. Re-derived the gap from the pinned version rather than trusting the issue's list, and by diffing the two SDK versions' member sets directly: it is exactly six fields, no more. Two have a real input member to source a value from, so they are modelled on the domain type and echoed exactly as supplied, never defaulted when absent: ServerlessCache.NetworkType (CreateServerlessCacheInput.NetworkType, serializers.go:6709 — create-only, no Modify member) and ReplicationGroup.Durability (Create serializers.go:6506, Modify :8171). The other four have no input member anywhere — StorageEncryptionType is KMS-key-state-derived, EffectiveDurability is resolved server-side from engine and cluster mode, and Snapshot.Durability comes from a source replication group this model does not track. They are present on the wire structs as omitempty and deliberately never populated. A fabricated encryption type or durability a client can read and act on is worse than an absent field; this follows the FullEngineVersion precedent already set here. The wire tests assert on the raw XML rather than the SDK-parsed value, so a field that serialises as an empty element instead of being omitted is caught — a parsed zero value looks identical either way. elasticacheSnapshotVersion stays at 1. Both new domain fields are additive omitempty on structs that persist whole, and bumping for an additive field discards every persisted snapshot (see cb188a8 earlier in this branch). Closes gopherstack-31dm Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ticache fields Operations badge 6163 -> 6169: fourteen family entries that the parser was skipping on a key-charset technicality now count, plus the elasticache additions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…guration The audit that claimed CreateScheduledQuery and GetScheduledQuery modelled the full DestinationConfiguration was run against v1.80.0 while go.mod pins v1.81.1, which added LookupTableConfiguration as an alternative to S3Configuration (types.go:778, type at :1561). All five members are client-supplied — roleArn and tableName required, description, kmsKeyId and tags optional — so every one is stored and echoed verbatim; nothing here needed modelling shape-only. S3Configuration is genuinely no longer required: validateDestinationConfiguration (validators.go:2451) recurses into whichever member is non-nil and never checks that at least one is set. A config with neither is accepted, and a test pins that rather than leaving us stricter than the real API. The three operations that carry the destination — CreateScheduledQuery, GetScheduledQuery, ListScheduledQueries — pass the struct through whole, so adding the field was sufficient. UpdateScheduledQuery is untouched: the real input is a full replace including DestinationConfiguration while this backend only accepts state, which is a separate pre-existing gap already tracked. cwlSnapshotVersion stays at 1 — the field is additive omitempty and old snapshots decode with it absent. Closes gopherstack-09o8 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er the audit Both audits ran against a stale sdk_module pin, so "every wired field diffed" was true of the wrong SDK. mediatailor: AdsPersonalizationConcurrency and AdsPersonalizationTimeouts (api_op_PutPlaybackConfiguration.go:58 and :63) fell outside extractExtraConfig's fixed fourteen-key allowlist and were silently discarded, which falsified the round-trip fidelity claim outright. Rather than adding two keys to the list, the list is inverted: extractExtraConfig now passes through everything except the four members the handler reads by name. That closes the recurrence class — the next sub-config AWS adds survives without touching this file. It is a small change only because these sub-configs were already stored as decoded-JSON pass-through rather than typed structs. The tradeoff is that an unrecognised key now round-trips instead of being dropped. Real MediaTailor would ignore it, so this is slightly over-permissive — but a client using the AWS SDK can only serialise modelled members, so it is reachable only by a hand-rolled HTTP caller, and silently eating fields the SDK does model is the worse failure. No test pins the unknown-key behaviour, so this stays a judgement call rather than something entrenched. DualStackPlaybackEndpointPrefix and DualStackSessionInitializationEndpointPrefix (types.go:1049,1053) are response-only with no input member. They are modelled on the struct and never populated — an invented endpoint prefix a client might actually dial is worse than an absent field — so PutPlaybackConfiguration stays wire: partial rather than being claimed whole. mediaconvert: MaximumConcurrentFeeds (api_op_CreateQueue.go:47) is threaded through Create and Update. No equivalent mechanism fix applies there — createQueueInput and updateQueueInput are hand-modelled typed structs, so every accepted field must be declared and there is no allowlist to invert. Neither snapshot version constant is bumped; both stay at 1. Refs gopherstack-gt9o Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…aconvert parity updates Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eserved word services/rds/PARITY.md has both a genuine top-level `leaks:` section at column 0 and an indented `leaks:` family entry. matchEntry rejected any line whose key was reserved regardless of indent, while isBlockTerminator only accepts a match at column 0 — so the indented entry fell through both, counted as neither, and was dropped from the families total. Since 29d3136 it also produced a warning on every run, about a line that is not malformed. matchEntry now rejects a line only when isBlockTerminator would claim it. That ties the two functions together by construction, so no line can fall through both, and it generalises: the same collision was waiting for any service naming a family `gaps`, `protocol` or `gaps`-adjacent. Indentation is the only workable discriminator here. The obvious alternative — that a section header carries no brace on its own line — is false: rds's real `leaks:` header is written `leaks: {status: ..., note: "..."}`, brace-identical to a family entry. A 0-space entry whose key is reserved is still read as that key's section header. At column 0 the two forms are genuinely indistinguishable, and parseFrontmatter re-reads the line as the scalar field, so the content becomes LeaksStatus rather than being lost. The existing tolerance for 0-space entries with non-reserved keys (services/mwaa, services/rekognition) is unaffected — isReservedKey never applied to those. Families across all 159 PARITY.md files go 1016 -> 1017, and the false warning count goes 1 -> 0. Closes gopherstack-jw5s Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…difyClientProperties wiping the others Continues the stale-pin sweep. Both services were audited against an older SDK than go.mod pins — the cache still holds ssoadmin@v1.38.0 and workspaces@v1.68.3 alongside the pinned v1.43.1 and v1.73.1, and the fields in question do not exist in the older ones. The workspaces half turned up a bug the issue did not mention: ModifyClientProperties replaced the whole stored struct on every call, so setting one property silently cleared every other. The real operation is a partial update. It now merges, leaving an omitted field at its previous value. ClientExperiencePolicy (types.go:269) and LogUploadEnabled (:275) are both threaded; the latter was unwired too. ClientExperiencePolicy is deliberately unvalidated: unlike its neighbours LogUploadEnabled and ReconnectEnabled, which have generated enum types with Values(), it is a bare *string with no @enum trait. The FORCE_CLASSIC/FORCE_UI_2026/USER_CHOICE values in its doc comment are illustrative, so rejecting anything else would be stricter than AWS. ssoadmin: PermissionSetsEnabled (api_op_DescribeInstance.go:77) is stored as a *bool, so an instance that never set it stays nil and is omitted rather than reported as a fabricated false. AWS documents that it cannot be disabled once enabled, but that is prose rather than an SDK-pinned constraint, so both values are accepted verbatim. InstanceMetadata.Regions is populated from real AddRegion state via ListRegions. PrimaryRegion is modelled shape-only and never set: nothing in this backend can source it, since RegionMetadata.IsPrimaryRegion is always false here. Neither snapshot version constant is touched — workspaces stays 1, ssoadmin stays 2. workspaces' clientProperties map is pre-existing ephemeral state that was never in backendSnapshot, so the new fields inherit that gap rather than creating one. A round-trip test was written, confirmed to fail against that pre-existing non-persistence, and reverted rather than expanding scope; recorded in PARITY.md instead. Refs gopherstack-gt9o Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oadmin/workspaces fields rds picks up its 'leaks' family row, which the parser had been dropping on a reserved-word collision. Operations badge 6169 -> 6172. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both fields arrived in the SDK after this service was audited (types.go:52 and
:550 at the pinned v1.53.5) and were silently omitted from every response.
Unlike most of this sweep these are not caller-supplied — they are derived from
the org tree gopherstack already models, so leaving them unset would have been
the wrong answer. Getting the format wrong would be worse than omitting them
though, and the Go doc comments pin nothing ("The paths in the organization
where the account exists"), so the format comes from the AWS API Reference
example responses and the published regex, cited in buildPath's comment:
o-<org>/r-<root>/(ou-<id>/)*<ownID>/ — org, root, ancestor OUs top-down, the
resource's own id, trailing slash.
Paths is plural but Organizations is a strict single-parent tree — moving an
account between roots is an error, MoveAccount takes one source and one
destination, and this backend stores a single accountParent. It therefore
always returns exactly one path and never fabricates a second.
Populated on the seven operations that actually return these types, found by
searching for the types rather than trusting the gap note: DescribeAccount,
ListAccounts, ListAccountsForParent, DescribeOrganizationalUnit,
UpdateOrganizationalUnit, ListOrganizationalUnitsForParent and
CreateOrganizationalUnit. ListChildren and ListParents are excluded because
they return summary types that carry no path in real AWS.
The ancestor walk is bounded, so a cyclic or dangling parent chain cannot spin:
it returns no path at all rather than a partial or invented one. That state is
unreachable through the API and only constructible via a corrupted snapshot,
which is how the test builds it.
Nothing new is persisted. Both fields are json:"-" and computed at read time
from state that was already stored, so organizationsSnapshotVersion stays at 1.
Refs gopherstack-gt9o
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NetworkType arrived after this service was audited and was dropped end to end — absent from the inputs, never echoed, error not in the lookup table. DBCluster.NetworkType (types.go:236) is settable on CreateDBCluster (api_op_CreateDBCluster.go:171) and ModifyDBCluster (:136), so it is accepted, stored and echoed. It defaults to IPV4 because the SDK documents that as the default in as many words — "IPV4 – ( the default )" — not because a default seemed reasonable. DBInstance.NetworkType (types.go:764) has no input member on either CreateDBInstance or ModifyDBInstance; the SDK says it is inherited from the DB cluster, so that is where this takes it from rather than inventing an option the API does not offer. NetworkType is a bare *string with no entry in types/enums.go, so any value is accepted. Restricting it to IPV4/DUAL would be stricter than the real API. Two things are deliberately left inert, and both would have been easy to fake: SupportedNetworkTypes on DBSubnetGroup (types.go:945) and OrderableDBInstanceOption (types.go:1291) is modelled on the wire in its real member-wrapped list shape but never populated. Subnets here are opaque ID strings with no CIDR data, and the orderable-options catalog is static, so there is no honest basis to say which network types are supported. A fabricated capability list is worse than an absent one, and a test asserts it is genuinely absent from the XML rather than present and empty. NetworkTypeNotSupportedFault (errors.go:1417) is not added to the error lookup table. Real Neptune raises it when a requested network type conflicts with the subnet group's actual CIDR support — a condition this backend cannot detect. Inventing a rejection so the error had something to raise would be the more-restrictive-than-AWS bug class. neptuneSnapshotVersion stays at 1; the new fields are additive omitempty. Refs gopherstack-gt9o Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ates Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…berately never populated The field arrived after this service was audited (types.go:803, :969, :6079 at the pinned v1.73.4) and was missing from GetAutomationExecution, DescribeAutomationExecutions and DescribeAutomationStepExecutions. It is modelled and left permanently unset, which is the honest outcome rather than a shortcut. Real SSM sets it when its engine detects a non-critical issue mid-run; StepExecution's doc adds "Present only if the step status includes a warning". There is no such status in the enum, so it is engine-detected, not a modelled transition. This backend has nothing to detect: completeAutomationLocked drives every step to Success unconditionally, and automationStatusFailed is declared in store.go but never assigned anywhere — there is no failure, timeout, retry or degraded path to report a warning from. Inventing a warning string would put text in front of an operator that no real condition produced. Same call as apigatewayv2's failOnWarnings on this branch, which is validated but documented as inert because the emulator generates no import warnings. The test asserts the field is genuinely absent from the raw response body, not merely empty when parsed — those are indistinguishable through the SDK, and omitempty is the only thing separating them. ssmSnapshotVersion stays at 1. Refs gopherstack-gt9o Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…PC config Both fields arrived after this service was audited and were read nowhere. Connector.IpAddressType (types.go:720) is set by CreateConnector (api_op_CreateConnector.go:86) and UpdateConnector (:80), and echoed on DescribeConnector. WebAppVpcConfig (:2745) and UpdateWebAppVpcConfig (:2648) carry their own, set through Create/UpdateWebApp's EndpointDetails. Two absences here are real AWS behaviour and are deliberately preserved, with tests pinning them so a later pass does not "fix" them into existence: DescribedWebAppVpcConfig (types.go:1417) has no IpAddressType and no deserializer case for one, so a client sets it and cannot read it back. The describe output is untouched. This is the same asymmetry PARITY.md already records for SecurityGroupIds. ListedConnector (types.go:1897) carries only Arn, ConnectorId and Url, so ListConnectors keeps omitting the field. Neither enum is validated. Both are IPV4/DUALSTACK, but the sibling Server.IPAddressType — the same enum shape — is threaded through servers.go with no validation, while EndpointType, Domain and TLSSessionResumptionMode in that same file do validate. Following the established local precedent for this exact shape rather than inventing strictness AWS may not have. The web-app value is stored despite never being echoed: it round-trips through Snapshot/Restore and is readable from the backend struct, matching how SecurityGroupIDs is already handled here. transferSnapshotVersion stays at 1. Refs gopherstack-gt9o Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous checkpoint described chore/parity-upgrade, which has since merged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # .beads/issues.jsonl
…t add git refuses any `git add` naming a path that matches an exclude pattern, regardless of whether the path is already tracked. bd's auto-export hook runs `git add .beads`, so it failed on every create/close, and resolving a merge conflict in issues.jsonl needed -f. Narrow the pattern to .beads/* with a negation for issues.jsonl. Directories are pruned whole, so embeddeddolt/ (88M) and backup/ (53M) stay out. Closes gopherstack-nejg
Closes gopherstack-nejg Closes gopherstack-ky42
…ings Refs gopherstack-7rq1
…count The outpost path kept a maxInstancesPerRunInstancesRequest-sized capacity hint, reserving ~16KB for a one-instance request. The non-outpost path at store.go:956 already solved this: make([]*Instance, 0) with no hint and //nolint:prealloc, keeping count out of the make() size so CodeQL alert 253 (go/uncontrolled-allocation-size) stays closed. Mirror that here. Extract the ID-minting loop into newOutpostReservedInstanceIDs. The new test asserts cap(ids) <= count*4, which fails against the old fixed-1000 code for any count under 250. Closes gopherstack-2vgi
…efix on the wire Shape-only, deliberately never populated, following b4f91c2. Gopherstack has no real dual-stack endpoint, and a fabricated dialable URL is worse than an absent field. Also correct the PARITY.md gap entry: GetHlsManifestConfiguration does not exist in mediatailor v1.63.4 (48 ops, no such operation), and there is no separate SessionInitializationEndpoint type carrying its own dual-stack prefix - that field appears once, on PlaybackConfiguration, already covered by gt9o. Closes gopherstack-ic73
…ations All six take a required ResourceId member (workspaces v1.73.1, serializers.go 8368/8423/8442/8461/8480/8499). Gopherstack read DirectoryId, so every real client's identifier was dropped and the call looked for a key no client sends. The tests asserted DirectoryId too, so they enshrined the bug instead of catching it. Not a blanket rename: ModifyEndpointEncryptionMode really does take DirectoryId, and ModifyClientProperties already read ResourceId. Both verified and left alone. Also wire ModifyCertificateBasedAuthProperties.PropertiesToDelete, which acts on the same persisted ds.Properties map the set path already writes. TestDirectoryModifyOps_RejectsLegacyDirectoryIdKey sends only the legacy key against a registered directory and expects 404, so a revert fails the suite. Closes gopherstack-7rq1
Refs gopherstack-9q6f
… read All four were non-functional against a real client payload. sesv2 CreateExportJob/CreateImportJob took a flat DataSource string where the API requires a nested ExportDataSource/ImportDataSource struct, and dropped the required ExportDestination/ImportDestination entirely. awsconfig Delete/PutRemediationExceptions read an invented ResourceGroupName and had no field for the required ResourceKeys list. RemediationExceptionResourceKey gets its own type rather than reusing the existing ResourceKey: the two look identical but serialize with different casing (PascalCase vs lowerCamelCase, serializers.go 7686 vs 7875), so sharing one would have reintroduced exactly the casing bug this sweep is chasing. Error codes come from each op's own deserializer switch, not from habit. Neither remediation op declares ValidationException, so Delete treats empty input as a documented no-op and Put uses InvalidParameterValueException. Fields with no engine behind them (export dimensions/metrics, S3 fetch, DescribeConfigRules EvaluationMode) are modeled and accepted but left inert and documented as such. Closes gopherstack-rcmn Closes gopherstack-m0ow
ec2 CreateVolume read vals.Get("KmsKeyID"); the wire key is KmsKeyId
(v1.319.1 serializers.go:73594). url.Values is an exact-string map, so every
customer-managed key silently fell back to alias/aws/ebs. A grep for the same
Go-identifier-vs-wire-key conflation across ec2 found no other instance;
ModifyEbsDefaultKmsKeyId already reads it correctly.
iam ChangePassword never read the required OldPassword at all, so any caller
could change the password with no proof of the current one. PasswordPolicyViolation
is chosen from the op's own declared error set (deserializers.go:766-816);
InvalidUserType is the root-credentials case, and NoSuchEntity and
EntityTemporarilyUnmodifiable do not fit.
The backend has no per-request caller identity, so this tracks one
account-wide password rather than a per-user one.
Closes gopherstack-9q6f
s3 CreateBucket parsed only LocationConstraint, discarding the client's initial Tags (v1.106.5 types.go:923, serialized as Tags>Tag children of the request root). They now land on the same StoredBucket.Tags that PutBucketTagging already uses. cloudfront ListDistributionTenantsByCustomization read WebACLArn from the query string, but its HTTP-bindings serializer returns nil - all four members travel in the XML body. Verification also turned up a worse bug behind it: the route matched GET distribution-tenants/by-customization while the SDK sends POST /2020-05-31/distribution-tenants-by-customization, so the operation 404'd NoSuchOperation for every real client. Both fixed; CertificateArn filtering and Marker/MaxItems pagination implemented. rds Add/RemoveRoleFromDBInstance ignored the required FeatureName, so two roles for different feature slots collapsed onto one another. instanceRoles is now keyed instance -> feature -> role. That last change makes the persisted shape incompatible ([]string cannot decode as map[string]string), so rdsSnapshotVersion goes 1->2. Note the guard discards ALL rds state on mismatch, not just this field - unavoidable here, unlike the additive-field bump reverted in cb188a8. The cluster-level role ops are deliberately untouched: FeatureName is optional there, not required. Recorded in PARITY.md. Closes gopherstack-difi Closes gopherstack-i101
… dropped A dropped filter is worse than an unimplemented one: the call returned 200 with unfiltered results, so a client could not tell. These now actually narrow and reorder rather than merely parsing. dms: 14/14 candidates confirmed real, 13 wired. Reuses the service's existing filterEntry/extractFilterValue convention rather than a new one. The seven metadata-model Describe ops share one helper. DescribeReplicationTableStatistics is left inert and documented - ReplicationTableStatistics is always empty here, so filtering it is a no-op by construction. ce: the audit claimed Filter and SortBy across 9 ops with the same shape. Only Filter held for all 9. SortBy is []SortDefinition on three, *SortDefinition on three, and does not exist at all on GetSavingsPlansPurchaseRecommendation, GetReservationPurchaseRecommendation or GetCostComparisonDrivers - no SortBy was added to those. GetTags, GetSavingsPlansCoverage sort and GetCostComparisonDrivers are wired but genuinely inert (nothing populates CostEntry.Tags; single-item lists; no comparison engine) and documented as such rather than faked. Closes gopherstack-o53q Closes gopherstack-a8y0
… an outposts subtest sagemaker ListAssociations declared an anonymous inline struct carrying four of eleven real members. The audit found six absent; verification found a seventh, SourceType. The struct is now named, which also makes it visible to the wire-field tooling (gopherstack-oc9v). All seven filter, sort or paginate for real - proven against narrowed and reordered result sets, not just parsed. athena StartSession gains MonitoringConfiguration and its three nested logging blocks, round-tripped through GetSession. fsx CreateFileSystemFromBackup gains FileSystemTypeVersion, falling back to the source file system's version. FileSystemType stays absent: it is derived from the backup, not requestable. ecs DiscoverPollEndpoint read clusterArn/containerInstanceArn; the real keys are cluster/containerInstance (serializers.go:10302, lowerCamelCase, unlike the ARN-bearing output fields alongside them). Still inert - the handler discards its input - fixed so a future change that consults it is not silently broken. The outposts ConnectionLifecycle/get skip cited gopherstack-vpoh, which is fixed. Unskipped; the integration slice runs 57/57 with the subtest passing. No MatchPriority was touched. Closes gopherstack-cgq3 Closes gopherstack-h0x1 Closes gopherstack-vh89
…e-struct blind spot
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.
Eleven commits off the follow-up queue. Every issue was spot-checked against live code before an agent was spent on it, which turned out to matter — two of the queued issues were already fixed.
Fixes
apigateway snapshot version — data loss (
cb188a8a7).apigatewaySnapshotVersionwent 1→2 in d39bf33 alongside a purely additiveTags *tags.Tags json:"tags,omitempty"on the nestedstageSnapshot. An older snapshot still decodes fine withTagszero-valued, so the bump bought nothing — butRestorediscards on any version mismatch, resetting the registry and all nine dirty tables. Every instance with a persisted apigateway snapshot would have lost its state on the first start after that commit.TestSnapshotVersionGuarddid not catch it: version comparison lived only inside branches keyed on the field list changing, so version-only drift fell through silently — and the drift was real, source at 2 while the golden still said 1. Split into a purediffSnapshotswith adefault:branch, so this now fails loudly instead of riding along on the next-update.Two apigateway fixtures pinned
"version":2literally. With the constant at 2 they passed — through the discard path, not the restore path. They now pin 1 and exercise the real one.ec2 RunInstances silent clamp (
e44858734). The backend clamped count to 1000 and carried on, so cloudformation and tests calling it directly got fewer instances than requested and were told it succeeded. Now errors. The bound was also reported asInvalidParameterValue, framing gopherstack's own allocation-safety cap (CodeQL alert #253) as a malformed request; AWS documentsResourceCountExceededfor exactly this — "more instances than AWS allows in a single request... separate from your individual resource limit". EC2 models no typed exceptions in the SDK, so the code is verified against the API error-code reference and cited inerrors.go.datasync ServerHostname, all three location types (
609864859,4983d442e). NFS, then SMB and ObjectStorage.ServerHostnamewasn't declared at all, so a hostname change reported success whileLocationUrikept pointing at the old server. Each URI is rebuilt in the shape its own Create produces — these differ (nfs://host/subdir,smb://host/subdir,object-storage://host/bucket/subdir), and bucket is preserved from stored state sinceUpdateLocationObjectStorageInputhas noBucketNamemember. AWS shipped the capability on all three at once (SDK CHANGELOG:268). The SMB and ObjectStorage PARITY rows had claimedwire: fixed ... FIXED this sweepwhile the member was missing.databrew CreateJob (
f735a8a3e) and workspaces image ops (973aa011e,b4682808b). Both accepted references to resources that were never created. Validation runs before any write, so a rejected call leaves nothing behind — the workspaces tests prove that directly by asserting the ID counter advances by exactly one across a rejected create, rather than arguing it from code order.CreateWorkspaceImagewas worse than unvalidated: it tookworkspaceIdas_ /*workspaceId*/and discarded it, though the handler had been threading it through all along.CopyWorkspaceImageis the one deliberately left partly open: this service runs one backend per (account, region),b.imagesis flat andstoredImagehas no region field, so a genuine cross-region copy's source lives somewhere this instance cannot see. It validates only whenSourceRegionis empty or matches; rejecting cross-region would be more restrictive than AWS. A test pins that as a choice.Existing tests across the two services created resources against IDs that were never created — asserting behaviour the real services reject. They now create the referenced resource first rather than having the fix weakened around them.
Tooling
make lint-changed(c3d844000). Every per-change gate here has been scoped to a fixed directory, so nothing coveredtest/— which is how agovetshadow intest/integration/datasync_test.goreached a commit and was only caught by CI's repo-wide run at merge time. The new gate resolves the actual diff to package directories: working tree unioned with branch-vs-merge-base, since verifying before committing and verifying at commit time need different halves. Verified by reintroducing that exact shadow — caught, exit 1.gendocs silently dropped PARITY entries (
29d3136fc).entryLineRerequired a bare identifier for the key, so every family key naming several operations or carrying a parenthetical —AddPermission/RemovePermission,Database/TableMetadata (Get/List)— was skipped without a word. The operations badge moves 6111 → 6163 and 49 generated files change; none of it is new work, it is documentation that was written and not being read. Widening was checked against every<prefix>: {inservices/*/PARITY.md: 165 additional distinct keys match, all legitimate, nothing spurious.The silence was the real defect. A looser detector now reports entry-like lines that fail to parse, with file and line. Sixteen exist today (commas,
*,->) and were previously invisible; filed asgopherstack-42va. Warnings are non-fatal on purpose —ParseParityFilepromises graceful degradation, and CI's docs job already fails on generated diff.Docs
guardduty PARITY.md (
3ab51d46a). Each status claim was re-verified against current code before being recorded, not copied from the commit message.GetRemainingFreeTrialDaysstays gradedpartial, notok— it computes a real value under the right shape, butfeatures[]can only report the three always-on base sources. Three implemented operations had no ops-table row at all; that's the +3 in the operations badge (6108→6111), not new work.ListCoverage's filter is recorded as a gap and deliberately not built — nothing holds coverage-resource state, so it would filter a permanently-empty list and read as working.apigatewayv2 basepath transforms (
572c89ee9). Test-only.prependhad no assertion on the resulting route keys, which is how a review misread it as accepted-then-ignored. Now covers all four modes against a spec with a/v1base path and one with none, for both operations, asserting route keys rather than status codes.Gates
go build ./...go vet ./...golangci-lint run ./...go test ./...make check-pinsmake docs+ regentest/terraformThe terraform suite times out locally as one process (25m, zero
--- FAILlines — it panics on the timer with cases still mid-flight). CI shards it 8×15m, so a single local run is roughly 8× a CI chunk. CI settled it:terraform-tests, all fourintegration-testsshards, all fourunit-testsshards,lint,e2e-tests,modernize,govulncheckandcodeql (go)all passed. The local timeout was machine capacity, not a regression.Queue triage
gopherstack-66dr(route53resolverFilters) closed with no code change — already fully implemented in the same PR the follow-up was filed against.gopherstack-jni0narrowed rather than closed. My first pass on this was wrong: I greppedvalidateBasepath, saw only validation, and reported thatbasepathwas accepted then ignored. It is not —prependis implemented inapplyOpenAPIToAPI(handler_apis.go:322-324) and applied by both ImportApi and ReimportApi. Onlysplitfalls back toignore, and that was already documented honestly. It stays unimplemented deliberately: the SDK models the enum values but defers the semantics to prose, so building it would mean guessing at client-observable routing. The route-key transforms for all four modes are now pinned by tests soprependcan't regress silently.Filed
gopherstack-2vgi(ec2 outpost fixed reservation — no local CodeQL to prove a tighter shape),gopherstack-42va(16 PARITY keys with commas/*/->that still don't parse, now at least warned about).Both
gopherstack-7xcwandgopherstack-plmbwere filed and then fixed in this same PR.Two commit trailers name issue IDs that do not exist —
4983d442esaysCloses gopherstack-2xhy. I misreadbd createoutput and invented the ID; the real issue isgopherstack-7xcw, closed correctly. Recording it here rather than rewriting pushed history.Needs a human decision
gopherstack-ylyb— during PR #2414 a subagent dismissed CodeQL alert 254 viagh api PATCHwithout being asked. The SRP reasoning holds:xis a transient protocol intermediate, only the verifier persists, and a slow KDF would structurally break every real-SDK login. Butv = g^x mod Nis stored at rest, so a store leak plus known salt/pool/username permits an offline dictionary attack — the KDF-hardness CodeQL asks for is precisely what SRP lacks. That makes the honest label "true positive, unfixable without breaking the emulated protocol" rather than "false positive". No alert state was touched during this review.🤖 Generated with Claude Code