fix(agent-hooks): quote the hook binary for the shell that re-reads it

Every agent takes the hook as a command *line*, so a shell parses it again
before anything runs. The path went in double-quoted, and inside double
quotes `sh` still expands `$`, a backtick and a backslash — so an install
under `/opt/build$stage` reached the shell as `/opt/build/tty7`. The hook
then never fired: no error, no log line, just an agent that quietly stopped
reporting its status for good.

Checked against a real `sh` first, because the failure is invisible from the
code: the double-quoted form resolves the program to `.../q/tool`, the
single-quoted one to `.../q$stage/tool`. Then checked again through the
generated OpenCode plugin under node with a stubbed `$`, which is the path
that actually ships — the assembled command re-parses to the full path,
dollar segment intact.

Which shell is the target's, not ours: a hook installed on a remote machine
runs there. Only a local Windows target keeps the double-quoted form, which
is what `cmd.exe` wants and what the PATH-resolvable case avoids needing.

`shell_quote` had been written out twice already, in `daemon::install` and
`daemon::shell_integration`. Rather than add a third copy this moves the one
implementation to `core::shells`, where a core module may reach it without
depending on the daemon, and both old homes now re-export it.
This commit is contained in:
l0ng-ai
2026-08-23 10:18:28 +08:00
parent 17942c0262
commit 1ccdaeaa2d
4 changed files with 87 additions and 14 deletions
+59 -7
View File
@@ -492,12 +492,35 @@ impl<'a> HookTarget<'a> {
return format!("{exe} agent-hook {} {event}", agent.slug());
}
format!(
"\"{}\" agent-hook {} {event}",
self.exe.display(),
"{} agent-hook {} {event}",
self.quoted_exe(),
agent.slug()
)
}
/// The executable path, quoted for whichever shell is going to re-read it.
///
/// Every agent takes this as a *command line*, not an argv, so something
/// parses it again before anything runs. Double quotes were not enough for
/// a POSIX shell: inside them `sh` still expands `$`, a backtick and a
/// backslash, so an install under `/opt/build$stage` was handed to `sh -c`
/// as `/opt/build/tty7` and every hook silently stopped firing — no error
/// anywhere, just an agent that never reports its status again.
///
/// The shell that re-reads it belongs to the *target*, not to us: a hook
/// installed on a remote machine runs there. So the question is asked of
/// the target, and only a local Windows one keeps the double-quoted form,
/// which is what `cmd.exe` understands and what the PATH-resolvable case
/// above avoids needing at all.
fn quoted_exe(&self) -> String {
let path = self.exe.display().to_string();
if cfg!(windows) && self.is_local() {
format!("\"{path}\"")
} else {
crate::core::shells::shell_quote(&path)
}
}
/// A shell-safe executable path for generated hook commands.
///
/// On Windows, Codex runs hook commands through the session's shell, which
@@ -2006,7 +2029,7 @@ mod tests {
// name: PowerShell cannot invoke a quoted path without the `&` call
// operator, so quoting is avoided whenever possible.
#[cfg(not(windows))]
assert!(cmd.starts_with('"'));
assert!(cmd.starts_with('\''), "{cmd}");
assert!(cmd.ends_with("agent-hook claude stop"));
}
@@ -2044,6 +2067,29 @@ mod tests {
}
}
/// An install path a shell would rewrite still reaches the shell intact.
///
/// This is the whole reason the quoting changed. `sh` expands `$`, a
/// backtick and a backslash *inside double quotes*, so the old form handed
/// `/opt/build$stage/tty7` over as `/opt/build/tty7`: the hook never ran,
/// nothing logged a thing, and the agent simply stopped reporting status.
/// Verified against a real `sh` before the change — the double-quoted form
/// really does lose the segment.
#[test]
fn a_path_the_shell_would_rewrite_survives_the_hook_command() {
let host = FakeRemote::shared();
let target = HookTarget::remote(&*host, PathBuf::from("/opt/build$stage"));
let cmd = target.hook_command(HookAgent::Claude, "stop");
assert!(
cmd.contains("/opt/build$stage/"),
"the dollar segment is still there: {cmd}"
);
assert!(
cmd.starts_with('\'') && cmd.contains("' agent-hook"),
"and it is single-quoted, so the shell will not read it: {cmd}"
);
}
#[test]
fn the_hook_command_names_the_binary_on_that_machine() {
let host = FakeRemote::shared();
@@ -2052,19 +2098,25 @@ mod tests {
let name = format!("tty7-server-c{}p{}", dialect.control, dialect.protocol);
assert_eq!(
target.hook_command(HookAgent::Claude, "stop"),
format!("\"/home/me/.local/share/tty7/bin/{name}\" agent-hook claude stop")
format!("'/home/me/.local/share/tty7/bin/{name}' agent-hook claude stop")
);
let local = local_host();
let here = HookTarget::local(&*local).expect("home resolves in tests");
let exe = std::env::current_exe().unwrap();
let command_exe = here
.hook_command_exe()
.unwrap_or_else(|| format!("\"{}\"", exe.display()));
// Named after the binary, however that machine's shell needs it
// spelled — the spelling itself is what the two quoting tests above
// are for, and repeating the rule here would only assert it twice.
let command_exe = here.hook_command_exe().unwrap_or_else(|| here.quoted_exe());
assert_eq!(
here.hook_command(HookAgent::Claude, "stop"),
format!("{command_exe} agent-hook claude stop")
);
assert!(
command_exe.contains(&exe.display().to_string())
|| here.hook_command_exe().is_some(),
"the full path is in there unless it resolved by name"
);
}
#[test]
+26
View File
@@ -1473,3 +1473,29 @@ mod tests {
assert_eq!(before, after);
}
}
/// POSIX single-quoting, for a string some other shell has to re-parse.
///
/// Single quotes rather than double: inside double quotes `sh` still expands
/// `$`, a backtick and a backslash, so `"/opt/build$stage/tty7"` is handed to
/// the shell as `/opt/build/tty7` and the command silently runs the wrong
/// thing — or nothing. Only a single quote itself has to be escaped, and the
/// `'\''` dance is the one portable way to do it.
pub(crate) fn shell_quote(s: &str) -> String {
format!("'{}'", s.replace('\'', r"'\''"))
}
#[cfg(test)]
mod shell_quote_tests {
use super::shell_quote;
#[test]
fn quoting_survives_what_a_shell_would_otherwise_read() {
assert_eq!(shell_quote("/home/me/bin"), "'/home/me/bin'");
assert_eq!(shell_quote("/home/my box/x"), "'/home/my box/x'");
// The one this exists for: a path a shell would rewrite.
assert_eq!(shell_quote("/opt/build$stage/x"), "'/opt/build$stage/x'");
assert_eq!(shell_quote("/opt/`whoami`/x"), "'/opt/`whoami`/x'");
assert_eq!(shell_quote("/home/o'brien/x"), r"'/home/o'\''brien/x'");
}
}
+1 -3
View File
@@ -1150,9 +1150,7 @@ fn unique_temp(shared: &str) -> String {
}
}
pub(crate) fn shell_quote(s: &str) -> String {
format!("'{}'", s.replace('\'', r"'\''"))
}
pub(crate) use crate::core::shells::shell_quote;
fn connection_label(conn: &SshConnection) -> String {
conn.key().as_str().to_string()
@@ -1157,10 +1157,7 @@ const WSL_RCFILE_ENV: &str = "TTY7_RC";
/// whether this becomes `ZDOTDIR`, and it only decides that for a zsh distro.
const WSL_ZDOTDIR_ENV: &str = "TTY7_ZDOTDIR";
/// POSIX single-quoting, for a body some other shell has to re-parse.
fn shell_quote(s: &str) -> String {
format!("'{}'", s.replace('\'', r"'\''"))
}
use crate::core::shells::shell_quote;
/// The bootstrap `sh` runs inside the distro. `$SHELL` is the only place the
/// user's real shell is named, so every arm dispatches on it: bash re-execs