v0.19 final porting touches #1486

vyzo · 2026-09-07T17:18:44Z

vyzo commented

2026-09-07 17:18:44 +00:00

Ports std/instance, std/net/rep, std/net/s3 (with synthetic server tests) and std/net/smtp (with synthetic server tests).

Done with sol/astra with oversight by the gerbilosaurus rex.

Ports std/instance, std/net/rep, std/net/s3 (with synthetic server tests) and std/net/smtp (with synthetic server tests). Done with sol/astra with oversight by the gerbilosaurus rex.

vyzo added 6 commits

2026-09-07 17:18:44 +00:00

v0.19: port std/instance 6661c89abb

v0.19: port std/net/repl 50248c7d33

agents f55847fab0

fix(core): preserve positional arguments in interface keyword methods ef35eb878d

v0.19: port std/net/s3 with synthetic client tests 1d83e145ee

v0.19: port std/net/smtp with synthetic client tests 055a56600e

vyzo requested review from fare

2026-09-07 17:18:52 +00:00

fare commented

2026-09-07 17:33:17 +00:00

While I'm ogling at the code, the much faster answer from my pal Sol:

I would request changes on this as it stands. The overall port looks substantially better than the old WIP code, and the synthetic SMTP/S3 tests are a real improvement, but the S3 port has at least two concrete regressions and one SigV4 correctness hole. The PR is also not exactly “final touches”: 43 files, +2498/−2134, including an unrelated core contract fix. (cons.io git)

1. BLOCKER: `s3-client`'s default endpoint doesn't satisfy its own parser

This is the wonderfully simple kind of bug where the API's default argument appears to make the API fail by default:

(def (parse-s3-endpoint endpoint)
  ...
  (unless (member url.proto '("http" "https"))
    ...))

(def (s3-client
  endpoint: (endpoint : :string := "s3.amazonaws.com")
  ...))

The old implementation treated endpoint as a hostname and explicitly constructed "https://" + host + path. The new implementation has sensibly changed endpoint into an actual URL, but forgot to change the default accordingly. It should presumably be:

endpoint: (endpoint : :string := "https://s3.amazonaws.com")

The synthetic test doesn't catch this because test-client always supplies its own http://127.0.0.1:20004 endpoint. (cons.io git)

I'd insist on a test that at least constructs:

(s3-client access-key: "x" secret-key: "y")

without touching the network.

2. BLOCKER: several S3 operations stopped closing their HTTP requests

The previous code explicitly did request-close after list-buckets, list-objects, get and copy-to!. The port has silently dropped those closes from exactly those methods. Meanwhile create-bucket!, put!, delete!, exists?, etc. still explicitly call Request-close, so this clearly isn't some systematic new “Request-content owns the close” API convention. (cons.io git)

For example:

(defmethod {get bucket}
  ...
  (let (request ...)
    (Request-content request)))

and:

(defmethod {list-objects bucket}
  ...
  (let* ((request ...)
         (xml (s3-parse-xml request)))
    ...))

No close.

copy-to! is even worse because every normal and exceptional path after parsing the response leaks the request. (cons.io git)

I wouldn't merely put the old closes back after the operations. The cleanup should be exception-safe, because XML parsing or subsequent processing can throw. Whatever the v0.19 idiom is for finally/unwind cleanup, use it.

This one deserves tests that can observe connection/request closure, much like the SMTP tests explicitly check EOF so a higher-level cleanup doesn't hide a leak. The SMTP tests actually do this correctly. (cons.io git)

3. SigV4 canonical headers aren't actually canonicalized

canonicalized-headers currently does only:

(cons (string-downcase (car header))
      (cdr header))

and canonical-headers emits the value verbatim. (cons.io git)

AWS SigV4 requires header values to have leading/trailing whitespace trimmed and sequential spaces collapsed before signing. AWS explicitly specifies this canonicalization. (AWS Documentation)

So, for example, a caller supplying a content type or eventually another signed header whose value isn't already canonical can produce a signature different from AWS's. This may be inherited from the old implementation, rather than newly introduced, but if we're taking this code out of v0.19-WIP and blessing it as the stdlib S3 implementation, now is a particularly good time not to preserve known-bad SigV4 semantics.

Add a canonical-header test with something like:

"  foo   bar  "

and expect:

"foo bar"

Also worth testing duplicate header names eventually, since AWS wants multi-valued headers combined rather than repeated in SignedHeaders.

The rest

The contract.ss fix looks right to me. Passing pos instead of arg through the keyword-argument recursion is exactly the sort of one-character screwup that would lose the accumulated positional arguments, and the added (a b scale:) regression test targets it directly. (cons.io git)

The SMTP port is the strongest part of this PR. In particular, testing malformed/rejected protocol stages, DATA producer failure and observing peer EOF is exactly the kind of synthetic testing network code needs. I didn't spot anything there I'd hold the PR for.

The new REPL lifecycle code also looks broadly sane. I don't love thread-group-kill! swallowing termination/join exceptions and looping until it sees no active threads: if termination ever fails without changing the thread state, that's an infinite loop disguised as cleanup. But I don't currently have enough evidence that Gambit's thread-terminate! can leave you in that state to call it a merge blocker.

And then there's the inevitable microscopic nit in AGENTS.md:

script in the the opt ... If you want a repl, just ./run.sh, It runs ...

the the, repl, comma splice, capitalized It. Not relevant to correctness, just conspicuous in a “final touches” PR. (cons.io git)

Verdict: NAK until #1 and #2 are fixed. I'd fix #3 in this PR too, because otherwise we're knowingly promoting an incomplete SigV4 implementation. After those, I think this is in pretty decent shape.

While I'm ogling at the code, the much faster answer from my pal Sol: I would **request changes** on this as it stands. The overall port looks substantially better than the old WIP code, and the synthetic SMTP/S3 tests are a real improvement, but the S3 port has at least two concrete regressions and one SigV4 correctness hole. The PR is also not exactly “final touches”: 43 files, +2498/−2134, including an unrelated core contract fix. ([cons.io git][1]) ### 1. BLOCKER: `s3-client`'s default endpoint doesn't satisfy its own parser This is the wonderfully simple kind of bug where the API's default argument appears to make the API fail by default: ```scheme (def (parse-s3-endpoint endpoint) ... (unless (member url.proto '("http" "https")) ...)) (def (s3-client endpoint: (endpoint : :string := "s3.amazonaws.com") ...)) ``` The old implementation treated `endpoint` as a hostname and explicitly constructed `"https://" + host + path`. The new implementation has sensibly changed `endpoint` into an actual URL, but forgot to change the default accordingly. It should presumably be: ```scheme endpoint: (endpoint : :string := "https://s3.amazonaws.com") ``` The synthetic test doesn't catch this because `test-client` always supplies its own `http://127.0.0.1:20004` endpoint. ([cons.io git][2]) I'd insist on a test that at least constructs: ```scheme (s3-client access-key: "x" secret-key: "y") ``` without touching the network. ### 2. BLOCKER: several S3 operations stopped closing their HTTP requests The previous code explicitly did `request-close` after `list-buckets`, `list-objects`, `get` and `copy-to!`. The port has silently dropped those closes from exactly those methods. Meanwhile `create-bucket!`, `put!`, `delete!`, `exists?`, etc. still explicitly call `Request-close`, so this clearly isn't some systematic new “Request-content owns the close” API convention. ([cons.io git][2]) For example: ```scheme (defmethod {get bucket} ... (let (request ...) (Request-content request))) ``` and: ```scheme (defmethod {list-objects bucket} ... (let* ((request ...) (xml (s3-parse-xml request))) ...)) ``` No close. `copy-to!` is even worse because every normal and exceptional path after parsing the response leaks the request. ([cons.io git][2]) I wouldn't merely put the old closes back after the operations. The cleanup should be exception-safe, because XML parsing or subsequent processing can throw. Whatever the v0.19 idiom is for `finally`/unwind cleanup, use it. This one deserves tests that can observe connection/request closure, much like the SMTP tests explicitly check EOF so a higher-level cleanup doesn't hide a leak. The SMTP tests actually do this correctly. ([cons.io git][2]) ### 3. SigV4 canonical headers aren't actually canonicalized `canonicalized-headers` currently does only: ```scheme (cons (string-downcase (car header)) (cdr header)) ``` and `canonical-headers` emits the value verbatim. ([cons.io git][2]) AWS SigV4 requires header values to have leading/trailing whitespace trimmed **and sequential spaces collapsed** before signing. AWS explicitly specifies this canonicalization. ([AWS Documentation][3]) So, for example, a caller supplying a content type or eventually another signed header whose value isn't already canonical can produce a signature different from AWS's. This may be inherited from the old implementation, rather than newly introduced, but if we're taking this code out of `v0.19-WIP` and blessing it as the stdlib S3 implementation, now is a particularly good time not to preserve known-bad SigV4 semantics. Add a canonical-header test with something like: ```text " foo bar " ``` and expect: ```text "foo bar" ``` Also worth testing duplicate header names eventually, since AWS wants multi-valued headers combined rather than repeated in `SignedHeaders`. ### The rest The `contract.ss` fix looks right to me. Passing `pos` instead of `arg` through the keyword-argument recursion is exactly the sort of one-character screwup that would lose the accumulated positional arguments, and the added `(a b scale:)` regression test targets it directly. ([cons.io git][2]) The SMTP port is the strongest part of this PR. In particular, testing malformed/rejected protocol stages, DATA producer failure and observing peer EOF is exactly the kind of synthetic testing network code needs. I didn't spot anything there I'd hold the PR for. The new REPL lifecycle code also looks broadly sane. I don't love `thread-group-kill!` swallowing termination/join exceptions and looping until it sees no active threads: if termination ever fails without changing the thread state, that's an infinite loop disguised as cleanup. But I don't currently have enough evidence that Gambit's `thread-terminate!` can leave you in that state to call it a merge blocker. And then there's the inevitable microscopic nit in `AGENTS.md`: > `script in the the opt ... If you want a repl, just ./run.sh, It runs ...` `the the`, `repl`, comma splice, capitalized `It`. Not relevant to correctness, just conspicuous in a “final touches” PR. ([cons.io git][2]) **Verdict: NAK until #1 and #2 are fixed.** I'd fix #3 in this PR too, because otherwise we're knowingly promoting an incomplete SigV4 implementation. After those, I think this is in pretty decent shape. [1]: https://git.cons.io/mighty-gerbils/gerbil/pulls/1486 "#1486 - v0.19 final porting touches - mighty-gerbils/gerbil - cons.io git" [2]: https://git.cons.io/mighty-gerbils/gerbil/pulls/1486/files "#1486 - v0.19 final porting touches - mighty-gerbils/gerbil - cons.io git" [3]: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-create-signed-request.html?utm_source=chatgpt.com "Create a signed AWS API request - AWS Identity and Access Management"

fare reviewed

2026-09-07 17:58:32 +00:00

src/gerbil/core/contract.ss Outdated

					
				@ -2803,3 +2803,3 @@

				              ((pgetq key keywords)

				               => (lambda (contract)

				                    (loop #'rest-args rest-pos args

				                    (loop #'rest-args rest-pos pos

Yikes, do we have tests for that?

yes, astra added a test.

vyzo marked this conversation as resolved

src/std/net/repl.ss Outdated

					
				@ -0,0 +283,4 @@

				(##repl-context-command-set! repl-context-command)

				;; Recover the network channel for REPL and tainted thread groups.

Can you explain to me what this "tainted" business is?

that was me, a long time ago in a galaxy far far away.

basically a tainted thread is one that is using the repl channel so that it can be inspected in the repl with some of the advanced repl commands (e.g. visit thread)

that was me, a long time ago in a galaxy far far away. basically a tainted thread is one that is using the repl channel so that it can be inspected in the repl with some of the advanced repl commands (e.g. visit thread)

OK, so do we still want this feature, and want it here, rather than e.g. the threat voluntarily tainting itself if it wants?

At the very least, this kind of feature ought to be documented and explained, with some comments in the code.

OK, so do we still want this feature, and want it here, rather than e.g. the threat voluntarily tainting itself if it wants? At the very least, this kind of feature ought to be documented and explained, with some comments in the code.

i think we still want it, at least for now -- i don't have a good solution for auto-tainting other than rewriting the repl commands. we can revisit this in the future, i'll add a TODO.

added comment and TODO.

vyzo marked this conversation as resolved

src/std/sync/threads.ss Outdated

					
				@ -0,0 +10,4 @@

				;; Make a thread group with no parent.

				(def (make-detached-thread-group (name : :t := (void)))

				  => :thread-group

				  (make-thread-group name (:- #f :thread-group)))

Uh, are you lying to the typesystem here? Sounds fishy. Maybe the function should explicitly accept #f rather than requiring you to lie.

yea, we should fix the signature instead of this.

i had astra fix the signature.

Is this PR?

yes, fixed it.

vyzo marked this conversation as resolved

vyzo commented

2026-09-07 18:32:37 +00:00

I'll address the review comments with astra.

vyzo commented

2026-09-07 18:36:00 +00:00

1 is actually a non-issue, URLs default to https. astra nonetheless decided to improve the code.

vyzo commented

2026-09-07 18:42:12 +00:00

for 2 it agrees it is a problem and is making a fix.

vyzo commented

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

the bug was mine, Request-content needs to try/finally close the request.

vyzo commented

2026-09-07 18:56:59 +00:00

it fixed 3 as well.

fare reviewed

2026-09-07 19:03:43 +00:00

src/std/AGENTS.md Outdated

					
				@ -48,0 +46,4 @@

				script in the the opt of the gerbil source tree. If you want a repl,

				just `./run.sh`, It runs `gxi` by default with the appropriate

				environment variables. For more advanced `gxi` usage with arguments,

				use `./run.sh gxi arg ...`.

Is it normal that -:te is not enabled by default in ./run.sh ? You the script explicitly include these flags? Should gxi be fixed to better autodetect? What's going on?

Is it normal that `-:te` is not enabled by default in `./run.sh` ? You the script explicitly include these flags? Should `gxi` be fixed to better autodetect? What's going on?

i don't think it is needed at all, astra happily uses it.

If ./run.sh is run naked, it's probably for terminal use. Otherwise, I presume astra uses ./run.sh gxi -e ...

Thus, I think either gxi should better autodetect the terminal, or -:te should be the default at least for ./run.sh.

If `./run.sh` is run naked, it's probably for terminal use. Otherwise, I presume astra uses `./run.sh gxi -e ...` Thus, I think either gxi should better autodetect the terminal, or `-:te` should be the default at least for `./run.sh`.