NEXUS-504: Refactor Nexus frontend interceptors

[mavemuri]

What changed?

Refactoring Nexus frontend interceptors

Why?

https://temporalio.atlassian.net/browse/NEXUS-504

How did you test it?

• built
• run locally and tested manually
• covered by existing tests
• added new unit test(s)
• added new functional test(s)

Potential risks

• Medium risk of regression, will be covered through tests

[bergundy]

I have submitted the WIP review so you have something actionable until I complete the full review.

[bergundy]

Not blocking: You could try to consolidate some of the interceptor logic for nexus and grpc because there's a high risk now that the behaviors will drift.

[bergundy]

Being a bit nitpicky here but I wonder if it wouldn't be simpler to not have this separate wrapper and move the nexus logic into interceptor.Redirection

[mavemuri]

I did consider this but that would create an import cycle between as the nexus forwarder depends on the frontend and it already imports rpc/interceptor - breaking that up seemed like a larger effort than using this small wrapper

[bergundy]

Remind me again what were the considerations to make this an exception in the interceptor flow?

[mavemuri]

The issue is that currently Nexus has telemetry outermost in chain but gRPC has it after auth+fwding. imo telemetry as outermost is the right way forward for unified chain to continue capturing auth failures/redirects etc in same path and not have different paths for those. However, making that change for gRPC looks risky as it would change the current reporting - like forwarded requests being capture both at source and target clusters, authz failures etc - those should be relatively small overall but its still a change and didnt want to introduce any such behavior changes through this refactor

[github-actions]

Reviewing...

View job run

Note: Cancelled in favor of analysis below

[mavemuri]

Claude code review and analysis

Raw summary

Holistic review — interceptors-2

Target: unified final state of 891d1b648..HEAD plus working tree (tree was clean), reviewed as one diff.
54 files, +3450/−1495. Reviewed not commit-by-commit, per instruction.

Method: 8 parallel dimension agents, each given the repo guidelines (.github/copilot-instructions.md)
and pre-refactor snapshots (git show 891d1b648:<path>), followed by an adversarial verification pass in
which I re-derived every consequential claim myself against the current tree.

I ran nothing. No go test, go build, go vet, lint or CI — per instruction. Everything below is
from reading the working tree and comparing against snapshots. Treat this list as a lower bound.
Claims that only a test run could settle are marked [needs a run].

Where the agents disagreed, I adjudicated. Three of eight agents reported the
SkipServiceErrorReporting gap as a live regression; two reported it as latent. I refuted the "live"
reading myself — see F9. One of my own early candidate findings was also refuted (see Refuted).

---

Findings

F1 — high — nexus_handler.go has no unit tests at all, and ExposeDetails is untested repo-wide

high — All 8 test functions were deleted from nexus_handler_test.go, leaving a fixture and zero tests; the user-facing error-detail flag ExposeDetails now has no test anywhere.

service/frontend/nexus_handler_test.go is 45 lines and contains zero func Test* — only the
newOperationContext() fixture at :14. The snapshot had eight:

TestNexusInterceptRequest_InvalidNamespaceState_ResultsInBadRequest          (:183)
TestNexusInterceptRequest_NamespaceConcurrencyLimited_ResultsInResourceExhausted (:207)
TestNexusInterceptRequest_NamespaceRateLimited_ResultsInResourceExhausted    (:231)
TestNexusInterceptRequest_GlobalRateLimited_ResultsInResourceExhausted       (:255)
TestNexusInterceptRequest_ForwardingDisabled_ResultsInUnavailable            (:279)
TestNexusInterceptRequest_ForwardingEnabled_ResultsInNotActiveError          (:304)
TestNexusInterceptRequest_InvalidSDKVersion_ResultsInBadRequest              (:328)
TestNexusInterceptRequest_HeadersSanitization                                (:355)

The outcome tags those tests pinned are each re-covered by a per-interceptor unit test. What is not
re-covered is the user-facing error type and message. The old tests asserted, e.g.,
require.Equal(t, "namespace rate limit exceeded", handlerError.Message) — the only assertion that
ConvertGRPCError(err, /*exposeDetails=*/true) was actually reached.

ExposeDetails (common/rpc/interceptor/nexus/nexus.go:63) is written in four places
(namespace_rate_limit.go:123, rate_limit.go:105, sdk_version.go:62,
nexus_completion_http_handler.go:279) and read in exactly one (nexus_handler.go:144). A repo-wide grep
for ExposeDetails returns no test file, and grep -rn "rate limit exceeded" tests/ returns 0 hits.

Also untested as a result: finalStartHandler (nexus_handler.go:384-432, five distinct error returns
including the payload size limit at :409 and the matching_timeout outcome at :422),
getOperationContext's namespace_not_found metric (:291-297), and finalizeOperationRequest's panic
boundary (:152-163).

Suggestion: Add the missing end-to-end leg to service/frontend/nexus_interceptor_chain_test.go, which
already has a chain-building helper. Two cases are enough to re-pin the contract:

{
    name: "rate limited exposes detail",
    err:  &interceptornexus.InterceptorError{Err: rpcinterceptor.ErrNamespaceRateLimitServerBusy, Outcome: "namespace_rate_limited", ExposeDetails: true},
    assertErr: func(t *testing.T, err error) {
        var handlerErr *nexus.HandlerError
        require.ErrorAs(t, convertInterceptorError(err), &handlerErr)
        require.Equal(t, nexus.HandlerErrorTypeResourceExhausted, handlerErr.Type)
        require.Equal(t, "namespace rate limit exceeded", handlerErr.Message)
    },
},
// mirror case with ExposeDetails:false asserting the message is masked

Comments: Not a functional issue but fixed

F2 — med — Namespace state validation now runs inward of forwarding, so forwarded Nexus requests skip it entirely

med — ValidateState moved below the forwarding interceptor in the unified chain; because the forwarder short-circuits without calling next, state validation no longer runs on either forward branch.

service/frontend/frontend_interceptors.go:91 places redirectionWrapper (whose Nexus half is
nexusForwarder.InterceptNexus, :75) outward of namespaceValidatorInterceptor at :94. First-listed
is outermost — I verified the fold direction in ChainInterceptors (nexus/nexus.go:334-342,
slices.Backward, fresh per-iteration next := final).

nexusForwardingInterceptor.InterceptNexus returns without calling next on both branches:
nexus_forward_interceptor.go:76-82 (forwarding disabled) and :113-127 (forward). The only Nexus caller of
ValidateState is namespace_validator.go:293, inward of the forwarder. So it never runs.

Old code validated state first, in both entry points:

// before/service_frontend_nexus_handler.go:197
if err := c.namespaceValidationInterceptor.ValidateState(c.namespace, c.apiName, namespace.EmptyBusinessID); err != nil {
    c.metricsHandler = c.metricsHandler.WithTags(metrics.OutcomeTag("invalid_namespace_state"))
    return commonnexus.ConvertGRPCError(err, false)
}
//nolint:forbidigo
if !c.namespace.ActiveInCluster(c.clusterMetadata.GetCurrentClusterName()) {   // :203 forward decision

Identical shape in before/service_frontend_nexus_completion_http_handler.go:613 → :619.

ValidateState = checkNamespaceState + checkReplicationState — so the handover gate is skipped too,
which matters because the whole point of a handover gate is to stop traffic.

Important context: the new order matches the pre-existing gRPC chain, which always had redirection
outward of state validation. This is very likely a deliberate unification, not an oversight. It is still a
real Nexus behavior change, and nothing in the code says so.

Failure scenario: Global namespace ns1 is active in cluster B, passive in A, and in
NAMESPACE_STATE_DELETED. A Nexus StartOperation hits A with auto-forwarding on. Old: rejected locally,
outcome=invalid_namespace_state, no network. New: A opens an HTTP client to B, proxies the full request
body, and relays B's rejection as outcome=forwarded_request_error. Note the rate limiters
(frontend_interceptors.go:95-97) are also inward of the forwarder, so nothing local throttles this
amplification.

Suggestion: If intentional, say so on the ordering comment at frontend_interceptors.go:78-80 — that
comment currently justifies routing-before-namespace and telemetry-after-redirection, but not
state-after-redirection. If not, the fix needs a per-transport position for this one entry, since moving it
in the shared list would also change gRPC.

Comments: Acknowledged as minor collateral of unifying interceptor chains

---

F3 — med — Nexus completion outcome vocabulary collapsed from ~9 values to error_internal

med — Every non-NotFound completion failure is now hard-coded to outcome=error_internal, erasing the outcome dimension operators use to distinguish load-shedding from real faults.

service/frontend/nexus_completion_http_handler.go:281:

return nil, &interceptornexus.InterceptorError{Err: err, Outcome: "error_internal"}

CompleteOpInput.Outcome returns ie.Outcome verbatim when set (nexus/nexus.go:291-296), so that string
reaches metrics.NexusCompletionRequests.

Old code returned the converted error and derived the tag from its type:

// before/..._completion_http_handler.go:246
return commonnexus.ConvertGRPCError(err, false)

// before/..._completion_http_handler.go:550-554
if he, ok := errors.AsType[*nexus.HandlerError](*errPtr); ok {
    c.metricsHandler = c.metricsHandler.WithTags(metrics.OutcomeTag("error_" + strings.ToLower(string(he.Type))))
} else {
    c.metricsHandler = c.metricsHandler.WithTags(metrics.OutcomeTag("error_internal"))
}

I confirmed ConvertGRPCError produces ten distinct handler-error types (common/nexus/failure.go:340, 348, 358, 366, 374, 382, 390, 398, 406, 414): BadRequest, Unavailable, Internal ×2, Unauthenticated,
Unauthorized, NotFound, ResourceExhausted, NotImplemented, UpstreamTimeout. So error_resource_exhausted,
error_upstream_timeout, error_unavailable, error_bad_request, error_unauthenticated,
error_unauthorized, error_not_implemented are all now unreachable on this path.

The HTTP response is still correct — convertInterceptorError re-runs ConvertGRPCError — so only the
metric lost the information, which means it fails silently until someone reads a dashboard.

Failure scenario: History shard rate-limits a completion → serviceerror.ResourceExhausted. Old:
nexus_completion_requests{outcome="error_resource_exhausted"}. New: outcome="error_internal". An alert on
error_internal now fires on ordinary throttling, and the error_resource_exhausted panel goes permanently
to zero.

Suggestion:

converted := commonnexus.ConvertGRPCError(err, false)
outcome := "error_internal"
if he, ok := errors.AsType[*nexus.HandlerError](converted); ok {
    outcome = "error_" + strings.ToLower(string(he.Type))
}
return nil, &interceptornexus.InterceptorError{Err: err, Outcome: outcome}

---

F4 — med — Four pre-chain completion failure paths lost latency and service-level metrics, and one changed its outcome value

med — Completion requests that fail before the interceptor chain now emit only a counter; NexusCompletionLatencyHistogram, ServiceRequests and ServiceLatency* are gone, and auth-parse failures moved from error_internal to error_bad_request.

service/frontend/nexus_completion_http_handler.go:157-164:

// recordBadRequest is for pre-interceptor chain error recording
recordBadRequest := func() {
    metrics.NexusCompletionRequests.With(h.MetricsHandler).Record(
        1,
        metrics.NamespaceTag(ns.Name().String()),
        metrics.OutcomeTag("error_bad_request"),
    )
}

Called at :172, :183, :192, :215 — all of which return before h.chainedHandler(...) at :222, so
TelemetryInterceptor.InterceptNexusOutermost never runs. defer finalizeCompletionRequest is installed at
:155, but that function records no metrics (:512-523) — only handleRequestError +
convertInterceptorError.

I verified the old defer's position covers exactly these paths. capturePanicAndRecordMetrics was deferred at
before/..._completion_http_handler.go:182, and the URL-unescape check (abs :184-190), the
namespace/token mismatch (abs :192-197) and interceptRequest — which contained the TLS/claims parse —
(abs :201) all come after it. That defer emitted four metrics:

// before/..._completion_http_handler.go:557-563
c.metricsHandler.Counter(metrics.NexusCompletionRequests.Name()).Record(1)
c.metricsHandler.Histogram(metrics.NexusCompletionLatencyHistogram.Name(), metrics.Milliseconds).Record(...)
metrics.ServiceRequests.With(c.metricsHandlerForInterceptors).Record(1)
c.TelemetryInterceptor.RecordLatencyMetrics(*ctxPtr, c.requestStartTime, c.metricsHandlerForInterceptors)

NexusCompletionLatencyHistogram now has exactly one writer, telemetry.go:267, reachable only inside the chain.

Separately, the outcome value changed for the auth path: old returned the raw GetClaims error from
interceptRequest with no outcome tag set, so it fell through to error_internal (before/:553). It is now
hard-coded error_bad_request.

(The four returns that were already before the old defer — invalid callback token ×3, namespace lookup —
recorded nothing then either. That part is at parity.)

Failure scenario: A namespace has a misconfigured claim mapper, so every completion callback fails in
parseTLSAndAuthInfo. Old: nexus_completion_requests{outcome=error_internal} climbs, service_requests
and service_latency climb, latency histogram shows the traffic. New: only
nexus_completion_requests{outcome=error_bad_request} moves — the error_internal alert never fires,
service_requests shows zero completion traffic, and the burst is invisible as frontend load.

Suggestion: Take the outcome as a parameter so the auth path keeps error_internal, and record the same
four metrics the old defer did against rCtx.metricsHandlerForInterceptors.

---

F5 — med — Nothing pins the production interceptor order for either transport

med — NewInterceptorsProvider is now the single ordering authority for both the gRPC and Nexus chains, and it has zero test coverage; the one chain test hand-builds an unrelated 4-element chain.

grep -rn "NewInterceptorsProvider\|GrpcInterceptors()\|NexusInterceptors()" --include='*_test.go' .
returns nothing.

service/frontend/nexus_interceptor_chain_test.go:148-161 builds its own chain with no relationship to
production:

return interceptornexus.ChainInterceptors(
    func(...) (any, error) { ... },
    []interceptornexus.Interceptor{
        telemetry.InterceptNexusOutermost,
        mask.InterceptNexus,
        serviceErrors.InterceptNexus,
        frontendServiceErrors.InterceptNexus,
    },
)

Its assertions are about error type / outcome / metrics (:110-121), never about which interceptor ran when.

This matters more than it did before the change: the refactor collapses three previously independent
orderings (a gRPC slice and two hand-written inline sequences) into one 21-entry shared list at
frontend_interceptors.go:81-103, whose order is safety-critical for both transports and whose intent
differs between them (telemetry is outermost for Nexus via :135, position 11 for gRPC). The file's own
comment at :78-80 states three ordering invariants that nothing enforces. F2 is exactly this class of
change, and no test caught it.

Failure scenario: Someone moves authInterceptor below redirectionWrapper to satisfy a gRPC concern.
Every Nexus request is then forwarded cross-cluster before authorization runs. Build passes, full suite green.

Suggestion: A golden-order test is cheap and sufficient — construct NewInterceptorsProvider with
recording stubs and require.Equal on the ordered concrete type names for both GrpcInterceptors() and
NexusInterceptors(), plus len(nexus) == len(grpc)+1 and nexus[0] is the telemetry outermost hook.

---

F6 — med — Panics inside the chain lost their message string and every Nexus correlation tag

med — A panic anywhere inward of chain position 3 is now recovered by ServiceErrorInterceptor with a bare logger and logged as "Panic is captured", not "Panic captured" with the operation's tags.

Panic recovery did not regress — I verified defer finalizeOperationRequest(oc, &retErr) is installed at
nexus_handler.go:323, before h.chainedHandler at :372, so anything escaping the chain is still
caught (same for completion at :155/:222). What regressed is which recover fires and what it logs.

ServiceErrorInterceptor sits at position 3 (frontend_interceptors.go:83) and recovers first:

// common/rpc/interceptor/service_error_interceptor.go:57
resp, err := i.capturePanicHandlerNexus(ctx, in, next)
// :96-101
defer metrics.CapturePanic(i.logger, i.metricsHandler, &retError)

metrics.CapturePanic logs "Panic is captured" (common/metrics/panic.go:25) on i.logger — the plain
frontend service logger.

Old code recovered in the handler on the enriched logger:

// before/service_frontend_nexus_handler.go:115
c.logger.Error("Panic captured", tag.SysStackTrace(st), tag.Error(err))

where c.logger carried tag.NexusStageHandlerInbound, tag.Operation, tag.WorkflowNamespace,
tag.NexusService, tag.NexusOperation, tag.Endpoint and tag.RequestID (before/:412, :329-338).

The old-style line survives at nexus_handler.go:158 but its own comment concedes it now only covers "the
edge case where the operation panics before the interceptor chain is invoked" (:322).

One side effect is an improvement worth keeping: Nexus panics now also increment ServicePanic, which the
old path never recorded.

Failure scenario: A malformed nexuspb.Link panics inside finalStartHandler for namespace acme,
endpoint payments, operation charge. Old: one Panic captured line naming all four. New:
Panic is captured with only a stack trace — you cannot tell which namespace or endpoint triggered it
without parsing the stack, and any alert keyed on the old string silently stops matching.

Suggestion: Derive the logger from the input in capturePanicHandlerNexus
(tag.WorkflowNamespace(in.NamespaceName()), tag.Operation(in.MethodName()),
tag.Endpoint(in.EndpointName()), tag.NexusOperation(in.OperationName())), and align the message strings.

---

F7 — med — Forwarding error logs lost their correlation tags

med — nexusForwardingInterceptor logs from the bare fx logger, dropping the workflow/run/request IDs the completion handler had already built and the Nexus stage tag on all three routes.

service/frontend/nexus_forward_interceptor.go:99-113:

logTags := []tag.Tag{
    tag.SourceCluster(i.clusterMetadata.GetCurrentClusterName()),
    tag.TargetCluster(targetCluster),
    tag.Operation(in.MethodName()),
    tag.WorkflowNamespace(namespaceEntry.Name().String()),
}
...
logger := log.With(i.logger, logTags...)   // :113 — i.logger is the plain frontend logger
...
logger.Error("unable to get HTTP client for forward request", tag.Error(err))   // :215
logger.Error("failed to construct forwarding request URL", tag.Error(err))      // :220

For a CompleteOpInput, EndpointName() and OperationName() are both empty, so the conditional appends at
:105-112 are skipped and the logger carries exactly four tags.

Old built the forwarding logger from the request logger:

// before/..._completion_http_handler.go:403
logger := log.With(rCtx.logger, tag.SourceCluster(...), tag.TargetCluster(targetCluster))

where rCtx.logger carried tag.WorkflowNamespaceID, tag.WorkflowID, tag.WorkflowRunID,
tag.RequestID, on a base tagged tag.NexusStageCallerInbound. Start/cancel used oc.logger, which carried
tag.NexusService and tag.NexusStageHandlerInbound.

The same file proves the rich logger is reachable — withForwardingTrace already uses it:
if rCtx, ok := requestContextFromContext(ctx); ok { traceLogger = rCtx.logger } (:303-304).

Failure scenario: The active cluster's HTTP client cache is misconfigured. Every completion for a global
namespace logs unable to get HTTP client for forward request with no wf-id / wf-run-id / request-id,
so on-call cannot tell which workflows are stuck, or whether it is one workflow retrying or thousands.

Suggestion:

base := i.logger
if rCtx, ok := requestContextFromContext(ctx); ok {
    base = rCtx.logger
} else if oc, ok := operationContextFromContext(ctx); ok {
    base = oc.logger
}
logger := log.With(base, logTags...)

---

F8 — small — DC-redirection operation tag value changed for Nexus, and two dispatch routes collapsed into one series

small — Forwarded Nexus requests now tag redirection metrics DCRedirection<MethodName> instead of the full API name, breaking existing dashboards and merging the by-endpoint and by-task-queue routes.

service/frontend/nexus_forward_interceptor.go:88-90:

metricsHandler, forwardStartTime := i.redirectionInterceptor.BeforeCall(
    interceptor.DCRedirectionMetricsPrefix + in.MethodName(),
)

BeforeCall feeds this straight into metrics.OperationTag (redirection.go:343), tagging
ClientRedirectionRequests / ClientRedirectionLatency / ClientRedirectionFailures.

Old start/cancel passed the raw API name (before/..._nexus_handler.go:208, c.apiName =
/temporal.api.nexusservice.v1.NexusService/DispatchByEndpoint or .../DispatchByNamespaceAndTaskQueue);
old completion passed "CompleteNexusOperation" with no prefix (before/..._completion:634).

The comment at :86-87 says the change is deliberate, for consistency with gRPC. Worth noting it also
collapses DispatchByEndpoint and DispatchByNamespaceAndTaskQueue into a single
DCRedirectionStartNexusOperation series — which is relevant because NexusForwardRequestUseEndpoint picks
a different forward URL per route (:269-273).

Failure scenario: After deploy,
client_redirection_failures{operation="/temporal.api.nexusservice.v1.NexusService/DispatchByEndpoint"}
reads zero forever while cross-cluster Nexus traffic is unchanged.

Suggestion: No code change needed if intended — but it needs a release note. If per-route visibility
matters, use interceptor.DCRedirectionMetricsPrefix + api.MethodName(in.APIName()).

---

F9 — small — SkipServiceErrorReporting is honored on the completion path and ignored on start/cancel

small — The flag is set in 15 places across three packages and read in exactly one; the start/cancel twin of handleRequestError silently drops it. Latent today, not live — see the adjudication below.

service/frontend/nexus_handler.go:112-131 unwraps the error but never consults the flag:

if taggedErr, ok := errors.AsType[*interceptornexus.InterceptorError](err); ok {
    err = taggedErr.Err
}
source, ok := c.responseHeaders[commonnexus.FailureSourceHeaderName]
if !ok || source == commonnexus.FailureSourceWorker {
    return
}

Its twin does (nexus_completion_http_handler.go:493-498):

if taggedErr, ok := errors.AsType[*interceptornexus.InterceptorError](err); ok {
    if taggedErr.SkipServiceErrorReporting {
        return
    }
    err = taggedErr.Err
}

Producers on the start/cancel path: authorization/interceptor.go:190,206;
namespace_validator.go:161,168,290,297; nexus_forward_interceptor.go:68,80,125,167,200,216,221,227,235,266,276.

Adjudication — this does NOT fire today. Three agents reported this as a live regression, with a scenario
in which a remote cluster returns Temporal-Nexus-Failure-Source: server. I refuted that:
common/nexus/failure.go:24-27 defines exactly one value, FailureSourceWorker = "worker". The only
non-relay writer is nexus_dispatch_result.go:160, which writes FailureSourceWorker. The forward relay
(nexus_forward_interceptor.go:345-346) copies whatever the remote returned — and the remote is a Temporal
cluster, which also only ever writes "worker". So the guard at :119-122 always returns, and
handleRequestError is effectively unreachable on the start/cancel path — in both the old and the new
tree (the old code had the identical guard). There is no parity loss.

What remains is a real but latent contract inconsistency: a field written by three packages and read by one
of its two consumers, with two tests (namespace_validator_test.go:190,
nexus_forward_interceptor_test.go:182) asserting it is set, which reads as confidence that it does
something.

Failure scenario (latent): The day anything starts emitting a non-worker failure source — a new
attribution value, a proxy, a version-skewed peer — every forwarded start/cancel failure begins incrementing
service_error_with_type and logging "service failures" on the origin cluster for a failure that belongs
to the destination, while the identical completion failure stays correctly silent.

Suggestion: Either honor it, matching the completion handler:

if taggedErr, ok := errors.AsType[*interceptornexus.InterceptorError](err); ok {
    if taggedErr.SkipServiceErrorReporting {
        return
    }
    err = taggedErr.Err
}

or, if the failure-source guard genuinely subsumes it here, drop the flag from the start/cancel producers and
say so once on the field's doc comment. The current middle state is the worst of the three.

Comments: the start/cancel already has the header which is known to be set. Added the SkipServiceErrorReporting to aboid confusion

---

F10 — small — Dead code left by the extraction (grep-confirmed zero callers)

small — Three half-finished removals: a write-only struct field, two unused constructor parameters, and a constructor with no callers.

nexusHandler.clusterMetadata — grep -n 'clusterMetadata' service/frontend/nexus_handler.go returns
exactly three lines: :216 (field), :229 (ctor param), :241 (assignment). Zero reads. The old code
read it at before/..._nexus_handler.go:203, 214, 524, 539, 617, 637; all of that moved to
nexus_forward_interceptor.go, which takes its own copy. It is still threaded in from
nexus_operation_http_handler.go:82.

newNexusCompletionHandler — grep -n 'clusterMetadata\|httpTraceProvider' service/frontend/nexus_completion_http_handler.go returns exactly two lines, :61 and :70, both parameter
declarations. Neither appears in the struct (:43-54) or any body. Both moved to the forwarding interceptor
(:27, :31).

interceptor.NewFrontendServiceErrorInterceptor — repo-wide grep returns only the declaration at
frontend_service_error.go:39 and its own doc comment. Both former callers migrated to
NewFrontendServiceErrorInterceptorWrapper. It carries no Deprecated: marker, unlike
WithChainedFrontendGrpcInterceptors, which this same change did mark. Comments: NO, this cannot be removed as its an existing exported func and cannot be removed for now

Suggestion: Delete all three. If the constructor must stay for external importers, mark it
// Deprecated: use NewFrontendServiceErrorInterceptorWrapper(logger).Intercept. — and note that once it is
gone, NewFrontendServiceErrorInterceptorWrapper should take back the plain name, since it returns a
*FrontendServiceErrorInterceptor, not a wrapper.

---

F11 — small — The two namespace-validator types have names inverted from their behavior, and a doc comment names two deleted methods

small — NamespaceStateValidatorInterceptor validates namespace names; NamespaceValidatorInterceptor validates state. The chain order is correct but now reads as though it is wrong.

common/rpc/interceptor/namespace_validator.go:25:

// NamespaceValidatorInterceptor contains NamespaceValidateIntercept and StateValidationIntercept

Neither method exists — a repo-wide grep finds them only in this comment and in test names.

:131 func (nsvi *NamespaceStateValidatorInterceptor) Intercept(...) → setNamespaceIfNotPresent +
maxNamespaceLength check, i.e. the name check. Its own struct doc at :34-37 says so, contradicting the
type name. :247 // Intercept runs ValidateState on NamespaceValidatorInterceptor → the state check.

The chain positions are byte-for-byte correct against the old list (name-validation at position 5,
state-validation at 13) — this is purely a naming/comment defect. The risk is that a future reader "fixes"
the apparent inversion.

Related, same file: the tests still use the old method names and violate the repo's own convention (§2, "no
underscore after Test") — Test_StateValidationIntercept_* ×7, Test_NamespaceValidateIntercept.

Suggestion: Swap the type names to match behavior, and delete the stale comment at :25. At minimum,
delete :25 and add an ordering note at frontend_interceptors.go:86 explaining why the "State" one comes
first.

---

F12 — small — NamespaceHandoverInterceptor.InterceptNexus is ~45 lines that can never run

small — The Nexus handover gate is unreachable, and reads as if handover-wait were enforced for Nexus when it is not.

common/rpc/interceptor/namespace_handover.go:102-105 gates on handlesMethod(apiName), which
(:83-93) matches only api.WorkflowServicePrefix plus additionalServicePrefixes. I verified:

• All Nexus API names are /temporal.api.nexusservice.v1.NexusService/...
  (service/frontend/configs/quotas.go:18-20).
• WithAdditionalServicePrefixes has zero callers in the repo — grep returns only its own definition and
  two doc comments.

So handlesMethod always returns false for Nexus and the body — MustGetNamespaceName,
waitNamespaceHandoverUpdate, a ServiceRequests counter, HandoverWaitLatency — is dead weight.

Handover rejection for Nexus is still enforced, but by ValidateState → checkReplicationState
(namespace_validator.go:462-479) — which per F2 is now skipped on forwarded requests.

Latent hazard: the body records metrics.ServiceRequests at :126 with the comment "count the request as
this will not be counted" — true for gRPC (position 9, outward of telemetry at 11), false for Nexus
(telemetry is outermost at position 0 and already counted). If an embedder ever adds a Nexus prefix, this
double-counts.

Suggestion: Replace the body with the explicit no-op + comment that HealthInterceptor already uses
(health.go:43-49):

// InterceptNexus is a no-op: the handover gate only applies to WorkflowService methods, and Nexus
// handover rejection is enforced by NamespaceValidatorInterceptor.ValidateState.
func (i *NamespaceHandoverInterceptor) InterceptNexus(
    ctx context.Context, in nexus.InterceptorInput, next nexus.HandlerFunc,
) (any, error) {
    return next(ctx, in)
}

---

F13 — small — Undocumented mutual exclusion between the two custom-interceptor server options

small — Setting both WithChainedFrontendGrpcInterceptors and WithChainedFrontendInterceptors fails server startup, but neither godoc says so and the error names neither option.

temporal/server_options.go:135-140:

if len(so.customFrontendInterceptors) > 0 &&
    len(so.customFrontendUnifiedInterceptors) > 0 {
    return errors.New("configure either custom gRPC or unified interceptors, not both")
}

server_option.go:205 says only // Deprecated: Use WithChainedFrontendInterceptors instead.; :215-217
describes only chain position. Neither mentions the exclusion.

Credit where due: this is the one place the change got early-vs-late validation right — it runs in
loadAndValidate during ServerOptionsProvider, not at fx graph construction.

Failure scenario: An embedder with existing WithChainedFrontendGrpcInterceptors(a, b) adds
WithChainedFrontendInterceptors(c) for a new Nexus need. temporal.NewServer(...) fails with a message
naming neither option and no migration hint.

Suggestion: Document on both options and name them in the error:

return errors.New("WithChainedFrontendGrpcInterceptors and WithChainedFrontendInterceptors cannot both be set")

---

F14 — small — Test quality gaps in the new Nexus tests

small — The new tests would not catch several of the regressions above: no Outcome() test, Start-only forwarding coverage, an unasserted header check, and one shared variable across goroutines.

common/rpc/interceptor/nexus/nexus_test.go (85 lines) tests only chaining. Untested in-package:
CompleteOpInput.Outcome (nexus.go:287-302, including the "error_" + strings.ToLower(...) tag-value
format that dashboards depend on), StartOpInput.Outcome, CancelOpInput.Outcome, errorOutcome, and
NewCompleteOpInput's nil-request guard (:269-271). telemetry_test.go covers only StartOpInput, so
F3's collapse and F4's missing histogram would both pass unnoticed.

service/frontend/nexus_forward_interceptor_test.go drives only NewStartOpInput (:163). Never
reached: forwardCancelOperation (:175), forwardCompleteOperation (:205), completeOperationOptions
(:240), the default: unknown-type branch (:122-127), and the forwarded_request_error outcome (the stub
server only ever returns 201). Most concretely, the test sets up the header assertion and never makes it:
forwardingInfo.OriginalRequestHeaders is seeded with X-Original at :139-140, receivedHeaders is
captured at :46, but :191-194 assert only the two redirection headers — the security-relevant
"copy original headers without sanitization" behavior has no assertion.

receivedHeaders (:43) is also written from the httptest handler goroutine (:46) and read from the test
goroutine (:193-194) with no synchronization, which blocks t.Parallel(). Whether -race reports it
depends on net/http internals — [needs a run].

Repo-convention items (§3): concurrent_request_limit_test.go:167,186,196 use bare blocking channel
receives where the guidelines ask for await.Rcv/await.Snd, so a regression hangs to the package deadline
instead of failing; nexus_interceptor_chain_test.go:99 has t.Parallel() on the inner t.Run but not the
outer.

Suggestion: Highest value per line: a table test over the three Outcome implementations (covering nil
error + each result type, *InterceptorError with and without Outcome, and a bare *nexus.HandlerError on
CompleteOpInput). That single test would catch F3. Then add cancel/complete rows to the forwarding table and
make the X-Original assertion the test already set up.

---

F15 — small / nit — Consolidated API-surface, naming and hygiene items

small — A cluster of small guideline deviations in the new package and its consumers, grouped as one comment since the fix is a single pass.

• Over-export (§1). InterceptorsProvider and its two methods (frontend_interceptors.go:32,37,121,129)
  have no caller outside package frontend. Unexport to interceptorsProvider / newInterceptorsProvider /
  grpcInterceptors() / nexusInterceptors(). Interceptor (:16) is legitimately exported — it is used
  by temporal/server_option.go:219.
• OutcomeOverride (nexus/nexus.go:80-114). Get() violates §2 ("no Get prefix"). The nil guards at
  :86 and :95 are unreachable — NewOutcomeOverrideContext:104 always returns a non-nil pointer. The
  mutex guards a single string written and read on the same goroutine (sole writer
  nexus_forward_interceptor.go:84, sole reader telemetry.go:248). Consider collapsing to a *string in
  the context.
• InterceptorError.Error() (nexus/nexus.go:69) calls t.Err.Error() where %v already handles both
  cases; it panics on a nil Err. All 24 construction sites set it today, but three interceptors reassign
  the field afterwards (service_error_interceptor.go:59, frontend_service_error.go:62,
  mask_internal_error.go:74). Use %v with t.Err.
• Import aliases (§6). chasm/interceptors.go:8 aliases the new package as n. Worse,
  service_error_interceptor.go:7 and mask_internal_error.go:8 alias the third-party SDK as nexusrpc,
  which is the repo's own well-known name for go.temporal.io/server/common/nexus/nexusrpc — so
  nexusrpc.HandlerError in those two files means something different from nexusrpc.CompletionRequest two
  files over. Use interceptornexus for the new package, as service/frontend already does.
• NamespaceName() TODO (nexus/nexus.go:28). Shipping a new public interface with
  // TODO: this should just use NamespaceEntry() instead on one of its methods invites callers onto the
  wrong one. The two agree on both current paths, so this is redundancy rather than a bug — but resolving it
  removes the "one accessor works, the other doesn't" failure mode.
• recordDispatchOutcome (nexus_dispatch_result.go:158-162) no longer records an outcome; its comment
  spends its second sentence explaining what it does not do (§7). Rename to attributeFailureToWorker.
• Auth log tags (common/authorization/interceptor.go:193-201). tag.Operation(apiName) puts the full
  /temporal.api.nexusservice.v1.NexusService/... path where every other Nexus log puts the short method
  name — namespace_logger.go:70 does api.MethodName(in.APIName()). Completions also get an empty
  tag.Endpoint, since CompleteOpInput never sets one.
• testOperationContext (nexus_dispatch_result_test.go:68-70) is a one-line alias for
  newOperationContext() now that the options struct is gone; and the comment at :27 still describes
  interceptRequest, which no longer exists.
• Redundant nil check (§1): grpcfaults/interceptor.go:64 if g.h == nil — g.h is set by the sole
  constructor from a function that returns a closure literal and cannot be nil. - wrong - if the provided generator is nil, it will be nil

Suggestion: These are independent one-liners; the export and alias changes are the two worth doing before
review, since they are the ones that affect how the new public surface reads.

---

Verified clean

Things that could plausibly have broken in a refactor of this shape and demonstrably did not. I re-derived the
starred ones myself rather than taking an agent's word.

• ★ gRPC chain order is byte-for-byte preserved — all 24 positions of
  before/service_frontend_fx.go:290-329 match frontend_interceptors.go:81-113, including custom → faults →
  retryable at the tail. Two independent agents reached this separately.
• ★ ChainInterceptors folds in the correct direction — slices.Backward with a fresh per-iteration
  next := final, so first-listed is outermost, matching gRPC. No closure-capture bug. Pinned by
  nexus_test.go:34-64 and the short-circuit case at :66-84.
• ★ Telemetry does not double-count on Nexus — TelemetryInterceptor.InterceptNexus (telemetry.go:210-216)
  is an explicit documented no-op; only InterceptNexusOutermost records. TestTelemetryInterceptNexusRecordsNothing
  asserts an empty snapshot.
• ★ Panic in an interceptor is still recovered — defer finalizeOperationRequest at nexus_handler.go:323
  precedes h.chainedHandler at :372; same shape for completion. (Only the logging quality regressed — F6.)
• ★ No concurrency-slot leak — ConcurrentRequestLimitInterceptor.Allow returns a non-nil cleanup on every
  return path (concurrent_request_limit.go:87-116) and InterceptNexus does defer cleanup() before the
  error check (:126-129), so the rejection path and a panic both decrement.
• ★ ExposeDetails matches the old per-site ConvertGRPCError(err, bool) booleans exactly — concurrency
  limit false, namespace rate limit true, global rate limit true, unsupported client true.
• ★ The start/cancel outcome vocabulary is fully preserved — all 14 old values still have exactly one
  producer each. (The completion path is the exception — F3.)
• ★ The forward predicate is equivalent — old !ActiveInCluster(current) ≡ new
  IsGlobalNamespace() && targetCluster != current, because defaultReplicationResolver.ActiveInCluster
  (replication_resolver.go:72-79) is literally that expression and is the only implementation. The new
  RoutingKey{ID: info.BusinessID} is a no-op for start/cancel, whose BusinessID is always empty — the sole
  producer is the completion path (nexus_completion_http_handler.go:206), which already used it.
• ★ The new server option is plumbed end to end and its default is inert — WithChainedFrontendInterceptors
  → serverOptions → ServiceProviderParamsCommon → fx.Supply → NewInterceptorsProvider → appended at
  :110. Unset means a nil slice, and append(interceptors, nil...) is a no-op.
• Authorization runs on all three Nexus entry points, including the completion callback, and outside
  forwarding (authInterceptor at position 8, redirectionWrapper at 10). The CallTarget field set matches
  the old code on every path, and still sees unsanitized headers.
• Unknown/empty API names fail closed — defaultAuthorizer.Authorize denies on nil claims, and an
  unmatched prefix yields ScopeUnknown → resultDeny.
• No fail-open error paths in the security-adjacent interceptors — every if err != nil returns rather
  than calling next; a namespace-lookup failure surfaces as NamespaceEntry() erroring and is rejected with
  interceptor_failed.
• errors.AsType survives the whole chain — the three error-shaping interceptors rewrite ie.Err in place
  rather than re-wrapping, so errors.AsType still matches through the chain. nexus_interceptor_chain_test.go:22-126
  exercises this for *nexus.OperationError, *nexus.HandlerError and a status.Error.
• Nexus-native errors are not remapped by the new boundary conversion — ConvertGRPCError returns the
  error untouched when common.GetRPCStatus says no, which is the case for both *nexus.HandlerError and
  *nexus.OperationError (neither has Status()/GRPCStatus()).
• All size limits, timeouts and token handling survive — MaxNexusAPIRequestBodyBytes on all three routes,
  payloadSizeLimit (nexus_handler.go:409), BlobSizeLimitError (:259), MaxNexusOperationTokenLength
  (:242-245), GetResultTimeout. Completion-token decode/validate is byte-for-byte unchanged.
• Header sanitization still strips and still copies rather than mutating — sanitizeRequestHeaders
  (nexus_handler.go:165-178) builds a fresh map, and still runs last before dispatch. The forwarding path
  still deliberately forwards the original headers, byte-identical to the old wrapper.
• Request body is not consumed before the forward — the only consumers are in the terminal handlers, and
  the forwarder short-circuits above them. No re-read, no missing rewind, no lost limit.
• OutcomeOverride round-trips correctly and is not a dead write — installed before next, written by
  the forwarder, read after, and only on the success branch, which reproduces the old behavior (forward-then-fail
  still reports the error outcome, not request_forwarded).
• headers.Propagate / PopulateCallerInfo / AddMetricsContext were not lost — they moved into
  CallerInfoInterceptor.InterceptNexus and InterceptNexusOutermost, both still upstream of the matching
  dispatch.
• Tracing is intact — annotateServerSpan still runs for start and cancel with the same four attributes;
  withForwardingTrace reproduces the old tag sets for all three request kinds, including using the rich
  request logger for completions.
• fx_test.go was not weakened — the only changes are three constructor-rename call sites, no assertion
  removed or relaxed. history_engine_test.go (+6) and tools/flakereport/report.go (+6) are genuinely
  incidental (a method rename and a fmt.Fprintf lint fix).
• common/rpc/tlsinfo/context.go is a pure code move, not new context state — both functions only read
  peer.FromContext. No per-request state, no cross-request leakage. Live consumers, no stale references.
• Pre-existing, not a branch regression: handleRequestError reads c.responseHeaders at
  nexus_handler.go:119 without the mutex — but the old code passed the same map to its cleanup functions
  unlocked (before/:130). Same for the nil-map .Set shape on OriginalRequestHeaders. Four agents flagged
  the former; all four also noted it predates the branch. Not counted as findings.

---

Refuted during verification

Recording these because they are the kind of claim that looks right and is not.

1. HandleError(nil, ...) replacing HandleError(request, ...) (my own early candidate) — looks like a
   lost correlation field. It is not: both old and new pass fullMethod="", and
   logtags.WorkflowTags.Extract (workflow_tags.go:32-51) switches on the method prefix and returns nil
   for "". Zero tags either way.
2. SkipServiceErrorReporting firing today (claimed live by 3 of 8 agents) — refuted via the constant
   table; see the adjudication in F9. Downgraded from med to small.
3. Namespace state validation being a pure loss — it is a Nexus behavior change, but it aligns Nexus with
   the pre-existing gRPC order, so it is plausibly deliberate. Reported as F2 with that framing rather than as
   a straight regression.

---

Readiness assessment

What's solid. The core abstraction is sound. The fold direction is right, the sealed interface actually
seals, errors.AsType survives the chain because the error-shaping interceptors mutate in place rather than
re-wrap, the outcome override round-trips correctly, and there is no double-counting — the InterceptNexus
no-op on TelemetryInterceptor is exactly the right call and is documented. Critically, gRPC is untouched:
24 chain positions preserved byte-for-byte, verified independently by two agents and spot-checked by me. The
security-adjacent properties I most expected to break did not: auth still runs on all three Nexus entry points
including the completion callback, still outside forwarding; nothing fails open; no concurrency-slot leak;
ExposeDetails reproduces the old per-site booleans exactly. Panic recovery still covers the chain.

The real remaining risk is observability, not correctness. Nothing here loses or corrupts a request. What
this branch does lose is the ability to see what happened: a collapsed completion outcome vocabulary (F3),
four failure paths that vanished from ServiceRequests (F4), panics that no longer say which namespace
(F6), and forwarding failures that can no longer be joined to a workflow (F7). Those degrade exactly when you
need them — during an incident. Combined with F1 (the handler has no tests) and F5 (nothing pins chain order),
the branch is in a state where the next refactor is the dangerous one.

In priority order, before this goes up for review:

1. F3 and F4 — one-line and small fixes, and they are the two that silently break dashboards and alerts.
   Do these first; they are cheap and high-consequence.
2. F2 — decide and document. If forwarding-before-state-validation is the intended unification with gRPC,
   one sentence on frontend_interceptors.go:78-80 settles it. If not, it needs a per-transport position.
   This is the only finding where I cannot tell intent from the code.
3. F1 and F14 — restore the ExposeDetails assertion and add the Outcome() table test. Together they are
   maybe 60 lines and would have caught F3 outright.
4. F5 — the golden-order test. Cheap insurance on the invariant this whole refactor rests on.
5. F6 and F7 — logger plumbing; the fix pattern already exists in withForwardingTrace.
6. F10 through F13 — dead code, naming, and the two doc fixes. Mechanical.
7. F8 — no code change needed, but the redirection metric rename needs a release note, as does the
   "Slow gRPC call" → "Slow request" rename and the forwarding log-message punctuation changes.
8. F9 and F15 — resolve the flag one way or the other, then the hygiene pass.

One caveat on scope. I read code and ran nothing. The findings above are what static reading can establish;
a go test ./service/frontend/... ./common/rpc/interceptor/... run and a -race pass on
nexus_forward_interceptor_test.go would both tell you things this review cannot. Given F1, I'd want that run
before trusting the suite as a safety net at all.

---

---

Post-remediation pass — revised status

Re-reviewed at commit d604bc834 ("address claude feedback"), working tree clean. Everything below was
re-derived against the current files on disk, not against the original findings.

Same standing caveat as the original review: I ran nothing — no go test, go build, go vet, lint or
CI. I verified that the new tests exist, target the right code, and are internally consistent with the
production shapes they assert against. I did not verify that they pass.

Verdict: nothing blocking remains. Two nit-level items and one small test-quality cluster are open;
all three are non-blocking under the repo's severity rubric. No high or med findings survive.

---

Corrections to the original review

Two items in the report above were wrong. Recording them here rather than editing them out, so the record
shows what the verification pass actually caught.

C1 — F1 was mis-rated high; it should have been med

.github/copilot-instructions.md lists test gaps explicitly under med, and reserves high for security
vulnerabilities, data loss, crash/panic, race conditions, broken functionality, or architectural violations. A
deleted test is none of those. The original report even says so in its own readiness section ("nothing here
loses or corrupts a request") and then contradicted itself with the severity. Read F1 as med.

C2 — F15's grpcfaults nil-check bullet was wrong, and the suggested fix was dangerous

The report claimed if g.h == nil at common/rpc/grpcfaults/interceptor.go:64 was a redundant nil check
because UnaryServerInterceptor "returns a closure literal and cannot be nil." That is false:

// common/rpc/grpcfaults/interceptor.go:27-30
func UnaryServerInterceptor(generator Generator) grpc.UnaryServerInterceptor {
	if generator == nil {
		return nil
	}

NewFaultsInterceptor therefore sets h: nil whenever the generator is nil — which is the production
path, since grpcfaultstest.NewGenerator returns nil in non-test_dep builds. The check at :64 is
load-bearing; removing it would nil-panic every gRPC request. The bullet is withdrawn entirely.

---

Original findings — final disposition

#	Severity	Disposition
F1	med (was high, see C1)	Fixed. ExposeDetails now asserted both exposed and masked (nexus_interceptor_chain_test.go:171-195, :250).
F2	—	Accepted as-is. Confirmed collateral of unifying the chains; old gRPC had the same order (before/service_frontend_fx.go:306 redirection → :310 state validation). Not a regression anyone chose.
F3	med	Fixed exactly as suggested — outcome derived from the converted error (nexus_completion_http_handler.go:281-285).
F4	med	Fixed. recordPreInterceptorFailure emits all four metrics across all four pre-chain paths; auth path retains error_internal.
F5	med	Fixed. TestInterceptorsProviderOrder pins all 25 chain positions by type, plus telemetry-outermost and the len(nexus) == len(grpc)+1 invariant.
F6	med	Fixed. capturePanicHandlerNexus builds Operation / Namespace / Endpoint / NexusOperation / NexusStage / RequestID tags.
F7	med	Fixed. baseLogger falls back to rCtx.logger / oc.logger.
F8	small	Fixed via api.MethodName(in.APIName()), which restores the per-route split. Tag value still changes vs. main — release note still needed.
F9	small	Fixed. Flag now honored on the start/cancel path.
F10	small	Fixed. Dead field and params removed; exported constructor deliberately retained (public API) and now carries a Deprecated: marker.
F11	small	Mostly fixed. Renamed to NamespaceLengthValidatorInterceptor — better than the suggested swap, since the name now states behavior. One residual: see N1.
F12	small	Fixed. Explicit no-op with an explanatory comment.
F13	small	Fixed. Both godocs document the exclusion; the error names both options.
F14	small	Mostly fixed. TestOperationInputOutcomes covers all three input types incl. the error_ lowercase format, and the NewCompleteOpInput(nil) guard. Residuals: see N2.
F15	small/nit	Mostly fixed — aliases, Error() → %v, Get→Value, TODO resolved, attributeFailureToWorker, testOperationContext removed, auth api.MethodName. One bullet withdrawn (C2).

Also fixed during remediation: a dead clusterMetadata cluster.Metadata parameter on
NewNexusOperationHTTPHandler that became unused once the field was dropped from newNexusHandler — the
parameter and the now-unneeded cluster import are both gone.

---

Open items

N1 — nit — Stale doc comment names two methods that no longer exist

nit — namespace_validator.go:25 still documents NamespaceValidateIntercept and StateValidationIntercept, neither of which is in the tree.

// common/rpc/interceptor/namespace_validator.go:25
// NamespaceValidatorInterceptor contains NamespaceValidateIntercept and StateValidationIntercept
NamespaceValidatorInterceptor struct {

A repo-wide grep finds those two identifiers only in this comment and in test names. More conspicuous now
that the sibling type immediately below is correctly named NamespaceLengthValidatorInterceptor.

Suggestion: Replace with what the type actually does:

// NamespaceValidatorInterceptor validates that the resolved namespace is in a state
// that permits the requested API.

N2 — small — Forwarding-interceptor test remains Start-only, with one unsynchronized shared variable

small — Cancel, Complete and unknown-type forwarding branches are unexercised, and receivedHeaders is shared between the httptest goroutine and the test goroutine.

service/frontend/nexus_forward_interceptor_test.go:163 is the only input construction — every table row
builds NewStartOpInput. Unreached: forwardCancelOperation (nexus_forward_interceptor.go:175),
forwardCompleteOperation (:205), completeOperationOptions (:240) including its
HandlerErrorTypeBadRequest arm, and the default: unknown-operation-type branch (:122-127).

Note forwardCancelOperation's error_bad_request return (:194) is the one forwarding error that does
not set SkipServiceErrorReporting, unlike all ten of its siblings — untested, and inconsistent either
way.

receivedHeaders (:43) is written from the httptest handler goroutine (:46), reset from the test
goroutine (:130), and read from the test goroutine (:193-195) with no synchronization. This blocks
t.Parallel() on the subtests, which the repo guidelines ask for in plain t.Run tests. Whether -race
reports it depends on whether net/http's loopback path establishes a happens-before edge —
UNVERIFIABLE-WITHOUT-RUN.

Suggestion: Add a Cancel row and a Complete row to the existing table (the httptest server already accepts
any method), assert the redirection headers per input type, and replace the shared variable with a
per-subtest server or a mutex-guarded value so t.Parallel() can be added.

N3 — nit — Bare blocking channel receives in a test

nit — concurrent_request_limit_test.go:180,195 use bare channel receives where the guidelines ask for await.Rcv.

180:	<-blockUntilFirstReqStarted
195:	require.NoError(t, <-firstReqErrorCh)

Guideline §3: "Prefer await.Rcv and await.Snd for blocking channel operations so tests fail on timeout
instead of hanging indefinitely." If InterceptNexus ever stops calling next, this test hangs to the
package deadline rather than failing with a useful message.

Suggestion: Swap both for await.Rcv.

---

Revised readiness assessment

Ready for review. The six med findings that mattered — the collapsed completion outcome vocabulary, the
missing ServiceRequests/latency emissions, the unpinned chain order, the untagged panic logs, the
correlation-free forwarding logs, and the ignored SkipServiceErrorReporting — are all closed, and the
remediation went beyond the suggestions in two places worth noting: NamespaceLengthValidatorInterceptor is a
better rename than the type-swap the report proposed, and the golden-order test also pins the
telemetry-outermost and chain-length invariants rather than just the sequence.

Residual risk is now concentrated in one place: the test suite has never been run in this review. That is
the same caveat as the original pass, but it carries more weight now, because remediation added a substantial
amount of new test code — TestInterceptorsProviderOrder, TestOperationInputOutcomes,
TestServerOptionsRejectsBothFrontendInterceptorOptions, new chain cases. Static reading cannot distinguish a green golden-order
test from one that was written against a stale expected list.

Before merge, in priority order:

1. Release notes for the metric and log-string changes that are intentional but breaking for existing
   dashboards and saved searches: the DC-redirection operation tag value (F8), "Slow gRPC call" →
   "Slow request", and the forwarding log-message punctuation changes.
2. N2 and N3 — test-quality items; reasonable to defer to a follow-up, though N2's Cancel/Complete rows
   are the cheapest remaining coverage win in the branch.

One thing deliberately not on this list: F2. Confirmed as accepted collateral of unifying the two chains,
with old gRPC exhibiting the same ordering. No action.

Prompt for reference

Do a full holistic code review of this branch.

## Target construction

- Find the branch point with `git merge-base origin/main HEAD` and build ONE unified diff
  covering **all commits on the branch PLUS all uncommitted working-tree changes**, reviewed
  together as a single final state.
- **DO NOT do a commit-by-commit review.** I deliberately split and reordered the commits after
  the fact to make them easier to read, so intermediate commits contain known inconsistencies
  that are NOT findings. Only the final state matters.
- Confirm every finding against the **current file on disk**, never against the diff alone. If it
  is not still wrong in the working tree, it is not a finding.
- Save pre-refactor snapshots (`git show <branch-point>:<path>`) of any heavily rewritten file so
  behavior parity can be compared directly rather than guessed at.

## Constraints

- **Do not run tests, `go test`, builds, `go vet`, lint, or CI.** This is a read-only review.
- **Do not edit, write, or fix any repo file.** Report only.
- If you want to make a claim that only a test run could settle, say so explicitly and let me decide.

## Process

1. Scope the change first, then **give me a plan and wait for my go-ahead** before executing.
   Ask me for effort level and where I want the output.
2. Load the repo's own review skill / guidelines (e.g. `.claude/skills/review`,
   `.github/copilot-instructions.md`, `CLAUDE.md`) and pass them to every subagent so findings come
   back in the repo's severity vocabulary and comment format.
3. Fan out **8 parallel subagents**, one per dimension, scoped to specific files rather than the
   whole diff. Typical dimensions — adapt to the change:
   - Ordering/sequencing parity of whatever pipeline the change touches
   - The core new abstraction or framework
   - Any logic extracted or moved between components
   - **Behavior parity of rewritten code — what was LOST, not what moved** (usually the highest value)
   - Security-adjacent: auth, rate limits, admission control, fail-closed behavior
   - Metrics / tracing / logging parity
   - Wiring, DI, configuration, defaults
   - Conventions, API surface, structural simplicity, test quality
4. Run an **adversarial verification pass** on every candidate finding, especially every `high`.
   The verifier's job is to REFUTE the claim by reading the actual code, and it must return
   CONFIRMED / PLAUSIBLE / REFUTED with the decisive evidence and any corrections to the reasoning.
   Drop what cannot be verified rather than softening it.
5. Deduplicate and consolidate related symptoms into root-cause findings, rank most-severe first.

## What to look for — regressions of ANY size

Treat "this used to happen and no longer does" as the core question, and report it **however minor**:

- **Behavior parity:** validation checks, early returns, guard clauses, defers, cleanup, panic
  recovery, request/response size limits, timeouts, request ID handling.
- **Error handling:** error → status code / error type mapping, exact failure messages, whether
  detail is exposed or masked, retryable vs non-retryable classification, error wrapping that
  breaks `errors.As` / `errors.AsType`.
- **Metrics:** renamed metrics, changed or dropped tag sets, changed tag VALUE formats, lost
  outcome/classification vocabularies, unbounded cardinality, measurement windows that shrank or
  now double-count, emissions that are now dead writes.
- **Logging — treat these as real findings, not nits:** lost log lines, lost structured tags,
  lost correlation fields (request/trace/workflow/run IDs), a richly tagged logger replaced by a
  bare one, interpolated messages where static messages are required, messages that now name the
  wrong transport/component, changed log levels, duplicated tags.
- **Tracing:** lost spans, lost span attributes, broken propagation across hops.
- **Ordering:** anything whose position in a chain/pipeline moved, and what that breaks.
- **Concurrency & resources:** data races, shared mutable state, cross-request leakage, unreleased
  slots/counters on error or panic paths, unclosed bodies/handles, IO under locks.
- **Uninitialized or dead state:** fields declared but never assigned, writes never read, nil
  interface dereferences, half-finished removals where the producer went but the consumer stayed.
- **Config & options:** new options whose default changes existing behavior, undocumented
  mutual exclusions, validation that fails late (at graph construction) instead of early.
- **Dead code:** functions, types, fields, constants, imports left behind. Grep to confirm zero
  callers before reporting.
- **Test coverage:** deleted tests whose behavior is not re-covered elsewhere; new tests that only
  assert the happy path; tests that would not catch the regressions above.

## Output

- Use the repo's finding format and severity levels. Every finding needs:
  `file:line` against the current tree, a one-sentence summary that stands alone, why it matters,
  a **concrete failure scenario** (specific inputs/state → specific wrong behavior), and a concrete
  suggested fix with code where the fix is an edit.
- Prefer a small number of high-confidence findings over a long list.
- Also report what you **verified clean** — the things that could have broken and didn't.
- Finish with an honest readiness assessment: what's solid, what the real remaining risk is, and
  what you'd do before it goes up for review, in priority order.
- Write the full report to a file **and always give me the full absolute path** to anything you write.

## Reporting standards

- Report faithfully. Don't soften findings, don't inflate them, and don't claim verification you
  didn't do. If you only read code and didn't run anything, say that and treat your list as a
  lower bound.
- Don't take subagent results at face value — they are sometimes wrong or overstated. Verify the
  consequential ones yourself where a cheap grep settles it.
- If subagents stall or die (e.g. the machine sleeps mid-run), just relaunch the dead dimensions.