Conversation
|
This change is part of the following stack: Change managed by git-spice. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #719 +/- ##
==========================================
- Coverage 55.66% 55.65% -0.02%
==========================================
Files 126 126
Lines 13759 13789 +30
==========================================
+ Hits 7659 7674 +15
- Misses 6100 6115 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
geoffjay
left a comment
There was a problem hiding this comment.
Review: fix(wrap): handle long commands in tmux send_command via temp-script fallback
Stack position: issue-714 is directly on main but git-spice reports
(needs restack) — the branch is behind main and must be rebased before
the conductor can merge it.
The fix is correct, well-reasoned, and matches the approach recommended in
the triage notes. Three non-blocking observations below.
Code quality: approved
TMUX_SEND_KEYS_MAX_LEN = 8 KB — the empirical range (works up to ~12 KB,
fails at ~18 KB) is documented in the comment. 8 KB is a conservative choice
with room for the conductor's ~6 KB prompt. ✅
Debug log truncation — &command[..command.len().min(120)] prevents
multi-kilobyte log lines for long commands. ✅
send_command_via_script — correctly isolates the fallback path. Best-effort
remove_file on send-keys failure is appropriate (the script would otherwise
remain until the next trap EXIT). ✅
write_temp_script — trap 'rm -f "$0"' EXIT is the standard self-deleting
script pattern. The #[cfg(unix)] chmod 0o755 block is a good cross-platform
guard for the permission step. ✅
Test coverage — 4 meaningful tests for write_temp_script: file creation,
shebang + trap content, command embedding, path uniqueness. These cover the
new code paths well. ✅
Non-blocking observations
1. Script path is not quoted in send_cmd
let send_cmd = format!("sh {script_str}");If temp_dir() returns a path containing a space (e.g. a macOS user home
under a name with spaces), the send-keys invocation will split the path and
sh will error. Quoting the path is straightforward:
let send_cmd = format!("sh '{script_str}'");Single-quoting is safe here because temp_dir() paths don't contain single
quotes on any supported platform. In practice /tmp and
/var/folders/.../T/ have no spaces, so this won't cause failures today —
but quoting is the correct defensive practice.
2. uuid is already a dependency — stronger uniqueness available
The temp filename uses nanosecond timestamp + PID:
let name = format!(
"agentd-cmd-{}-{}.sh",
SystemTime::now().duration_since(UNIX_EPOCH)…as_nanos(),
std::process::id(),
);uuid is already in wrap/Cargo.toml. Two rapid calls in the same process
could theoretically collide if the system clock resolution is coarser than
nanoseconds (some environments round to microseconds). uuid::Uuid::new_v4()
gives a 122-bit random filename with negligible collision probability and no
clock dependency:
let name = format!("agentd-cmd-{}.sh", uuid::Uuid::new_v4());The current approach is fine in practice — this is a minor robustness note.
3. chmod 0o755 is unused
The script is invoked as sh {path}, not as ./{path}, so the executable
bit is never consulted by the kernel. The #[cfg(unix)] block can be removed
without any functional change. Leaving it in is harmless but adds a syscall
that does nothing.
4. Tests 6 and 7 test arithmetic, not behavior
test_short_command_does_not_need_script and test_long_command_needs_script
assert that "echo hello".len() <= 8192 and "x".repeat(8193).len() > 8192
— these are arithmetic identities, not behavioral coverage. They'd be better
rewritten to call send_command against a mock or at least verify that the
write_temp_script path is triggered. As unit tests that will never fail, they
don't add regression value. Non-blocking — the four write_temp_script tests
provide the meaningful coverage.
Action required before merge
Rebase onto main:
git-spice branch restack
git-spice branch submitOnce restacked, add merge-queue. The non-blocking observations above can
be addressed in the same commit or deferred to a follow-up.
fix: handle long commands in tmux send_command via temp script fallback
fix(wrap): handle long commands in tmux send_command via temp-script fallback (closes #714)