navicore/patch-seq

Fork 0

cp7 #439

Merged

navicore merged 2 commits from cp7 into main

2026-04-27 22:13:02 +00:00

navicore commented

2026-04-27 21:49:38 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-04-27 21:51:57 +00:00

(Migrated from github.com)

Code Review: PR #439 — OSC over UDP loopback test

Overview

Adds an end-to-end integration test (test_osc_loopback.seq) that proves the full chain: Seq OSC encoder → UDP send → UDP receive → byte-equality assert. This complements the existing test_osc.seq (byte-format unit tests) by promoting them to a real round-trip. The justfile wires it into test-integration alongside the encoder tests.

Code Quality & Style ✅

Stack annotation style (# ( ... )) and inline ; comments for the aux stack are consistent with the existing codebase (compare test-udp.seq and osc.seq).
Test word names follow the test-* convention and are self-describing.
The justfile comment mirrors the existing one above it in tone and detail.

Correctness ✅

Traced the stack through udp-loopback against the udp.send-to / udp.receive-from contracts established in test-udp.seq:

0 udp.bind → ( socket port success ) → drop → ( msg socket port ) ✓
After the swap / dup >aux / swap / "127.0.0.1" swap / rot shuffle, the stack is ( msg "127.0.0.1" port socket ) which matches udp.send-to ( bytes host port socket -- success ) ✓
The same socket is saved to aux for the udp.receive-from call, correct for single-socket loopback ✓
A second dup >aux before udp.receive-from preserves the handle for udp.close at the end ✓
drop drop drop peels ok / port / host off ( bytes host port ok ), leaving ( bytes ) ✓
aux> udp.close drop always runs — no socket leak ✓

Kernel buffering makes the single-socket bind/send/receive sequencing sound (same rationale called out in test-udp.seq).

Potential Issues / Suggestions

1. No osc.msg-i round-trip (minor gap, not a blocker)
test_osc.seq tests osc.msg-i for byte format, but test_osc_loopback.seq skips it. The coverage delta is small since msg-f and msg-if exercise the same serialisation path, but adding it would make the suite exhaustive for completeness.

2. Hang-on-failure vs. hard failure (acknowledged tradeoff)
The word-level comment documents the deliberate choice to let a failed send produce a receive hang rather than assert on the success flag. This is a reasonable CI tradeoff (a real loopback failure causes a visible timeout rather than a misleading equality failure), and the explanation is clear. Worth noting that CI jobs should have a test timeout configured if they don't already.

3. drop drop drop ordering comment (very minor)
The comment # ( bytes ) is correct, but adding a brief label — e.g. # drop ok port host → ( bytes ) — would make it immediately obvious which values are being discarded without having to reconstruct the stack. The existing test-udp.seq uses this more verbose style.

4. Success path only
All three round-trip tests use well-formed messages where osc.msg / osc.msg-f / osc.msg-if are known-good (validated in test_osc.seq). That's appropriate for this test's stated goal (prove the full chain aligns), not a gap.

Performance ✅

One UDP loopback socket per test word, closed after use. No unbounded allocation. Negligible overhead for an integration test suite.

Security ✅

Binds only to 127.0.0.1 (loopback); OS-assigned port avoids inter-test conflicts. No external network exposure.

Summary

Solid PR. The implementation is correct, the comments are clear and honest about the failure-mode tradeoffs, and it fills a real gap (no prior round-trip test existed). The suggestions above are all minor. Happy to approve as-is or with the osc.msg-i coverage added if you want exhaustive parity.

## Code Review: PR #439 — OSC over UDP loopback test ### Overview Adds an end-to-end integration test (`test_osc_loopback.seq`) that proves the full chain: Seq OSC encoder → UDP send → UDP receive → byte-equality assert. This complements the existing `test_osc.seq` (byte-format unit tests) by promoting them to a real round-trip. The `justfile` wires it into `test-integration` alongside the encoder tests. --- ### Code Quality & Style ✅ - Stack annotation style (`# ( ... )`) and inline `;` comments for the aux stack are consistent with the existing codebase (compare `test-udp.seq` and `osc.seq`). - Test word names follow the `test-*` convention and are self-describing. - The justfile comment mirrors the existing one above it in tone and detail. --- ### Correctness ✅ Traced the stack through `udp-loopback` against the `udp.send-to` / `udp.receive-from` contracts established in `test-udp.seq`: - `0 udp.bind` → `( socket port success )` → `drop` → `( msg socket port )` ✓ - After the `swap / dup >aux / swap / "127.0.0.1" swap / rot` shuffle, the stack is `( msg "127.0.0.1" port socket )` which matches `udp.send-to ( bytes host port socket -- success )` ✓ - The same socket is saved to `aux` for the `udp.receive-from` call, correct for single-socket loopback ✓ - A second `dup >aux` before `udp.receive-from` preserves the handle for `udp.close` at the end ✓ - `drop drop drop` peels `ok / port / host` off `( bytes host port ok )`, leaving `( bytes )` ✓ - `aux> udp.close drop` always runs — no socket leak ✓ Kernel buffering makes the single-socket bind/send/receive sequencing sound (same rationale called out in `test-udp.seq`). --- ### Potential Issues / Suggestions **1. No `osc.msg-i` round-trip** (minor gap, not a blocker) `test_osc.seq` tests `osc.msg-i` for byte format, but `test_osc_loopback.seq` skips it. The coverage delta is small since `msg-f` and `msg-if` exercise the same serialisation path, but adding it would make the suite exhaustive for completeness. **2. Hang-on-failure vs. hard failure** (acknowledged tradeoff) The word-level comment documents the deliberate choice to let a failed send produce a receive hang rather than assert on the success flag. This is a reasonable CI tradeoff (a real loopback failure causes a visible timeout rather than a misleading equality failure), and the explanation is clear. Worth noting that CI jobs should have a test timeout configured if they don't already. **3. `drop drop drop` ordering comment** (very minor) The comment `# ( bytes )` is correct, but adding a brief label — e.g. `# drop ok port host → ( bytes )` — would make it immediately obvious which values are being discarded without having to reconstruct the stack. The existing `test-udp.seq` uses this more verbose style. **4. Success path only** All three round-trip tests use well-formed messages where `osc.msg` / `osc.msg-f` / `osc.msg-if` are known-good (validated in `test_osc.seq`). That's appropriate for this test's stated goal (prove the full chain aligns), not a gap. --- ### Performance ✅ One UDP loopback socket per test word, closed after use. No unbounded allocation. Negligible overhead for an integration test suite. --- ### Security ✅ Binds only to `127.0.0.1` (loopback); OS-assigned port avoids inter-test conflicts. No external network exposure. --- ### Summary Solid PR. The implementation is correct, the comments are clear and honest about the failure-mode tradeoffs, and it fills a real gap (no prior round-trip test existed). The suggestions above are all minor. Happy to approve as-is or with the `osc.msg-i` coverage added if you want exhaustive parity.

claude[bot] commented

2026-04-27 22:01:08 +00:00

(Migrated from github.com)

Code Review — PR #439: OSC UDP Loopback Integration Test

Overview

This PR adds test_osc_loopback.seq, an end-to-end integration test that encodes OSC messages using the Seq encoder, round-trips them through a UDP loopback socket, and asserts byte equality on the received payload. It wires the test into justfile's test-integration target. A solid addition that closes the gap between the byte-format unit tests in test_osc.seq and actual wire delivery.

What's Good

0 udp.bind for port selection — letting the OS assign the ephemeral port is the right call; it eliminates port collision across concurrent CI runs without any coordination.
All four message-builder variants covered — osc.msg, osc.msg-i, osc.msg-f, and osc.msg-if each get a round-trip test. Good coverage of the existing encoder surface.
Socket is always closed — aux> udp.close drop runs unconditionally after the receive, so the handle is never leaked on the happy path.
Stack comments in udp-loopback — for a concatenative language the inline stack annotations (# ( msg socket port ) etc.) make the manipulation auditable without running the code mentally.

Concerns / Suggestions

1. No timeout on udp.receive-from — hangs silently if send is dropped

The comment acknowledges this explicitly:

"If loopback ever does start failing, the receive blocks and the test times out instead of asserting bogus equality."

That's a deliberate tradeoff, but it's worth noting: when udp.send-to drops its success flag and the send actually failed, the test will block indefinitely. The only backstop is a CI-level job timeout. If the Seq runtime ever gains a udp.receive-timeout or similar word, this would be the first place to use it.

2. drop drop drop ordering is inverted in the comment

udp.receive-from                 # ( bytes host port ok )
drop drop drop                   # drop ok port host → ( bytes )

The stack after udp.receive-from has ok on top, then port, then host, then bytes at the bottom. The drops execute TOS-first, so the actual order is: drop ok, drop port, drop host. The annotation "drop ok port host" correctly lists the discard order — that's fine. But the arrow notation drop ok port host → ( bytes ) slightly implies these are simultaneous rather than sequential, which could mislead someone reading fast. A clearer phrasing would be:

drop drop drop                   # ( bytes ) — discards ok, port, host

Minor, but the existing test_osc.seq has no inline comments at all, so any comment here sets a style precedent.

3. Missing round-trip test for negative int / edge-case float

test_osc.seq tests osc.int32 -1 and osc.float32 0.0 at the primitive level. The loopback suite only exercises osc.msg-i with 60 and osc.msg-f with 440.0. A round-trip of a negative integer (e.g. -1) through osc.msg-i would confirm the big-endian sign extension survives the UDP path end-to-end. Low priority since the byte-format tests already cover encoding; this is more about belt-and-suspenders for the transport layer.

4. Comment verbosity inconsistent with sibling file

test_osc.seq has zero inline comments — just word definitions. test_osc_loopback.seq opens with a 14-line block comment and sprinkles rationale throughout. Given how complex udp-loopback is as stack manipulation, the comments are justified here. Worth a quick team decision on whether the file-header rationale block should become standard for integration tests, or whether to trim it back to match the sibling file's style.

5. justfile comment is accurate but slightly redundant with the existing one

# OSC encoder byte-format tests live in the live-coding-csound POC dir
# because the encoder includes a relative file (`include "osc"`), which
# the test runner's tmp-file wrapper can't resolve cross-directory.
target/examples/projects-live-coding-csound-test_osc
# Same dir: end-to-end round-trip through UDP loopback. Proves the
# byte-cleanliness landing, the Seq OSC encoder, and the UDP runtime
# all line up.
target/examples/projects-live-coding-csound-test_osc_loopback

The new comment says "Same dir" which implies the reader needs to connect it to the comment three lines up. If the two lines ever get separated by a future addition, "Same dir" loses its meaning. Consider making it self-contained: # Lives in the live-coding-csound dir (same include-path constraint as test_osc above).

Stack Trace Audit — `udp-loopback`

Traced through manually to verify correctness:

Step	Stack	Aux
entry	`( msg )`	`[]`
`0 udp.bind drop`	`( msg socket port )`	`[]`
`swap`	`( msg port socket )`	`[]`
`dup >aux`	`( msg port socket )`	`[socket]`
`swap`	`( msg socket port )`	`[socket]`
`"127.0.0.1" swap`	`( msg socket "127.0.0.1" port )`	`[socket]`
`rot`	`( msg "127.0.0.1" port socket )`	`[socket]`
`udp.send-to drop`	`( )`	`[socket]`
`aux>`	`( socket )`	`[]`
`dup >aux`	`( socket )`	`[socket]`
`udp.receive-from`	`( bytes host port ok )`	`[socket]`
`drop drop drop`	`( bytes )`	`[socket]`
`aux> udp.close drop`	`( bytes )`	`[]`

Logic is correct. The >aux / aux> usage for carrying the socket across the send is clean.

Summary

Approve with minor suggestions. The test is correct, covers the important code paths, and the CI-parallel-safe port binding is a nice touch. The main thing to think about is the infinite-hang risk if send fails silently — acceptable today given CI timeouts, but worth documenting as a known limitation or filing a follow-up for udp.receive-timeout.

## Code Review — PR #439: OSC UDP Loopback Integration Test ### Overview This PR adds `test_osc_loopback.seq`, an end-to-end integration test that encodes OSC messages using the Seq encoder, round-trips them through a UDP loopback socket, and asserts byte equality on the received payload. It wires the test into `justfile`'s `test-integration` target. A solid addition that closes the gap between the byte-format unit tests in `test_osc.seq` and actual wire delivery. --- ### What's Good - **`0 udp.bind` for port selection** — letting the OS assign the ephemeral port is the right call; it eliminates port collision across concurrent CI runs without any coordination. - **All four message-builder variants covered** — `osc.msg`, `osc.msg-i`, `osc.msg-f`, and `osc.msg-if` each get a round-trip test. Good coverage of the existing encoder surface. - **Socket is always closed** — `aux> udp.close drop` runs unconditionally after the receive, so the handle is never leaked on the happy path. - **Stack comments in `udp-loopback`** — for a concatenative language the inline stack annotations (`# ( msg socket port )` etc.) make the manipulation auditable without running the code mentally. --- ### Concerns / Suggestions **1. No timeout on `udp.receive-from` — hangs silently if send is dropped** The comment acknowledges this explicitly: > _"If loopback ever does start failing, the receive blocks and the test times out instead of asserting bogus equality."_ That's a deliberate tradeoff, but it's worth noting: when `udp.send-to` drops its success flag and the send actually failed, the test will block indefinitely. The only backstop is a CI-level job timeout. If the Seq runtime ever gains a `udp.receive-timeout` or similar word, this would be the first place to use it. **2. `drop drop drop` ordering is inverted in the comment** ```seq udp.receive-from # ( bytes host port ok ) drop drop drop # drop ok port host → ( bytes ) ``` The stack after `udp.receive-from` has `ok` on top, then `port`, then `host`, then `bytes` at the bottom. The drops execute TOS-first, so the actual order is: drop `ok`, drop `port`, drop `host`. The annotation "drop ok port host" correctly lists the discard order — that's fine. But the arrow notation `drop ok port host → ( bytes )` slightly implies these are simultaneous rather than sequential, which could mislead someone reading fast. A clearer phrasing would be: ```seq drop drop drop # ( bytes ) — discards ok, port, host ``` Minor, but the existing `test_osc.seq` has no inline comments at all, so any comment here sets a style precedent. **3. Missing round-trip test for negative int / edge-case float** `test_osc.seq` tests `osc.int32 -1` and `osc.float32 0.0` at the primitive level. The loopback suite only exercises `osc.msg-i` with `60` and `osc.msg-f` with `440.0`. A round-trip of a negative integer (e.g. `-1`) through `osc.msg-i` would confirm the big-endian sign extension survives the UDP path end-to-end. Low priority since the byte-format tests already cover encoding; this is more about belt-and-suspenders for the transport layer. **4. Comment verbosity inconsistent with sibling file** `test_osc.seq` has zero inline comments — just word definitions. `test_osc_loopback.seq` opens with a 14-line block comment and sprinkles rationale throughout. Given how complex `udp-loopback` is as stack manipulation, the comments _are_ justified here. Worth a quick team decision on whether the file-header rationale block should become standard for integration tests, or whether to trim it back to match the sibling file's style. **5. justfile comment is accurate but slightly redundant with the existing one** ``` # OSC encoder byte-format tests live in the live-coding-csound POC dir # because the encoder includes a relative file (`include "osc"`), which # the test runner's tmp-file wrapper can't resolve cross-directory. target/examples/projects-live-coding-csound-test_osc # Same dir: end-to-end round-trip through UDP loopback. Proves the # byte-cleanliness landing, the Seq OSC encoder, and the UDP runtime # all line up. target/examples/projects-live-coding-csound-test_osc_loopback ``` The new comment says "Same dir" which implies the reader needs to connect it to the comment three lines up. If the two lines ever get separated by a future addition, "Same dir" loses its meaning. Consider making it self-contained: `# Lives in the live-coding-csound dir (same include-path constraint as test_osc above).` --- ### Stack Trace Audit — `udp-loopback` Traced through manually to verify correctness: | Step | Stack | Aux | |------|-------|-----| | entry | `( msg )` | `[]` | | `0 udp.bind drop` | `( msg socket port )` | `[]` | | `swap` | `( msg port socket )` | `[]` | | `dup >aux` | `( msg port socket )` | `[socket]` | | `swap` | `( msg socket port )` | `[socket]` | | `"127.0.0.1" swap` | `( msg socket "127.0.0.1" port )` | `[socket]` | | `rot` | `( msg "127.0.0.1" port socket )` | `[socket]` | | `udp.send-to drop` | `( )` | `[socket]` | | `aux>` | `( socket )` | `[]` | | `dup >aux` | `( socket )` | `[socket]` | | `udp.receive-from` | `( bytes host port ok )` | `[socket]` | | `drop drop drop` | `( bytes )` | `[socket]` | | `aux> udp.close drop` | `( bytes )` | `[]` | Logic is correct. The `>aux` / `aux>` usage for carrying the socket across the send is clean. --- ### Summary **Approve with minor suggestions.** The test is correct, covers the important code paths, and the CI-parallel-safe port binding is a nice touch. The main thing to think about is the infinite-hang risk if send fails silently — acceptable today given CI timeouts, but worth documenting as a known limitation or filing a follow-up for `udp.receive-timeout`.