feat: per-wrapper partial-failure error mask + v5preview parity

[sean-]

Summary

Replace Config.ReturnQueryErrors (a single boolean) with a 15-bit errmask.ErrorMask bitfield, where each bit corresponds to one wrapper schema in the proposed x-error-responses OpenAPI spec extension. Generators read those annotations, so v5preview/opensearchapi gets the same partial-failure handling that v4 has had as hand-written code. The same branch also reworks discriminated-union decoding (single-pass merge for success|error unions, lazy As<T>() for discriminator-less aggregation/suggest unions) and makes node discovery blocking.

Closes #816 (final piece of the gh-816 burn-down stack: #804 / #815 / #801 already merged).

Why

OpenSearch returns HTTP 200 for partial successes -- bulk item failures, search-shard failures, single-doc replica failures, MSearch sub-response errors -- so callers must remember a second check after err == nil. v4's ReturnQueryErrors bool flipped all partial-failure conversions on or off as one switch; users wanting shard-level errors but tolerating bulk item failures (or vice versa) had no way to express it.

The bitfield gives one bit per wire shape. Setting a bit masks (suppresses) that category; the unset state reports it. errmask.Empty reports everything; errmask.All masks everything.

Lifecycle

Version	Config.Errors == nil default	Behavior
v4 (this PR)	errmask.All	No behavior change for existing callers. Opt in to typed errors with Config.Errors: errmask.New().
v5	errmask.Empty	Every category reported by default. Callers opt out individual bits via Config.Errors or OPENSEARCH_GO_ERROR_MASK.
v6	n/a	Config.Errors and OPENSEARCH_GO_ERROR_MASK removed. Behavior is unconditionally errmask.Empty (report everything).

Usability

The caller surface is a typed error return + a typed response. Both are non-nil on partial failure: the response is fully populated even when the error fires. Config.Errors is a *errmask.ErrorMask; because the named values are constants (not addressable), build the pointer with errmask.New(...).

client, _ := opensearchapi.NewClient(opensearchapi.Config{
    Client: opensearch.Config{Addresses: addrs},
    Errors: errmask.New(), // report every partial-failure category
})

resp, err := client.Bulk(ctx, opensearchapi.BulkReq{Body: body})
switch e := err.(type) {
case nil:
    // every item succeeded
case *opensearchapi.PartialBulkError:
    // some items failed; resp is fully populated
    log.Printf("%d/%d items failed",
        len(e.FailedItems), e.SucceededCount+len(e.FailedItems))
    retryFailedItems(e.FailedItems)
default:
    return err // transport / HTTP / decode error
}

For ops that can fire multiple wrapper categories on a single response (MSearch, MSearchTemplate), the package-level opensearchapi.Errors helper flattens single- and multi-error returns so one switch handles both:

resp, err := client.MSearch(ctx, req)
for _, sub := range opensearchapi.Errors(err) {
    switch e := sub.(type) {
    case *opensearchapi.PartialSearchError:       // shard aggregation
    case *opensearchapi.MultiSearchItemError:     // per-sub-response Error envelope
    default:
        return err
    }
}

opensearchapi.Errors(nil) returns nil; a non-partial err returns a single-element slice. It only flattens recognized partial-failure wrappers, so a joined non-partial error keeps its top-level identity.

err = opensearchapi.RequireSuccessRate(err, 0.99) // nil unless <99% succeeded in EVERY category
err = opensearchapi.ToleratePartialFailures(err)  // nil for any partial failure

What changed

New: public errmask package

• Top-level github.com/opensearch-project/opensearch-go/v4/errmask, importable by external consumers.
• ErrorMask with one bit per wrapper schema: BulkItems, SearchShards, WriteShards, BroadcastShards, NodeFailures, BulkByScrollFailures, TaskFailures, MultiSearchItems, MultiDocItems, SnapshotCreateShardFailures, SnapshotGetShardFailures, SimulateDocFailures, RankEvalFailures, IngestionShardFailures, PitNodeFailures.
• errmask.Empty (zero value), errmask.All, errmask.None/errmask.Unknown aliases for Empty, and New(bits ...ErrorMask) *ErrorMask to build the Config.Errors pointer (New() = report everything).
• Has, canonical String(), and Parse(s, base) accepting comma-separated +/- snake_case tokens (e.g. +all,-bulk_items). Unknown tokens are dropped (forward-compatible) and reported via the debug logger.

v4 opensearchapi/

• Config.Errors *errmask.ErrorMask (pointer disambiguates "use the version's default" from "explicit Empty"). Config.ReturnQueryErrors is removed.
• Resolver merges cfg + OPENSEARCH_GO_ERROR_MASK.
• Typed errors: MultiSearchItemError, MSearchErrors, MSearchTemplateErrors (Go 1.20+ multi-error via Unwrap() []error). RequireSuccessRate evaluates every category a multi-error carries (not just the first match).
• Per-Resp helpers (BulkItemFailures, SearchShardFailures, WriteShardFailures, MultiSearchItemFailures) + PartialFailures(mask).
• Package-level opensearchapi.Errors(err) []error.

v5preview/opensearchapi

• errors.go mirrors v4 using the spec-driven types: BulkRespItem, ShardSearchFailure, ErrorRespBase (embedded in MultiSearchItemFailure).
• api.go wires Config.Errors and clientInit(rootClient, mask).
• Codegen emits per-Resp helpers + PartialFailures(mask); dispatch is trivial, with the per-op container referenced only when 2+ wrapper categories can fire.

Discriminated-union decoding (osgen)

• Merged single-pass decode for all-object unions with a discriminable primary branch (mget/msearch success|error items, indices-open, BulkByScrollTaskStatus | ErrorCause).
• Lazy As<T>() for discriminator-less, caller-keyed map-valued unions (aggregation/suggest result families): raw bytes retained, decoded on demand into the requested type.
• Raw bytes are aliased from the response buffer (no per-item copy); UnmarshalJSON resets prior branch state so reused decode targets are safe.

Transport

• DiscoverNodes is now blocking (concurrent callers wait on the in-flight cycle); added DiscoverNodesOnStart *bool. A waiter no longer inherits the runner's context.Canceled when its own context is healthy.

Spec (opensearch-openapi.yaml)

• 15 wrapper schemas under components.schemas as _common.errors___<WrapperName>; 115 operations annotated with x-error-responses.

cmd/osgen

• spec.go: reads x-error-responses from the OpenAPI extension into apiOperation.ErrorWrappers.
• errwrap: hardcoded fallback catalog for plugin operations not yet annotated upstream.
• ir.Operation.ErrorWrappers carries the resolved wrapper list.
• emit/frag_clients.go: generated Client struct has errors errmask.ErrorMask; clientInit takes the mask.
• emit/frag_dispatch.go: dispatch template simplified to call data.PartialFailures(...) -- per-wrapper logic moved out of the template.
• emit/frag_partial_failure.go (new): emits per-Resp helper methods + PartialFailures(mask) aggregator. Each wrapper has a RenderMethod function plus an Applies predicate that walks the typed response (including embedded structs via the type registry) to confirm the field path the emission references; spec annotations whose typed Resp lacks the matching field are silently skipped (e.g. v5preview's CreateResp has no _shards, msearch's union response item resolves through unionFromResponses to find the success/error branches).
• Mask-bit identifiers and method names derive from the wrapper string itself: errmask.<wrapper> for the bit, singularize(wrapper) + "Failures" for the method. No hand-maintained name maps.

Generated v5preview/opensearchapi

• clients_gen.go carries the new errors errmask.ErrorMask field and clientInit signature.
• 22 dispatch files updated: each declares its per-Resp <Wrapper>Failures() helpers + PartialFailures(mask) aggregator, and the dispatch handler delegates to them.

Migration

Existing v4 callers see no behavior change: zero-config Client{} continues to mask all partial-failure categories (preserving pre-bitfield behavior). Callers who were setting ReturnQueryErrors: true (added earlier in this PR's stack -- never shipped to a release) need to switch to Errors: &errmask.Empty.

// Before (deleted from this PR):
//   Errors: errmask.None,  // or ReturnQueryErrors: true

// After: pointer, with explicit "report everything"
mask := errmask.Empty
client, _ := opensearchapi.NewClient(opensearchapi.Config{
    Client: opensearch.Config{Addresses: addrs},
    Errors: &mask,
})

// Or selectively: report everything except bulk-item failures
mask := errmask.BulkItems
client, _ := opensearchapi.NewClient(opensearchapi.Config{
    Client: opensearch.Config{Addresses: addrs},
    Errors: &mask,
})

Env-var override:

# Mask everything except bulk-item errors
export OPENSEARCH_GO_ERROR_MASK="+all,-bulk_items"

# Only mask search-shard failures
export OPENSEARCH_GO_ERROR_MASK="search_shards"

# Reset to "mask everything" (mimics v4 default)
export OPENSEARCH_GO_ERROR_MASK="all"

# Reset to "report everything" (the v5+ default)
export OPENSEARCH_GO_ERROR_MASK="none"

Follow-ups (not in this PR)

• Spec PR adding x-error-responses upstream so the local patch in opensearch-openapi.yaml can be replaced by a re-bundle.
• Add hand-written detection for the 10 catalog wrappers without RenderMethod emission yet (NodeFailures, BulkByScrollFailures, TaskFailures, MultiDocItems, SnapshotCreateShardFailures, SnapshotGetShardFailures, SimulateDocFailures, RankEvalFailures, IngestionShardFailures, PitNodeFailures).
• Teach v5preview type-gen to surface _shards on CreateResp so the Applies guard can stop skipping that op.
• Enable router by default in v5preview NewClient (separate commit).

Related

• Spec PR: opensearch-project/opensearch-api-specification#1137 (x-error-responses extension proposal)
• Closes the [FEATURE] Operation classifier + partial failure errors (v4/v5 transition) #816 burn-down stack: Fix double-slash URL path bug across 74 GetRequest methods #804 / Add configurable shard cost multipliers via env var and RouterOption #815 / Add adaptive max_concurrent_shard_requests for search routing #801 already merged; this PR finishes the remaining work and closes [FEATURE] Operation classifier + partial failure errors (v4/v5 transition) #816.

[codecov]

Codecov Report

❌ Patch coverage is 77.46914% with 511 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.20%. Comparing base (f591483) to head (5a4d450).

Files with missing lines	Patch %	Lines
v5preview/opensearchapi/clear_scroll_gen.go	0.00%	52 Missing ⚠️
v5preview/opensearchapi/cat-recovery_gen.go	0.00%	50 Missing ⚠️
v5preview/opensearchapi/cat-snapshots_gen.go	0.00%	50 Missing ⚠️
v5preview/opensearchapi/delete_by_query_gen.go	0.00%	45 Missing ⚠️
v5preview/opensearchapi/cluster-remote_info_gen.go	0.00%	44 Missing ⚠️
cmd/osgen/emit/frag_dispatch.go	91.48%	22 Missing and 5 partials ⚠️
...ew/opensearchapi/cluster-allocation_explain_gen.go	0.00%	25 Missing ⚠️
v5preview/opensearchapi/api.go	51.06%	20 Missing and 3 partials ⚠️
v5preview/opensearchapi/cat-count_gen.go	16.00%	21 Missing ⚠️
v5preview/opensearchapi/cat-health_gen.go	16.00%	21 Missing ⚠️
... and 24 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #844      +/-   ##
==========================================
- Coverage   52.52%   47.20%   -5.32%     
==========================================
  Files         990     1000      +10     
  Lines       59059    69758   +10699     
==========================================
+ Hits        31020    32929    +1909     
- Misses      24935    33631    +8696     
- Partials     3104     3198      +94     

Flag	Coverage Δ
integration	29.99% <56.24%> (-3.66%)	⬇️
unit	41.83% <77.02%> (-4.38%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cmd/osgen/api_extract.go	80.27% <100.00%> (+0.82%)	⬆️
cmd/osgen/emit/frag_clients.go	91.37% <100.00%> (+0.15%)	⬆️
cmd/osgen/emit/frag_plugin.go	70.68% <100.00%> (-2.75%)	⬇️
cmd/osgen/ir/types.go	0.00% <ø> (ø)
cmd/osgen/naming.go	89.86% <100.00%> (+1.67%)	⬆️
cmd/osgen/spec.go	100.00% <100.00%> (ø)
errmask/errmask.go	100.00% <100.00%> (ø)
internal/envvars/envvars.go	100.00% <100.00%> (ø)
opensearchapi/api_bulk.go	88.23% <100.00%> (+1.56%)	⬆️
opensearchapi/api_document.go	100.00% <100.00%> (ø)
... and 209 more

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Jakob3xD]

Code review

Thanks for this — the underlying feature (typed partial-failure errors + a version/env-gated on-off migration) is the right direction, and the errmask bitfield itself is cleanly written. The feedback below splits into one design question and a set of verified correctness/doc issues. Inline comments carry findings #2, #3, #4, #7; the rest are below.

---

Design question: is a 15-bit global ErrorMask the right granularity?

Two things should stay regardless of the answer:

1. Partial failures as typed errors (PartialBulkError, PartialSearchError, ShardFailureError) + the helpers (errors.As, ToleratePartialFailures, RequireSuccessRate).
2. The coarse on/off migration gate (env var + v4→v5→v6 default flip).

The concern is only the per-category global mask layer in between:

• Wrong axis. Tolerance for partial failure is almost always per call-site, not per client (the mask is client-global — verified: no per-request override; every dispatch reads c.errors.Has(...)). The same client does a best-effort analytics search (shard failures tolerable) and a critical bulk index (item failures not). A global mask can't express that without two clients. And the per-call mechanism already exists: always return the typed error, let the caller decide with errors.As / ToleratePartialFailures / RequireSuccessRate. The mask is a second, coarser mechanism for the same goal.

• It re-introduces the footgun the feature exists to kill. The stated goal was "partial failures become impossible to silently ignore." A global mask that converts them back to (resp, nil) — set once at construction or via env var — does exactly that, invisibly, across every call site. ToleratePartialFailures(err) at the call site keeps the suppression visible in code review.

• It leaks the spec's internal taxonomy into permanently-stable public API. 15 wire-format wrapper bits each have to stay stable forever and be individually documented/tested.

• It necessitates a parallel codegen path (errwrap.OperationWrappers + per-bit emission running a second spec walk to decide which bit to emit) — and findings #5/#6 below live in exactly that path.

If a one-time org-wide policy is genuinely needed, a small semantic enum (None / TolerateShardFailures / TolerateAll) would serve it without the four problems above. And making detection a PartialFailure() error method keyed off the already-correctly-generated response struct (instead of a parallel error-wrap walk) would remove the bit-selection class of bug entirely.

---

Cross-cutting issues (not anchored to a single line)

1. Docs reference an env var that doesn't exist (high confidence). CHANGELOG.md, USER_GUIDE.md, UPGRADING.md, and guides/error_handling.md document OPENSEARCH_GO_PARTIAL_QUERY_ERRORS (and describe it as a boolean). The shipped code defines only OPENSEARCH_GO_ERROR_MASK (a token list, see internal/envvars/envvars.go). A user following the docs gets no effect.

5. v5preview MSearch never emits a partial-failure check (parity gap). v5preview/opensearchapi/msearch_gen.go emits no errmask/partial check at all, while v4 api_msearch.go does. Root cause appears to be in the generator: elementTypeHasShards resolves the Responses element type (MsearchMultiSearchResultResponsesItem), which is a TypeUnion — its shape is in Branches, not Fields, so responseHasField(..., "Shards", ...) returns false and the check is dropped. MSearch shard failures are never surfaced in v5preview regardless of mask. This defeats the "v5preview parity" goal for MSearch.

6. v5preview Create is missing its WriteShards check (parity gap). v5preview/opensearchapi/create_gen.go Create returns (&data, nil) with no shard-failure check, while index_gen.go has if !c.errors.Has(errmask.WriteShards) && data.Shards.Failed > 0 { ... } and v4 api_document.go Create has the check. Likely root cause: cmd/osgen/api_extract.go hardcodes the "200" response, but create returns only 201 — so CreateResp is built without _shards and the check is suppressed.

---

Additional, lower-confidence observations

• PartialSearchError.Failures type drift in docs: UPGRADING.md / guides/error_handling.md document []ResponseShardsFailure, but v5preview/opensearchapi/errors.go uses []ShardSearchFailure. The two packages diverge; the docs describe them as identical.
• DiscoverNodes() semantics changed (now blocks on an in-flight cycle and returns its error, vs. the previous immediate nil no-op). Likely intentional, but not noted in UPGRADING.md.
• Route interface breaking change: OpID() OperationID was added to the exported opensearchtransport.Route interface — a source break for any external implementer (semver-relevant on a v4 module).
• v5preview/opensearchapi/errors.go has no test coverage for the new helpers/types (the v4 side does).

🤖 Generated with Claude Code

_{If this review was useful, react with 👍. Otherwise react with 👎.}

[sean-]

Thanks for the review.

On the design question: per-call vs per-client mask

You're right that tolerance is mostly a per-call-site concern, and errors.As / ToleratePartialFailures / RequireSuccessRate give callers exactly that lever. The mask isn't replacing them: it's there to set the default for whether partial failures surface at all, which is the v4 → v5 → v6 lifecycle gate. Without it we can't ship "v4 preserves silent semantics; v5 surfaces by default" without per-version forks of the dispatch code. The mask is the single knob that toggles it.

The alternative ("always return typed errors; let the caller decide") is what v5+ defaults to (errmask.Empty). v4 defaults to errmask.All so existing v4 code keeps working unchanged. Both surfaces are the same generated code.

To address the "second mechanism for the same goal" concern, this PR also reworks the surface so per-call inspection happens through methods anchored on the typed *Resp: exactly the PartialFailure() error-style approach you suggested. Each *Resp now exposes BulkItemFailures(), SearchShardFailures(), WriteShardFailures(), MultiSearchItemFailures(), plus a PartialFailures(mask) aggregator. The dispatch handler delegates to the aggregator; callers wanting focused inspection at the call site invoke the per-wrapper methods directly without going through the dispatch error. The per-bit code-gen path you noted as the source of comment 5 and comment 6 are now driven by spec-applicable predicates that walk the typed response (the class of bugs you pointed out), and the codegen emits methods not inline conditions.

Cross-cutting

Comment 1 (env var name in docs): Fixed. OPENSEARCH_GO_PARTIAL_QUERY_ERRORS was the original name; it's been renamed to OPENSEARCH_GO_ERROR_MASK (bitfield grammar) and all docs (UPGRADING.md, USER_GUIDE.md, CHANGELOG.md, guides/error_handling.md) updated to match. The internal/envvars/envvars.go comment that listed the old short tokens (bulk, query, write, all, none) is also fixed to the snake-case wrapper names actually accepted by errmask.Parse.

Comment 5 (v5preview MSearch never emits): Fixed. The walker now resolves the Responses element type, detects when it's a TypeUnion/TypeLazyUnion, and the dispatch fragment emits union-branch dispatch (resp.Type() == ...Type + resp.MsearchMultiSearchItem() / resp.ErrorResponseBase()) instead of trying to read a non-existent Shards field on the union itself. v5preview/opensearchapi/msearch_gen.go now has both SearchShardFailures() (shard aggregation across success-shaped sub-responses) and MultiSearchItemFailures() (error-shaped sub-responses) methods plus the PartialFailures(mask) aggregator.

Comment 6 (v5preview Create missing WriteShards): Fixed. cmd/osgen/api_extract.go walked only the "200" response; now it walks every 2xx (200/201/202/204) so CreateResp picks up _shards from the 201 response. v5preview/opensearchapi/create_gen.go now has the WriteShardFailures() method + PartialFailures(mask) aggregator.

Additional observations

PartialSearchError.Failures type drift: Fixed. UPGRADING.md has separate v4 and v5preview tables that document the divergence (ResponseShardsFailure -> ShardSearchFailure); guides/error_handling.md table now also surfaces both types in the Fields column.

DiscoverNodes() semantics change: Now documented in UPGRADING.md under a dedicated "DiscoverNodes() blocking semantics" section, including the user-controlled flow (DiscoverNodesOnStart: &false, DiscoverNodesInterval: 0, then call manually).

Route interface OpID(): Documented as a breaking change in UPGRADING.md under "opensearchtransport.Route interface gained OpID()" with a code sample showing the new method shape. Built-in routes built via NewRouteMux are populated automatically; only hand-written Route implementations need updating.

v5preview errors.go test coverage: Added v5preview/opensearchapi/partial_failure_methods_test.go (table driven TestRespHelperMethods covering Bulk/Search/Index/Msearch helpers + mask gating; TestNilRespHelperMethods for typed-nil safety; TestPackageErrorsHelper).

[Jakob3xD]

Code review (follow-up)

Re-reviewed at head 3bf7aa9. First: thanks for the thorough rework — I verified every prior finding against the current tree and all are addressed:

• Env-var name (OPENSEARCH_GO_ERROR_MASK) and the errmask token list in internal/envvars/envvars.go are now correct.
• The Config.Errors zero-value trap is resolved via the pointer (*errmask.ErrorMask, nil = unset) — exactly the disambiguation needed.
• v5preview MSearch shard-path and Create WriteShardFailures() parity now generate.
• The WriteShards doc no longer over-claims Bulk replica handling.
• The design concern is resolved: the mask is now scoped to the v4→v5→v6 lifecycle gate, and per-call inspection runs through the typed *Resp methods (PartialFailures(mask) + per-wrapper methods). That's the right split.

The rework introduced new code; the items below are found in that new surface.

---

Found 1 high-confidence bug + 3 medium + several latent.

1. (high) The union decoder routes every error sub-response to the success branch, so MultiSearchItemFailures() / MGet per-doc error detection silently never fires.

The generated union UnmarshalJSON is try-each "first branch that unmarshals without error wins", with the success branch tried first and no discriminator/required-field guard. For MSearch, MSearchMultiSearchItem has no error field and a Status *float64, so an error sub-response {"error":{…},"status":400} unmarshals into it without error (the error key is ignored, 400 fits *float64). The ErrorRespBase branch is therefore unreachable for wire-decoded responses, and MultiSearchItemFailures() (which dispatches on ...ErrorRespBaseType) always returns nil. Net effect: MSearch() / MSearchTemplate() return a nil error even when sub-queries fail, and the MultiSearchItems wrapper is dead end-to-end. The unit test passes only because it builds the union via New…FromErrorRespBase, bypassing UnmarshalJSON. The same pattern breaks per-doc error detection in MGet (GetResult tried before MGetMultiGetError).

Fix: decode the union by discriminator (e.g. presence of the "error" key, or try the error branch first / guard on a required field) rather than first-successful-unmarshal.

Root cause (template):

opensearch-go/cmd/osgen/emit/frag_union.go

Lines 164 to 177 in 3bf7aa9

	func (u *{{$t.Name}}) UnmarshalJSON(data []byte) error {
	u.raw = append(u.raw[:0], data...)
	if len(data) == 0 || bytes.Equal(data, build.NullJSON) {
	return nil
	}
	{{- range $t.Branches}}
	{
	var v {{qualify .GoType}}
	if err := json.Unmarshal(data, &v); err == nil {
	u.typ = {{constName $t.Name .Name}}
	u.value = v
	return nil
	}
	}

Generated symptoms:

opensearch-go/v5preview/opensearchapi/msearch_gen.go

Lines 283 to 305 in 3bf7aa9

	func (u *MSearchMultiSearchResultResponsesItem) UnmarshalJSON(data []byte) error {
	u.raw = append(u.raw[:0], data...)
	if len(data) == 0 || bytes.Equal(data, build.NullJSON) {
	return nil
	}
	{
	var v MSearchMultiSearchItem
	if err := json.Unmarshal(data, &v); err == nil {
	u.typ = MSearchMultiSearchResultResponsesItemMSearchMultiSearchItemType
	u.value = v
	return nil
	}
	}
	{
	var v ErrorRespBase
	if err := json.Unmarshal(data, &v); err == nil {
	u.typ = MSearchMultiSearchResultResponsesItemErrorRespBaseType
	u.value = v
	return nil
	}
	}
	return fmt.Errorf("MSearchMultiSearchResultResponsesItem: no branch matched JSON: %s", data[:min(len(data), 64)])
	}

opensearch-go/v5preview/opensearchapi/msearch_gen.go

Lines 346 to 372 in 3bf7aa9

	func (r *MSearchResp) MultiSearchItemFailures() *MultiSearchItemError {
	if r == nil {
	return nil
	}
	var failed []MultiSearchItemFailure
	succeeded := 0
	for i, resp := range r.Responses {
	if resp.Type() == MSearchMultiSearchResultResponsesItemErrorRespBaseType {
	failed = append(failed, MultiSearchItemFailure{
	Index: i,
	ErrorRespBase: resp.ErrorRespBase(),
	})
	} else {
	succeeded++
	}
	}
	if len(failed) == 0 {
	return nil
	}
	return &MultiSearchItemError{
	Items: failed,
	SucceededCount: succeeded,
	}
	}
	// PartialFailures returns the partial-failure sub-errors detected on the
	// MSearchResp, gated by mask. Mask bits suppress their corresponding

(same union reused in msearch_template_gen.go; same class in mget_gen.go)

---

2. (medium) NewFromClient / NewFromClientWithErrors bypass the v5preview default-router injection, so a client built via these constructors is behaviorally inconsistent with NewClient (no default router) despite the package's "routing on by default" intent.

opensearch-go/v5preview/opensearchapi/api.go

Lines 159 to 168 in 3bf7aa9

	func NewFromClient(client *opensearch.Client) *Client {
	return clientInit(client, resolveErrorMask(Config{}))
	}
	// NewFromClientWithErrors creates an api client from an existing
	// opensearch.Client with every partial-failure category reported as an error.
	func NewFromClientWithErrors(client *opensearch.Client) *Client {
	none := errmask.None
	return clientInit(client, resolveErrorMask(Config{Errors: &none}))
	}

3. (medium) v5preview RequireSuccessRate has no *MultiSearchItemError case, so for an MSearch whose only failures are item-level it skips the threshold and returns the error unchanged (diverges from the v4 helper). Currently masked by bug #1, but becomes live once #1 is fixed.

opensearch-go/v5preview/opensearchapi/errors.go

Lines 297 to 330 in 3bf7aa9

	func RequireSuccessRate(err error, threshold float64) error {
	if err == nil {
	return nil
	}
	var succeeded, total int
	switch e := err.(type) { //nolint:errorlint // unwrapped switch is intentional; falls through to errors.As below
	case *PartialBulkError:
	succeeded = e.SucceededCount
	total = e.SucceededCount + len(e.FailedItems)
	case *PartialSearchError:
	succeeded = e.TotalShards - e.FailedShards
	total = e.TotalShards
	case *ShardFailureError:
	succeeded = e.TotalShards - e.FailedShards
	total = e.TotalShards
	default:
	// Try unwrapping -- the partial error may be wrapped.
	var bulkErr *PartialBulkError
	var searchErr *PartialSearchError
	var shardErr *ShardFailureError
	switch {
	case errors.As(err, &bulkErr):
	succeeded = bulkErr.SucceededCount
	total = bulkErr.SucceededCount + len(bulkErr.FailedItems)
	case errors.As(err, &searchErr):
	succeeded = searchErr.TotalShards - searchErr.FailedShards
	total = searchErr.TotalShards
	case errors.As(err, &shardErr):
	succeeded = shardErr.TotalShards - shardErr.FailedShards
	total = shardErr.TotalShards
	default:
	return err
	}

4. (latent) collapsePerOpErrors(errs, nil) is emitted for every single-wrapper op; if a future spec change adds a second wrapper to such an op without a PerOpErrType, the len(errs) >= 2 path calls the nil wrap and panics. A defensive if wrap != nil guard would close this.

opensearch-go/opensearchapi/errors.go

Lines 193 to 210 in 3bf7aa9

	func collapsePerOpErrors(errs []error, wrap func([]error) error) error {
	switch len(errs) {
	case 0:
	return nil
	case 1:
	return errs[0]
	default:
	return wrap(errs)
	}
	}
	// Errors returns the partial-failure sub-errors carried by err,
	// flattening any per-op multi-error wrapper (e.g. [MSearchErrors]) into
	// a flat slice.
	//
	// Use this from caller code so a single switch/default block handles
	// both the single-sub-error case (collapse rule returned the bare
	// sub-error) and the multi-sub-error case (collapse returned a per-op

5. (latent) PluginMethodName does not apply the idiomatic-abbreviation rewriting that now drives type names (*Req/*Resp). A future plugin op whose suffix contains msearch/mget/termvectors/forcemerge would get e.g. type MSearchFooReq but method MsearchFoo — a mismatch. Unreachable today, but the divergence is live.

opensearch-go/cmd/osgen/emit/frag_plugin.go

Lines 412 to 420 in 3bf7aa9

	func PluginMethodName(suffix string) string {
	parts := strings.FieldsFunc(suffix, func(r rune) bool {
	return r == '_'
	})
	var sb strings.Builder
	for _, p := range parts {
	if len(p) > 0 {
	sb.WriteString(strings.ToUpper(p[:1]) + p[1:])
	}

6. (latent) errwrap fallback catalog for msearch lists only MultiSearchItems, missing SearchShards. Harmless for the core path (spec x-error-responses supplies both), but a plugin/unannotated msearch-like op falling back to errwrap.For("msearch") would silently lose the shard wrapper.

opensearch-go/cmd/osgen/errwrap/errwrap.go

Lines 68 to 72 in 3bf7aa9

	GroupUpdateByQuery = "update_by_query"
	GroupDeleteByQuery = "delete_by_query"
	GroupMSearch = "msearch"
	GroupMSearchTemplate = "msearch_template"
	GroupMGet = "mget"

7. (latent) v4 MSearchResp.SearchShardFailures() accumulates Shards.Total from every sub-response unconditionally (the v5 generated version restricts to the success branch). A server response carrying both error and a non-zero _shards.total would over-count the denominator; current OpenSearch doesn't emit that shape.

opensearch-go/opensearchapi/partial_failure_methods.go

Lines 165 to 180 in 3bf7aa9

	func (r *MSearchResp) SearchShardFailures() *PartialSearchError {
	if r == nil {
	return nil
	}
	var totalShards, failedShards int
	var failures []ResponseShardsFailure
	for _, resp := range r.Responses {
	totalShards += resp.Shards.Total
	failedShards += resp.Shards.Failed
	failures = append(failures, resp.Shards.Failures...)
	}
	if failedShards == 0 {
	return nil
	}
	return &PartialSearchError{
	FailedShards: failedShards,

8. (nit) Stale doc comments in cmd/osgen/api_extract.go still say "the 200 response body" after the change to walk all 2xx; and the outer-loop continue on a non-JSON 2xx would abandon a path's remaining 2xx codes (no current spec path hits this).

opensearch-go/cmd/osgen/api_extract.go

Lines 73 to 81 in 3bf7aa9

	ResponseRef string // schema key for the 200 response body (e.g. "cluster.health___HealthResponseBody")
	// ErrorWrappers lists the partial-failure wrapper-schema names this
	// operation may surface alongside its primary 2xx response. Populated
	// from the x-error-responses extension on the spec operation.
	ErrorWrappers []string
	// ResponseSchemaRef is the resolved schema for the 200 response body,
	// used to walk inline schemas that aren't in Components.Schemas.

🤖 Generated with Claude Code

_{If this review was useful, react with 👍. Otherwise react with 👎.}

[sean-]

Thanks for the re-review, and for verifying the prior round against the tree. All eight items are addressed; details per finding below.

1. (high) Union decoder routed error sub-responses to the success branch

Fixed at the generator template, so it covers msearch, msearch_template, and mget uniformly. The generated union UnmarshalJSON is now a two-pass decode instead of first-successful-unmarshal:

• Pass 1 only considers branches that declare required (discriminator) keys, gated by a key probe. For this union that means the ErrorRespBase branch is tried first, guarded by build.HasJSONKeys(data, "error", "status").
• Pass 2 is the permissive fallback (MSearchMultiSearchItem), tried only when no discriminated branch matched.

So {"error":{...},"status":400} now decodes to ErrorRespBase, and MultiSearchItemFailures() (and the MGet per doc equivalent) fire on wire-decoded responses, not just on values built via the New...FromErrorRespBase constructors.

While wiring this up I also found and fixed a latent bug in the probe itself: build.HasJSONKeys originally unmarshalled into a map[string]struct{}, which makes encoding/json error on any scalar value ({"status":400} cannot decode 400 into struct{}), so the probe returned false for every object carrying scalar fields. It now decodes into map[string]json.RawMessage. Added internal/build/union_test.go as regression coverage, and the integration suite (which decodes real wire payloads, the gap you noted) now exercises the discriminated path end to end.

2. (medium) NewFromClient bypassed default-router injection

Resolved by removing NewFromClient / NewFromClientWithErrors entirely. All construction now flows through NewClient, which centrally applies the router-injection rule (inject the default router unless the caller supplied one or OPENSEARCH_GO_ROUTER=false). There is no longer a constructor that can sidestep it.

3. (medium) v5preview RequireSuccessRate had no *MultiSearchItemError case

Added the case, matching the v4 helper:

case errors.As(err, &msearchErr):
    succeeded = msearchErr.SucceededCount
    total = msearchErr.SucceededCount + len(msearchErr.Items)

4. (latent) collapsePerOpErrors(errs, nil) could nil-wrap and panic

Added the defensive guard you suggested; with no PerOpErrType wrapper it falls back to errors.Join:

default:
    if wrap != nil {
        return wrap(errs)
    }
    return errors.Join(errs...)

5. (latent) PluginMethodName skipped the idiomatic-abbreviation rewrite

Removed PluginMethodName. Plugin method names now come from ir.Operation.MethodName, computed in package main via methodNameFromSuffix(pluginGroupSuffix(group)), which ends in applyIdiomaticAbbreviations -> the same rewrite that drives *Req/*Resp type names. Method and type names share one code path, so a msearch/mget/termvectors/forcemerge plugin op cannot diverge.

6. (latent) errwrap fallback for msearch missing SearchShards

GroupMSearch and GroupMSearchTemplate now both map to {WrapperSearchShards, WrapperMultiSearchItems}, so an unannotated/plugin msearch-like op falling back to the catalog keeps the shard wrapper.

7. (latent) v4 MSearchResp.SearchShardFailures() over-counted the denominator

Both v4 MSearchResp and MSearchTemplateResp SearchShardFailures() now skip error-shaped sub-responses before accumulating shard counts, matching the v5 generated behavior:

for _, resp := range r.Responses {
    if resp.Error != nil {
        continue
    }
    totalShards += resp.Shards.Total
    failedShards += resp.Shards.Failed
    failures = append(failures, resp.Shards.Failures...)
}

8. (nit) Stale doc comments + outer-loop continue in api_extract.go

Doc comments now read "JSON 2xx response body". The JSON-content check moved inside the per-code scan, so a non-JSON 2xx (e.g. a text/plain 200) no longer abandons a path's remaining 2xx codes: the loop keeps scanning and only breaks once it finds a JSON 2xx (e.g. a 201), then resolves the schema from that.

All unit suites (three modules), the full integration suite (both opensearchapi packages plus the plugins), and make lint.local are green. Happy to re-check anything against the pushed head.

[Jakob3xD]

Code review (full pass)

I re-verified the prior two rounds against the current head (94d7dfa): all 8 findings from the last review are addressed, and the design rework (per-*Resp methods + PartialFailures(mask) aggregator, mask reduced to the lifecycle gate) is in place. Thanks for that.

This is a fresh full-PR pass; the items below are new and were cross-checked against the existing comment threads (none are already resolved). Most are posted as inline comments — F1 (internal errmask → the mask is unusable by external consumers) and F3 (RequireSuccessRate short-circuit) are the two worth prioritizing. The remaining low/latent items that don't map to a changed line:

• F7 — perOpErrorTypeName returns MSearchErrors/MSearchTemplateErrors unconditionally for those groups. Correct today (both wrappers are jointly applicable), but if a group ever transitions to a single wrapper while the dispatch still references the per-op type, it's a compile break. The coupling between emittableWrappers and the hardcoded perOpErrorTypeName switch is implicit and untested. (cmd/osgen/emit/frag_dispatch.go, perOpErrorTypeName.)

• F9 — classifyRefBranch omits VersionAdded. Inline union branches read x-version-added, but $ref branches always get VersionAdded: "", so sortBranchesNewestFirst orders them last regardless of actual version. No current spec union has versioned $ref branches, so no impact today. (cmd/osgen/resp_union.go, classifyRefBranch.)

• F10 — GoName collision resolver leaves a stale seenGoName entry. After renaming the underscore-prefixed variant, seenGoName[gn] still points at the old name; a three-way _field/field collision in one schema could emit a duplicate Go field name. No such triple in the current spec. (cmd/osgen/resp_walk.go, ~seenGoName loop.)

• F11 — envvars_test.go "unset" case sets an empty value (t.Setenv(key, "")) instead of truly unsetting, so the LookupEnv ok==false path is never exercised (duplicates the "empty string" row). (internal/envvars/envvars_test.go.)

• F2 (doc nit) — opensearchapi/errors.go L259 doc example reads c.Msearch(ctx, req); should be c.MSearch after the naming regen.

🤖 Generated with Claude Code

_{If this review was useful, react with 👍. Otherwise react with 👎.}

[Jakob3xD]

Follow-up items for a separate cleanup PR

The library code looks good — this list is non-blocking and intended to be addressed in a separate PR. Findings are grouped doc vs codegen; severities are rough (all > 10). Permalinks pinned to 5a4d450.

Docs (user-facing — examples that don't compile / mislead)

1. NewMuxRouter() example won't compile. custom, _ := opensearchtransport.NewMuxRouter() — NewMuxRouter() Router returns a single value, not (Router, error). Drop the , _.
   

   opensearch-go/v5preview/opensearchapi/README.md

   Line 335 in 5a4d450

   custom, _ := opensearchtransport.NewMuxRouter()

2. replace directive in MIGRATING.md doesn't resolve. …/v5/opensearchapi is a package path (module path stops at …/v5), and the RHS …/v4/v5preview/opensearchapi is not a module (no go.mod there). Pasting it yields go: no module provides …. The forward-compat replace trick can't remap a subpath across module trees as written.
   

   opensearch-go/v5preview/opensearchapi/MIGRATING.md

   Line 34 in 5a4d450

   replace github.com/opensearch-project/opensearch-go/v5/opensearchapi => github.com/opensearch-project/opensearch-go/v4/v5preview/opensearchapi v4.7.0

3. *string printed with %s in the v5preview samples. In v5preview, ErrorCause.Reason is *string, so item.Error.Reason passed to %s prints a pointer, not the text (and Reason can be nil). Needs a nil-checked deref. (The v4 sample at L70 is fine — there Reason is string.)
   

   opensearch-go/guides/error_handling.md

   Line 96 in 5a4d450

   item.Error.Type, item.Index, id, item.Error.Reason)

   
   

   opensearch-go/guides/error_handling.md

   Line 281 in 5a4d450

   log.Printf("sub-query %d failed: %s", item.Index, item.Error.Reason)

4. Dead switch cases in the MSearch Errors() example. MSearch only yields *PartialSearchError / *MultiSearchItemError; the *PartialBulkError and *ShardFailureError cases can never match for this op.
   

   opensearch-go/v5preview/opensearchapi/README.md

   Lines 241 to 244 in 5a4d450

   	case *opensearchapi.PartialBulkError:
   	log.Printf("%d items failed", len(e.FailedItems))
   	case *opensearchapi.ShardFailureError:
   	log.Printf("%s: %d/%d shards failed", e.Operation, e.FailedShards, e.TotalShards)

Code / codegen (low / latent — most not triggered by the current spec)

5. errDiscoveryInterrupted is unexported. DiscoverNodes returns it to external callers on the interrupt path, but they can't errors.Is it (only string-match). Consider exporting (ErrDiscoveryInterrupted) or a typed sentinel so the "retryable" signal is detectable.
   

   opensearch-go/opensearchtransport/discovery.go

   Line 51 in 5a4d450

   // errDiscoveryInterrupted is returned to a waiting caller when the discovery

6. OpOther.IsWrite() == true. OpOther = -1 sets the R/W sign bit, so the unrecognized-op sentinel reports as a write. Routes using Op(OpOther) (e.g. /_snapshot/{repo}/_mount, /{index}/_settings) make external retry-safety middleware treat them as writes. Consider a sentinel that leaves bit 63 clear, or document IsWrite/IsRead as undefined for OpOther.
   

   opensearch-go/opensearchtransport/operation.go

   Line 385 in 5a4d450

   const OpOther OperationID = -1

7. Cross-allOf GoName collision bypasses the resolver (latent). resolveAllOf/collectFields dedupe merged fields by JSON name only; the per-property collision resolver runs per sub-schema, so _score from one allOf member and score from another can both map to Score → duplicate Go field. Not triggered by the committed spec.
   

   opensearch-go/cmd/osgen/resp_walk.go

   Lines 253 to 256 in 5a4d450

   	if !seen[f.JSONName] {
   	seen[f.JSONName] = true
   	t.Fields = append(t.Fields, f)
   	}

8. "int32" is dead in decodeEquivalentGroups, and two tests mislabel it. No codegen path produces the Go type string "int32" (format: int32 maps to "int"), so the "int32" entries are unreachable; the tests named int32 … actually exercise int collapse/dedup.
   

   opensearch-go/cmd/osgen/resp_union.go

   Line 325 in 5a4d450

   case "int32":

9. Version-filtered properties still occupy the name set (latent). A property dropped by the version-range filter still claims its Go name, so a surviving _X sibling gets an unnecessary Raw suffix even though the bare X slot is free.
   

   opensearch-go/cmd/osgen/resp_walk.go

   Line 25 in 5a4d450

   // excludedFields collects properties dropped by the version-range

10. Dep-bump commit message lists stale versions. The "Bump Go dependencies" message says config 1.32.18 → 1.32.20 / credentials → 1.19.19, but the committed bumps are → 1.32.22 / → 1.19.21. Documentary only (matters for CVE/audit tracking).

🤖 Generated with Claude Code