Skip to content

Migrate published-package smoke tests in-tree with per-release pinning - #1639

Closed
kixelated wants to merge 5 commits into
mainfrom
claude/practical-carson-ec0f3f
Closed

Migrate published-package smoke tests in-tree with per-release pinning#1639
kixelated wants to merge 5 commits into
mainfrom
claude/practical-carson-ec0f3f

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Summary

Brings the cross-language interop smoke harness (formerly the standalone moq-dev/smoke repo) into the monorepo at test/smoke, and shifts it from a daily cross-OS cron to a Linux-only nightly backstop + per-release slices pinned to the version just published. macOS now runs only when a macOS artifact ships (the Swift package), removing the daily macOS spend that had already forced those cron cells to be disabled.

Motivation: the daily matrix was expensive (macOS runners), and "always latest" gives a fuzzy signal. Running a slice immediately after a release, pinned to the exact version, is cheaper and tests what was actually shipped.

What changed

  • test/smoke/smoke.sh, token.sh, smoke.toml, freshness.sh, README, and all clients/ moved in-tree. Clients install from public registries and sit outside the cargo/bun workspaces, so they resolve published artifacts, never workspace source. git ls-files scoping keeps the freshness lockfile guard local to the subtree (the monorepo's own root lockfiles are untouched).
  • just smoke <lang> — per-language slices (rust, python, go, swift, kotlin, c, gst, js, js-native) wired into the root justfile as a module; each pairs the named client with rust on the opposite axis.
  • Pinned modeMOQ_{RS,GO,NPM,SWIFT,KT,LIBMOQ,GST}_VERSION (rust via RELAY_BIN/MOQ_BIN) install an exact version instead of latest, skip the freshness guard, and poll the registry until the version resolves (publish can finish before the artifact is downloadable). Package.swift / build.gradle.kts read the env to pick their dependency requirement.
  • CI.github/workflows/smoke.yml is the nightly Linux backstop (cargo/apt/nix/docker) + on-demand + PR-on-harness-change. smoke-release.yml is a reusable per-language slice that release-py, release-swift, release-kt, release-go, libmoq, and moq-gst call right after publishing, pinned to the version they just cut.
  • Docs — README rewritten for the in-tree context + pinned mode; CLAUDE.md gains the test/smoke structure entry, a testing note, and a Cross-Package Sync row.

Test plan / verification

  • just smoke rust (latest mode) passes
  • MOQ_RS_VERSION=0.2.16 just smoke python (pinned mode) passes 4/4, freshness skipped, exact wheel installed
  • just smoke freshness exit 0
  • shellcheck + shfmt clean on all test/smoke shell files (monorepo .editorconfig)
  • actionlint green on all 8 touched workflows (incl. reusable-workflow input validation)
  • justfile fmt + remark markdown clean
  • Release-workflow hooks: correct by construction but only exercised by an actual release

Reviewer notes

  • Scope can be trimmed. The original ask was just "run just smoke swift after a release." This wires pinned hooks for 6 release workflows. If that's more surface than wanted, the leanest cut is to keep the nightly + only the swift and python hooks (the two verified locally) and drop the kt/go/c/gst hooks until there's demand — go and gst can't pass today anyway. The reusable workflow makes re-adding them a ~6-line job.
  • Per-release CI cost: each hook cargo installs a reference relay+cli (~3–5 min cold); moq-ffi-v* fans out to swift+kt+go = three such jobs per release. Much cheaper than daily-all-OS, but not free.
  • Deliberately not wired: release-js (publishes per-package versions, no single output to pin); moq-cli/moq-relay (rust is the reference, covered by every slice + nightly); token-interop pinning (stays nightly/latest).
  • Maven Central lag: the kotlin slice gets a 20-min poll budget; if Central sync is slower the job will time out and need a re-run.
  • Local-nix gap: the monorepo nix develop shell carries no Chromium, so just smoke js falls back to a Playwright download there. Adding playwright-driver.browsers to the flake is a clean follow-up.
  • Follow-up: archive moq-dev/smoke once this merges — its role is now fully in-tree.

(Written by Claude)

🤖 Generated with Claude Code

@kixelated

Copy link
Copy Markdown
Collaborator Author

CI finding: browser → gst interop gap

The first smoke run on this PR surfaced a real (pre-existing, published-package) gap, not a harness bug:

  • rust → gst and python → gst: PASS (a moq-gst release now exists and moqsrc loads + reads those broadcasts)
  • js-vite/js-esbuild/js-jsdelivr → gst: FAILmoqsrc gets no frame within the 30s timeout from a browser-published (WebCodecs H.264) broadcast

So moqsrc can read rust/python-published content but not browser-published content. Likely a catalog/codec-string difference between the browser publisher and what moqsrc expects. This is in the published moq-gst/hang, independent of this migration.

Handling here: the PR smoke run is scoped to harness mechanics (cargo channel, gst dropped) so this gap doesn't block harness PRs. The nightly keeps the full matrix including gst, where browser → gst will show red until the gap is fixed (or gst is declared browser-incompatible). Flagging for triage as a separate issue.

(Written by Claude)

kixelated and others added 2 commits June 6, 2026 14:36
Bring the cross-language interop smoke harness (formerly moq-dev/smoke)
into the monorepo at test/smoke, and shift it from a daily cross-OS cron
to a Linux-only nightly backstop plus per-release slices pinned to the
version just published. macOS now runs only when a macOS artifact ships
(the Swift package), which removes the daily macOS spend that had already
forced those cron cells to be disabled.

What changed:

- test/smoke: smoke.sh, token.sh, smoke.toml, freshness.sh, README, and
  all clients moved in-tree. Clients install from public registries and
  sit outside the cargo/bun workspaces, so they resolve published
  artifacts, never workspace source. git ls-files scoping keeps the
  freshness lockfile guard local to the subtree.
- just smoke <lang>: per-language slices (rust, python, go, swift,
  kotlin, c, gst, js, js-native) wired into the root justfile as a
  module. Each pairs the named client with rust on the opposite axis.
- Pinned mode: MOQ_{RS,GO,NPM,SWIFT,KT,LIBMOQ,GST}_VERSION (rust via
  RELAY_BIN/MOQ_BIN) make a client install an exact version instead of
  latest, skipping the freshness guard and polling the registry until the
  version resolves (publish can finish before the artifact is
  downloadable). Package.swift and build.gradle.kts read the env to pick
  their dependency requirement.
- CI: .github/workflows/smoke.yml is the nightly Linux backstop
  (cargo/apt/nix/docker) plus on-demand and PR-on-harness-change runs.
  smoke-release.yml is a reusable per-language slice that release-py,
  release-swift, release-kt, release-go, libmoq, and moq-gst call right
  after publishing, pinned to the version they just cut.
- Docs: README rewritten for the in-tree context and pinned mode; CLAUDE
  .md gains the test/smoke structure entry, a testing note, and a
  Cross-Package Sync row.

Verified locally: latest-mode rust and pinned-mode python (moq-rs 0.2.16)
slices pass; shellcheck/shfmt/just-fmt/remark clean; actionlint green on
all touched workflows. The release-workflow hooks are correct by
construction but only exercised by an actual release.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The PR trigger (which fires only when test/smoke changes) was running the
full nightly cross-product on every channel, so a pre-existing
published-package interop gap blocked an unrelated harness PR. The first
run surfaced browser -> gst: moqsrc reads rust/python-published broadcasts
fine but times out on browser (WebCodecs H.264) ones. That is a real gap
in the published moq-gst, not the harness.

Scope the PR run to validate mechanics: the fast cargo channel only, and
drop gst (the one red cell) from the subscriber set. The nightly and
on-demand runs keep all four channels and the full matrix including gst,
so browser -> gst stays visible there as honest signal.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kixelated
kixelated force-pushed the claude/practical-carson-ec0f3f branch from 68c9bc0 to c6cd846 Compare June 6, 2026 21:36
@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e719ec47-1b80-4c75-aaad-fede6a9c75ba

📥 Commits

Reviewing files that changed from the base of the PR and between 02d0653 and 4b0c65f.

📒 Files selected for processing (3)
  • .github/workflows/smoke-release.yml
  • .github/workflows/smoke.yml
  • flake.nix
🚧 Files skipped from review as they are similar to previous changes (3)
  • flake.nix
  • .github/workflows/smoke-release.yml
  • .github/workflows/smoke.yml

Walkthrough

This PR establishes a comprehensive cross-language smoke test suite for published moq packages. It adds post-release validation hooks to six existing release workflows, introduces a nightly CI workflow with four installation channels (cargo, apt, nix, docker), and implements publish/subscribe clients across multiple languages. The orchestration harness coordinates synthetic H.264 streaming through a local relay, verifies frame reception in a publisher-subscriber matrix with per-language failure isolation, and enforces version freshness policies. A separate token verification harness validates JWT/JWK compatibility across implementations.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.03% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: migrating the cross-language smoke test harness in-tree and establishing per-release version pinning.
Description check ✅ Passed The description comprehensively explains the PR's objectives, changes, test plan, and reviewer notes, and is clearly related to the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch claude/practical-carson-ec0f3f

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (4)
test/smoke/clients/token/js/resolve-bin.mjs (1)

1-10: 💤 Low value

Consider condensing the header comment.

The comment explains a genuinely non-obvious WHY (Node's strict ESM resolver vs. bun's permissive behavior), but the current 10-line documentation style could be more conversational. Based on learnings, one short line is almost always enough.

Suggested condensed version
-// Print the absolute path to the published `@moq/token` CLI entrypoint.
-//
-// token.sh runs this with BOTH node and bun so each runtime resolves the same
-// installed package and we drive the *published* bin (compiled dist), not the
-// in-tree TypeScript source. We read the installed package.json straight off
-// disk rather than via module resolution: `@moq/token`'s `exports` map doesn't
-// expose ./package.json, which Node's strict ESM resolver refuses (bun allows
-// it), so require.resolve would work under bun but throw under node. Reading the
-// file keeps both runtimes on the same path, and the bin name is still taken
-// from the published manifest so a rename surfaces here.
+// Print the absolute path to the published `@moq/token` CLI entrypoint. We read
+// package.json from disk instead of via module resolution because Node's strict
+// ESM resolver refuses unexported paths (bun allows it), and this keeps both
+// runtimes on the same path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/smoke/clients/token/js/resolve-bin.mjs` around lines 1 - 10, Condense
the long header comment at the top of resolve-bin.mjs into a single,
conversational line that states its purpose: print the absolute path to the
published `@moq/token` CLI entrypoint for both node and bun, and note briefly that
we read the installed package.json from disk (not via module resolution) to
avoid Node’s strict ESM resolver difference with bun while still using the
published manifest’s bin name. Reference this behavior in one short sentence so
the intent (why we read package.json and that the test targets the published
bin) remains clear.

Source: Learnings

test/smoke/clients/js-native/subscribe.ts (1)

35-35: ⚡ Quick win

Remove redundant fallback for timeout default.

Line 35 uses values.timeout ?? "20" but the parseArgs options on line 28 already define default: "20", so values.timeout will never be undefined. The nullish coalescing is dead code.

♻️ Simplify timeout parsing
-const timeoutMs = Number.parseFloat(values.timeout ?? "20") * 1000;
+const timeoutMs = Number.parseFloat(values.timeout) * 1000;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/smoke/clients/js-native/subscribe.ts` at line 35, The timeout parsing
contains a redundant fallback: remove the nullish coalescing and parse
values.timeout directly (since parseArgs already sets default "20"); change the
expression used to compute timeoutMs (currently using
Number.parseFloat(values.timeout ?? "20") * 1000) to parse values.timeout
without the "?? '20'" fallback so timeoutMs = Number.parseFloat(values.timeout)
* 1000.
test/smoke/clients/js/driver.ts (2)

41-41: ⚡ Quick win

Remove redundant fallback for timeout default.

Line 41 uses values.timeout ?? "20" but parseArgs options on line 23 already define default: "20", so values.timeout will never be undefined. The nullish coalescing is dead code.

♻️ Simplify timeout parsing
-const timeoutMs = Number.parseFloat(values.timeout ?? "20") * 1000;
+const timeoutMs = Number.parseFloat(values.timeout) * 1000;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/smoke/clients/js/driver.ts` at line 41, The timeout parsing uses a
redundant nullish fallback because parseArgs already sets a default; remove the
"?? '20'" and parse the provided value directly (update the line creating
timeoutMs to use Number.parseFloat(values.timeout) * 1000), referencing the
parseArgs usage that defines the default and the timeoutMs constant to locate
the change.

95-95: ⚡ Quick win

Replace magic number with named constant.

Line 95 uses timeoutMs / 2 to determine the reload timing. The 0.5 fraction is a magic number. As per coding guidelines, use a named constant to clarify intent.

♻️ Extract reload timing constant
+const RELOAD_TIMEOUT_FRACTION = 0.5;
+
 const browser = await chromium.launch({
-			if (!reloaded && Date.now() - start > timeoutMs / 2) {
+			if (!reloaded && Date.now() - start > timeoutMs * RELOAD_TIMEOUT_FRACTION) {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/smoke/clients/js/driver.ts` at line 95, Replace the magic fraction used
in the reload timing check with a named constant: introduce a constant (e.g.,
RELOAD_THRESHOLD_FRACTION = 0.5 or RELOAD_BEFORE_TIMEOUT_RATIO) near the top of
the module or the enclosing scope and use it in the condition instead of
`timeoutMs / 2` so the check becomes `Date.now() - start > timeoutMs *
RELOAD_THRESHOLD_FRACTION`; update any related comments to explain the threshold
and keep references to the existing variables `reloaded`, `start`, and
`timeoutMs`.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/smoke-release.yml:
- Around line 123-124: The workflow pins Gradle via the gradle-version key with
the outdated value "8.14.4"; update that value to the current Gradle release
(e.g. "9.5.1") in the smoke-release workflow so the action uses the latest
Gradle; locate the gradle-version: "8.14.4" entry and replace the version string
with "9.5.1" (or the value returned by services.gradle.org/versions/current).

In `@test/smoke/README.md`:
- Line 5: The sentence in the README contains a lowercase start after a period
("header. none"); update the text to capitalize the new sentence (change
"header. none" to "header. None") in the README content so sentences start with
a capital letter, and scan nearby lines for any similar lowercase-after-period
occurrences to correct them as well.
- Around line 98-116: The fenced code block in test/smoke/README.md is missing a
language identifier; change the opening fence from ``` to ```text so the
directory listing is rendered/highlighted correctly (update the block that
begins with justfile per-language slices and ends before freshness.sh), ensuring
the closing fence remains ```; no other content changes required.

---

Nitpick comments:
In `@test/smoke/clients/js-native/subscribe.ts`:
- Line 35: The timeout parsing contains a redundant fallback: remove the nullish
coalescing and parse values.timeout directly (since parseArgs already sets
default "20"); change the expression used to compute timeoutMs (currently using
Number.parseFloat(values.timeout ?? "20") * 1000) to parse values.timeout
without the "?? '20'" fallback so timeoutMs = Number.parseFloat(values.timeout)
* 1000.

In `@test/smoke/clients/js/driver.ts`:
- Line 41: The timeout parsing uses a redundant nullish fallback because
parseArgs already sets a default; remove the "?? '20'" and parse the provided
value directly (update the line creating timeoutMs to use
Number.parseFloat(values.timeout) * 1000), referencing the parseArgs usage that
defines the default and the timeoutMs constant to locate the change.
- Line 95: Replace the magic fraction used in the reload timing check with a
named constant: introduce a constant (e.g., RELOAD_THRESHOLD_FRACTION = 0.5 or
RELOAD_BEFORE_TIMEOUT_RATIO) near the top of the module or the enclosing scope
and use it in the condition instead of `timeoutMs / 2` so the check becomes
`Date.now() - start > timeoutMs * RELOAD_THRESHOLD_FRACTION`; update any related
comments to explain the threshold and keep references to the existing variables
`reloaded`, `start`, and `timeoutMs`.

In `@test/smoke/clients/token/js/resolve-bin.mjs`:
- Around line 1-10: Condense the long header comment at the top of
resolve-bin.mjs into a single, conversational line that states its purpose:
print the absolute path to the published `@moq/token` CLI entrypoint for both node
and bun, and note briefly that we read the installed package.json from disk (not
via module resolution) to avoid Node’s strict ESM resolver difference with bun
while still using the published manifest’s bin name. Reference this behavior in
one short sentence so the intent (why we read package.json and that the test
targets the published bin) remains clear.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5fa872e9-a973-4db6-a9d4-c1e8b9a7ea2d

📥 Commits

Reviewing files that changed from the base of the PR and between 00aa477 and b309dbc.

📒 Files selected for processing (41)
  • .github/workflows/libmoq.yml
  • .github/workflows/moq-gst.yml
  • .github/workflows/release-go.yml
  • .github/workflows/release-kt.yml
  • .github/workflows/release-py.yml
  • .github/workflows/release-swift.yml
  • .github/workflows/smoke-release.yml
  • .github/workflows/smoke.yml
  • CLAUDE.md
  • justfile
  • test/justfile
  • test/smoke/.gitignore
  • test/smoke/README.md
  • test/smoke/clients/c/subscribe.c
  • test/smoke/clients/docker/moq-cli
  • test/smoke/clients/docker/moq-relay
  • test/smoke/clients/go/go.mod
  • test/smoke/clients/go/smoke.go
  • test/smoke/clients/js-native/package.json
  • test/smoke/clients/js-native/subscribe.ts
  • test/smoke/clients/js/build-esbuild.ts
  • test/smoke/clients/js/driver.ts
  • test/smoke/clients/js/index.html
  • test/smoke/clients/js/jsdelivr/index.html
  • test/smoke/clients/js/jsdelivr/setup.js
  • test/smoke/clients/js/package.json
  • test/smoke/clients/js/src/main.ts
  • test/smoke/clients/js/vite.config.ts
  • test/smoke/clients/kotlin/build.gradle.kts
  • test/smoke/clients/kotlin/settings.gradle.kts
  • test/smoke/clients/kotlin/src/main/kotlin/Main.kt
  • test/smoke/clients/python/smoke.py
  • test/smoke/clients/swift/Package.swift
  • test/smoke/clients/swift/Sources/smoke/main.swift
  • test/smoke/clients/token/js/package.json
  • test/smoke/clients/token/js/resolve-bin.mjs
  • test/smoke/freshness.sh
  • test/smoke/justfile
  • test/smoke/smoke.sh
  • test/smoke/smoke.toml
  • test/smoke/token.sh

Comment thread .github/workflows/smoke-release.yml Outdated
Comment thread test/smoke/README.md Outdated
Comment thread test/smoke/README.md Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
test/smoke/README.md (2)

98-98: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Specify language for fenced code block.

The fenced code block should specify a language identifier for proper syntax highlighting and rendering. For directory structure listings, use text.

📝 Proposed fix
 ## Layout
 
-```
+```text
 justfile                 per-language slices (`just smoke <lang>`), wired into the root justfile
 smoke.sh                 orchestrator: relay + media interop matrix
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/smoke/README.md` at line 98, The fenced code block in the README (the
directory listing containing "justfile ... smoke.sh") is missing a language
identifier; update the triple-backtick fence to include a language (use `text`)
so the block reads ```text and retains the same contents, e.g., the lines with
"justfile                 per-language slices (`just smoke <lang>`), wired into
the root justfile" and "smoke.sh                 orchestrator: relay + media
interop matrix".

Source: Linters/SAST tools


5-5: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix capitalization after period.

Line 5 ends with "header. none" but a new sentence must start with a capital letter.

📝 Proposed fix
-The per-language unit tests (`just test`) build every client from workspace source. That proves the code in the tree works; it does **not** prove a real user can install the published artifacts and have them talk to each other. A missing wheel, a stale Homebrew formula, a broken `.deb`, an export that didn't survive packaging, a Go module missing its header. none of that shows up until someone installs from a registry.
+The per-language unit tests (`just test`) build every client from workspace source. That proves the code in the tree works; it does **not** prove a real user can install the published artifacts and have them talk to each other. A missing wheel, a stale Homebrew formula, a broken `.deb`, an export that didn't survive packaging, a Go module missing its header. None of that shows up until someone installs from a registry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/smoke/README.md` at line 5, The sentence fragment "header. none" should
start the new sentence with a capital letter; locate the text "header. none" in
the README (the sentence ending with "header. none") and change "none" to "None"
so the sentence reads "header. None". Ensure punctuation and spacing remain
unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/smoke/clients/swift/Sources/smoke/main.swift`:
- Around line 90-97: The code currently calls withTimeout(args.timeout) which
will convert a Double to UInt64 and can trap for negative/NaN/infinite values;
before calling withTimeout (and before invoking subscribe), validate
args.timeout returned by parseArgs() is finite and >= 0 (and optionally within a
sane max) and handle invalid values by printing the usage/error and exiting
(same path used when args.url or args.broadcast are empty). Update the call
sites around parseArgs(), args.timeout, withTimeout and subscribe to perform
this check and only call withTimeout when the timeout is safe to convert.

---

Duplicate comments:
In `@test/smoke/README.md`:
- Line 98: The fenced code block in the README (the directory listing containing
"justfile ... smoke.sh") is missing a language identifier; update the
triple-backtick fence to include a language (use `text`) so the block reads
```text and retains the same contents, e.g., the lines with "justfile           
per-language slices (`just smoke <lang>`), wired into the root justfile" and
"smoke.sh                 orchestrator: relay + media interop matrix".
- Line 5: The sentence fragment "header. none" should start the new sentence
with a capital letter; locate the text "header. none" in the README (the
sentence ending with "header. none") and change "none" to "None" so the sentence
reads "header. None". Ensure punctuation and spacing remain unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 473d1ef7-3923-4c48-919c-e08fb87591ea

📥 Commits

Reviewing files that changed from the base of the PR and between b309dbc and c6cd846.

📒 Files selected for processing (41)
  • .github/workflows/libmoq.yml
  • .github/workflows/moq-gst.yml
  • .github/workflows/release-go.yml
  • .github/workflows/release-kt.yml
  • .github/workflows/release-py.yml
  • .github/workflows/release-swift.yml
  • .github/workflows/smoke-release.yml
  • .github/workflows/smoke.yml
  • CLAUDE.md
  • justfile
  • test/justfile
  • test/smoke/.gitignore
  • test/smoke/README.md
  • test/smoke/clients/c/subscribe.c
  • test/smoke/clients/docker/moq-cli
  • test/smoke/clients/docker/moq-relay
  • test/smoke/clients/go/go.mod
  • test/smoke/clients/go/smoke.go
  • test/smoke/clients/js-native/package.json
  • test/smoke/clients/js-native/subscribe.ts
  • test/smoke/clients/js/build-esbuild.ts
  • test/smoke/clients/js/driver.ts
  • test/smoke/clients/js/index.html
  • test/smoke/clients/js/jsdelivr/index.html
  • test/smoke/clients/js/jsdelivr/setup.js
  • test/smoke/clients/js/package.json
  • test/smoke/clients/js/src/main.ts
  • test/smoke/clients/js/vite.config.ts
  • test/smoke/clients/kotlin/build.gradle.kts
  • test/smoke/clients/kotlin/settings.gradle.kts
  • test/smoke/clients/kotlin/src/main/kotlin/Main.kt
  • test/smoke/clients/python/smoke.py
  • test/smoke/clients/swift/Package.swift
  • test/smoke/clients/swift/Sources/smoke/main.swift
  • test/smoke/clients/token/js/package.json
  • test/smoke/clients/token/js/resolve-bin.mjs
  • test/smoke/freshness.sh
  • test/smoke/justfile
  • test/smoke/smoke.sh
  • test/smoke/smoke.toml
  • test/smoke/token.sh
✅ Files skipped from review due to trivial changes (5)
  • test/smoke/clients/kotlin/settings.gradle.kts
  • test/smoke/clients/js/package.json
  • test/smoke/clients/js-native/package.json
  • test/justfile
  • test/smoke/.gitignore
🚧 Files skipped from review as they are similar to previous changes (32)
  • test/smoke/clients/token/js/package.json
  • .github/workflows/release-py.yml
  • test/smoke/clients/js/vite.config.ts
  • test/smoke/clients/docker/moq-relay
  • CLAUDE.md
  • test/smoke/clients/js/jsdelivr/index.html
  • test/smoke/smoke.toml
  • test/smoke/clients/js/index.html
  • test/smoke/clients/js/jsdelivr/setup.js
  • .github/workflows/release-swift.yml
  • test/smoke/clients/swift/Package.swift
  • test/smoke/clients/js/driver.ts
  • .github/workflows/release-kt.yml
  • test/smoke/clients/docker/moq-cli
  • .github/workflows/libmoq.yml
  • justfile
  • .github/workflows/release-go.yml
  • test/smoke/clients/go/smoke.go
  • test/smoke/clients/js/src/main.ts
  • test/smoke/clients/go/go.mod
  • test/smoke/clients/js/build-esbuild.ts
  • test/smoke/clients/js-native/subscribe.ts
  • test/smoke/clients/python/smoke.py
  • .github/workflows/moq-gst.yml
  • .github/workflows/smoke-release.yml
  • test/smoke/clients/kotlin/build.gradle.kts
  • test/smoke/justfile
  • test/smoke/clients/c/subscribe.c
  • test/smoke/token.sh
  • test/smoke/freshness.sh
  • .github/workflows/smoke.yml
  • test/smoke/smoke.sh

Comment thread test/smoke/clients/swift/Sources/smoke/main.swift
kixelated and others added 3 commits June 6, 2026 14:52
- README: capitalize "None" after a period; tag the layout fence as `text`.
- driver.ts: name the reload-timing fraction (RELOAD_AT_FRACTION) instead of
  the bare `timeoutMs / 2`.
- resolve-bin.mjs: condense the header comment to the essential WHY.

Skipped, with reason:
- smoke-release.yml Gradle 8.14.4: kept to match the repo's other Gradle
  workflows (release-kt) and avoid Gradle 9 breaking changes; not chasing latest.
- driver.ts / subscribe.ts `values.timeout ?? "20"`: util.parseArgs types the
  value as `string | undefined` even with a `default`, so the fallback is a type
  guard, not dead code.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the per-language setup-actions (setup-go/bun/node/uv/java/gradle) and
the generic `apt-get install ffmpeg curl jq coreutils procps` with a `smoke`
devShell, so the toolchains are pinned via flake.lock and reproducible between a
local `nix develop` and CI. The default devShell stays lean: smoke toolchains
live in their own shells, so day-to-day `nix develop` doesn't pull
go/jdk/gradle/Chromium (this was a reason the smoke harness lived in a separate
repo; a separate devShell output solves it without a separate repo).

flake.nix:
- Add a `smoke` shell (full: every client + the cargo channel + a pinned
  Chromium) for the nightly, plus lean per-slice shells (`smoke-python`,
  `smoke-go`, `smoke-kotlin`, `smoke-js`, `smoke-min`) so a release runs only
  what its slice needs (the swift job on the macOS runner pulls just the harness,
  not jdk/Chromium). Composed from shared package groups.

Workflows now run `nix develop .#smoke[-<slice>] --command ./test/smoke/...`:
- smoke.yml (nightly): one nix install for all channels; the cargo channel's
  `cargo install` uses the devShell's rust.
- smoke-release.yml: per-slice shell; reference relay/cli from `nix build`.

Two toolchains stay system-level, by design:
- GStreamer (gst): the prebuilt moq-gst plugin dynamically links a *system*
  GStreamer (the .deb/brew scenario it tests); a nix-store gst wouldn't satisfy
  its NEEDED libs. apt-installed only on the runs that include the gst cell.
- C compiler (c): the C client links the prebuilt libmoq.a with the system cc
  (CC=/usr/bin/cc), the real consumer scenario. Swift likewise uses Xcode.

Bonus: the nix-pinned Chromium (PLAYWRIGHT_BROWSERS_PATH) drops the
`playwright install-deps` apt step, and freshness.sh now *enforces* the
playwright pin (PLAYWRIGHT_VERSION is set) instead of warning.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The first nix-based run segfaulted on every `* -> c` and `* -> go` cell while
all other clients passed. Root cause: `nix develop` sets LIBRARY_PATH / CPATH /
NIX_CFLAGS_*, so forcing the C client (and Go's cgo) to compile with the system
/usr/bin/cc linked the prebuilt libmoq static lib against a mix of system and nix
libs; running the result under `nix develop` then crashed. (python -> python
passes because moq-rs is loaded by the nix python, one consistent libc.)

Drop the CC=/usr/bin/cc override and let c/go use the devShell's own stdenv cc,
so the binary and its runtime share one libc. gst and swift still use the system
toolchain: the gst plugin and gst-launch are prebuilt binaries with no compile
step, so the nix compile-time vars don't affect them.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kixelated kixelated closed this Jun 8, 2026
@kixelated
kixelated deleted the claude/practical-carson-ec0f3f branch June 26, 2026 21:57
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.

1 participant