feat(clipboard): add Zellij support to OSC 52 fallback - #1737
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
📝 WalkthroughWalkthroughThis PR adds k3s cluster remote access configuration and extends clipboard clipboard support for ZELLIJ terminal multiplexer. The k3s changes conditionally apply TLS Subject Alternative Names and intelligently restart the service only on config changes. A new kubeconfig generation script enables remote access via Tailscale. The clipboard utility now recognizes ZELLIJ as a valid terminal for OSC 52 escape sequence fallback, with corresponding test coverage. ChangesK3S Remote Access Setup
Clipboard ZELLIJ Terminal Multiplexer Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Mesa DescriptionTL;DRImproved clipboard functionality by adding Zellij support to the OSC 52 fallback mechanism, and enhanced k3s remote access by integrating Tailscale SANs, providing a remote kubeconfig generator, and optimizing k3s restarts. What changed?
Description generated by Mesa. Update settings |
3e29837 to
2bc99d5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/clipboard_copy_spec.sh (1)
164-184: ⚡ Quick winRestore
TMUXandZELLIJin cleanup for full test isolation.Setup unsets both vars (Line 171), but cleanup doesn’t restore prior values. This can leak state across specs as this file evolves.
Suggested patch
setup() { MOCK_BIN="$(mktemp -d)" MOCK_ORIGINAL_PATH="${PATH:-}" MOCK_ORIGINAL_WAYLAND="${WAYLAND_DISPLAY:-}" MOCK_ORIGINAL_SSH_TTY="${SSH_TTY:-}" + MOCK_ORIGINAL_TMUX="${TMUX:-}" + MOCK_ORIGINAL_ZELLIJ="${ZELLIJ:-}" ln -sf "$(command -v bash)" "$MOCK_BIN/bash" export PATH="$MOCK_BIN" unset WAYLAND_DISPLAY SSH_TTY TMUX ZELLIJ - export MOCK_BIN MOCK_ORIGINAL_PATH MOCK_ORIGINAL_WAYLAND MOCK_ORIGINAL_SSH_TTY + export MOCK_BIN MOCK_ORIGINAL_PATH MOCK_ORIGINAL_WAYLAND MOCK_ORIGINAL_SSH_TTY MOCK_ORIGINAL_TMUX MOCK_ORIGINAL_ZELLIJ } cleanup() { export PATH="$MOCK_ORIGINAL_PATH" if [ -n "$MOCK_ORIGINAL_WAYLAND" ]; then export WAYLAND_DISPLAY="$MOCK_ORIGINAL_WAYLAND" fi if [ -n "$MOCK_ORIGINAL_SSH_TTY" ]; then export SSH_TTY="$MOCK_ORIGINAL_SSH_TTY" + else + unset SSH_TTY + fi + if [ -n "$MOCK_ORIGINAL_TMUX" ]; then + export TMUX="$MOCK_ORIGINAL_TMUX" + else + unset TMUX + fi + if [ -n "$MOCK_ORIGINAL_ZELLIJ" ]; then + export ZELLIJ="$MOCK_ORIGINAL_ZELLIJ" + else + unset ZELLIJ fi rm -rf "$MOCK_BIN" - unset MOCK_BIN MOCK_ORIGINAL_PATH MOCK_ORIGINAL_WAYLAND MOCK_ORIGINAL_SSH_TTY + unset MOCK_BIN MOCK_ORIGINAL_PATH MOCK_ORIGINAL_WAYLAND MOCK_ORIGINAL_SSH_TTY MOCK_ORIGINAL_TMUX MOCK_ORIGINAL_ZELLIJ }As per coding guidelines, "
**/*.{sh,bash}: Always use non-interactive flags with file operations (cp -f, mv -f, rm -f, rm -rf, cp -rf) to avoid hanging on confirmation prompts".🤖 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 `@spec/clipboard_copy_spec.sh` around lines 164 - 184, The cleanup function fails to restore TMUX and ZELLIJ which setup unsets, causing env leakage; update setup to save originals (e.g., MOCK_ORIGINAL_TMUX="${TMUX:-}" and MOCK_ORIGINAL_ZELLIJ="${ZELLIJ:-}" and export them) and update cleanup to restore them if non-empty (or unset otherwise) similar to how MOCK_ORIGINAL_WAYLAND and MOCK_ORIGINAL_SSH_TTY are handled; reference the setup and cleanup functions and the TMUX and ZELLIJ variable names when making the changes.
🤖 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 `@config/k3s/activate.sh`:
- Line 21: The cp invocation using $SUDO_CMD (the line: $SUDO_CMD cp
"$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml) is missing the
non-interactive flag; update this command to use cp -f to ensure file copying is
forced/non-interactive (i.e., change to use $SUDO_CMD cp -f ...), preserving the
existing paths and variables.
- Around line 20-24: The diff check can fail with permission errors when reading
/etc/rancher/k3s/config.yaml causing unnecessary copy/restart; change the check
to run diff under sudo (use $SUDO_CMD) so it can read the target file reliably
(i.e., replace the current diff invocation with $SUDO_CMD diff -q
"$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1)
and keep the surrounding logic (the cp and systemctl restart calls) unchanged so
restarts only occur when contents actually differ.
---
Nitpick comments:
In `@spec/clipboard_copy_spec.sh`:
- Around line 164-184: The cleanup function fails to restore TMUX and ZELLIJ
which setup unsets, causing env leakage; update setup to save originals (e.g.,
MOCK_ORIGINAL_TMUX="${TMUX:-}" and MOCK_ORIGINAL_ZELLIJ="${ZELLIJ:-}" and export
them) and update cleanup to restore them if non-empty (or unset otherwise)
similar to how MOCK_ORIGINAL_WAYLAND and MOCK_ORIGINAL_SSH_TTY are handled;
reference the setup and cleanup functions and the TMUX and ZELLIJ variable names
when making the changes.
🪄 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: f7f801e2-7ab3-4dbb-8d38-329fcb6f4282
📒 Files selected for processing (5)
config/k3s/activate.shconfig/k3s/config.yamlconfig/k3s/generate-remote-kubeconfig.shhome-manager/modules/local-scripts/clipboard-copy.shspec/clipboard_copy_spec.sh
| if ! diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then | ||
| $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml | ||
| if systemctl is-active --quiet k3s; then | ||
| $SUDO_CMD systemctl restart k3s | ||
| echo "k3s restarted to apply config changes" |
There was a problem hiding this comment.
Run the change-detection diff with sudo to avoid false-positive restarts.
At Line 20, diff may fail on permission denied for /etc/rancher/k3s/config.yaml, which makes this block copy/restart even when contents are unchanged.
Proposed fix
- if ! diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then
+ if ! $SUDO_CMD diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ! diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then | |
| $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml | |
| if systemctl is-active --quiet k3s; then | |
| $SUDO_CMD systemctl restart k3s | |
| echo "k3s restarted to apply config changes" | |
| if ! $SUDO_CMD diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then | |
| $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml | |
| if systemctl is-active --quiet k3s; then | |
| $SUDO_CMD systemctl restart k3s | |
| echo "k3s restarted to apply config changes" |
🤖 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 `@config/k3s/activate.sh` around lines 20 - 24, The diff check can fail with
permission errors when reading /etc/rancher/k3s/config.yaml causing unnecessary
copy/restart; change the check to run diff under sudo (use $SUDO_CMD) so it can
read the target file reliably (i.e., replace the current diff invocation with
$SUDO_CMD diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml
>/dev/null 2>&1) and keep the surrounding logic (the cp and systemctl restart
calls) unchanged so restarts only occur when contents actually differ.
| $SUDO_CMD mkdir -p /etc/rancher/k3s | ||
| $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml | ||
| if ! diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then | ||
| $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml |
There was a problem hiding this comment.
Use non-interactive copy flag for consistency with repo shell rules.
At Line 21, switch to cp -f to keep file operations non-interactive.
As per coding guidelines, "Always use non-interactive flags with file operations (cp -f, mv -f, rm -f, rm -rf, cp -rf) to avoid hanging on confirmation prompts".
🤖 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 `@config/k3s/activate.sh` at line 21, The cp invocation using $SUDO_CMD (the
line: $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml)
is missing the non-interactive flag; update this command to use cp -f to ensure
file copying is forced/non-interactive (i.e., change to use $SUDO_CMD cp -f
...), preserving the existing paths and variables.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the k3s configuration to include Tailscale SANs, adds a script for generating remote kubeconfigs, and extends the clipboard copy utility to support Zellij. Feedback was provided regarding a permission issue in the k3s activation script that could cause unnecessary restarts and a security improvement for the kubeconfig generation script to ensure sensitive files are not created with world-readable permissions.
I am having trouble creating individual review comments. Click here to see my feedback.
config/k3s/generate-remote-kubeconfig.sh (22-27)
The kubeconfig file is created with default permissions before chmod 600 is called. Since kubeconfigs contain sensitive credentials (CA data and tokens), they should not be world-readable at any point. It is safer to set the umask or create the file with restricted permissions from the start to avoid this race condition.
(umask 077; $SUDO_CMD cat "$K3S_KUBECONFIG" | sed "s|https://127.0.0.1:6443|https://${TAILSCALE_DNS}:6443|g" | sed "s|default|kyber-remote|g" > "$OUTPUT")
config/k3s/activate.sh (20)
The diff command is executed as the current user. Since /etc/rancher/k3s/config.yaml is typically root-owned and restricted (mode 600), this check will likely fail with a 'Permission denied' error (exit code 2). This causes the script to always copy the file and restart k3s, even if the configuration hasn't changed. Using $SUDO_CMD diff ensures the comparison works correctly.
if ! $SUDO_CMD diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="config/k3s/activate.sh">
<violation number="1" location="config/k3s/activate.sh:20">
P2: The change-detection check treats `diff` errors as config changes, so comparison failures can incorrectly trigger a copy and k3s restart. Run the comparison with sudo (at minimum) so permission failures don’t look like content changes.</violation>
</file>
<file name="spec/clipboard_copy_spec.sh">
<violation number="1" location="spec/clipboard_copy_spec.sh:136">
P2: The new Zellij test is not isolated: `SSH_TTY`/`TMUX` are left untouched, so it can false-pass via another OSC 52 trigger instead of validating Zellij detection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if [ -n "$SUDO_CMD" ]; then | ||
| $SUDO_CMD mkdir -p /etc/rancher/k3s | ||
| $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml | ||
| if ! diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then |
There was a problem hiding this comment.
P2: The change-detection check treats diff errors as config changes, so comparison failures can incorrectly trigger a copy and k3s restart. Run the comparison with sudo (at minimum) so permission failures don’t look like content changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/k3s/activate.sh, line 20:
<comment>The change-detection check treats `diff` errors as config changes, so comparison failures can incorrectly trigger a copy and k3s restart. Run the comparison with sudo (at minimum) so permission failures don’t look like content changes.</comment>
<file context>
@@ -17,7 +17,13 @@ fi
if [ -n "$SUDO_CMD" ]; then
$SUDO_CMD mkdir -p /etc/rancher/k3s
- $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml
+ if ! diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then
+ $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml
+ if systemctl is-active --quiet k3s; then
</file context>
| if ! diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then | |
| if ! $SUDO_CMD diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then |
| ln -sf "$(command -v "$cmd")" "$MOCK_BIN/$cmd" | ||
| done | ||
| export PATH="$MOCK_BIN" | ||
| unset WAYLAND_DISPLAY |
There was a problem hiding this comment.
P2: The new Zellij test is not isolated: SSH_TTY/TMUX are left untouched, so it can false-pass via another OSC 52 trigger instead of validating Zellij detection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/clipboard_copy_spec.sh, line 136:
<comment>The new Zellij test is not isolated: `SSH_TTY`/`TMUX` are left untouched, so it can false-pass via another OSC 52 trigger instead of validating Zellij detection.</comment>
<file context>
@@ -120,6 +120,46 @@ The output should start with $'\033]52;c;'
+ ln -sf "$(command -v "$cmd")" "$MOCK_BIN/$cmd"
+ done
+ export PATH="$MOCK_BIN"
+ unset WAYLAND_DISPLAY
+ export ZELLIJ=0
+ export MOCK_BIN MOCK_ORIGINAL_PATH MOCK_ORIGINAL_WAYLAND MOCK_ORIGINAL_ZELLIJ
</file context>
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml | ||
| if ! diff -q "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then | ||
| $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml | ||
| if systemctl is-active --quiet k3s; then |
There was a problem hiding this comment.
Bare systemctl/diff may not resolve in the home-manager activation PATH. The sibling script home-manager/services/k3s/activate.sh is built via pkgs.replaceVars substituting @diff@/@systemctl@ (see home-manager/services/k3s/default.nix:11-14) precisely because activation PATH can't be relied on. This script is wired in config/k3s/default.nix as ${./activate.sh} with no substitution, so it ships bare commands.
With set -euo pipefail, both if ! diff … and if systemctl is-active … consume the exit code, so a missing binary returns 127 and the wrappers silently degrade: diff missing → cp always runs (mostly harmless), but systemctl missing → restart is skipped without any error. The net effect on a host where systemctl isn't on activation PATH is that the new tls-san: entries get copied to /etc/rancher/k3s/config.yaml but k3s is never restarted, so remote kubectl over Tailscale continues to fail TLS verification.
Fix by rendering this script through pkgs.replaceVars in config/k3s/default.nix and switching to @diff@/@systemctl@, matching the established pattern.
|
|
||
| $SUDO_CMD cat "$K3S_KUBECONFIG" \ | ||
| | sed "s|https://127.0.0.1:6443|https://${TAILSCALE_DNS}:6443|g" \ | ||
| | sed "s|default|kyber-remote|g" \ |
There was a problem hiding this comment.
Global sed s|default|kyber-remote|g can corrupt cert data. k3s.yaml stores certificate-authority-data, client-certificate-data, and client-key-data as single-line base64 in the alphabet [A-Za-z0-9+/=], which contains every character of default. The substring can (rarely) appear inside the encoded blob, in which case this rewrite silently mangles the cert and kubectl fails later with a tls: failed to find any PEM data style error rather than a clean diagnostic.
Probability is low per cert, but the failure mode is hard to debug. Prefer targeted YAML-aware rename, e.g.:
sed -E 's/(name|user|cluster|current-context):[[:space:]]*default$/\1: kyber-remote/'or pipe through yq to mutate only the cluster/user/context name fields. That keeps certificate-authority-data byte-identical no matter what the random base64 ends up containing.
| # Generate a kubeconfig for remote access via Tailscale | ||
| set -euo pipefail | ||
|
|
||
| TAILSCALE_IP="100.72.158.65" |
There was a problem hiding this comment.
TAILSCALE_IP is declared but never used — only TAILSCALE_DNS is referenced in the sed substitution below. Either drop this line or wire it in (e.g., write a second kubeconfig variant that uses https://${TAILSCALE_IP}:6443 so DNS-less clients still work).
| done | ||
| export PATH="$MOCK_BIN" | ||
| unset WAYLAND_DISPLAY | ||
| export ZELLIJ=0 |
There was a problem hiding this comment.
This setup sets ZELLIJ=0 and unsets WAYLAND_DISPLAY, but doesn't unset TMUX or SSH_TTY. If the suite runs inside a tmux pane or over SSH, the OSC 52 fallback would fire from those variables and the test would pass even if the new ZELLIJ branch in clipboard-copy.sh were broken.
The "no backend" cleanup already does unset WAYLAND_DISPLAY SSH_TTY TMUX ZELLIJ (line 171) for exactly this reason — worth doing the same here:
MOCK_ORIGINAL_TMUX="${TMUX:-}"
MOCK_ORIGINAL_SSH_TTY="${SSH_TTY:-}"
…
unset WAYLAND_DISPLAY TMUX SSH_TTY
export ZELLIJ=0and restore both in cleanup(). The pre-existing SSH_TTY block (lines 83–121) has the same gap and would benefit from the same treatment.
1 participant
Summary
ZELLIJenv var detection to clipboard-copy OSC 52 fallbackZELLIJin the "no backend" test to prevent false passesTest plan
pwd | copyworks inside a Zellij sessionSummary by cubic
Add Zellij support to the OSC 52 clipboard fallback so copy works inside Zellij sessions when no native clipboard backend is available.
ZELLIJinclipboard-copy.shto enable OSC 52 fallback.ZELLIJin the "no backend" test to avoid false passes.Written for commit 2bc99d5. Summary will update on new commits.