Skip to content

Conversation

@larsgerber
Copy link

@larsgerber larsgerber commented Nov 29, 2025

Description

Resolves #1722

Replaced direct variable expansion with ${VAR:-} to avoid “parameter not set” errors when running the Mac App Store command-line interface.

Checklist
  • Formatted with cargo fmt
  • Built with nix build
  • Ran flake checks with nix flake check
  • Added or updated relevant tests (leave unchecked if not applicable)
  • Added or updated relevant documentation (leave unchecked if not applicable)
  • Linked to related issues (leave unchecked if not applicable)

Summary by CodeRabbit

  • Bug Fixes
    • macOS remote-building setup now robustly handles unset SSH/session variables in non-interactive SSH sessions. This prevents errors when those variables are absent and ensures the Nix profile sourcing behavior remains consistent across different shell environments.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 29, 2025

Walkthrough

This PR fixes an issue where unset environment variables (SSH_CONNECTION and SHLVL) were causing "parameter not set" errors in shell scripts. Parameter expansions now use default values to ensure safe evaluation in all shell contexts without altering control flow.

Changes

Cohort / File(s) Summary
SSH parameter expansion defaults
src/action/macos/configure_remote_building.rs, tests/fixtures/macos/macos.json
Updated shell snippet to use safe parameter expansion: ${SSH_CONNECTION:-} (empty default) and ${SHLVL:-0} (zero default) to prevent "parameter not set" errors in non-interactive SSH sessions. Test fixture updated to match.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Straightforward parameter expansion defaults applied consistently across implementation and test fixture
  • No logic changes or control flow modifications
  • Simple, homogeneous pattern applied to two related files

Poem

🐰 A shell was crying, "Variables not set!"
So we gave them defaults—no need to fret.
With :- and :-0, the problem's done,
SSH connections now flow like fun!
🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: adding safe variable expansion for SSH_CONNECTION and SHLVL to prevent parameter not set errors.
Linked Issues check ✅ Passed The PR fully addresses issue #1722 by implementing the exact fix proposed: using ${SSH_CONNECTION:-} and ${SHLVL:-0} safe expansions in the shell conditional.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the SSH_CONNECTION and SHLVL variable expansion issue; no out-of-scope modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac037fb and db1eeef.

📒 Files selected for processing (2)
  • src/action/macos/configure_remote_building.rs (1 hunks)
  • tests/fixtures/macos/macos.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/fixtures/macos/macos.json
🔇 Additional comments (1)
src/action/macos/configure_remote_building.rs (1)

22-31: Safe parameter expansion change looks correct and preserves behavior

The updated condition

[ -n "${SSH_CONNECTION:-}" ] && [ "${SHLVL:-0}" -eq 1 ]

is generated correctly via

"${{SSH_CONNECTION:-}}"  // -> "${SSH_CONNECTION:-}"
"${{SHLVL:-0}}"         // -> "${SHLVL:-0}"

and does what you want:

  • When SSH_CONNECTION/SHLVL are set, behavior matches the previous version.
  • When either is unset under set -u/NOUNSET, the test now safely evaluates to false instead of erroring, so mas (and other tools) won’t see /etc/zshenv: SSH_CONNECTION: parameter not set while still skipping Nix profile sourcing.

No issues spotted with quoting or the integer comparison; this cleanly fixes the linked bug without changing the positive-path semantics.


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.

@larsgerber larsgerber changed the title fix(1722): use safe expansion for SSH_CONNECTION and SHLVL variable Use safe expansion for SSH_CONNECTION and SHLVL variable Nov 29, 2025
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.

/etc/zshenv SSH_CONNECTION: parameter not set

2 participants