v0.19: ensembles: network #1493

vyzo · 2026-09-08T17:29:23Z

vyzo commented

2026-09-08 17:29:23 +00:00

This is the first part of the v0.19 ensembles; complete network implementation.
It is industrial strength with excellent coverage and performance; interaction diagrams for humans included in the documentation.

codesigned and implemented with astra, with oversight from the gerbilosaurus rex 🦖.

This is the first part of the v0.19 ensembles; complete network implementation. It is industrial strength with excellent coverage and performance; interaction diagrams for humans included in the documentation. codesigned and implemented with astra, with oversight from the gerbilosaurus rex 🦖.

vyzo added 7 commits

2026-09-08 17:29:24 +00:00

vyzo added 1 commit

2026-09-08 19:35:29 +00:00

add ensemble ucan capability utilities and tests e0ce5603c0

vyzo added 3 commits

2026-09-08 22:17:41 +00:00

add schema execution and improve statement caching bd6bd2b404

add cached ucan database backend and documentation 1aeaad12ce

expand stdlib coding conventions 3ce6efadfb

vyzo added 1 commit

2026-09-08 22:20:22 +00:00

recommend shared SQL schema execution helper ff79064150

vyzo added 1 commit

2026-09-09 09:20:25 +00:00

add ucan capability context with cached keys 2a18a058be

vyzo added 1 commit

2026-09-09 09:58:16 +00:00

add ucan capability context extensions 4796d316c8

vyzo added 14 commits

2026-09-09 23:35:44 +00:00

add ensemble network interfaces and DID-named TLS contexts 0b9f23f526

document ensemble network design 40c9920424

add ensemble network configuration and limits 30f6c0674d

extend ensemble network interface contracts f56398d96f

align ensemble network contract annotations ad805f963b

reuse network no-timeout default sentinel b213f8a9dc

summarize ensemble Network interface 866b740ab1

add ensemble network wire codecs f40b4040c8

fix BIO buffer cache capacity buckets 5bf97df53f

prioritize BIO buffer cache positive-size requests 7b2439787b

refine ensemble wire codec types and access f7186ef53c

verify capability context anchors before insertion 5b38d74fae

trust capability context principals as implicit roots 84bf594c69

add ensemble network authentication foundations 0e441dcfcf

vyzo added 28 commits

2026-09-11 22:21:55 +00:00

remove vacuous ensemble network interface tests 06de26a6c5

fix buffered UTF-8 writer retries after buffer growth f37561188c

remove redundant buffered writer documentation 7e864eff10

allow an explicit timeout for SSL server upgrade b2e5328a27

remove redundant BIO buffer cache documentation 395d239b0d

add thread join with original exception propagation 9acc5815fc

add staged ensemble network handshakes b534b10f1f

fix stream-listen internet endpoint dispatch 6247a71709

add ensemble network listener lifecycle cfacd83bd9

add ensemble network outgoing transport setup 8ed8ec74a3

add ensemble connector credential retry choices 464d97745a

specialize stdlib network setup errors 4923049534

fix SSL cleanup after shared socket closure 6b7319686c

add ensemble connection retry and election foundations fcfddcd48d

add bounded ensemble stream buffers and credits 4ec678fb1e

fix condition variable type predicate 5cedbfb328

add blocking ensemble stream IO 6299d5f5a5

avoid copying queues for non-destructive iteration b69375e026

add bounded ensemble connection scheduling 7e389ba96b

fix incremental make dependency freshness 065799d6d1

record scheduler and make fix handoff bf3f657a7c

integrate ensemble stream control scheduling 3451b16be5

simplify stream quantum positivity checks 0c1580f811

add ensemble parent wakeups and idle stream expiry 2b4e1eb220

add explicit Interrupt exception 8eed781648

fix RWLock interrupted acquisition cleanup 1f2c9e21aa

fix socket Reader minimum-input handling f2ad527ee6

enforce ensemble connection transport lifetimes 930ab6d391

vyzo added 11 commits

2026-09-12 21:14:34 +00:00

record public network end-to-end milestone 337979b813

add exception-only unwind protection b10054c119

support unwind-protect! in gerbil-mode f7b3bac4a5

use exception-only cleanup in stdlib resource paths c6ebead6cd

adopt exception-only cleanup in audited ensemble paths 853f126c61

record framed transport implementation handoff 59d352a5d2

translate thread join timeouts to Timeout 20b00ad06b

add framed ensemble connection transport 4fd23e11f1

remove Interrupt and asynchronous lock recovery 4fed7fed14

implement public ensemble networks and authorized streams 9c29d58d67

fix ensemble whole-buffer writes and test large transfers 837e16223a

vyzo force-pushed v0.19-std-ensemble from 837e16223a to deb9873961

2026-09-14 17:05:42 +00:00

Compare

vyzo changed title from ~~WIP: v0.19: ensembles~~ to v0.19: ensembles: network

2026-09-14 17:06:42 +00:00

vyzo requested reviews from fare, HMarcien, jay

2026-09-14 17:08:06 +00:00

vyzo referenced this pull request

2026-09-17 18:18:56 +00:00

WIP: v0.19 ensemble hosts #1502

vyzo force-pushed v0.19-std-ensemble from deb9873961 to d6d13b539f

2026-09-17 18:20:37 +00:00

Compare

vyzo commented

2026-09-17 18:22:21 +00:00

rebased on staging.

fare commented

2026-09-17 18:41:41 +00:00

First comments from Sol:

I’d request changes on the current rebased head. I found two concrete identity/authentication problems and one verification blocker. There are also two lesser API concerns. The PR is huge—192 files, +43,678/−1,437—and the current head was force-pushed/rebased onto staging only minutes before this review. (cons.io git)

P1 — did->public-key deliberately accepts invalid did:key identifiers. The new tests explicitly require acceptance of U, m, and M multibase forms, plus strings containing !, ==ignored, and even a trailing newline. The documentation likewise says this permissiveness is intentional. (cons.io git) But the current did:key method ABNF permits exactly base58btc z... or unpadded base64url u...; characters such as !, =, or newline are outside the grammar. (W3C CCG) RFC 4648 also says that, absent an explicit contrary rule, decoders must reject non-alphabet characters, specifically warning that ignoring them can create covert channels or defeat string equality checks—which is particularly relevant here because capability identities are compared as strings. (RFC Editor)

I would ask for this before merge: restrict did->public-key to the z and u encodings actually permitted by did:key, validate the complete lexical representation rather than relying on the permissive generic base64 decoder, and reverse those “permissive Base64url decoding” tests into rejection tests. I’d also reject noncanonical base64 pad bits (...LB decoding like ...LA); otherwise there are still multiple u spellings for one byte sequence.
P1 — network DID canonicalization and UCAN root trust have incompatible semantics. The network explicitly canonicalizes API DIDs by decoding and re-encoding them, so e.g. a valid base58btc did:key:z... supplied as a peer identity becomes its did:key:u... form. Signed token fields are intentionally left untouched. (cons.io git) Meanwhile CapabilityContext.add-root! deliberately stores the root string verbatim, and token-rooted-at? tests the issuer with plain equal?. (cons.io git) The DID tests demonstrate that a base58 and canonical-base64url DID can decode to exactly the same public key. (cons.io git)

So there is a very plausible failure mode:

add-root!("did:key:z...") → succeeds
network authenticates that same key as canonical did:key:u... → token signature verifies
token-rooted-at? → false
→ AnchorVerificationError.

This isn't covered by the root tests: they only add roots using the already-canonical DID spelling. Interestingly, the context tests do already exercise a base58 alias for private-key lookup, so the representation issue is known elsewhere in the API. (cons.io git)

I’d canonicalize and validate explicit roots on insertion/removal, or otherwise make the network/trust semantics agree, and add exactly the base58-root/canonical-token regression test above. Fixing finding 1 does not eliminate this one because both z and u are valid did:key encodings.
P1 before merge — rerun the broader regression that previously crashed natively. The branch's own acceptance record says that the earlier full regression produced native invalid-return/dispatch crashes whose exact cause remains unknown. After the staging rebase, both ensemble-only runs passed all 30 modules/375 named cases, including the formerly crashing paths—but the broader regression was interrupted and explicitly not rerun. (cons.io git)

Given that this PR also changes shared code outside Ensemble—BIO writers/cache, SSL cleanup/timeouts, socket semantics, thread joining, make freshness, resource unwinding, etc.—the ensemble-only suite isn't enough evidence that the unexplained crash disappeared rather than merely stopped being exercised. The PR commit list confirms those supporting-library changes are part of the change set. (cons.io git) I would require at least the previously failing broad command from the implementation notes, preferably the full regression, on current d6d13b539f before approving.
P2/design — Network.close has a surprisingly dangerous context restriction. The public interface explicitly warns that calling Network.close from any monitor callback can deadlock because the closer waits for the supervisor, while the supervisor waits for the callback's worker. (cons.io git) The implementation indeed sets closed? and synchronously joins the supervisor unless invoked by the supervisor itself; it doesn't recognize one of its job workers. (cons.io git)

The documentation makes this not a hidden bug, but I still dislike it as an API contract. A monitor callback is exactly where application policy may naturally decide “fatal condition, shut the network down.” Requiring it to remember to dispatch that operation to some unrelated application worker is a liveness footgun. I’d prefer Network.close to detect an owned worker and request shutdown without self-dependent joining, even if synchronous completion is only guaranteed to outside callers.
Minor/scope — public revocation API is knowingly a trap. REVOKE is a public token type, tokens have revocation-specific args, save-token! is documented as being for future revocation, and revoke! is exported—but revoke! unconditionally raises TODO; the docs explicitly admit this. (cons.io git) Since this PR claims a complete network rather than complete UCAN, I wouldn't necessarily block on implementing revocation. I would consider not exporting revoke! as an apparently callable API until it exists.

I also chased two things that initially looked suspicious and would not comment on them: the conversion of an absolute handshake deadline through IOTimeout is intentional given the timeout representation, and the asymmetric renewal protocol is intentional—connection renewal is mutual while stream renewal is one-way authorization.

Of these, #1 is the clearest code defect, #2 is the most interesting cross-layer bug, and #3 is the strongest reason not to merge the branch yet. #4 is worth discussing with vyzo; #5 is cleanup/scope.

I couldn't independently run this branch: the Forgejo web diff is too large for its normal files view and the local environment couldn't resolve git.cons.io, so this is source-level review plus examination of the branch's own recorded test evidence, not an independent test run.

First comments from Sol: I’d **request changes** on the current rebased head. I found two concrete identity/authentication problems and one verification blocker. There are also two lesser API concerns. The PR is huge—192 files, +43,678/−1,437—and the current head was force-pushed/rebased onto staging only minutes before this review. ([cons.io git][1]) 1. **P1 — `did->public-key` deliberately accepts invalid `did:key` identifiers.** The new tests explicitly require acceptance of `U`, `m`, and `M` multibase forms, plus strings containing `!`, `==ignored`, and even a trailing newline. The documentation likewise says this permissiveness is intentional. ([cons.io git][2]) But the current `did:key` method ABNF permits exactly base58btc `z...` or *unpadded* base64url `u...`; characters such as `!`, `=`, or newline are outside the grammar. ([W3C CCG][3]) RFC 4648 also says that, absent an explicit contrary rule, decoders **must reject non-alphabet characters**, specifically warning that ignoring them can create covert channels or defeat string equality checks—which is particularly relevant here because capability identities are compared as strings. ([RFC Editor][4]) I would ask for this before merge: restrict `did->public-key` to the `z` and `u` encodings actually permitted by `did:key`, validate the complete lexical representation rather than relying on the permissive generic base64 decoder, and reverse those “permissive Base64url decoding” tests into rejection tests. I’d also reject noncanonical base64 pad bits (`...LB` decoding like `...LA`); otherwise there are still multiple `u` spellings for one byte sequence. 2. **P1 — network DID canonicalization and UCAN root trust have incompatible semantics.** The network explicitly canonicalizes API DIDs by decoding and re-encoding them, so e.g. a valid base58btc `did:key:z...` supplied as a peer identity becomes its `did:key:u...` form. Signed token fields are intentionally left untouched. ([cons.io git][5]) Meanwhile `CapabilityContext.add-root!` deliberately stores the root string **verbatim**, and `token-rooted-at?` tests the issuer with plain `equal?`. ([cons.io git][6]) The DID tests demonstrate that a base58 and canonical-base64url DID can decode to exactly the same public key. ([cons.io git][2]) So there is a very plausible failure mode: `add-root!("did:key:z...")` → succeeds network authenticates that same key as canonical `did:key:u...` → token signature verifies `token-rooted-at?` → false → `AnchorVerificationError`. This isn't covered by the root tests: they only add roots using the already-canonical DID spelling. Interestingly, the context tests *do* already exercise a base58 alias for private-key lookup, so the representation issue is known elsewhere in the API. ([cons.io git][7]) I’d canonicalize and validate explicit roots on insertion/removal, or otherwise make the network/trust semantics agree, and add exactly the base58-root/canonical-token regression test above. Fixing finding 1 does **not** eliminate this one because both `z` and `u` are valid `did:key` encodings. 3. **P1 before merge — rerun the broader regression that previously crashed natively.** The branch's own acceptance record says that the earlier full regression produced native invalid-return/dispatch crashes whose exact cause remains unknown. After the staging rebase, both ensemble-only runs passed all 30 modules/375 named cases, including the formerly crashing paths—but the broader regression was interrupted and explicitly **not rerun**. ([cons.io git][8]) Given that this PR also changes shared code outside Ensemble—BIO writers/cache, SSL cleanup/timeouts, socket semantics, thread joining, make freshness, resource unwinding, etc.—the ensemble-only suite isn't enough evidence that the unexplained crash disappeared rather than merely stopped being exercised. The PR commit list confirms those supporting-library changes are part of the change set. ([cons.io git][1]) I would require at least the previously failing broad command from the implementation notes, preferably the full regression, on current `d6d13b539f` before approving. 4. **P2/design — `Network.close` has a surprisingly dangerous context restriction.** The public interface explicitly warns that calling `Network.close` from any monitor callback can deadlock because the closer waits for the supervisor, while the supervisor waits for the callback's worker. ([cons.io git][9]) The implementation indeed sets `closed?` and synchronously joins the supervisor unless invoked by the supervisor itself; it doesn't recognize one of its job workers. ([cons.io git][10]) The documentation makes this not a hidden bug, but I still dislike it as an API contract. A monitor callback is exactly where application policy may naturally decide “fatal condition, shut the network down.” Requiring it to remember to dispatch that operation to some unrelated application worker is a liveness footgun. I’d prefer `Network.close` to detect an owned worker and request shutdown without self-dependent joining, even if synchronous completion is only guaranteed to outside callers. 5. **Minor/scope — public revocation API is knowingly a trap.** `REVOKE` is a public token type, tokens have revocation-specific `args`, `save-token!` is documented as being for future revocation, and `revoke!` is exported—but `revoke!` unconditionally raises `TODO`; the docs explicitly admit this. ([cons.io git][11]) Since this PR claims a complete **network** rather than complete UCAN, I wouldn't necessarily block on implementing revocation. I would consider not exporting `revoke!` as an apparently callable API until it exists. I also chased two things that initially looked suspicious and **would not comment on them**: the conversion of an absolute handshake deadline through `IOTimeout` is intentional given the timeout representation, and the asymmetric renewal protocol is intentional—connection renewal is mutual while stream renewal is one-way authorization. Of these, **#1 is the clearest code defect**, **#2 is the most interesting cross-layer bug**, and **#3 is the strongest reason not to merge the branch yet**. #4 is worth discussing with vyzo; #5 is cleanup/scope. I couldn't independently run this branch: the Forgejo web diff is too large for its normal files view and the local environment couldn't resolve `git.cons.io`, so this is source-level review plus examination of the branch's own recorded test evidence, not an independent test run. [1]: https://git.cons.io/mighty-gerbils/gerbil/pulls/1493 "https://git.cons.io/mighty-gerbils/gerbil/pulls/1493" [2]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/did-test.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/did-test.ss" [3]: https://w3c-ccg.github.io/did-key-spec/ "https://w3c-ccg.github.io/did-key-spec/" [4]: https://www.rfc-editor.org/info/rfc4648/ "https://www.rfc-editor.org/info/rfc4648/" [5]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/auth.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/auth.ss" [6]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/context.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/context.ss" [7]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/context-test.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/context-test.ss" [8]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/acceptance.md "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/acceptance.md" [9]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/interface.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/interface.ss" [10]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/network.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/network.ss" [11]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/interface.ss "gerbil/src/std/ensemble/ucan/interface.ss at v0.19-std-ensemble - mighty-gerbils/gerbil - cons.io git"

vyzo commented

2026-09-17 19:01:53 +00:00

some preliminary review comments, before we discuss and address with astra:

this is already mostly addressed in the follow up pr that implements hosts, where we do add did normalization primitives. we can pull the relevant changes down to this pr and ensure the capability context is normalized.
see above, we can address this in this pr.
the crash was almost certainly caused by raising in a thread-interrupt, which is a very dangerous operation that can leave the interpreter in corrupted state. i have very little doubt that this was the cause of the crash, but we can rerun the full test suite including all stdlib tests before merge.
the best we can do here is have a thread local value that marks the network thread and attempting to call close raise an exception. better than a deadlock and not much more we can do without dramatically complicating the implementation.
ok, we can remove it for now.

some preliminary review comments, before we discuss and address with astra: 1. this is already mostly addressed in the follow up pr that implements hosts, where we do add did normalization primitives. we can pull the relevant changes down to this pr and ensure the capability context is normalized. 2. see above, we can address this in this pr. 3. the crash was almost certainly caused by raising in a thread-interrupt, which is a very dangerous operation that can leave the interpreter in corrupted state. i have very little doubt that this was the cause of the crash, but we can rerun the full test suite including all stdlib tests before merge. 4. the best we can do here is have a thread local value that marks the network thread and attempting to call close raise an exception. better than a deadlock and not much more we can do without dramatically complicating the implementation. 5. ok, we can remove it for now.

vyzo added 4 commits

2026-09-18 06:48:00 +00:00

add strict encoding validation and lightweight DID normalization 80f1e8f3c6

unify UCAN identity comparisons with context-owned normalization a53478b4a0

guard network worker close and normalize thread failures 49e7c75db1

remove revocation stub and record broad regression results 76968e6d39

vyzo commented

2026-09-18 06:49:50 +00:00

Astra says:

Addressed the review findings:

Strict DID validation: only valid Ed25519 did🔑z… and unpadded did🔑u… representations are accepted. Invalid alphabets, padding, trailing junk and nonzero unused bits are rejected. Added allocation-light encoding inspection; canonical DID normalization does not decode into temporary bytevectors or native keys.
Consistent identity semantics: roots, delegation edges, anchors and network credential selection recognize equivalent z/u identities. Normalization uses a bounded capability-context cache for aliases. Signed token fields and serialized bytes remain unchanged. Added mixed-encoding, persistence and cache regressions.
Broader validation: eight-core stdlib/tools builds passed. The previously requested supporting-library regression passed all 43 modules, including all 31 ensemble modules. The full ./build.sh test std/... run completed across 142 modules without a native crash. It reported 14 failures in the recently introduced HTTP compliance tests, explicitly outside this PR’s scope; the other 141 modules had no harness-recorded failures. HTTP source/tests were left unchanged. The historical native-crash cause remains unproved.
Safe close restriction: Network.close now raises ContractViolation from network-owned workers before locking, changing state or joining, rather than risking deadlock. Workers use actor-thread-local markers. Their failures are debug-logged and wrapped internally; joins preserve the original exception without noisy unhandled stack traces.
Revocation API cleanup: removed the unimplemented revoke! export and its placeholder test. Reserved token representation remains, with documentation explicitly stating that revocation is unsupported.
Commits: 80f1e8f3, a53478b4, 49e7c75d, 76968e6d.

Astra says: Addressed the review findings: 1. Strict DID validation: only valid Ed25519 did:key:z… and unpadded did:key:u… representations are accepted. Invalid alphabets, padding, trailing junk and nonzero unused bits are rejected. Added allocation-light encoding inspection; canonical DID normalization does not decode into temporary bytevectors or native keys. 2. Consistent identity semantics: roots, delegation edges, anchors and network credential selection recognize equivalent z/u identities. Normalization uses a bounded capability-context cache for aliases. Signed token fields and serialized bytes remain unchanged. Added mixed-encoding, persistence and cache regressions. 3. Broader validation: eight-core stdlib/tools builds passed. The previously requested supporting-library regression passed all 43 modules, including all 31 ensemble modules. The full ./build.sh test std/... run completed across 142 modules without a native crash. It reported 14 failures in the recently introduced HTTP compliance tests, explicitly outside this PR’s scope; the other 141 modules had no harness-recorded failures. HTTP source/tests were left unchanged. The historical native-crash cause remains unproved. 4. Safe close restriction: Network.close now raises ContractViolation from network-owned workers before locking, changing state or joining, rather than risking deadlock. Workers use actor-thread-local markers. Their failures are debug-logged and wrapped internally; joins preserve the original exception without noisy unhandled stack traces. 5. Revocation API cleanup: removed the unimplemented revoke! export and its placeholder test. Reserved token representation remains, with documentation explicitly stating that revocation is unsupported. Commits: 80f1e8f3, a53478b4, 49e7c75d, 76968e6d.

vyzo commented

2026-09-19 22:02:05 +00:00

any progress on the review here?

fare commented

2026-09-21 20:30:36 +00:00

Sol says:

Second pass changes my assessment substantially. The four September 18 follow-up commits address the first-pass findings rather well: strict DID validation, canonical identity comparison throughout UCAN, the Network.close worker guard, removal of the fake revocation API and the broader regression run are all real fixes, not cosmetic patches. (cons.io git)

I did not find another definite line-level correctness bug in the renewal, stream-I/O, thread-join or authorization machinery on this pass. I do have one remaining verification objection and one security-semantics question worth raising.

P1-ish / pre-merge verification: run the failing regression against the exact staging base. The new full std/... run is much better evidence: 142 modules ran, there was no recurrence of the native invalid-return/dispatch crash and 141 modules had no harness-recorded failures. But std/net/http/server/server-test.ss has 14 failures, and the log also contains websocket/SSL worker exceptions that aren't counted as harness failures. Most importantly, the notes themselves explicitly say that v0.19-staging was not actually run for comparison. (cons.io git)

I don't think git diff v0.19-staging -- src/std/net/http being empty is quite enough here. This PR changes code under HTTP: socket Reader minimum-input behavior, SSL cleanup and SSL error-queue handling, network setup errors, thread joins and general stdlib resource cleanup. (cons.io git) The apparent HTTP failures do look plausibly like pre-existing HTTP validation omissions—the failing cases are malformed request syntax, Host, Content-Length/Transfer-Encoding and 100 Continue, and the source inspection supports that interpretation. But the cheap conclusive experiment is to run at least server-test.ss on the exact staging parent, preferably also whatever triggered those websocket/SSL background exceptions.

The review comment I'd leave is essentially:

The broad regression is now useful evidence, but can we establish the baseline by running the failing HTTP test on the exact staging base under the same build/runtime? This PR changes shared socket/BIO/SSL machinery, so an unchanged std/net/http tree does not by itself prove that the 14 failures are pre-existing. Please also compare the websocket/SSL background exceptions, since those aren't reflected in the harness failure count. If staging reproduces both, I'm satisfied with excluding them from this PR.
Security/design question: should input-anchor matching really be type-independent? token-anchored-at? deliberately does not compare Token.type; it matches issuer, audience, protocol/group and bounds the submitted leaf expiration. The documentation explicitly calls this “type-independent.”

Consequently, if an application installs an INVOKE token A → B /svc as an input anchor, a separately signed DELEGATE A → B /svc can satisfy that anchor as well, assuming the other constraints hold. CapabilityContext.verify then regards it as trusted; whether that matters depends on what the surrounding operation does with the verified token.

This may be exactly the intended semantics—an input anchor may mean “trust assertions by this issuer to this audience over this capability,” with the token merely serving as a signed policy template. If so, I wouldn't request a change. But because INVOKE, DELEGATE and BROADCAST represent materially different authority, I would ask vyzo to confirm that widening explicitly. At minimum I'd want a cross-type test documenting that it is intentional. If instead an anchor is supposed to constrain the kind of authority being anchored, anchor.type needs to participate in the match.

Everything from the first review otherwise looks resolved:

The new Base64 validator is genuinely strict: alphabet, padding placement and canonical unused bits are checked, while the old permissive decoder remains separately available. normalize-did uses that strict path and converts valid z aliases to canonical u. (cons.io git)
Identity normalization was propagated through chain verification, signing/delegation checks, root matching, anchor matching and output-anchor selection; I couldn't find the former raw-DID-comparison hole elsewhere.
Network.close now fails before locking or changing shutdown state when invoked from a network-owned worker. I still find “spawn an application thread to close me” slightly inelegant, but the deadlock bug is gone and the restriction is now explicit and tested. (cons.io git)
The revoke! TODO masquerading as API is gone. (cons.io git)
I rechecked the Reader >→>= minimum-input change and the new thread-join wrapper in particular; both look right, with unusually thorough boundary/failure tests.

So my position now is not “request changes because the implementation is broken.” I'd leave the staging-baseline comment before merge, and I'd ask the anchor-type question because that's a security-policy decision worth making consciously. If staging reproduces those HTTP/SSL symptoms and type-independent anchors are confirmed intentional, I have no further substantive objection from this pass.

Sol says: Second pass changes my assessment substantially. The four September 18 follow-up commits address the first-pass findings rather well: strict DID validation, canonical identity comparison throughout UCAN, the `Network.close` worker guard, removal of the fake revocation API and the broader regression run are all real fixes, not cosmetic patches. ([cons.io git][1]) I **did not find another definite line-level correctness bug** in the renewal, stream-I/O, thread-join or authorization machinery on this pass. I do have one remaining verification objection and one security-semantics question worth raising. 1. **P1-ish / pre-merge verification: run the failing regression against the exact staging base.** The new full `std/...` run is much better evidence: 142 modules ran, there was no recurrence of the native invalid-return/dispatch crash and 141 modules had no harness-recorded failures. But `std/net/http/server/server-test.ss` has 14 failures, and the log also contains websocket/SSL worker exceptions that aren't counted as harness failures. Most importantly, the notes themselves explicitly say that `v0.19-staging` was **not actually run** for comparison. ([cons.io git][2]) I don't think `git diff v0.19-staging -- src/std/net/http` being empty is quite enough here. This PR changes code *under* HTTP: socket Reader minimum-input behavior, SSL cleanup and SSL error-queue handling, network setup errors, thread joins and general stdlib resource cleanup. ([cons.io git][1]) The apparent HTTP failures do look plausibly like pre-existing HTTP validation omissions—the failing cases are malformed request syntax, Host, Content-Length/Transfer-Encoding and `100 Continue`, and the source inspection supports that interpretation. But the cheap conclusive experiment is to run at least `server-test.ss` on the exact staging parent, preferably also whatever triggered those websocket/SSL background exceptions. The review comment I'd leave is essentially: > The broad regression is now useful evidence, but can we establish the baseline by running the failing HTTP test on the exact staging base under the same build/runtime? This PR changes shared socket/BIO/SSL machinery, so an unchanged `std/net/http` tree does not by itself prove that the 14 failures are pre-existing. Please also compare the websocket/SSL background exceptions, since those aren't reflected in the harness failure count. If staging reproduces both, I'm satisfied with excluding them from this PR. 2. **Security/design question: should input-anchor matching really be type-independent?** `token-anchored-at?` deliberately does **not** compare `Token.type`; it matches issuer, audience, protocol/group and bounds the submitted leaf expiration. The documentation explicitly calls this “type-independent.” Consequently, if an application installs an `INVOKE` token `A → B /svc` as an input anchor, a separately signed `DELEGATE A → B /svc` can satisfy that anchor as well, assuming the other constraints hold. `CapabilityContext.verify` then regards it as trusted; whether that matters depends on what the surrounding operation does with the verified token. This may be exactly the intended semantics—an input anchor may mean “trust assertions by this issuer to this audience over this capability,” with the token merely serving as a signed policy template. If so, I wouldn't request a change. But because `INVOKE`, `DELEGATE` and `BROADCAST` represent materially different authority, I would ask vyzo to confirm that widening explicitly. At minimum I'd want a cross-type test documenting that it is intentional. If instead an anchor is supposed to constrain *the kind of authority being anchored*, `anchor.type` needs to participate in the match. Everything from the first review otherwise looks resolved: * The new Base64 validator is genuinely strict: alphabet, padding placement and canonical unused bits are checked, while the old permissive decoder remains separately available. `normalize-did` uses that strict path and converts valid `z` aliases to canonical `u`. ([cons.io git][3]) * Identity normalization was propagated through chain verification, signing/delegation checks, root matching, anchor matching and output-anchor selection; I couldn't find the former raw-DID-comparison hole elsewhere. * `Network.close` now fails **before locking or changing shutdown state** when invoked from a network-owned worker. I still find “spawn an application thread to close me” slightly inelegant, but the deadlock bug is gone and the restriction is now explicit and tested. ([cons.io git][4]) * The `revoke!` TODO masquerading as API is gone. ([cons.io git][2]) * I rechecked the `Reader` `>`→`>=` minimum-input change and the new thread-join wrapper in particular; both look right, with unusually thorough boundary/failure tests. So my position now is **not “request changes because the implementation is broken.”** I'd leave the staging-baseline comment before merge, and I'd ask the anchor-type question because that's a security-policy decision worth making consciously. If staging reproduces those HTTP/SSL symptoms and type-independent anchors are confirmed intentional, I have no further substantive objection from this pass. [1]: https://git.cons.io/mighty-gerbils/gerbil/commits/branch/v0.19-std-ensemble "mighty-gerbils/gerbil - cons.io git" [2]: https://git.cons.io/mighty-gerbils/gerbil/commit/76968e6d3987a42d613f183317f98f4c024fb180 "remove revocation stub and record broad regression results · 76968e6d39 - mighty-gerbils/gerbil - cons.io git" [3]: https://git.cons.io/mighty-gerbils/gerbil/commit/80f1e8f3c6dd50fb0efcd8b8450ad7db33b34ae9 "add strict encoding validation and lightweight DID normalization · 80f1e8f3c6 - mighty-gerbils/gerbil - cons.io git" [4]: https://git.cons.io/mighty-gerbils/gerbil/commit/49e7c75db1778761ec0a737dc553cd982747ff46 "guard network worker close and normalize thread failures · 49e7c75db1 - mighty-gerbils/gerbil - cons.io git"

vyzo commented

2026-09-22 12:09:42 +00:00

i don't think we need a pre-merge verification, nothing relevant changed in the base branch. the token anchor test is intentionally type independent.