Skip to content

Commit df8e035

Browse files
authored
Merge pull request #1346 from rtk-ai/feat-hook-engine
fix: P0+P1 fixes from pre-merge review of hook engine
2 parents e7b7f9a + d8ddefe commit df8e035

9 files changed

Lines changed: 507 additions & 14 deletions

File tree

‎Cargo.toml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ flate2 = "1.0"
3333
quick-xml = "0.37"
3434
which = "8"
3535
automod = "1"
36-
libc = "0.2"
3736

3837
[target.'cfg(unix)'.dependencies]
3938
libc = "0.2"
Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
from __future__ import annotations
2+
3+
import asyncio
4+
import subprocess
5+
import tempfile
6+
from pathlib import Path
7+
8+
from .config import TaskConfig
9+
from .manifest import (
10+
RunManifest,
11+
SessionEntry,
12+
TbEntry,
13+
TbTaskEntry,
14+
write_manifest,
15+
)
16+
from .session import run_all_sessions, setup_codebase, setup_rtk
17+
from .terminal_bench import run_terminal_bench
18+
from .vm import create_vm_pool, destroy_vm_pool
19+
20+
ROOT_DIR = Path(__file__).resolve().parent.parent
21+
22+
23+
def _create_tarball(source_dir: Path) -> str:
24+
tarball = tempfile.mktemp(suffix=".tar.gz")
25+
subprocess.run(
26+
["tar", "czf", tarball, "-C", str(source_dir), "."],
27+
check=True,
28+
)
29+
return tarball
30+
31+
32+
def _print_step(step: int, total: int, msg: str):
33+
print(f"\n[{step}/{total}] {msg}")
34+
35+
36+
def _session_to_entry(r) -> SessionEntry:
37+
return SessionEntry(
38+
vm_name=r.vm_name,
39+
group=r.group,
40+
stdout_json=f"{r.vm_name}-stdout.json",
41+
otel_log=f"{r.vm_name}-otel.log",
42+
rtk_db=f"{r.vm_name}-tracking.db" if r.rtk_db_path else None,
43+
exit_code=r.exit_code,
44+
error=r.error or None,
45+
)
46+
47+
48+
def _tb_to_entry(r) -> TbEntry:
49+
return TbEntry(
50+
vm_name=r.vm_name,
51+
group=r.group,
52+
total=r.total,
53+
passed=r.passed,
54+
failed=r.failed,
55+
tasks=[TbTaskEntry(name=t.name, passed=t.passed, duration_s=t.duration_s) for t in r.tasks],
56+
error=r.error,
57+
)
58+
59+
60+
async def run_benchmark(
61+
task: TaskConfig,
62+
vms: int,
63+
api_key: str,
64+
output_dir: Path,
65+
cloud_init: Path | None = None,
66+
terminal_bench: bool = False,
67+
keep_vms: bool = False,
68+
) -> RunManifest:
69+
if cloud_init is None:
70+
cloud_init = ROOT_DIR / "cloud-init-base.yaml"
71+
72+
output_dir.mkdir(parents=True, exist_ok=True)
73+
74+
total_steps = 5 if terminal_bench else 4
75+
vm_names: list[str] = []
76+
77+
manifest = RunManifest(
78+
task_name=task.name,
79+
model=task.model,
80+
vm_count=vms,
81+
)
82+
83+
try:
84+
_print_step(1, total_steps, f"Creating {vms * 2} VMs ({vms} RTK ON + {vms} RTK OFF)")
85+
vm_names = await create_vm_pool(vms, cloud_init)
86+
print(f" VMs ready: {', '.join(vm_names)}")
87+
88+
_print_step(2, total_steps, "Setting up codebases")
89+
local_tarball = None
90+
if not task.codebase.is_github:
91+
local_tarball = _create_tarball(task.codebase.local_path())
92+
93+
await asyncio.gather(*(
94+
setup_codebase(name, task.codebase, local_tarball)
95+
for name in vm_names
96+
))
97+
print(" Codebases deployed")
98+
99+
_print_step(3, total_steps, "Configuring RTK on ON VMs")
100+
setup_script = ROOT_DIR / "setup-rtk.sh"
101+
on_vms = [n for n in vm_names if "-on-" in n]
102+
off_vms = [n for n in vm_names if "-off-" in n]
103+
await asyncio.gather(*(setup_rtk(vm, setup_script) for vm in on_vms))
104+
print(f" RTK configured on {len(on_vms)} VMs")
105+
106+
_print_step(4, total_steps, f"Running Claude sessions (timeout: {task.timeout_minutes}min)")
107+
results = await run_all_sessions(vm_names, task, api_key, output_dir)
108+
109+
on_ok = [r for r in results if r.group == "on" and not r.error]
110+
off_ok = [r for r in results if r.group == "off" and not r.error]
111+
errors = [r for r in results if r.error]
112+
print(f" Completed: {len(on_ok)} ON, {len(off_ok)} OFF, {len(errors)} errors")
113+
for r in errors:
114+
print(f" {r.vm_name}: {r.error}")
115+
116+
manifest.sessions = [_session_to_entry(r) for r in results]
117+
118+
if terminal_bench:
119+
_print_step(5, total_steps, "Running terminal-bench precision tests")
120+
tb_on = await asyncio.gather(*(
121+
run_terminal_bench(vm, "on", task.model, api_key)
122+
for vm in on_vms
123+
))
124+
tb_off = await asyncio.gather(*(
125+
run_terminal_bench(vm, "off", task.model, api_key)
126+
for vm in off_vms
127+
))
128+
129+
manifest.terminal_bench = [_tb_to_entry(r) for r in list(tb_on) + list(tb_off)]
130+
131+
ok_on = [r for r in tb_on if not r.error]
132+
ok_off = [r for r in tb_off if not r.error]
133+
if ok_on and ok_off:
134+
on_total = sum(r.total for r in ok_on)
135+
on_passed = sum(r.passed for r in ok_on)
136+
off_total = sum(r.total for r in ok_off)
137+
off_passed = sum(r.passed for r in ok_off)
138+
on_rate = on_passed / on_total if on_total else 0
139+
off_rate = off_passed / off_total if off_total else 0
140+
print(f" terminal-bench: ON pass rate={on_rate:.0%}, OFF pass rate={off_rate:.0%}, delta={on_rate - off_rate:+.0%}")
141+
142+
tb_errors = [r for r in list(tb_on) + list(tb_off) if r.error]
143+
for r in tb_errors:
144+
print(f" {r.vm_name}: {r.error}")
145+
146+
write_manifest(manifest, output_dir)
147+
print(f"\n Manifest written to {output_dir / 'manifest.json'}")
148+
149+
finally:
150+
if not keep_vms and vm_names:
151+
print("\nCleaning up VMs...")
152+
await destroy_vm_pool(vm_names)
153+
print(" VMs destroyed")
154+
155+
return manifest

‎src/cmds/rust/runner.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,12 @@ fn extract_test_summary(output: &str, command: &str) -> String {
242242
if failures.len() > 10 {
243243
output.push_str(&format!(" ... +{} more failures\n", failures.len() - 10));
244244
}
245+
for f in failure_lines.iter().take(20) {
246+
output.push_str(&format!(" {}\n", f.trim()));
247+
}
248+
if failure_lines.len() > 20 {
249+
output.push_str(&format!(" ... +{} more\n", failure_lines.len() - 20));
250+
}
245251
output.push('\n');
246252
}
247253

‎src/cmds/system/pipe_cmd.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -495,7 +495,7 @@ mod tests {
495495
let output = grep_wrapper(&input);
496496
let savings = 100.0 - (count_tokens(&output) as f64 / count_tokens(&input) as f64 * 100.0);
497497
assert!(
498-
savings >= 40.0,
498+
savings >= 40.0, // TODO: grep pipe filter below 60% target — improve grouping
499499
"grep filter: expected ≥40% savings, got {:.1}% (in={}, out={})",
500500
savings, count_tokens(&input), count_tokens(&output)
501501
);
@@ -516,7 +516,7 @@ mod tests {
516516
let output = find_wrapper(&input);
517517
let savings = 100.0 - (count_tokens(&output) as f64 / count_tokens(&input) as f64 * 100.0);
518518
assert!(
519-
savings >= 40.0,
519+
savings >= 40.0, // TODO: find pipe filter below 60% target — improve grouping
520520
"find filter: expected ≥40% savings, got {:.1}% (in={}, out={})",
521521
savings, count_tokens(&input), count_tokens(&output)
522522
);

‎src/core/runner.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,12 @@ pub fn run(
7979
let raw_stdout = &result.raw_stdout;
8080

8181
if opts.skip_filter_on_failure && exit_code != 0 {
82+
if !result.raw_stdout.trim().is_empty() {
83+
print!("{}", result.raw_stdout);
84+
}
85+
if !result.raw_stderr.trim().is_empty() {
86+
eprint!("{}", result.raw_stderr);
87+
}
8288
timer.track(&cmd_label, &format!("rtk {}", cmd_label), raw, raw);
8389
return Ok(exit_code);
8490
}

‎src/core/stream.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,7 @@ pub struct StreamResult {
191191
pub exit_code: i32,
192192
pub raw: String,
193193
pub raw_stdout: String,
194+
pub raw_stderr: String,
194195
pub filtered: String,
195196
}
196197

@@ -239,6 +240,7 @@ pub fn run_streaming(
239240
exit_code: status_to_exit_code(status),
240241
raw: String::new(),
241242
raw_stdout: String::new(),
243+
raw_stderr: String::new(),
242244
filtered: String::new(),
243245
});
244246
}
@@ -432,6 +434,7 @@ pub fn run_streaming(
432434
exit_code,
433435
raw,
434436
raw_stdout,
437+
raw_stderr,
435438
filtered,
436439
})
437440
}
@@ -519,6 +522,7 @@ pub(crate) mod tests {
519522
exit_code: 0,
520523
raw: String::new(),
521524
raw_stdout: String::new(),
525+
raw_stderr: String::new(),
522526
filtered: String::new(),
523527
};
524528
assert!(r.success());
@@ -530,6 +534,7 @@ pub(crate) mod tests {
530534
exit_code: 1,
531535
raw: String::new(),
532536
raw_stdout: String::new(),
537+
raw_stderr: String::new(),
533538
filtered: String::new(),
534539
};
535540
assert!(!r.success());
@@ -541,6 +546,7 @@ pub(crate) mod tests {
541546
exit_code: 137,
542547
raw: String::new(),
543548
raw_stdout: String::new(),
549+
raw_stderr: String::new(),
544550
filtered: String::new(),
545551
};
546552
assert!(!r.success());

‎src/hooks/hook_cmd.rs‎

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ fn get_rewritten(cmd: &str) -> Option<String> {
123123
fn handle_vscode(cmd: &str) -> Result<()> {
124124
let verdict = permissions::check_command(cmd);
125125
if verdict == PermissionVerdict::Deny {
126+
audit_log("deny", cmd, "");
126127
return Ok(());
127128
}
128129

@@ -138,6 +139,8 @@ fn handle_vscode(cmd: &str) -> Result<()> {
138139
_ => "ask",
139140
};
140141

142+
audit_log("rewrite", cmd, &rewritten);
143+
141144
let output = json!({
142145
"hookSpecificOutput": {
143146
"hookEventName": PRE_TOOL_USE_KEY,
@@ -152,6 +155,7 @@ fn handle_vscode(cmd: &str) -> Result<()> {
152155

153156
fn handle_copilot_cli(cmd: &str) -> Result<()> {
154157
if permissions::check_command(cmd) == PermissionVerdict::Deny {
158+
audit_log("deny", cmd, "");
155159
return Ok(());
156160
}
157161

@@ -160,6 +164,8 @@ fn handle_copilot_cli(cmd: &str) -> Result<()> {
160164
None => return Ok(()),
161165
};
162166

167+
audit_log("rewrite", cmd, &rewritten);
168+
163169
let output = json!({
164170
"permissionDecision": "deny",
165171
"permissionDecisionReason": format!(
@@ -210,7 +216,10 @@ pub fn run_gemini() -> Result<()> {
210216
.unwrap_or_default();
211217

212218
match rewrite_command(cmd, &excluded) {
213-
Some(rewritten) => print_rewrite(&rewritten),
219+
Some(ref rewritten) => {
220+
audit_log("rewrite", cmd, rewritten);
221+
print_rewrite(rewritten);
222+
}
214223
None => print_allow(),
215224
}
216225

@@ -420,7 +429,9 @@ pub fn run_cursor() -> Result<()> {
420429
}
421430
};
422431

423-
if permissions::check_command(&cmd) == PermissionVerdict::Deny {
432+
let verdict = permissions::check_command(&cmd);
433+
if verdict == PermissionVerdict::Deny {
434+
audit_log("deny", &cmd, "");
424435
let _ = writeln!(io::stdout(), "{{}}");
425436
return Ok(());
426437
}
@@ -433,17 +444,33 @@ pub fn run_cursor() -> Result<()> {
433444
}
434445
};
435446

447+
let decision = match verdict {
448+
PermissionVerdict::Allow => "allow",
449+
_ => "ask",
450+
};
451+
452+
audit_log("rewrite", &cmd, &rewritten);
453+
436454
let output = json!({
437-
"permission": "allow",
455+
"permission": decision,
438456
"updated_input": { "command": rewritten }
439457
});
440458
let _ = writeln!(io::stdout(), "{output}");
441459
Ok(())
442460
}
443461

444-
/// Process a Cursor hook payload from a string (for testing).
445462
#[cfg(test)]
446463
fn run_cursor_inner(input: &str) -> String {
464+
run_cursor_inner_with_rules(input, &[], &[], &[])
465+
}
466+
467+
#[cfg(test)]
468+
fn run_cursor_inner_with_rules(
469+
input: &str,
470+
deny_rules: &[String],
471+
ask_rules: &[String],
472+
allow_rules: &[String],
473+
) -> String {
447474
let v: Value = match serde_json::from_str(input) {
448475
Ok(v) => v,
449476
Err(_) => return "{}".to_string(),
@@ -458,14 +485,19 @@ fn run_cursor_inner(input: &str) -> String {
458485
None => return "{}".to_string(),
459486
};
460487

461-
if permissions::check_command(&cmd) == PermissionVerdict::Deny {
488+
let verdict = permissions::check_command_with_rules(&cmd, deny_rules, ask_rules, allow_rules);
489+
if verdict == PermissionVerdict::Deny {
462490
return "{}".to_string();
463491
}
464492

465493
match get_rewritten(&cmd) {
466494
Some(rewritten) => {
495+
let decision = match verdict {
496+
PermissionVerdict::Allow => "allow",
497+
_ => "ask",
498+
};
467499
let output = json!({
468-
"permission": "allow",
500+
"permission": decision,
469501
"updated_input": { "command": rewritten }
470502
});
471503
output.to_string()
@@ -738,7 +770,8 @@ mod tests {
738770
fn test_cursor_rewrite_flat_format() {
739771
let result = run_cursor_inner(&cursor_input("git status"));
740772
let v: Value = serde_json::from_str(&result).unwrap();
741-
assert_eq!(v["permission"], "allow");
773+
// Default permission (no explicit allow rule) → "ask"
774+
assert_eq!(v["permission"], "ask");
742775
assert_eq!(v["updated_input"]["command"], "rtk git status");
743776
assert!(v.get("hookSpecificOutput").is_none());
744777
}
@@ -772,7 +805,7 @@ mod tests {
772805
let result = run_cursor_inner(&cursor_input("cargo test"));
773806
let v: Value = serde_json::from_str(&result).unwrap();
774807
assert!(v.get("hookSpecificOutput").is_none());
775-
assert_eq!(v["permission"], "allow");
808+
assert_eq!(v["permission"], "ask");
776809
}
777810

778811
// --- Audit logging ---

0 commit comments

Comments
 (0)