Skip to content

fix(dev): keep the dev shell out of the MOQ_* namespace - #7979

Merged
kixelated merged 1 commit into
mainfrom
fix/dev-shell-quest-env
Nov 5, 2026
Merged

kixelated merged 1 commit into
mainfrom
fix/dev-shell-quest-env

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Summary

moq-dev/moq#7710 made the dev shell export MOQ_QUEST. Every moq-* binary rejects unknown MOQ_* variables, so moq-relay/moq exited 1 inside the dev shell and just test interop --all failed before any check.

  • Rename the variable to DEV_SHELL_QUEST (outside both MOQ_*, which the binaries own, and QUEST_*, which quest reads); update sh/quest.sh and the three tests.
  • Guard: flake.nix asserts the shell's env contains no MOQ_* key, so evaluating the shell fails loudly if one is added again. The binaries' refusal is unchanged.

Public API / wire impact: none. Anyone scripting against MOQ_QUEST should use DEV_SHELL_QUEST.

Testing

  • Before: MOQ_QUEST=/x target/debug/moq-relay -> unknown environment variables ... MOQ_QUEST, exit 1. After: the dev shell exports no MOQ_*; relay gets past the env check.
  • Guard verified: reintroducing MOQ_QUEST fails eval with dev shell must not export MOQ_*.
  • quest gate just test interop --all: all checks passed.
  • quest gate just check: pass.

Review

quest review --base origin/main (codex gpt-6.1-sol, medium) at head 830af86: 0 findings.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

#7710 exported MOQ_QUEST, which every moq binary rejects as an unknown
variable. Rename it to DEV_SHELL_QUEST and assert the shell exports no MOQ_*.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 22 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6f858b86-c37a-454f-afaf-d9f833700823
Commits

Reviewing files that changed from the base of the PR and between d1c4e5b and 830af86.

Files selected for processing: 5

  • flake.nix
  • sh/dispatch-query.test.py
  • sh/js/workspace.test.py
  • sh/local.test.py
  • sh/quest.sh
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed head: 830af86098c4bce10c3fae6b3826271271efbd8b.

Findings: No actionable defects found in the five-file diff. The flake export, launcher, and test fixtures consistently use DEV_SHELL_QUEST; the launcher preserves quoting, arguments, exit status, and existing fallback behavior. The Nix assertion checks the merged platform-specific env keys, and the new name avoids the binaries' strict MOQ_* validation.

Direction: Keep this tooling variable outside both reserved prefixes rather than weakening binary validation. Existing launcher tests cover pinned selection and failure propagation; no public API or wire changes are introduced.

Verification limits: GitHub-only static review of the diff, surrounding launcher/validation code, and tests. I did not run Nix evaluation, Python tests, or interop checks. The head's Check CI job is still running, so this is not a CI pass or merge-readiness claim.

@pullfrog pullfrog 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.

✅ No blocking issues found. The rename is complete and the guard sits at the right layer.

Verified against the base:

  • MOQ_QUEST on the base appears only in these five files; nothing else in the tree (docs, workflows, .claude, quest plans) referenced it, so there is no missed consumer. The working tree now has zero MOQ_QUEST hits and nine DEV_SHELL_QUEST hits.
  • The guard's MOQ_ prefix filter matches what the binaries actually refuse: moq_tokio::cli::check_env filters the process environment by PREFIX = "MOQ_" (rs/moq-tokio/src/cli/env.rs:9), so IN_MOQ_DEV_SHELL correctly stays outside the check. The assert pkgs.lib.assertMsg (...) idiom also matches the repo's existing use in nix/runner/config-test.nix.
  • The guard is exercised in CI: workflows enter the shell via nix develop --command ... (.github/workflows/interop.yml, ts.yml), so a reintroduced MOQ_* key fails there rather than only in a local shell.
  • The tests are updated consistently (sh/local.test.py:28/81/104, sh/dispatch-query.test.py:291, sh/js/workspace.test.py:82), and the stderr assertion at sh/local.test.py:109 still matches the script's unchanged Dev-shell quest: line.

Optional, non-blocking: the guard inspects only the env set. shellHook (flake.nix:666) is the shell's other export site, so an export MOQ_* added there would reproduce the original failure without tripping the assert. Today's shellHook exports only PATH and TMPDIR, so this is just a note on the comment's "the shell must export none" wording — not a defect.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

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