Skip to content

Name the failing component when a relayed request does not reach the upstream - #127

Merged
aszarama merged 1 commit into
mainfrom
as/ppo-39-failure-class-header
Aug 27, 2026
Merged

Name the failing component when a relayed request does not reach the upstream#127
aszarama merged 1 commit into
mainfrom
as/ppo-39-failure-class-header

Conversation

@aszarama

@aszarama aszarama commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Linear: https://linear.app/cortexio/issue/PPO-39/cross-project-x01-prove-the-vertical-slice-and-protocol-contract

Paired with cortexapps/brain-backend#17029 - the two halves of one contract. This
side emits the classes; that side reads them.

Why

The reflector had one failure class, AXON_DESTINATION_REJECTED, written only as
a response body. A caller cannot tell that apart from an upstream's own 403,
because both arrive as a body on the same path. Two other things that fail here
had no way to be told at all.

What changed

x-cortex-failure-class on the response. The existing body text stays, for
whoever is reading logs. The header is the machine-readable half. ModifyResponse
deletes any copy an upstream sets, so only components on this side can name a
class.

AXON_CREDENTIAL_FAILURE. A failing plugin previously logged and continued,
which left {{plugin:google-adc}} in the header value. That placeholder then
travelled upstream as the credential, and Google's refusal came back as an
authorization failure - the provider failure was invisible, disguised as the one
thing it is most important to distinguish from. Resolution now returns an error,
and serve() stops the request before it is dialed.

AXON_NETWORK_FAILURE. DNS, connection, and TLS failures land in the reverse
proxy's ErrorHandler. They are this agent's own network, not the upstream's
answer, so a caller retrying against an upstream that was never reached is the
wrong response.

Shape

ValueResolver.Resolve now returns (string, error). Header resolution moved out
of the Director - which has no way to fail a request - into proxyEntry.serve(),
so there is one path to the upstream and the provider runs once per request. The
resolved values travel to the Director by request context, the same shape
withDynamicTarget already uses. Tests that drove entry.handler.ServeHTTP
directly now call serve(), so they exercise the real path.

Verified

  • go test ./... - the full agent suite, green
  • Four new tests in reflector_failure_class_test.go: the class header on a
    destination rejection, a credential provider failing without the upstream being
    dialed, an unreachable upstream, and an upstream that tries to name a class
  • Live, against the assembled stack in the paired PR - brain-backend,
    relay-dispatcher, snyk-broker, and this agent all running together:
    • a relayed request reached the upstream carrying the credential this agent minted,
      with neither the relay token nor a placeholder on it
    • no routing metadata reached the upstream
    • the upstream set x-cortex-failure-class on every response and none survived
    • a destination outside the origin policy came back 403 AXON_DESTINATION_REJECTED
    • with the identity source refusing to mint, the same request came back
      502 AXON_CREDENTIAL_FAILURE - where before this change it would have gone
      upstream carrying {{plugin:google-adc}} and returned a 401

…upstream

The reflector had one failure class, AXON_DESTINATION_REJECTED, written only as
a response body. A caller cannot tell that apart from an upstream's own 403,
because both arrive as a body on the same path.

Carry the class in x-cortex-failure-class as well, and add the two classes that
had no way to be told at all: a credential provider that produced no value, and
a dial that never reached the upstream.

A failing provider previously logged and left its placeholder in the header
value, so the placeholder travelled upstream as the credential and the refusal
came back as an authorization failure. Resolution now fails the request before
it is dialed, which is what serve() exists to own.

An upstream copy of the header is deleted on the way back, so only components on
this side of the boundary can name a class.
@aszarama
aszarama merged commit f3b10e8 into main Aug 27, 2026
21 of 23 checks passed
@aszarama
aszarama deleted the as/ppo-39-failure-class-header branch August 27, 2026 05:06
shawnburke pushed a commit that referenced this pull request Aug 27, 2026
#127 changed ResolverMap.ToStringMap to return an error so a provider that
produces no value stops the request instead of leaving its placeholder in the
header, but it did not update acceptfile/router.go, which calls that method on
the tunnel path. main does not compile:

    server/snykbroker/acceptfile/router.go:95:21: multiple-value
    ruleHeaders.ToStringMap() (value of type (map[string]string, error))
    in single-value context

Handle the error the way the reflector's serve() does: log the failure against
the rule and refuse. A plain error lands in the default arm of grpctunnel's
RouteError mapping, which is a 502 — the same status the reflector answers, so
both transports refuse a failed provider identically.

The gap was a compile error rather than a behaviour one, so a test would not
have prevented it; the tunnel path had no coverage of provider failure either
way, which is what let the call site read as fine. Added both directions, and
confirmed the failing one goes red when the error is swallowed again.

Merges current main in, so this carries the fix at the bottom of the stack.
shawnburke added a commit that referenced this pull request Aug 27, 2026
* Never let an accept file stop the agent

Enabling the tunnel switches deployments that are running on snyk-broker
today, and that switch has to be transparent: a file the broker accepts
must still start. Constructs the Router cannot carry are warned about and
ignored rather than refused.

What lands here are the ones snyk-broker honours and the Router does not
implement — body and query "valid" filters, requiredCapabilities. Ignoring
one widens the rule, so the warning says exactly that rather than leaving
an operator to discover it from traffic.

Inbound "public" rules are warned about at parse and ignored. They cannot
affect outbound routing, and enough accept files carry a block copied from
a snyk-broker config that refusing them would break working deployments
over a section that routes nothing. The warning names the section and says
support for it will be removed. An empty block stays silent — that is the
shape Render itself emits, and warning about it would train everyone to
ignore the warning.

The rule warnings are at Router construction, not at parse. Parsing is
shared with the snyk-broker path, where the Node broker honours these, so
warning at parse would tell an operator their working rule is being
dropped when it is not. TestSnykBrokerConstructsStillParse pins that
boundary from the other side: those files parse, and parse silently.

Also adds TestOriginContract. Origin() is the one accessor this package
shares with the reflector — relay_instance_manager.go reads it to detect a
wildcard rule, build the proxy URI and report a bad origin — and it is
pinned here, ahead of the routing work that refactors it, so a change to
the shared accessor cannot quietly alter what the reflector sees.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFYsRuHoCbCBWEFvuaj9kV

* Fail the request when a credential provider fails

#127 changed ResolverMap.ToStringMap to return an error so a provider that
produces no value stops the request instead of leaving its placeholder in the
header, but it did not update acceptfile/router.go, which calls that method on
the tunnel path. main does not compile:

    server/snykbroker/acceptfile/router.go:95:21: multiple-value
    ruleHeaders.ToStringMap() (value of type (map[string]string, error))
    in single-value context

Handle the error the way the reflector's serve() does: log the failure against
the rule and refuse. A plain error lands in the default arm of grpctunnel's
RouteError mapping, which is a 502 — the same status the reflector answers, so
both transports refuse a failed provider identically.

The gap was a compile error rather than a behaviour one, so a test would not
have prevented it; the tunnel path had no coverage of provider failure either
way, which is what let the call site read as fine. Added both directions, and
confirmed the failing one goes red when the error is swallowed again.

Merges current main in, so this carries the fix at the bottom of the stack.

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants