Skip to content

DRAFT (phase 3, after T063): broker-path credential hardening (follows T080): a minted token keeps a built-in credential's rules (plan 034) - #64

Draft
brettheap wants to merge 11 commits into
build/034-p3b-t080-raw-key-refusalfrom
build/034-p3-broker-credential-hardening
Draft

brettheap wants to merge 11 commits into
build/034-p3b-t080-raw-key-refusalfrom
build/034-p3-broker-credential-hardening

Conversation

@brettheap

@brettheap brettheap commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Lane: openxfactory-4 (openXfactory-4-openDox_extraction)

Arc: neutral-product-standalone-operability

Plan 034, phase 3. This PR is not one of the plan's tasks. It follows T080 (#63), and gives the broker path the rules T080 gave a built-in credential. It is claimed on openxFactory#656, comment 5889279351.

Ruled. Brett Heap answered #63's closing question, "Should the broker path follow it?", in-session on 2026-09-29: "Yes, separate phase-3 draft (Recommended)". No GitHub comment records that answer yet. It extends the T080 ruling "Refuse unless loopback (Recommended)", which is recorded at openxFactory#656 comment 5880893901 and ends "The broker path is unchanged in this task". T080's scope is unchanged. This is the separate draft.

Ruled, for the runner. Brett Heap answered this PR's second flag on 2026-09-29: "Yes, add to #64 (Recommended)". It is recorded at openxFactory#656 comment 5901112350, the lane's latest RULED comment, as item 2. When a broker misbehaves (non-zero exit, oversize answer, or timeout after writing), the shared runner's refusal carries no broker output, no cause and no context. This PR now does that for all four broker operations. To keep the diagnostics useful, each refusal names the operation and the failure class, and never the bytes. See The broker runner below.

This is drafted ahead, and it stays a draft. Every phase-3 PR comes after T063, which has not landed. This PR does not go READY, and gets no READY line, until T063 lands and the holder says so.

Stacked. Its base is #63's branch (build/034-p3b-t080-raw-key-refusal). That is based on #62, which is based on #61. Land #61, #62 and #63 first, then this PR. Retarget this PR to main before #63's branch is deleted.

What changes

At main, and unchanged at #63's head 3f14bb96, the broker path had the four gaps #63's body listed for Brett. Each was measured over real sockets and a real broker child before it was closed, at main 2d116415 and at 3f14bb96 alike. Each is now closed by T080's own rule, reusing #63's helpers.

the gap at 3f14bb96 now
1. A minted token could be declared over plain http:// to any host. Refused when the binding is declared, by the same predicate (is_a_private_route) and with the same fixed ENDPOINT_NOT_PRIVATE sentence. mint asks the predicate again before it asks the broker for anything.
2. A POST answered 301, 302 or 303 reached the redirect's target as a GET carrying the token. With http_proxy set, a request to 127.0.0.1 went to the proxy with it. The request follows no redirect (DIAG_PROVIDER_REDIRECTED), and over plain http:// it uses no proxy.
3. A refused connection chained urllib's URLError, and do_open.headers, _send_request.headers, _send_output.msg, send.data and request.headers held the token. Every refusal of a turn that presented a token chains nothing: no cause and no context.
4. The token was not checked. One outside latin-1 failed inside urllib as DIAG_PROVIDER_UNREACHABLE, which names the wrong party. One with a space or another non-ASCII character was sent as it was. mint asks _presentable of it. Any other token is a malformed answer (DIAG_BROKER_MALFORMED), refused before the port holds it or any provider is contacted.

How, in the code:

  • doxbench_binding._require_a_private_route asks the rule of every record that presents a credential. Only the auth kind none, which presents nothing, keeps whatever route it declares. ENDPOINT_NOT_PRIVATE now names both credentials.

  • BrokeredProviderPort._call_provider makes every provider request, on both paths. With a credential, it uses _open_with_a_credential (DRAFT (phase 3, after T063): T080, 16.3: the credential stays a reference, and a raw key is refused (plan 034) #63's opener, renamed) and raises any refusal afresh. DRAFT (phase 3, after T063): T080, 16.3: the credential stays a reference, and a raw key is refused (plan 034) #63's _dispatch_without_a_broker is rebuilt on it and behaves as before.

  • The broker branch of dispatch answers an expiry outside every handler. The one re-mint and the one paid retry of the 2026-08-26 ruling do what they did, and a refusal raised by either keeps no context.

  • mint reads the answer through _minted_token. Any refusal of it is raised again, afresh, after the answer has left the frame that read it. So the refusal of an unpresentable token keeps no frame that holds it, as the built-in resolver's refusal of an unpresentable value keeps none. The first flag below says what else this covers. Since the runner ruling, the wrapper that does this is _broker_operation, shared by all four operations.

  • After Copilot's second review, an answer that could not be read at all is refused too, with the same fixed sentence:

    • an expiry that is no finite number (an integer past a float's range, NaN, Infinity or -Infinity), in _parse_expires_at;
    • an answer nested past the recursion limit, in _answer_document, for all four broker operations.

    Before, the first escaped mint as an OverflowError and the last as a RecursionError, each with the answer still in its frames. The three non-finite expiries minted a token that would never expire, or would always have expired.

  • DIAG_PROVIDER_REDIRECTED, _DeclineRedirects, _presentable and _PresentedCredential now name both credentials. The four gaps left FIXED_DIAGNOSTICS at eleven. The runner ruling adds a twelfth (below).

The broker runner (the second ruling)

Measured at 788d764b, this PR's head before the ruling, with a stand-in broker that wrote a fake token first:

the broker at 788d764b now
exits non-zero refused, but the token stayed in the runner's frame: answer, and the child's _fileobj2output DIAG_BROKER_REFUSED, keeping nothing
answers past the 64 KiB bound the same, and refused as DIAG_BROKER_MALFORMED, like an answer of the wrong shape DIAG_BROKER_OVERSIZE, a new sentence, keeping nothing
times out, with its output open or closed the refusal chained the TimeoutExpired, whose output, and whose frames inside subprocess, held the token DIAG_BROKER_TIMEOUT, with no cause and no context
answers in bytes that are not UTF-8 no refusal at all: a UnicodeDecodeError escaped every operation, holding the token in object DIAG_BROKER_MALFORMED (or DIAG_BROKER_TIMEOUT if it then hangs), keeping nothing
cannot be started refused, chaining the FileNotFoundError DIAG_BROKER_UNREACHABLE, with no cause and no context
writes without end (Copilot at 25788f91) read in full until the timeout, then refused as a timeout: the bound limited what was accepted, not what was read refused with DIAG_BROKER_OVERSIZE as soon as it passes the bound, and killed
has a descendant holding its output open (Copilot at b847ef3d and a603a032) at e3eec6b1, never refused: the reader waited on the pipe after the broker was killed (still waiting after 10 s, with a 0.5 s timeout). At a603a032, a descendant that left the group held the refusal for a 2 s grace and left a blocked reader thread, with its descriptor, behind. refused at the timeout. The broker's process group is killed whole, and this process's ends of both pipes are closed, so no thread or descriptor is left behind.
never reads a credential larger than its pipe at a603a032, the refusal waited past 10 s: the timeout began only once the credential was written refused at the timeout, which now covers the credential's streaming too
its credential source fails mid-copy (the operator's input) at b847ef3d, the broker was left running with its reader blocked, and the interpreter aborted at exit (Fatal Python error: _enter_buffered_busy) the broker is reaped before the error goes on, and what it wrote is dropped. The error itself is unchanged (flag 9).

At 788d764b no refusal named its operation either. Two of the sentences said "so no token could be minted", whichever operation had failed.

How, in the code:

  • The runner's work moves to _run_broker, which returns an answer or a sentence and raises no refusal. subprocess_broker_runner raises the refusal after it returns. So the refusal is outside every handler, in a frame that never held the child or its answer.
  • One selector loop in the calling thread (_answer_of) streams the credential to the broker in PIPE_BUF writes and reads the answer, as communicate does on POSIX. There is no reader thread. One deadline covers both. At most one byte past MAX_BROKER_ANSWER_BYTES is read, straight into one list, so no other name holds it. The child's exit is awaited within what is left of the timeout.
  • The broker starts in its own process group (process_group=0; the session and the terminal stay this process's). _reap kills that group, waits for the broker, and closes this process's ends of both pipes. Anything else that escapes the runner, such as a failing credential source, is reaped the same way before it goes on, and what the broker wrote is dropped.
  • The answer is decoded as UTF-8, JSON's own encoding, where communicate used the locale's. An answer that is not UTF-8 is malformed.
  • The four operations ask through one wrapper, _broker_operation, which replaces mint's own. It raises every refusal again, afresh, with the operation named, after the answer has left its frame. So a runner that was injected is covered too, as are refused intake, revoke and list answers.
  • BrokerRefused takes operation=, from OPERATIONS only and only beside a broker's sentence (BROKER_DIAGNOSTICS). Its message reads broker <operation>: <sentence>, for example broker intake: the credential broker exited non-zero, and its answer is withheld by design. .diagnostic is still the sentence alone, so every caller that compares it is unchanged.
  • The broker's five sentences now name a failure class and no operation. FIXED_DIAGNOSTICS has twelve.

Files: src/opendox/doxbench_binding.py, src/opendox/doxbench_provider.py and tests/test_model_provider_broker.py, all in P3-B's row. The runner's five commits touch the last two only. cli_model_binding.py, serve_workbench.py, validate.yml and pyproject.toml are untouched.

Flags, for Brett and the holder

  1. One step past the fourth gap's letter. The wrapper that keeps gap 4's own refusal clean covers every refusal of the mint answer. So a malformed answer beside a good token no longer keeps the token either. At 3f14bb96 it did: mint.answer, mint.document, _answer_document.text and _answer_document.document. Copilot's second review took the same rule to answers that raised something other than a refusal (see How above). This is T080's rule, that no frame a refusal keeps holds the raw credential, applied in the same function. The runner ruling then took it to all four operations. Narrowing it would need a separate cleanup for gap 4's refusal alone; say so if you would rather have that.

  2. Closed: the broker runner's own refusals. This was the open question at 788d764b. It is ruled (see Ruled, for the runner) and done (see The broker runner).

  3. A stored record can stop reading. A broker binding already stored with plain http:// to another host now refuses when the document is read. The entry point falls back to the harness declaration and says so. model-binding edit and remove read the whole document first, so they refuse too, and the operator edits the file by hand to an https:// or loopback endpoint. T080's refusal of a key in a stored URL set the precedent (test_a_stored_document_whose_endpoint_carries_a_key_does_not_read).

  4. T080's scope pin is replaced. test_the_loopback_rule_is_the_built_in_resolvers_alone pinned T080's scope by declaring a broker binding on two plain-http:// routes. That half is gone. The case is now test_a_none_binding_keeps_a_route_that_is_not_private, over all nine routes. DRAFT (phase 3, after T063): T080, 16.3: the credential stays a reference, and a raw key is refused (plan 034) #63's body cites the old case among the broker path's pre-existing gaps, which this PR closes.

  5. A reading. An opener installed process-wide with urllib.request.install_opener no longer serves a request that carries a credential. It already did not serve a built-in one. Nothing in openDox installs one.

  6. The operator reads new text. model-binding set-credential and the console's intake route both show a broker refusal's str(). That now reads broker <operation>: <sentence>, and the broker's sentences are reworded. No test here or in openXdox-code compares the old text; every test compares the constants. serve_workbench.py's intake comment still says a refusal carries "one of FIXED_DIAGNOSTICS and nothing else". The operation it now names comes from a closed vocabulary. The file is left alone because T055, serve and generate standalone (5.5, 4.3 part) (plan 034) #59 edits it.

  7. A twelfth sentence. An answer past the bound had DIAG_BROKER_MALFORMED, and now has DIAG_BROKER_OVERSIZE, so the refusal names that class. The provider's own bound stays on DIAG_PROVIDER_MALFORMED. Say so if you would rather keep eleven, and the old class.

  8. Two cases past the ruling's three.

    • An answer that is not UTF-8 was not a refusal at all at 788d764b. It escaped as a UnicodeDecodeError holding the token. It is the same runner and the same rule, so it is refused here too.
    • Copilot's review at 25788f91 found the bound was checked only after the whole answer was read. The bound now limits the read itself.
  9. Not changed: the input side of intake. This was measured at 25788f91 and again at b847ef3d. Since e3eec6b1 the broker is reaped first, and the error is otherwise unchanged. When the credential's own source fails while it is copied to the broker, the error escapes the runner and is not refused:

    • A lone surrogate in the credential escapes as a UnicodeEncodeError whose object holds the credential. sys.stdin can yield one under surrogateescape. The stand-in broker still received, and stored, the credential without that character.
    • A source that fails to decode mid-copy escapes as a UnicodeDecodeError.

    This is the operator's input, not the broker's output, so it is outside the ruling. Should a follow-up refuse it?

  10. The runner is POSIX-only now. The selector loop reads and writes pipes, which Windows' selectors cannot watch, and the process group needs os.killpg. On a platform without killpg the broker alone is killed. Nothing in this repository runs on Windows, and CI is Linux. Say so if the runner must also run there.

  11. Ruled: the operator's input stays without a time limit. Copilot at e75900ff said a credential source whose read() blocks (sys.stdin in the CLI, the request body at the console) holds the loop, and the broker, past the deadline. That is an operator or a client still sending the credential, not a broker that misbehaves. Brett Heap ruled on 2026-09-30, openxFactory#656 comment 5916000030, item 5: "Leave unbounded (Recommended)". Operator input is outside the broker-runner ruling. No code changed for it.

For the holder, downstream:

The tests

The four gaps' cases fail at 3f14bb96

Each gap's cases ran with 3f14bb96's src (the base, whose broker path is main's) and this branch's test module. They fail there on the behaviour itself, never on a missing name: DID NOT RAISE, the wrong sentence, a cause or a context set, a frame holding the token, or an OverflowError or RecursionError escaping. The controls beside them pass there too.

gap its cases at 3f14bb96
1 a broker binding refused on nine routes, by the constructor and from a stored record; mint asking no broker for a binding forced past the record; the operator door refusing one and storing nothing 11 fail. 15 controls pass: six private routes, and nine none routes.
2 and 3 five redirect codes declined over real sockets, with nothing heard elsewhere; a stand-in proxy hearing nothing; a real refused connection keeping no frame that holds the token; four refusals (unreachable, refused, malformed, expired twice) and a refused re-mint, each with no cause and no context 12 fail. T080's refactored redirect and proxy cases (6) pass.
4 eight unpresentable tokens refused before any request, with the catalog unavailable and nothing recorded; one outside latin-1, over a real socket; an unpresentable token kept in no frame; eight malformed answers (a bad instant, an undeclared key, JSON cut short, an expiry past a float, NaN, Infinity, -Infinity, nesting past the recursion limit) keeping no frame, cause or context 18 fail. 2 controls pass: every printable ASCII character but the space is presented unchanged, and the declared answer mints.

The proxy cases, T080's included, now drop urlopen's cached global opener first (_an_environment_proxy). urlopen reads the proxy environment only when it builds that opener. Without the reset, the broker's proxy case passed at 3f14bb96 whenever an earlier test had built it.

T080's redirect and proxy scaffolds are shared with the broker's cases (_a_provider_that_redirects, _an_environment_proxy). T080's two route lists are shared parametrize marks (ON_A_PRIVATE_ROUTE, NOT_ON_A_PRIVATE_ROUTE). T080's node ids do not change.

The runner's cases fail at 788d764b

Each stand-in broker writes a fake token and marks that it did, then misbehaves. Each case checks the mark, so a slow start cannot pass it vacuously. Then it checks three things, in this order: no cause and no context; nothing kept (no frame local, no attribute of one, and nothing in the refusal itself holds the token); and the sentence, the operation and the message. With 788d764b's src and this branch's test module, 43 of the 44 cases fail, on the behaviour:

  • 10 hold the token in a frame;
  • 10 let a UnicodeDecodeError escape;
  • 10 chain a TimeoutExpired;
  • 4 chain a FileNotFoundError;
  • 1 reads until the timeout;
  • 5 name no operation;
  • 1 prints the old text;
  • 2 are the two updated counts.
the runner's cases cases at 788d764b
the shared runner, called directly, for six misbehaviours: exits non-zero, answers past the bound, times out, closes its output and times out, answers in no UTF-8, and answers in no UTF-8 and times out 6 6 fail
each of the four operations, through the real runner, for the same six 24 24 fail
a broker that writes without end, refused at the bound well inside a 5 s timeout 1 fails. It is also the one case that fails at 25788f91 (5.1 s measured).
a broker that cannot be started, by the runner and by each operation 4 4 fail
an injected runner's refusal, named by each operation 4 4 fail
the operation vocabulary, and a provider's sentence refused an operation 1 fails
the operator's set-credential door, with a broker that wrote and exited non-zero 1 fails
updated: twelve sentences; the bound's own sentence, with an answer at the bound as its control 2 2 fail
a guard that the cases ask every declared operation 1 passes

Five later cases answer Copilot and a finding of this PR's own, each measured at the head it answers:

  • a descendant holding the broker's output, in its group and out of it (2). Each is refused at the timeout, leaves no thread and no descriptor, and an in-group descendant is killed. Both hang at e3eec6b1. The one that left the group takes 2.5 s at a603a032.
  • a broker that reads 5000 bytes of a 1 MB credential and stops is refused at its 0.5 s timeout. At a603a032 it held the refusal past 10 s.
  • a credential source that fails mid-copy, in a child interpreter, exits 1 with its own error. At b847ef3d it aborts, 3 of 3 runs.
  • the same in this process, after the answer has been read: what escapes keeps nothing the broker wrote, and the broker is not left running. It found a local, chunk, holding the answer, which is removed. Its two mutants are killed.

Thirty-five mutants, each run against the whole module at e75900ff, and each killed:

mutant tests that fail
the declaration's rule back to the built-in resolver alone 10: the nine broker routes, and the operator door
mint's own route check removed 1: test_mint_asks_no_broker_for_a_token_on_a_route_that_is_not_private
the rule asked of none too 10: the nine none routes, and T080's operator-door case
the credential opener kept for the built-in resolver alone 6: the five broker redirects, and the broker's proxy case
the redirect-declining handler dropped 10: every redirect, on both paths
the proxy bypass removed 2: the proxy case, on both paths
a refusal re-raised with its chain, whatever was presented 5: three chain cases, and both refused-connection cases
the expiry answered inside its handler, as at the base 2: expired twice, and the refused re-mint
the token's check back to non-blank text 10: the eight tokens, the real socket, and the frame case
each operation's refusals raised where they happen (no wrapper) 42
the answer kept in the operation's frame 9: the eight malformed answers, and the unpresentable token's frames
the operation's refusal raised inside its handler 43
the finite check on an expiry removed 4: an expiry past a float, NaN and both infinities
an integer past a float's range left to float() 1: an expiry past a float
a RecursionError from the JSON reader not caught 1: the answer nested past the limit
runner: the non-zero exit's answer handed back beside the refusal 1: the runner's exit case
runner: the non-zero exit refused where it is seen, as at 788d764b 1: the same
runner: past the bound refused as DIAG_BROKER_MALFORMED, as at 788d764b 7
runner: past the bound refused where it is seen 2
runner: the bound's check removed, so the whole output is read 7
runner: a timeout on the exit refused inside its handler 1: the runner's closed-output case
runner: a timeout in the loop refused where it is seen 2: the runner's two open-output timeouts
runner: no deadline on the child's exit 5: the closed-output case, by the runner and by each operation
runner: the credential written whole, blocking past the deadline 1: the broker that never reads the credential
runner: the decode error not caught 5: no UTF-8, by the runner and by each operation
runner: a broker that cannot be started refused inside the handler 4
runner: anything else escaping, the broker not reaped 1: the failing source, in this process
runner: anything else escaping, what the broker wrote kept 1: the same
runner: a descendant, the broker killed alone 1: the descendant in its group
runner: a descendant, the broker given no group of its own 1: the same
the refusal names no operation 34
an undeclared operation accepted 1: the vocabulary case
an operation named beside a provider's sentence 1: the same
the message does not name the operation 30
(control) the presentability test too strict 2: the printable-ASCII cases, on both paths

Review

Each inline thread is answered with evidence and resolved.

Copilot at verdict threads answered in
af457e66 Needs a closer look the redirect case's docstring read as repeating 303 a2c838a0, the docstring only
a2c838a0 Changes recommended an expiry past a float's range escaped mint, and NaN and the infinities were taken 788d764b, which also closed the RecursionError of the same class
788d764b Needs a closer look none. Its summary names the stacked prerequisites, which are the draft flag at the top.
25788f91 Changes recommended the answer's bound limited what was accepted, not what was read (high); this description was stale (low) b847ef3d, and this description
b847ef3d Changes recommended a descendant holding the broker's output made _reap wait forever; this description was stale (read before it was updated) a603a032, and this description. e3eec6b1 between them reaps a broker whose credential source fails.
a603a032 Changes recommended a descendant that left the group left a blocked reader thread and its descriptor behind each refusal; that reader could add to the answer after it was dropped e75900ff
e75900ff Changes recommended a credential source whose read() blocks holds the loop past the deadline ruled: left unbounded (flag 11). The thread is answered with the ruling and resolved, and Copilot is asked again at this head.
e75900ff, asked again Needs a closer look none. Its summary asks for a final human review of the subprocess rewrite, and names the stacked prerequisite, which is the draft flag at the top.

SonarCloud's check passed at each head. Its API, read at af457e66 and at 788d764b, reports 0 issues and the quality gate OK: 0.0% duplication on new code, and every security hotspot reviewed.

The repository's own suite

Local runs of the whole suite, as CI runs it (python -m pytest -q), use a PostgreSQL 16 service for tests_runtime and LANG=C.UTF-8:

tree passed skipped failed
#63's head 3f14bb96 2630 11 0
788d764b, before the runner ruling 2686 11 0
this head e75900ff 2733 11 0

The +103 cases are all in tests/test_model_provider_broker.py:

  • 24 for gap 1 (26 added, and T080's two-route scope case removed);
  • 12 for gaps 2 and 3;
  • 20 for gap 4, with the malformed answers;
  • 47 for the runner.

None skips, and skipped stays at the pinned 11. F16.1's record block still passes (dialect and model declared; a raw key is refused in a field and in the URL), and tests/test_provider_boundary.py passes (24).

CI's validate at e75900ff (run 36745758101) read selected=2744 passed=2733 skipped=11 failures=0 errors=0, against the pins 2476, 2465 and exactly 11. The floors are not raised here: validate.yml is outside the draft-ahead scope, as it is for #61 to #63.

🤖 Generated with Claude Code

brettheap and others added 4 commits September 29, 2026 11:38
…T080)

A broker binding over plain http:// to any host but this one is now refused
when it is declared, with the one ENDPOINT_NOT_PRIVATE sentence a built-in
credential earns on the same route. `mint` asks the same predicate before it
asks the broker for anything, as the built-in resolver does before it reads,
so a binding forced past the record still mints nothing. Only the auth kind
`none`, which presents no credential, keeps a route that is not private.

This is the first of the four broker-path gaps openDox-code#63 listed for
Brett. His word of 2026-09-29, given in-session on #63's closing question:
"Yes, separate phase-3 draft (Recommended)". The T080 ruling it extends is
recorded at openxFactory#656 comment 5880893901.

ENDPOINT_NOT_PRIVATE now names both credentials. T080's two route lists
become shared parametrize marks, so the broker cases reuse them and T080's
node ids do not change. T080's case that pinned the old scope (a broker
binding declared on these routes) keeps only its `none` half, over all nine.

Tests: 17 broker cases and 9 `none` cases added, 2 removed. With T080's head
3f14bb9 as the source, the 11 gap cases fail (DID NOT RAISE) and the 15
controls pass.

Arc: neutral-product-standalone-operability
Lane: openxfactory-4 (openXfactory-4-openDox_extraction)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…hains nothing

Gaps 2 and 3 of the four broker-path gaps openDox-code#63 listed for Brett,
closed with T080's own rules for a built-in credential (his word of
2026-09-29, given in-session).

One port method, `_call_provider`, now makes every provider request, on both
paths. A request that carries a credential (a broker's minted token, or what
the built-in resolver read) goes through `_open_with_a_credential`, which is
T080's opener renamed. It declines every redirect, and over plain http:// it
uses no proxy. Its refusal is raised afresh, with no cause and no context.
The auth kind `none` presents nothing, so it keeps the default opener and
its refusal's chain, as before.

The broker branch of `dispatch` answers an expiry outside every handler. The
one re-mint and the one paid retry of the 2026-08-26 ruling do what they
did, and a refusal raised by either keeps no context.
DIAG_PROVIDER_REDIRECTED and the redirect handler now name both credentials.

Measured at T080's head 3f14bb9, over real sockets and a real broker child:
- a POST answered 301, 302 or 303 reached the redirect's target as a GET
  that carried the token;
- with http_proxy set, a request to 127.0.0.1 went to the proxy with it;
- a refused connection chained urllib's URLError, and do_open.headers,
  _send_request.headers, _send_output.msg, send.data and request.headers
  held the token.

Tests: 12 broker cases. With 3f14bb9 as the source all 12 fail, and T080's
two refactored cases pass. T080's redirect and proxy cases now share their
scaffolds with the broker's (`_a_provider_that_redirects`,
`_an_environment_proxy`). The proxy scaffold drops urlopen's cached global
opener, so the proxy environment is read the way a process started with it
reads it. The ENDPOINT_NOT_PRIVATE sentence is rewrapped, with the same text.

Arc: neutral-product-standalone-operability
Lane: openxfactory-4 (openXfactory-4-openDox_extraction)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…keeps it

Gap 4 of the four broker-path gaps openDox-code#63 listed for Brett, closed
with T080's own rule for a built-in credential (his word of 2026-09-29,
given in-session).

`mint` now asks `_presentable`, the built-in resolver's own test, of the
token: non-empty printable ASCII with no whitespace. Any other token is a
malformed answer (DIAG_BROKER_MALFORMED). It is refused before the port
holds it or any provider is contacted, so the catalog reads unavailable and
no mint is recorded. At T080's head 3f14bb9:
- a token outside latin-1 failed inside urllib as DIAG_PROVIDER_UNREACHABLE,
  which names the wrong party, with do_open.headers and putheader.values
  holding it;
- one with a space or another non-ASCII character was sent as it was.

The refusal keeps no frame that holds the token, as the built-in resolver's
refusal of an unpresentable value keeps none. `mint` reads the answer
through `_minted_token` and raises any refusal again, afresh, after the
answer has left its frame. That covers every refusal of the mint answer,
which goes one step past the fourth gap's letter: a malformed answer beside
a good token also kept it at 3f14bb9 (mint.answer, mint.document,
_answer_document.text and _answer_document.document).

Not changed: the broker runner's own refusals (a non-zero exit, an answer
past the bound, a timeout) still keep what a misbehaving broker wrote. The
runner is shared by all four broker operations and sits outside the
provider call the ruling names. The PR flags it for Brett.

Tests: 15 cases. With 3f14bb9 as the source the 13 gap cases fail, and
the 2 controls pass (every printable ASCII character but the space is
presented unchanged; the declared answer mints).

Arc: neutral-product-standalone-operability
Lane: openxfactory-4 (openXfactory-4-openDox_extraction)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The two unpresentable-token literals outside ASCII are written as
backslash-u escapes, as the rest of this module writes them, so the
source adds no non-ASCII character. The gap-1 control and the operator
door's case each gain a line saying what they hold. The `none` case says
how T080 once pinned its scope there (two routes, with a broker binding).
No test changes what it runs.

Arc: neutral-product-standalone-operability
Lane: openxfactory-4 (openXfactory-4-openDox_extraction)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 29, 2026 12:11
@sourcery-ai

sourcery-ai Bot commented Sep 29, 2026

Copy link
Copy Markdown

Reviewer's Guide

Hardens broker-minted credentials by extending T080’s private-route, redirect, proxy, token-presentability, and clean-refusal rules from built-in credentials to the broker path, centralizing provider dispatch and adding comprehensive regression and traceback-hygiene tests.

Sequence diagram for broker token minting and protected provider dispatch

sequenceDiagram
    participant Binding as ModelProviderBinding
    participant Port as BrokeredProviderPort
    participant Broker as Broker
    participant Provider as Provider

    Binding->>Binding: is_a_private_route(endpoint)
    alt route is not private
        Binding-->>Port: BindingRefused(ENDPOINT_NOT_PRIVATE)
    else private route
        Port->>Broker: mint(binding)
        Broker-->>Port: minted token
        Port->>Port: _minted_token(answer, binding)
        alt token is not presentable
            Port-->>Port: BrokerRefused(DIAG_BROKER_MALFORMED)
        else token is presentable
            Port->>Provider: _call_provider(token, model, prompt)
            alt redirect or refusal
                Port-->>Port: BrokerRefused(DIAG_PROVIDER_REDIRECTED or refusal)
            else provider response
                Provider-->>Port: prose
            end
        end
    end
Loading

State diagram for broker token lifecycle

stateDiagram-v2
    [*] --> RouteChecked
    RouteChecked --> RefusedRoute: not private
    RouteChecked --> BrokerAsked: private route
    BrokerAsked --> AnswerValidated: mint answer received
    AnswerValidated --> RefusedMalformed: token not presentable
    AnswerValidated --> TokenHeld: valid token
    TokenHeld --> ProviderCalled: _call_provider
    ProviderCalled --> RefusedRequest: redirect or credential refusal
    ProviderCalled --> TokenExpired: _TokenExpired
    TokenExpired --> BrokerAsked: re-mint
    TokenExpired --> RefusedExpiredTwice: retry token expired
    ProviderCalled --> [*]: prose returned
    RefusedRoute --> [*]
    RefusedMalformed --> [*]
    RefusedRequest --> [*]
    RefusedExpiredTwice --> [*]
Loading

File-Level Changes

Change Details Files
Apply private-route enforcement consistently to all bindings that present credentials.
  • Reject broker-backed bindings over non-private HTTP routes with the shared fixed diagnostic.
  • Recheck the route in mint before invoking the broker.
  • Preserve unrestricted routes only for the credential-free auth kind.
src/opendox/doxbench_binding.py
src/opendox/doxbench_provider.py
tests/test_model_provider_broker.py
Harden broker-token provider requests using the same transport and error-handling rules as built-in credentials.
  • Route credential-bearing requests through the redirect-blocking, proxy-bypassing opener.
  • Centralize provider calls across broker and built-in paths.
  • Re-raise credential-bearing refusals without causes or contexts, including expiry retries.
src/opendox/doxbench_provider.py
tests/test_model_provider_broker.py
Validate minted tokens and prevent rejected mint answers from retaining credential-bearing state.
  • Require minted tokens to satisfy the existing presentability predicate before any provider request.
  • Re-raise mint-answer refusals after answer parsing frames are released.
  • Expand credential-related diagnostics and helper documentation for broker tokens.
src/opendox/doxbench_provider.py
tests/test_model_provider_broker.py
Expand broker-path regression coverage for route, transport, refusal, and token-validation behavior.
  • Add real-socket tests for redirects, proxy bypass, refused connections, and malformed tokens.
  • Test stored bindings, CLI rejection, mint short-circuiting, and traceback/context cleanliness.
  • Share route and transport fixtures with built-in credential tests and retain controls for allowed behavior.
tests/test_model_provider_broker.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Security-sensitive credential handling and unlanded stacked dependencies require final human review.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
What changed in this PR

Hardens broker-minted credentials to match built-in credential protections.

Changes:

  • Enforces private routes for credential-bearing bindings.
  • Blocks redirects/proxies and removes exception chains containing credentials.
  • Validates minted tokens and expands regression coverage.
File Description
src/​opendox/​doxbench_binding.py Applies private-route rules to broker bindings.
src/​opendox/​doxbench_provider.py Secures token minting and provider requests.
tests/​test_model_provider_broker.py Adds route, transport, validation, and leakage tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/test_model_provider_broker.py Outdated
Copilot's review of openDox-code#64 at af457e6 (thread 4133295457): the
docstring's "301, 302 or 303" wrapped so that it read as repeating 303, and
"a 307 or 308 read as the provider refusing" was hard to parse. It now says
that at T080's head urllib followed only the 301, 302 and 303 codes, with a
GET that still carried the token, and refused a 307 or a 308 with
DIAG_PROVIDER_REFUSED, as the base probe measured. No test changes what it
runs.

Arc: neutral-product-standalone-operability
Lane: openxfactory-4 (openXfactory-4-openDox_extraction)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 29, 2026 12:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Malformed expiry values can still expose minted tokens through unsanitized exception tracebacks.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread src/opendox/doxbench_provider.py
Copilot's review of openDox-code#64 at a2c838a (thread 4133360345):
`_parse_expires_at` let an integer past a float's range escape as an
OverflowError, and took NaN and the infinities as an expiry. `mint`
re-raises only a BrokerRefused afresh, so the OverflowError escaped with the
answer still in its frames. That defeats this PR's rule that no refusal of a
mint keeps the answer. A NaN or infinite expiry also minted a token that
would never expire, or would always have expired.

An expiry must now be a finite number, or an ISO-8601 instant as before.
Anything else is a malformed answer (DIAG_BROKER_MALFORMED), which `mint`
raises again holding nothing. The same measurement found one more escape of
the same class: arrays nested past the recursion limit fit inside the 64 KiB
answer bound (30,000 of them in 60 KB), and json.loads' RecursionError
escaped `_answer_document`. That is now malformed too, for all four broker
operations.

Tests: 5 cases join the malformed-answer case (an expiry past a float, NaN,
Infinity, -Infinity, and an answer nested past the limit), and each answer
is held inside the runner's bound. With 3f14bb9 as the source all 8 of that
case's params fail. Three mutants are killed: the finite check removed (4
fail), the overflow left to float() (1), and RecursionError not caught (1).

Arc: neutral-product-standalone-operability
Lane: openxfactory-4 (openXfactory-4-openDox_extraction)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 29, 2026 12:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Security-sensitive credential handling and the explicitly unmet stacked prerequisite require final human review.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Brett Heap's word of 2026-09-29 (openxFactory#656, comment 5901112350):
"Yes, add to #64". When a broker misbehaves (it exits non-zero, answers
past the size bound, or times out after writing), the shared runner's
refusal carries no broker output, no cause and no context, for all four
operations. The refusal names the operation and the failure class, and
never the bytes.

What was measured at 788d764, with a broker that wrote a fake token
first:
- exit non-zero, or past the bound: the token stayed in the runner's
  frame (answer, and the child's _fileobj2output). Past the bound read
  as MALFORMED.
- timeout: the refusal chained the TimeoutExpired, whose output and
  whose frames inside subprocess held the token.
- an answer that is not UTF-8: a UnicodeDecodeError escaped holding the
  token, and no refusal was raised at all.
- no refusal named its operation, and two of the sentences said "so no
  token could be minted" for intake, revoke and list too.

The runner's work moves to _run_broker, which returns an answer or a
sentence and raises no refusal of its own. subprocess_broker_runner
raises the refusal after that call returns, so it is outside every
handler and in a frame that never held the child. A decode error is
caught, and so is a decode error while a refused child is reaped.

The four operations now ask through one wrapper, _broker_operation. It
raises each refusal again, afresh, with the operation named, so an
injected runner is covered too. BrokerRefused takes
operation=, but only from OPERATIONS and only beside a broker's sentence
(BROKER_DIAGNOSTICS). Its message reads "broker <operation>: <sentence>",
and .diagnostic is unchanged. The broker's sentences no longer name an
operation. An answer past the bound has its own sentence,
DIAG_BROKER_OVERSIZE, so there are twelve fixed diagnostics.

Tests: 36 new cases. They cover the shared runner for five misbehaviours,
each of the four operations for each misbehaviour, a broker that cannot
be started, an injected runner, the operation vocabulary, and the
operator's set-credential door. With 788d764 as the source, 37 cases
fail: the 36 new ones except the one guard, plus the two tests that were
updated. 14 new mutants are killed, and all 16 earlier ones are still
killed.

Arc: neutral-product-standalone-operability
Lane: openxfactory-4 (openXfactory-4-openDox_extraction)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Broker stdout remains unbounded in memory, and the PR description materially understates the current scope.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)

Comment thread src/opendox/doxbench_provider.py Outdated
Comment thread tests/test_model_provider_broker.py
Copilot's review of #64 at 25788f9 (high): communicate() read all of a
broker's output before MAX_BROKER_ANSWER_BYTES was checked. So the bound
limited nothing in memory, and a broker that wrote without end was read
until the timeout and refused as a timeout.

_run_broker now reads the answer on a reader thread, at most one byte past
the bound. A broker that writes that byte is refused with
DIAG_BROKER_OVERSIZE and killed at once. The thread starts before the
credential is written, so the answer drains while intake's stdin is
streamed. The runner then waits for the child's exit within what is left
of the timeout. The answer is decoded as UTF-8 (JSON's encoding; the
locale's before), and an answer that is not UTF-8 is malformed. Every
refusal is still raised by subprocess_broker_runner after _run_broker has
returned, so it keeps no cause, no context and nothing the broker wrote.

Tests: a broker that writes without end is refused at the bound, well
inside a 5 s timeout. At 25788f9 it read until the timeout (5.1 s
measured), and that is the one case that fails there. A sixth
misbehaviour, a broker that closes its output and then hangs, is refused
as a timeout by the runner and by each operation. With 788d764 as the
source, all six new cases fail too (43 of the 44 runner cases). The
runner's mutants are rewritten for the new code. All 31 mutants are
killed, among them the whole output read as at 25788f9, and no deadline
on the child's exit.

Arc: neutral-product-standalone-operability
Lane: openxfactory-4 (openXfactory-4-openDox_extraction)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 00:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Broker cleanup can hang beyond its timeout, and the PR description materially understates the changed scope.

Review effort: Balanced
Findings: 1 High severity · 2 Low severity

Open (3)
Resolved since last review (1)

Comment thread src/opendox/doxbench_provider.py Outdated
Comment thread src/opendox/doxbench_provider.py
b847ef3's reader is a daemon thread. When the credential's own source
failed mid-copy (the operator's input, which the ruling does not reach),
the error escaped with the child still running and the reader blocked on
it, and the interpreter aborted at exit ("Fatal Python error:
_enter_buffered_busy"), measured 3 of 3 times.

_run_broker now reaps the child before any other exception goes on, and
drops what the child wrote. The work after the reader starts moves to
_answer_of, unchanged. The runner's tests also assert that no refusal
keeps a frame holding the child. That kills three mutants that the reap
had made equivalent: a refusal raised where it is seen, for the exit, the
bound and the timeout.

Tests: the failing source in a child interpreter exits 1 with its own
UnicodeDecodeError and no fatal error. At b847ef3 it aborts. In this
process, with a broker that wrote first, what escapes keeps nothing the
broker wrote. All 33 mutants are killed.

Arc: neutral-product-standalone-operability
Lane: openxfactory-4 (openXfactory-4-openDox_extraction)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 15:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Broker intake can block indefinitely before its timeout begins, and the PR description contradicts the expanded runner scope.

Review effort: Balanced
Findings: 2 High severity · 2 Low severity

Open (4)

Comment thread src/opendox/doxbench_provider.py Outdated
…boundedly

Copilot's review of #64 at b847ef3: a descendant that inherits the
broker's standard output kept the pipe open after the broker was killed,
so the reader, and the refusal, waited forever. That was measured at
e3eec6b: a broker with such a descendant and a 0.5 s timeout was still
not refused after 10 s.

The broker now starts in its own process group (process_group=0; the
session and terminal stay this process's). A refusal kills the whole
group, and waits at most BROKER_REAP_SECONDS (2 s) for the reader. A
descendant that left the group is left to the daemon reader, and the
refusal does not wait on it. The reader reads the descriptor with
os.read, so a reader still blocked at interpreter exit holds no buffered
lock that finalization needs.

Tests: a descendant in the group, refused within the timeout plus the
grace, and one that left it (setsid), refused within the grace plus a
margin. Both hang at e3eec6b. The failing-source case now also asserts
that the broker is not left running. All 36 mutants are killed, among
them the broker killed alone, no group of its own, and an unbounded wait
for the reader.

Arc: neutral-product-standalone-operability
Lane: openxfactory-4 (openXfactory-4-openDox_extraction)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 15:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Broker cleanup can still deadlock, leak resources, and re-expose buffered broker output.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
Resolved since last review (4)

Comment thread src/opendox/doxbench_provider.py Outdated
Comment thread src/opendox/doxbench_provider.py Outdated
Copilot's review of #64 at a603a03: a descendant that left the broker's
process group left the timed-out reader thread blocked, with its pipe
descriptor, behind every refusal, and that reader could still add to
what had been read after received was cleared.

The reader thread is gone. One selector loop in the calling thread
streams the credential to the broker's stdin in PIPE_BUF writes and
reads the answer, as communicate does on POSIX, under one deadline that
now covers the credential's streaming too. A refusal kills the broker's
process group and closes this process's ends of both pipes. So a
descendant that left the group costs no thread and no descriptor, and
nothing can add to the answer once the loop stops. The answer is read
straight into received, so no other local holds it. BROKER_REAP_SECONDS
is gone.

Measured at a603a03:
- a broker whose descendant left the group was refused only after the
  2 s reap grace (2.5 s with a 0.5 s timeout), and left its reader behind;
- a broker that never read a 1 MB credential held the refusal past 10 s,
  because the timeout began only after the credential was written.

Tests: the descendant case now also asserts that no thread and no
descriptor is left behind, and that an in-group descendant is killed. A
broker that reads 5000 bytes of a 1 MB credential is refused at its
0.5 s timeout. The failing-source case now lets the answer be read first.
That exposed a local, chunk, holding the answer in the escaping
traceback, and it is removed. All 35 mutants are killed.

Arc: neutral-product-standalone-operability
Lane: openxfactory-4 (openXfactory-4-openDox_extraction)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 16:39
@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

A blocking credential-source read can bypass the broker deadline and prevent timely cleanup.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread src/opendox/doxbench_provider.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The security-sensitive subprocess rewrite and outstanding stacked prerequisite warrant final human review despite comprehensive coverage.

Review effort: Balanced
Findings: None

Resolved since last review (1)

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