feat(broker): unregistered_url_handler seam on AppContainer for discovery misses

[ren-jentic]

Summary

Traffic to an unregistered METHOD+URL dies in the broker's catch-all as a 404 before any integrator seam can see it, so observing/serving discovery misses (audit sinks, catalog-suggestion UIs, monitor modes) currently requires a fragile global OperationNotFoundError exception handler that cannot distinguish the discovery-miss 404 from other 404s. This adds a first-class, default-preserving hook on the existing AppContainer seam: None (the default) reproduces today's behaviour byte-identically.

Changes

• shared/web/protocols.py (new): UnregisteredUrlHandler protocol (Response to short-circuit, None to fall through). The contract is deliberately web-shaped (Request/Response), so it lives in shared/web — shared/broker/protocols stays transport-neutral. The docstring carries the load-bearing contract: the URL has passed the pre-flight egress check only (validate_upstream_url is TOCTOU-vulnerable by design — any fetch the handler performs must go through HttpClientProvider for DNS pinning), and the full tradeoff callout — only authn, the per-actor execute rate limit, and the egress pre-check have run (no PBAC, no credential injection), core's body/response caps, deadlines, and resilience stack do not apply, the raw Request carries the caller's token/cookies/body and reaches the full deployment context via request.app.state (a handler is fully trusted deployment code, not a sandboxed plugin), fires for HEAD/OPTIONS and for Prefer: respond-async requests (synchronous response), never sees anonymous traffic (401s first), and __call__ must be async def.
• shared/web/container.py: AppContainer.unregistered_url_handler: UnregisteredUrlHandler | None = None — noun-shaped like the container's other seam fields. Named for what actually fires it: the pinned-revision miss raises the same operation_not_found problem type but deliberately never invokes the handler (a caller pin error on a registered API is not an unregistered flow).
• broker/web/routers/execute.py: _handle's unregistered-URL miss calls the new _handle_unregistered_url helper (the _resolve_broker shape) before raising. The handler call is failure-isolated: a raised exception (other than a deliberate ProblemDetailException, which propagates) is logged with the handler's qualified name — inside the router, while request_id is still bound — counted, and falls through to the documented 404, so a broken passive observer never converts the miss into a 500 (CancelledError still propagates). The broker.unregistered_url.handled counter is outcome-attributed (handled/declined/error, bounded cardinality, matching the repo's counter convention) — the core-side observability floor, since a handled miss writes no execution row and emits no event. Streaming responses return verbatim (nothing in core buffers); the async path and worker run post-discovery and are out of scope.
• shared/web/app_factory.py: both factories stash the handler on app.state (the injected-broker pattern); unset by default. Wire-up is logged (unregistered_url_handler_installed); a handler set on an app with no broker surface — where the hook can never fire — logs unregistered_url_handler_unreachable instead of failing silent.
• jentic_one.testing: BaseUnregisteredUrlHandlerComplianceTest (isinstance + exact __call__ signature + test_call_is_coroutine_function, so a sync __call__ — which the router would await into a 500 — fails compliance rather than production). assert_signature_matches now resolves annotations via typing.get_type_hints, so string vs object annotations and Optional[X] vs X | None compare equal, and its failure message diffs per-parameter with a concrete fix hint. Exported and exercised against a no-op reference implementation in the OSS suite.
• docs/development/extending-jentic-one.md: seam-table row, an "Unregistered-URL tradeoff" callout mirroring the injected-Broker resilience note, and a broker-only composition example — the hook fires only in the broker catch-all and the broker runs as the sole surface of its process, so the seam is wired via jentic_one.broker.web.app.create_app + install_broker_registry_resolver, never via create_combined_app.
• tests/arch/test_testing_public_api.py (new gate): every jentic_one.testing symbol the extending doc references must be exported via __all__, and every __all__ export must resolve — so a merge resolution can never silently delete documented public API.

Out of scope: richer telemetry (events) on handler short-circuits beyond the counter, a timeout around the handler call, narrowing the handler's argument to a value object (the raw-Request-with-loud-contract shape is deliberate — see the review thread), and any monitor-mode implementation — this PR is the neutral seam only.

Risk & rollback

• Low risk: with no container wiring the new field is None, the state attribute is never set (pinned by a hasattr assertion, not falsiness), and the raise site is reached exactly as today (same exception type and problem-details body — pinned by existing tests and the route-branch tests).
• No config, schema, or route changes (make openapi not needed — the catch-all's shape is unchanged).
• Rollback: revert the squash commit.

Test plan

• tests/unit/broker/test_unregistered_url_handler.py (new): container stashes the handler on surface + combined apps; default leaves the attribute absent; no handler / None-returning handler falls through to the 404; a returned Response (including a StreamingResponse, same instance) comes back verbatim; the handler receives method/URL/identity and the raw request object verbatim; route-branch tests through the real _handle: a handler short-circuits the miss and receives the validated URL (pinning the hook-after-validate_upstream_url ordering), no handler raises operation_not_found as today, a raising handler still yields operation_not_found (never a 500), and the pinned-revision miss never invokes the handler.
• Failure isolation pinned: a raising handler is counted (outcome="error") and logged with its qualified name; a ProblemDetailException propagates uncounted; CancelledError propagates uncounted (no swallow).
• Counter semantics pinned in both directions: handled/declined/error outcomes, and no metric at all on the unwired default path.
• tests/unit/testing/test_compliance_oss.py: compliance base (isinstance + signature + coroutine check) run against a no-op reference handler.
• tests/arch/test_testing_public_api.py: doc-referenced jentic_one.testing symbols resolve.
• Ran: full tests/unit (3279 passed), full tests/arch/ (300 passed), repo-wide ruff + uv run mypy (clean, 1226 files).

Review guide

Start with the protocol docstring in shared/web/protocols.py (the egress boundary + tradeoff callout), then the single guarded branch in execute.py::_handle and the failure isolation in _handle_unregistered_url; the rest is mechanical wiring mirroring the injected-broker seam. The review that shaped this state is in the PR comments.

Closes #1277

Made with Cursor

[ren-jentic]

Deep review — seam design

Verdict: the seam is conceptually right — a default-preserving, container-injected interception point at the discovery-miss site is the correct high-level shape, and it correctly mirrors the injected-Broker precedent (container field → app.state stash → getattr fallback → compliance base → docs row). Placing the hook after validate_upstream_url so the safe thing is the default is the best design decision here. Four things need fixing before this shape hardens into a rule; one (placement) is blocking.

1. The protocol is in the wrong module (blocking)

OperationNotFoundHandler takes a FastAPI Request and returns a Response — a web-edge contract, invoked only by the web router. Yet it lands in shared/broker/protocols.py, a module whose own header builds an identity around transport neutrality ("deliberately not HTTP-shaped"), and this PR adds that module's first from fastapi import Request. shared/broker/broker.py returning a Response is weak precedent — that seam is called by the router and the worker; this one is web-only and additionally consumes a raw Request. The tell is in the PR's own docs row: the seam is AppContainer.on_operation_not_found (defined in shared.web.container) but "Where" points at shared.broker.protocols, because the field and its contract ended up in different layers.

Fix: move the protocol to shared/web/ (next to the container). Rule candidate: web-shaped seam contracts (Request/Response in the signature) live in shared/web; shared/broker protocols stay transport-neutral.

2. The name over-promises and breaks the container's vocabulary

• It fires for only one of the two operation_not_found sites — the pinned-revision miss raises the identical exception type and problem-details type but (correctly) bypasses the hook. The precise semantics is unregistered upstream URL — the raise detail says exactly that, and the existing smoke test is even named test_unregistered_url_returns_operation_not_found.
• The container's existing fields are noun-shaped seams (broker, extra_routers, extra_installers, extra_lifespans); an event-flavoured on_* reads observe-only when this is an interceptor that can replace the response.

Fix: rename to AppContainer.unregistered_url_handler / UnregisteredUrlHandler (settles the issue's open question 2 — renaming a public seam after downstream wires it is far more expensive than now).

3. The tradeoff callout is missing — the seam opts out of more than it says

The docstring nails the egress contract but understates everything else a forwarding (monitor-mode) handler silently loses. Compare the injected-Broker seam, whose extending-docs entry carries an explicit "Resilience tradeoff" note — this seam needs its twin, enumerating:

• Body caps — the registered path enforces max_request_bytes(_by_type); a handler reading request.body() has no cap.
• Response-size caps + transfer deadlines — open_streaming_response owns those only on the registered streaming path; "returned verbatim, nothing buffers" is a feature whose flip side is uncapped.
• PBAC posture — "toolkit derivation/policy are meaningless here" frames it as inapplicable; frame it as security posture: the only controls that have run are authentication (RequireToolkitAccess) and the egress pre-check. A handler that forwards is a policy decision and owns its audit trail.
• Redaction — the handler receives the raw inbound Request including Authorization (the caller's platform token), cookies, and body; the contract must bind implementations to the redaction rule.
• Error semantics — a handler exception propagates and becomes a 500; say so.
• Visibility floor — the hook runs post-authn, so anonymous traffic to unregistered hosts never reaches it (401 first). Since the motivating consumer is a traffic viewer, state this where an integrator will read it.

I considered whether these gaps mean the abstraction is wrong — i.e. a narrow value object + constrained forward() capability instead of the raw Request. No: that is clunkier, forces core to buffer bodies, and is less consistent with the injected-Broker precedent (which likewise owns its transport, trusted via docstring + compliance test). Raw-Request-with-loud-contract is right — but the contract has to actually be loud.

4. A short-circuit is invisible to core — settle the telemetry floor now

When a handler short-circuits, core records nothing — no execution row, no event, no metric. Deferring the issue's open question 3 wholesale isn't free: retrofitting telemetry after downstream handlers exist is an observable behaviour change. The cheap floor is a counter metric — this module already holds the meter (get_meter("broker")). Add a broker.unregistered_url.handled counter in this PR; leave richer event emission open.

5. Tests under-deliver the issue's own test plan

The issue's test plan lists "pinned-revision miss never invokes the handler" — that test doesn't exist in the PR, and it's the most rule-load-bearing behaviour here (nothing currently fails if a future refactor adds the hook call to the pinned site). Also:

• The branch inside _handle is untested — tests exercise the private helper + app.state wiring separately, so deleting the if handled is not None: return handled lines breaks no unit test. Helper-level testing is the house pattern (test_injected_broker_seam.py does the same with _resolve_broker), but that file also drives one path end-to-end; this seam deserves one route-level test (real app + httpx.AsyncClient): handler short-circuits an unregistered URL; no handler → 404 operation_not_found; pinned miss → handler not called.
• The default-404 regression pin cited in the description lives only in a cluster-gated smoke test — fine, but not a unit-level guarantee.
• Nit: runtime_checkable on a __call__-only protocol matches every callable, so the isinstance compliance test is near-vacuous (the signature check does the real work); also a plain async def passes mypy + isinstance but fails assert_signature_matches(type(fn), …) — the compliance base implicitly requires a class-shaped handler; say so.

6. Small stuff

• Docs row "Where" should be jentic_one.shared.web.container (resolved by fix 1).
• method duplicates request.method — keep (explicit args are the contract) but note it.
• One contract line each: the hook fires for HEAD/OPTIONS too; a Prefer: respond-async request with an unregistered URL does hit the hook (discovery precedes the async branch) and gets a synchronous response.

Rule candidates this PR should seed

1. Web-shaped seam contracts live in shared/web; shared/broker stays transport-neutral.
2. Seam anatomy (this PR gets it right): optional container field defaulting to None → factory stashes on app.state only when set → consumer reads via getattr fallback → default path byte-identical; noun-shaped field names.
3. Any seam that bypasses built-in controls (resilience, caps, PBAC, audit) enumerates what no longer applies, in the protocol docstring and the extending-docs row.
4. Hooks near egress are placed after validation — the safe thing is the default.
5. A seam that can short-circuit core behaviour emits at least a counter metric from core.
6. Seam test floor: wiring test + helper test + one route-level test through the public surface including the negative site (the miss that must not invoke the hook) + compliance base against a no-op reference.

Bottom line: approve the shape; block on placement (1), settle the name now (2), and add the pinned-miss test + tradeoff callout + counter before merge. None of these change the abstraction — they make it the version the rules should be written from.

[ren-jentic]

Review items addressed in f994f35:

1. Placement — contract moved to shared/web/protocols.py (UnregisteredUrlHandler); shared/broker/protocols is transport-neutral again (fastapi import reverted).
2. Naming — seam renamed to AppContainer.unregistered_url_handler (noun-shaped, scoped to the unregistered-URL miss; settles issue open question 2).
3. Tradeoff callout — protocol docstring now enumerates everything the handler opts out of (PBAC posture, caps/deadlines/resilience, redaction on the raw Request, error semantics, HEAD/OPTIONS + respond-async, post-authn visibility floor); extending docs carry an "Unregistered-URL tradeoff" note mirroring the injected-Broker one.
4. Observability floor — broker.unregistered_url.handled counter increments on a short-circuit.
5. Tests — route-branch tests through the real _handle: handler short-circuit (asserts the handler receives the validated URL, pinning the egress ordering), no-handler 404, and the pinned-revision miss never invoking the handler. Compliance base documents the class-shaped requirement.

Verified: targeted pytest (24 passed), tests/arch/ (298 passed), ruff + mypy clean. PR title/description updated to the final state.

[Manuel-Jentic]

Review lap 1/7 — seam design & default-preservation

Starting a multi-lap adversarial review across the epic #1280 set (#1281, #1283, #1284) plus #1286. Each lap runs several independent lenses; findings are cumulative and I won't re-report across laps. Reviewed in a wired worktree at ff8736d3; I read the prior round (refactor(broker): reshape unregistered-URL seam per PR review) first and haven't re-raised anything resolved there.

Verdict: APPROVE-WITH-COMMENTS. The central claim — that OSS default behaviour is byte-identical and the seam is inert unless wired — holds up under attack; I tried several ways to break it and couldn't (details at the end). The findings are about what the seam promises an implementor, which matters because the whole point is that a downstream package writes against this contract without seeing the router.

The egress claim is false — the hook sits after only one of two egress layers

protocols.py:39-44 tells an implementor that by the time their handler runs, "the URL has already passed egress validation". egress.py:1-20 is explicit that there are two layers and that the first is insufficient on its own: validate_upstream_url is described as TOCTOU-vulnerable by design (it resolves DNS, then the actual connection re-resolves), with DnsPinningTransport closing that gap. The broker gets layer 2 via HttpClientProvider.

At the hook site (execute.py:651-655) only layer 1 has run. So a handler that takes the docstring at face value and fetches the URL itself has no DNS pinning, and is exposed to precisely the rebinding attack the module warns about. Worse, layer 1 fails open on resolution failure — url_validation.py:151-155 returns the URL unchanged on gaierror, where layer 2 raises. An unresolvable-then-resolvable host passes the pre-check and reaches the handler unvalidated.

Fix: state the boundary precisely and hand over the safe client, e.g. "the URL has passed the pre-flight allow/deny check (validate_upstream_url), which is TOCTOU-vulnerable by design; any fetch you perform must go through HttpClientProvider to get DNS pinning". Better still, pass the provider (or a pinned client) into the handler so the safe path is the easy one.

A sync handler passes both compliance tests, then 500s in the router

execute.py:611 awaits the handler, but neither compliance check can tell an async def from a def:

• compliance.py:114-140 compares inspect.signature, which is identical for sync and async.
• The isinstance check is worse than a no-op: UnregisteredUrlHandler is a runtime_checkable Protocol whose only member is __call__, so every callable matches. isinstance(lambda: None, UnregisteredUrlHandler) is True.

Probe: a sync handler with the right arity gets 3 passed, then at runtime TypeError: object Response can't be used in 'await' expression — a 500 on the execute path, found in production rather than in the downstream package's test suite. That defeats the purpose of shipping a compliance base.

Fix: assert inspect.iscoroutinefunction on the implementation (unwrapping functools.partial and __call__ for class-based handlers) in the compliance base, and mention "must be async def" in the protocol docstring.

The compliance base rejects correct implementations (minor)

compliance.py:29-41 compares annotation objects. protocols.py has from __future__ import annotations, so the protocol's annotations are strings; a downstream file that omits that import produces real objects, and the comparison fails with AssertionError: parameters diverge on code that is entirely correct. Optional[Response] vs Response | None fails the same way.

Fix: resolve both sides with typing.get_type_hints() before comparing.

protocols.py:47-50 understates its own guarantees (nit)

"The only controls that have run are authentication and the egress pre-check" — the per-actor execute rate limit has also run (deps.py:217 → :157-179). That's the one control an implementor might otherwise reimplement defensively, so it's worth claiming.

Verified clean — worth recording

I specifically tried to falsify the default-preservation and ordering claims:

• "Default byte-identical" holds. The raise site is untouched — the diff adds 5 lines above it and preserves the same detail and type. app.state is set only when the handler is non-None (app_factory.py:478-481,566-569), and there is no new metric emitted on the fall-through path. Both are pinned by test_default_container_leaves_handler_unset and test_route_no_handler_raises_operation_not_found.
• "A pinned miss never invokes the handler" holds, and is total. The hook is only inside the first resolved is None branch (execute.py:649-655); the pinned re-resolve miss (:670-674) has no call site at all. I checked the interesting combination too: a pin plus an unregistered URL correctly does reach the hook.
• The hook-after-validation ordering is pinned by execution, not by comment — the test asserts the handler receives the validator's output via a marker URL, so re-ordering the two actually fails a test. That's the right way to pin an ordering invariant.
• Wrong-arity handlers are caught (it's only the sync/async axis that leaks); both app factories are wired; the combined app uses include_router rather than Mount, so request.app is the root app and app.state resolves; Prefer: respond-async does reach the hook as documented.
• The new counter uses the existing get_meter("broker") and is attribute-free, so there's no cardinality risk, and it's incremented only on the short-circuit path.
• mypy and ruff clean; tests/arch 298 passed.

Process note for all four PRs in this set: every green check ran against the shared base 2333432a, not against a merge with today's main (all four are 17 commits behind), so green does not currently mean mergeable or still-green. Lap 6 will materialise the merges and re-run the gates. Also flagging cross-PR: #1284 in this set carries commits that duplicate work already merged to main and would revert it — worth being aware of before choosing a merge order for the epic.

_{Lap 1 of 7 · lens: seam design & default-preservation · head ff8736d3 · reviewing #1281, #1283, #1284, #1286 as a set}

[Manuel-Jentic]

Review lap 2/7 — seam blast radius · protocol evolution · merge topology · gates

Four independent lenses this lap. Findings are cumulative; I don't re-raise across laps.

Verdict: APPROVE-WITH-COMMENTS (unchanged), and this PR is the only one of the four that is cleanly mergeable today — see the topology note at the end. The findings below are all about the seam's contract, not its default behaviour, which continues to hold up.

The seam is a god-object: a handler reaches every secret in the deployment

Lap 1 flagged the docstring's egress claim. This is bigger. protocols.py:60's only disclosure bullet says request "carries the caller's Authorization header, cookies, and body" — it never mentions request.app.state. Built a real app via create_combined_app with a hostile handler and invoked _handle_unregistered_url with a real Request:

app.state keys: ['ctx', 'unregistered_url_handler']
DB session handles reachable: ['admin_db', 'control_db', 'registry_db']
decrypted keyset material: ERERERERERERERERERERERERERERERERERERERERERE=
jwt signing secret: ssssssssssssssssssssssssssssssssssssssss
registry DB password: reg_secret
can mint a token via ctx.encryption: EncryptionService
caller Authorization header: Bearer at_super_secret_platform_token

So the blast radius of a buggy or hostile downstream handler is every credential in the deployment (the decrypted keyset plus the control DB) and token forgery for any actor (the JWT signing secret) — not merely "no PBAC has run". The seam's design does nothing to narrow it.

And the returned Response is forwarded unvalidated:

raw headers: [..., (b'set-cookie', b'session=attacker-fixated; HttpOnly; Path=/; SameSite=lax'),
              (b'location',   b'https://evil.example/steal')]

Set-Cookie on the platform's own origin is session fixation, and a 3xx Location is an open redirect on the execute path. Both pass through with zero checks — pinned as intended by your own test_handler_response_is_returned_verbatim.

Fix: pass a narrowed frozen value object rather than the raw Request:

@dataclass(frozen=True)
class UnregisteredUrlCall:
    method: str
    upstream_url: str
    identity: Identity
    headers: Mapping[str, str]  # Authorization/Cookie stripped
    body: bytes

and in _handle_unregistered_url, reject a returned response carrying set-cookie or a 3xx location (raise rather than forward). If the raw Request must stay, say so explicitly in the "Security & resilience tradeoff" block: the handler holds the full deployment secret set.

The compliance base freezes the protocol at v1 — in both directions

assert_signature_matches asserts list equality of parameters, which makes it a two-way lock rather than a safety net:

• Core adds one additive, defaulted, keyword-only parameter → the unchanged downstream implementation gets FAIL -> DownstreamHandler.__call__ parameters diverge, despite being runtime-compatible and mypy-clean.
• A downstream that future-proofs with **kwargs: object against today's protocol also gets FAIL.

So no forward-compatible shape exists in either direction: every future core parameter is a breaking change for every downstream on the release it lands. And because the compliance base is itself exported public API (advertised in docs/development/extending-jentic-one.md), once a downstream's CI runs TestMyHandlerCompliance you can't loosen or tighten it either. git log shows the sibling Broker protocol has never been evolved, so there's no precedent to lean on.

Fix, while no downstream exists yet: either put **kwargs: object in the protocol and have assert_signature_matches treat a VAR_KEYWORD in the implementation as absorbing omitted protocol params — or, better, replace the four keyword params with one frozen UnregisteredUrlContext dataclass, which makes new fields additive by construction and composes with the fix above. Also consider making the return extensible now: Response | None cannot later carry a decision (fall_through vs handled).

Stack topology: this PR is the clean one — merge it first

Verified with git merge-tree --write-tree origin/main ff8736d3: zero conflicts, and it touches no generated artifact. On the merged tree, tests/arch gives 297 passed, 1 skipped, mypy is Success (59 files), and both the openapi and endpoints drift gates report IN SYNC.

For context, the other three all currently show CONFLICTING/DIRTY against main. Recommended order for the epic: #1281 → #1286 → #1283 (regen) → #1284 (recut). The src/jentic_one/testing/ conflict with #1284 is trivial — imports and __all__ auto-merge to the correct union, leaving only a one-line docstring conflict.

One cross-PR note in your favour: #1284's /capabilities document doesn't advertise this seam, and it shouldn't — unregistered_url_handler is a library composition point, not a client-observable HTTP surface.

Also worth knowing (stack-wide, affects this PR's CI)

ui/openapi.json is missing from the unit-tests path filter that runs its only semantic gate, and gh pr checks 1281 shows Generated UI client up to date skipping on this PR. Not caused by this PR and probably harmless here (you touch no UI artifact), but the gate you'd rely on if you ever did is not firing. Similarly, two CLI-side generated artifacts (ctl/assets/config-schema.json, ctl/generated/config.go) have no Python drift gate at all — mutating either still gives 297 passed — so local make test-arch won't catch them.

Verified clean this lap

• The new counter is not a side channel. broker.unregistered_url.handled carries no attributes — no actor, URL or host label — so even on an unauthenticated /metrics mount it exposes only an aggregate count, the same posture as every existing broker counter. I specifically checked this as a registry-enumeration oracle and it isn't one.
• No vacuous-test machinery. Grepped this PR's new/changed test files for mark.skip, mark.xfail, skipif, pytest.skip and empty parametrize lists: zero hits.
• The arch baseline at ff8736d3 is honest — 297 passed, 1 skipped, no pre-existing failures that could mask a regression.

_{Lap 2 of 7 · lenses: authz/blast radius · contract irreversibility · stack topology · gate adversary · head ff8736d3 vs origin/main 5817943a}

[Manuel-Jentic]

Review lap 3/7 — failure isolation · observability · attribution

Three lenses this lap. Findings are cumulative; I don't re-raise across laps.

Verdict: APPROVE-WITH-COMMENTS (unchanged). Everything below is about what happens when a downstream handler misbehaves — which is the case an extension seam has to get right, because the core project inherits the blame.

The hook has no error isolation, and the failure path emits nothing at all

execute.py:591-616 has no try/except and no logger call around await handler(...). Executed against the real function:

1. raiser -> propagates RuntimeError: downstream sink is down
   counter increments: []            (metric silent)

So a broken handler converts every unregistered-URL 404 into a 500, and core emits no counter, no log, no event and no execution row. That matters especially because the seam's advertised users are passive: protocols.py:19-21 names "audit sinks, catalog-suggestion UIs, monitor modes". A bug in a passive observer silently changes the route's contract for all callers.

None of the 13 tests in tests/unit/broker/test_unregistered_url_handler.py covers a raising handler.

Two related results at the same site:

• No timeout wraps the hook. 3. hanging handler after 2s: done=0 pending=1. There's no inbound request-timeout middleware — request_timeout_s is telemetry-only and upstream_timeout_s wraps only the runner. A hung handler hangs the request indefinitely.
• No re-entrancy guard. 5. recursed 41 levels; counter incremented 41 times for ONE request — so the counter measures invocations, not handled requests.

Fix: isolate, instrument, and let the documented default win:

except ProblemDetailException:
    raise                     # a deliberate downstream problem response
except Exception:             # NOT BaseException — CancelledError must propagate
    _unregistered_url_handler_errors.add(1)
    logger.exception("unregistered_url_handler_failed", handler=handler_name, method=method)
    return None               # fall through to the documented 404

and amend protocols.py:59-61, which currently states a raised exception propagates as a 500, to say it is counted, logged and falls through.

A handler failure is both unattributable and uncorrelatable

Beyond the missing log, the wire response gives an operator nothing to work with:

STATUS 500 | content-type: text/plain; charset=utf-8 | body: Internal Server Error

Not RFC 9457 — so there's no machine-readable type distinguishing "the broker failed" from "your extension failed". That distinction is commercially material: when an enterprise handler throws, the OSS project takes the support ticket.

Worse, RequestIDMiddleware unbinds request_id in its finally before the exception propagates out (shared/logging.py:187-188). Probe on a raising route:

resp x-request-id: None
request_id bound at logging time: None

So even once you add the log line above, it won't correlate with the client's failed call unless it's emitted inside the router, before the middleware unwinds. Worth doing deliberately rather than discovering later.

Fix: raise a typed ProblemDetail (e.g. type: "unregistered_url_handler_error") so the response is machine-distinguishable, and log inside the router while request_id is still bound. Also log the handler's module.qualname at wire-up time — there's currently no startup line saying an unregistered-URL handler is installed, which is an operability gap when debugging a deployment you didn't build.

The counter is success-only, against the codebase's own convention (minor)

if response is not None: _unregistered_url_handled.add(1) collapses three outcomes — handled, declined (None), raised — into one counted and two invisible. So "is my handler declining or erroring?" is unanswerable from metrics.

The convention here is outcome-attributed counters, e.g. login_counter.add(1, {"outcome": "success"|"failure"|"lockout"}) (admin/services/auth_service.py:117,145,170) and _executions_total.add(1, {..., "status": status}). The test currently pins the gap: counter.add.assert_called_once_with(1).

Fix: _unregistered_url_handled.add(1, {"outcome": "handled"|"declined"|"error"}) — bounded 3-value cardinality, matching the rest of the codebase.

Verified clean this lap

• Cancellation is handled correctly. 4. cancel -> CancelledError propagates cleanly (no swallow). Zero except BaseException and zero bare except: in this PR's new code, so the classic cancellation-swallowing bug isn't present — worth stating because the fix above must preserve it (catch Exception, not BaseException).
• No shared mutable state on the core path — getattr(request.app.state, …) is a read of a build-time attribute, so concurrent invocations don't interfere.
• The counter carries no attributes, so even on an unauthenticated /metrics mount it exposes only an aggregate count — no actor, URL or host label, no cardinality risk. (The fix above keeps that property.)
• ruff check --select ASYNC,RUF006 passes; no facade-rule violations (no logging.getLogger, no direct exporter/instrumentation imports).
• RED metrics and tracing come free via attach_http_observability, so per-route latency and status distribution for the execute path need no PR-side work.
• For reference, the repo ships no dashboards, alert rules or SLO doc (deploy/helm/observability/ is two files), so "new code absent from shipped dashboards" doesn't apply to this PR.

_{Lap 3 of 7 · lenses: concurrency & failure modes · observability & day-2 · head ff8736d3}

[Manuel-Jentic]

Review lap 4/7 — mutation adversary · compound risk

Two lenses this lap: per-claim mutation testing (does a test actually fail when I break each claimed behaviour?) and compound-risk analysis. Findings are cumulative; I don't re-raise across laps.

Verdict: APPROVE-WITH-COMMENTS (unchanged). No new bug in the shipped code — the findings are all test gaps, including one that supersedes a "verified clean" item from my lap 1 report.

Correction to lap 1: the default-preservation mechanism passes for the wrong reason

I recorded "app.state is set only when the handler is non-None, pinned by test_default_container_leaves_handler_unset". The code is right; the test doesn't pin it. Mutating the combined-app site:

app_factory.py:566    if True:    # was: if container.unregistered_url_handler is not None:

gives 17 passed. The test asserts getattr(app.state, "unregistered_url_handler", None) is None, which cannot distinguish "attribute absent" from "attribute present and set to None" — so it's satisfied either way.

Harmless today, because the router uses getattr with a default. But the docstring's guarantee — that leaving it unset "preserves today's 404 exactly" — is unenforced, and this is the single mechanism the whole default-preservation claim rests on.

Fix: assert absence rather than falsiness — assert not hasattr(app.state, "unregistered_url_handler"). That version fails under the mutation.

Three documented claims survive the entire 3570-test suite

Each of these mutations passed tests/unit tests/arch in full:

Mutation	Result
pass request=None to the handler	3570 passed
wrap the call in except Exception: return None (silently turn a broken handler into a 404)	3570 passed
add len(response.body) before the counter — raises AttributeError on any StreamingResponse	3570 passed

The causes are visible in the fixtures: _SpyHandler records only method, upstream_url and identity — never request — and no test constructs a streaming response. So the documented "streaming responses return verbatim" is pinned for the plain-Response case only; Response(content=response.body, …) is caught (2 failed), which is what makes the gap easy to miss.

The middle row is the notable one: it means the error-isolation change I recommended in lap 3 could be implemented incorrectly — swallowing every handler exception into a silent 404 — and no test would object. Worth adding the test before the fix.

Fix: assert spy.calls[0]["request"] is request in test_handler_receives_arguments_verbatim; add test_streaming_handler_response_is_returned_verbatim asserting the returned object is the same instance; add test_raising_handler_propagates to pin whichever behaviour you choose for lap 3's finding.

Compound risk: the seam's Context reach is worse than lap 2 established

Lap 2 showed a handler reaches every deployment secret. This lap I confirmed the corollary on the sibling seam in #1284, and it applies identically here: ctx.config is mutable at runtime.

AppConfig frozen? False
  MUTATED dns_pinning_enabled: True -> False
MUTATED auth.local_login.enabled -> True

So a handler doesn't merely read the deployment's secrets — it can switch off DNS pinning, the very layer egress.py documents as closing validate_upstream_url's TOCTOU window. That's directly relevant to lap 1's egress finding: a handler told "the URL has already passed egress validation" can also unvalidate future requests process-wide.

This strengthens the case for the narrowed value object I proposed in lap 2 (UnregisteredUrlCall with Authorization/Cookie stripped). Passing an immutable view rather than Request/Context closes the read and the write.

Failure-direction note

Across the four PRs, every fail-open path is a security control and every fail-closed path is an availability concern — the exact inversion of what you'd want. This PR contributes one of each: the handler's response is forwarded unvalidated (fail-open, lap 2), while a raising passive observer 500s the request (fail-closed, but needlessly so — lap 3). The fix for the second shouldn't widen the first.

Verified clean this lap

• 6 of 9 claims pinned (67%), and the pins that exist are real, not incidental. Each of these fails a distinct named test under the minimal mutation: inverting resolved is None (2 failed), moving the hook before validate_upstream_url (1 failed — a genuine ordering pin via the marker URL), changing the fall-through detail/type (1 failed), routing the pinned-revision miss into the hook (1 failed), and dropping or moving the counter guard (caught in both directions). Removing the await fails 5 tests.
• Availability ranking: this PR's hook-timeout gap sits below feat(shared): public capability document (GET /capabilities) #1284's unauthenticated contributor stall, because exploiting it needs both a downstream handler and a toolkit token — so it's the lower priority of the two, though the fix is the same shape.
• Assessed and not elevated: this PR's counter combined with feat(shared): public capability document (GET /capabilities) #1284's features map — the counter is attribute-free and features is import-time-bounded, so no request-controlled cardinality emerges in combination.
• Tenant isolation holds across all four PRs, and not vacuously — there is no tenant model to breach (rg 'tenant_id|organization_id|org_id' → zero hits) and the boundary is per-actor throughout.
• No mutation produced a false green from a collection failure; noop baselines confirmed the harness.

_{Lap 4 of 7 · lenses: mutation adversary (per-claim test-kill) · defence-in-depth & compound risk · head ff8736d3}

[Manuel-Jentic]

Lap 5 — integrator/consumer experience + docs-vs-code truth

Verdict: APPROVE-WITH-COMMENTS (unchanged). This lap is the most favourable of the five for this PR: a reviewer wrote a downstream handler from scratch using only the shipped artifacts and it worked first time. One real defect — the only documented composition path is a topology the repo forbids.

---

MAJOR (L-03) — the seam's only documented composition example is the one topology the repo forbids for the broker

docs/development/extending-jentic-one.md:121-131 gives exactly one wiring example:

create_combined_app(ctx, ctx.config.apps, container=container)

But the broker may never be part of a combined app:

        raise RuntimeError("broker must run as the sole surface; do not bundle it with others")

Since the hook fires only in the broker router, any deployment that can reach it must be broker-only — i.e. jentic_one.broker.web.app.create_app(ctx, container=container), which the doc never mentions. Following the doc verbatim produces an app where the handler is set (app_factory.py:566-569) and permanently unreachable — the worst failure shape for a passive observability hook, because there is no error, just silence (compounding H-01, the missing failure-path signal).

Compounding: a broker-only app also needs install_broker_registry_resolver (wiring.py:74) or execute.py:642 raises AttributeError. The reviewer counted 4 round trips to source to reach a working wiring (__main__.py for the sole-surface rule, broker/web/app.py for the right factory, wiring.py for the resolver, app_factory.py to confirm app.state placement).

Fix: replace the combined-app example for this seam with the broker-only one and state the constraint explicitly: "the hook fires only on the broker data plane, which must run as the sole surface — wire it via jentic_one.broker.web.app.create_app(ctx, container=container)", and mention the resolver requirement.

---

Verified clean — this PR's documentation is the best in the stack

Unusually, most of this lap's work on #1281 produced negative results. Recording them so later laps don't re-tread:

• A scratch downstream handler written from the docstring alone passed on the first run (2 passed in 0.73s). The class-shaped requirement, handler_factory override, and import path are all correctly documented (compliance.py:114-127, extending-jentic-one.md:191-193).
• Every question a seam implementer actually asks is answered by the protocol docstring — that it must be async, that it returns Response | None, that None means 404, the keyword-only shape (protocols.py:66-79), and explicitly whether blocking is safe and whether exceptions are safe (:70-71). No round trip to source was needed for any of these.
• AppContainer is discoverable and correctly typed — present in the seam table, the field is the last positional with a None default so existing constructions are unbroken, and the dataclass is non-frozen with unregistered_url_handler in dataclasses.fields.
• This is the only PR in the stack that documents its seam properly — it gets a seam-table row, a full tradeoff callout, and wired composition + compliance examples. (feat(shared): public capability document (GET /capabilities) #1284 adds a one-line row only.)
• Every prose claim in the protocol docstring verified true by execution: the catch-all really is methods=[…,"OPTIONS","HEAD"] (execute.py:1017), so "fires for every catch-all method incl. HEAD/OPTIONS" holds; the discovery miss genuinely precedes _context_from_discovery and all execution/event creation (:648-658 vs :684), so "records no execution row and emits no event" is exact; max_request_bytes(_by_type) has no call site before the miss, so "core's body caps do not apply" is true; the pinned-miss carve-out matches the single call site.
• One small correction: A-19's "the only controls that have run are authentication" understates itself — RequireToolkitAccess = Depends(require_execute_within_rate_limit) (broker/web/deps.py:217) means rate limiting has run too. Already registered.

Worktree clean at ff8736d3.

---

Cumulative for #1281: 0 BLOCKERs. The open items are L-03 (this lap), plus from earlier laps: G-03/H-01 (no error isolation or failure-path signal around the handler call), J-04 (three documented claims — the request argument, error propagation, and verbatim streaming — survive the entire 3,570-test suite), J-05 (the default-preservation test passes for the wrong reason: getattr(…, None) is None cannot distinguish "attribute absent" from "present and None"), and A-18/A-10/B-05 on the compliance base. All are fixable without redesign; the seam's shape is sound.

[Manuel-Jentic]

Lap 6 — merge-result verification (+ Lap 5 addendum)

Verdict: APPROVE-WITH-COMMENTS (unchanged) — and this lap is unambiguously the strongest result for this PR in the stack.

Every prior lap reviewed this branch in isolation. This lap materialised the merge into today's main (1846d24f) and tested the result.

---

Zero conflicts, no artifact churn, gates green

PR	conflicting files	hunks	add/add
this PR	0	0	0
#1286	1	1	0
#1283	3	8	0
#1284	20	131	4

merge result	unit + arch	drift
main baseline 1846d24f	3363 unit / 295 arch	—
main + this PR	3377 passed	control.openapi.yaml, config-schema.json, ctl asset schema, endpoints.* all IN SYNC
main + this PR + #1286 + #1283	3692 passed, 1 skipped, 0 failed	IN SYNC; go build ./... OK

It merges with zero conflicts, touches no generated artifact, adds +14 unit tests, and zero main-side commits have landed on app_factory.py, shared/state/context.py or broker/web/routers/execute.py since the shared base — so nothing changed underneath it. This is the PR to merge first.

---

MAJOR (M-01) — but merging it first also protects it, and here's why that matters

There is one stack interaction, and the ordering is the mitigation. This PR and #1284 mutually replace the same region of src/jentic_one/testing/compliance.py:

$ git diff origin/issue-1277-on-operation-not-found origin/issue-1279-capabilities \
    -- src/jentic_one/testing/compliance.py
 1 file changed, 18 insertions(+), 16 deletions(-)

This PR adds BaseUnregisteredUrlHandlerComplianceTest; #1284 adds BaseCapabilityContributorComplianceTest — at the identical position in the docstring, imports, __all__, and class body. The correct resolution is a union; git offers no default.

The hazard is asymmetric and falls on your work. docs/development/extending-jentic-one.md auto-merges cleanly, and this branch's side documents the base class with a full worked example (verified at :174 and :191). Because tests/unit/testing/test_compliance_oss.py also conflicts, a "take #1284's side" resolution would leave the doc documenting a symbol with no definition and no importer — and nothing would fail, since there is no arch gate:

$ grep -rln "testing.__all__\|jentic_one.testing" tests/arch/
(no matches)

(This supersedes my lap-2 note that this conflict was trivial — true at the older base, false at today's main.)

Fix (mostly on #1284's side): land this PR first, resolve all three testing/ files as an explicit union in #1284's recut, and add an arch assertion that every jentic_one.testing.__all__ symbol referenced in the doc is importable. That last one is worth adding here if you'd rather own it — it's the gate that would have caught this class.

---

Lap 5 addendum (N-03) — the compliance base's first failure is unactionable

A handler written from the doc's seam row and tradeoff callout alone — neither of which says annotations must match — fails with a raw two-signature dump:

AssertionError: MonitorHandler.__call__ parameters diverge from
UnregisteredUrlHandler.__call__: (self, *, method, upstream_url, identity, request)
!= (self, *, method: 'str', upstream_url: 'str', identity: 'Identity',
    request: 'Request') -> 'Response | None'

It never says "add annotations", and never mentions the from __future__ import annotations requirement that the string-vs-object comparison silently imposes (A-18). Net effect: the base helped only after reading protocols.py.

Fix: diff per-parameter and append "copy the signature from UnregisteredUrlHandler.__call__ and add from __future__ import annotations to your module." Adding test_call_is_coroutine_function (inspect.iscoroutinefunction) also closes A-10, since a sync __call__ currently passes.

---

Verified clean on the merge axis

• No gate that passes on this branch fails on its merge result — 3377 passed, all drift gates in sync.
• Zero main-side commits since the shared base on every file this branch touches.
• assert_signature_matches does accept a correct implementation — class-shaped, async, fully annotated, with the __future__ import → 2 passed in 0.56s first try. The "must be class-shaped, not a bare async def" warning is accurate and saves a round trip.
• The generated Go/TS types for the stack's other endpoints are unaffected by this branch, and features map[string]interface{} on feat(shared): public capability document (GET /capabilities) #1284 is a deliberate open-map decision, not an A-15-style regression — which narrows L-06's Go complaint to features ergonomics only.

---

Cumulative for #1281 — still 0 BLOCKERs, and it is the only PR in the stack merging with zero conflicts. Open items are all fixable without redesign: L-03 (the documented composition example wires the handler into an app where it can never fire), G-03/H-01 (no error isolation or failure-path signal around the handler call), J-04/J-05 (four documented claims survive the full 3,570-test suite; the default-preservation test passes for the wrong reason), and A-18/A-10/N-03 on the compliance base.

Merge order: this PR first → #1286 → #1283 (+ regen) → #1284 (recut).

[Manuel-Jentic]

Lap 7 (final) — merge-readiness triage · verdict: READY

Seven laps done. This PR ends the review with an empty blocking set — the only one of the four in epic #1280 to do so. Re-verified against live HEAD ff8736d3 (unmoved) with origin/main at 1846d24f:

$ git merge-tree --write-tree origin/main ff8736d3   # → 0 conflicts
$ pytest tests/arch tests/unit/broker tests/unit/testing tests/unit/shared/web
936 passed

No blocker was raised against this PR in any of the seven laps. Recommendation: merge this first, ahead of #1286, #1283, and #1284.

There is one reason the ordering matters rather than being a nicety, and it's the only thing I'd genuinely gate on:

Merge me before #1284 — otherwise documented public API gets silently deleted (M-01)

$ git merge-tree ff8736d3 6ea7f850
CONFLICT: src/jentic_one/testing/__init__.py
CONFLICT: src/jentic_one/testing/compliance.py
CONFLICT: tests/unit/testing/test_compliance_oss.py

Both PRs add a compliance base to the same region, so __all__ differs by exactly one symbol per side — BaseUnregisteredUrlHandlerComplianceTest here, BaseCapabilityContributorComplianceTest in #1284. The trap is that docs/development/extending-jentic-one.md auto-merges cleanly (confirmed: "Auto-merging") while documenting both symbols at :174 and :191. And nothing catches the mismatch:

$ grep -rln "jentic_one.testing" tests/arch/
(no output)

So a "take one side" resolution leaves the docs advertising a symbol that no longer exists, with no gate failing. Resolve that conflict as an explicit union, not a pick. Landing this PR first makes that the obvious resolution rather than a judgment call.

One line I'd fix before merge (L-03)

The seam's only worked example cannot fire. extending-jentic-one.md:131 composes:

create_combined_app(ctx, ctx.config.apps, container=container)

but the hook is read only at execute.py:607 (broker), and __main__.py:68-69 raises "broker must run as the sole surface". Default apps is ["registry", "admin", "control", "auth"] — no broker. An integrator who follows the doc verbatim sets a handler that is unreachable, with no error.

Fix: return jentic_one.broker.web.app.create_app(ctx, container=container), plus a note about install_broker_registry_resolver. One line, and it's the difference between a usable seam and a confusing one.

Fast-follow (do not block)

#	Finding	Fix
J-05	test_default_container_leaves_handler_unset:123 uses getattr(..., None) is None — cannot distinguish absent from set to None, so it passes for the wrong reason	assert not hasattr(app.state, "unregistered_url_handler")
A-10	isinstance(lambda: None, UnregisteredUrlHandler) → True; the protocol accepts a sync callable the seam will await	add test_call_is_coroutine_function to the compliance base (also closes N-03)
G-03	no error isolation or failure-path signal around the handler call — a broken passive observer becomes a silent 500	wrap in try/except, log, and count failures
H-01/H-02	counter is success-only, against repo convention	add the failure arm

A-18, B-05, D-02 stay as recorded — all cosmetic or docs.

---

Cumulative: 7 laps, 0 blockers, 0 conflicts, 936 tests green. The seam design itself held up under the mutation adversary (lap 4) and the integrator lens (lap 5); what surfaced were test-precision and documentation gaps, not design faults. Fix the L-03 doc line, merge, and take the rest as fast-follow.

[ren-jentic]

Review items addressed in 85e561f (all seven laps triaged — everything actionable is in this PR, nothing deferred to follow-ups):

Fix-before-merge

• L-03 — the seam's worked example is now broker-only: jentic_one.broker.web.app.create_app(ctx, container=container) + install_broker_registry_resolver, with the sole-surface constraint stated in the doc, the seam-table row, and the protocol docstring. The handler is removed from the combined-app example. Belt-and-braces: the factories now log unregistered_url_handler_installed at wire-up, and a handler set on an app with no broker surface logs a unregistered_url_handler_unreachable warning — the "set but permanently unreachable, no error, just silence" shape is no longer silent.

Fast-follow table — done now instead

• G-03/H-01 — the handler call is failure-isolated: except ProblemDetailException: raise (a deliberate downstream problem response), except Exception (never BaseException — cancellation propagates, pinned by a test) → counted + logger.exception with the handler's module.qualname inside the router while request_id is still bound → falls through to the documented 404. A broken passive observer no longer converts the miss into an unattributable 500. Pinned at both the helper and route level (test_route_raising_handler_falls_through_to_404), including the J-04 warning that the isolation must not be a silent swallow (count + log are asserted).
• H-02 — counter is outcome-attributed: broker.unregistered_url.handled{outcome=handled|declined|error} (bounded 3-value cardinality, matching login_counter/_executions_total), pinned in both directions including no-metric-on-default-path.
• J-05 — test_default_container_leaves_handler_unset asserts not hasattr(...); the lap-4 if True: mutation now fails.
• A-10/N-03 — test_call_is_coroutine_function added to the compliance base (a sync __call__ fails compliance instead of 500ing in production); the protocol docstring states the async def requirement.
• J-04 — the spy records request and the test asserts identity; test_streaming_handler_response_is_returned_verbatim asserts same-instance; raising-handler behaviour pinned before the fix per the lap-4 note.
• A-18/N-03 — assert_signature_matches resolves both sides with typing.get_type_hints, so the from __future__ import annotations trap is gone entirely (string vs object annotations and Optional[X] vs X | None compare equal) and the failure message is a per-parameter diff ending in "copy the signature of … verbatim".
• M-01 — took ownership of the gate here: tests/arch/test_testing_public_api.py asserts every jentic_one.testing symbol the extending doc references is exported and every __all__ export resolves, so a "take one side" resolution of the feat(shared): public capability document (GET /capabilities) #1284 conflict now fails a test instead of silently deleting documented API.

Lap 1/2 contract findings

• Egress claim — the docstring now states the boundary precisely: pre-flight validate_upstream_url only, TOCTOU-vulnerable by design, and any fetch must go through HttpClientProvider (or a DnsPinningTransport-wrapped transport). Rate limiting is claimed as having run (D-02).
• Blast radius — the raw-Request shape stays (per the lap-1 assessment that a value object is clunkier and less consistent with the injected-Broker precedent), but the tradeoff block now says the quiet part loudly: via request.app.state the handler reaches the full deployment context (DB sessions, decrypted key material, signing secrets, mutable config) — it is fully trusted deployment code, not a sandboxed plugin. Mirrored in the extending-docs callout.

Deliberately not done (all deferred by the review itself; recording the reasoning): the UnregisteredUrlCall value object and response set-cookie/3xx validation (design change to the seam's shape — the disclosure above is the accepted alternative; the injected Broker has the same trust posture), a timeout around the handler call (ranked below #1284's variant, same-shape fix can land with it), **kwargs/dataclass protocol evolution (would churn the contract this PR just froze; no downstream exists yet and the annotation-resolution fix removes the sharpest edge), and richer event telemetry (out of scope per the issue).

Verified: full tests/unit (3279 passed), tests/arch (300 passed — includes the two new gates), repo-wide ruff + mypy clean (1226 files). PR description updated to the final state.