From 31e20ae2613dac2c5df41df9df604516ec1f2f2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Thu, 21 May 2026 17:45:54 +0200 Subject: [PATCH] Recorder: pass directory to init and reuse arena Update `Recorder.init` to accept a `std.fs.Dir` and relative path, decoupling it from the current working directory. Also add a persistent arena allocator to `Recorder` to reuse allocations when scrubbing environment variables on each write. --- src/agent/Agent.zig | 2 +- src/mcp/tools.zig | 2 +- src/script/Recorder.zig | 91 ++++++++++++++--------------------------- 3 files changed, 32 insertions(+), 63 deletions(-) diff --git a/src/agent/Agent.zig b/src/agent/Agent.zig index 38d08b0c..2d717176 100644 --- a/src/agent/Agent.zig +++ b/src/agent/Agent.zig @@ -275,7 +275,7 @@ pub fn init(allocator: std.mem.Allocator, app: *App, opts: Config.Agent) !*Agent if (will_repl) self.terminal.attachCompleter(slash_schemas); if (recorder_path) |p| { - if (Recorder.init(allocator, p)) |r| { + if (Recorder.init(allocator, std.fs.cwd(), p)) |r| { self.recorder = r; self.terminal.printInfoFmt("recording to {s}", .{r.path}); } else |err| { diff --git a/src/mcp/tools.zig b/src/mcp/tools.zig index 1d03e44f..89c48f07 100644 --- a/src/mcp/tools.zig +++ b/src/mcp/tools.zig @@ -202,7 +202,7 @@ fn handleRecordStart(server: *Server, arena: std.mem.Allocator, id: std.json.Val return sendErrorContent(server, id, "path must be relative and must not contain '..' segments"); } - var recorder = Recorder.init(server.allocator, args.path) catch |err| { + var recorder = Recorder.init(server.allocator, std.fs.cwd(), args.path) catch |err| { const msg = std.fmt.allocPrint(arena, "could not open recording file: {s}", .{@errorName(err)}) catch return sendErrorContent(server, id, "could not open recording file"); return sendErrorContent(server, id, msg); diff --git a/src/script/Recorder.zig b/src/script/Recorder.zig index 4148d527..a0df0d21 100644 --- a/src/script/Recorder.zig +++ b/src/script/Recorder.zig @@ -36,23 +36,28 @@ path: []const u8, lines: u32, /// Reused between writes so each line doesn't alloc/free. buf: std.Io.Writer.Allocating, +/// Reset per write — backs short-lived scrub allocations so the first +/// recorded command pays the page setup and the rest reuse the bump. +arena: std.heap.ArenaAllocator, -/// Append-open `path`, inserting a leading newline if the file is non-empty. -pub fn init(allocator: std.mem.Allocator, path: []const u8) !Recorder { - const owned_path = try allocator.dupe(u8, path); +/// Append-open `sub_path` under `dir`, inserting a leading newline if the +/// file is non-empty. +pub fn init(allocator: std.mem.Allocator, dir: std.fs.Dir, sub_path: []const u8) !Recorder { + const owned_path = try allocator.dupe(u8, sub_path); errdefer allocator.free(owned_path); - const file = try openForAppend(path); + const file = try openForAppend(dir, sub_path); return .{ .allocator = allocator, .file = file, .path = owned_path, .lines = 0, .buf = .init(allocator), + .arena = .init(allocator), }; } -fn openForAppend(path: []const u8) !std.fs.File { - const f = try std.fs.cwd().createFile(path, .{ .truncate = false }); +fn openForAppend(dir: std.fs.Dir, sub_path: []const u8) !std.fs.File { + const f = try dir.createFile(sub_path, .{ .truncate = false }); errdefer f.close(); try f.seekFromEnd(0); const pos = try f.getPos(); @@ -62,6 +67,7 @@ fn openForAppend(path: []const u8) !std.fs.File { pub fn deinit(self: *Recorder) void { self.buf.deinit(); + self.arena.deinit(); if (self.file) |f| f.close(); self.allocator.free(self.path); } @@ -84,9 +90,8 @@ fn tryRecord(self: *Recorder, cmd: Command) !void { // Reverse-substitute any LP_* env-var values that snuck in as literals // (e.g. an agent that retyped a username it saw via getUrl) so the // recording stays portable instead of leaking the resolved secret. - var scrub_arena: std.heap.ArenaAllocator = .init(self.allocator); - defer scrub_arena.deinit(); - const scrubbed = lp.tools.reverseSubstituteEnvVars(scrub_arena.allocator(), self.buf.written()) catch self.buf.written(); + _ = self.arena.reset(.retain_capacity); + const scrubbed = lp.tools.reverseSubstituteEnvVars(self.arena.allocator(), self.buf.written()) catch self.buf.written(); try self.file.?.writeAll(scrubbed); self.lines += 1; @@ -128,15 +133,7 @@ test "record writes state-mutating commands" { var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); - const file = tmp.dir.createFile("test.lp", .{ .read = true }) catch unreachable; - - var recorder: Recorder = .{ - .allocator = std.testing.allocator, - .file = file, - .path = try std.testing.allocator.dupe(u8, "test.lp"), - .lines = 0, - .buf = .init(std.testing.allocator), - }; + var recorder = try Recorder.init(std.testing.allocator, tmp.dir, "test.lp"); defer recorder.deinit(); recorder.record(Command.parse("GOTO https://example.com")); @@ -152,8 +149,8 @@ test "record writes state-mutating commands" { recorder.record(Command.parse("EXTRACT '{\"title\":\".title\"}'")); recorder.recordComment("LOGIN"); - // Read back and verify - file.seekTo(0) catch unreachable; + const file = tmp.dir.openFile("test.lp", .{}) catch unreachable; + defer file.close(); var buf: [512]u8 = undefined; const n = file.readAll(&buf) catch unreachable; const content = buf[0..n]; @@ -176,15 +173,7 @@ test "record skips empty and comment lines" { var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); - const file = tmp.dir.createFile("test2.lp", .{ .read = true }) catch unreachable; - - var recorder: Recorder = .{ - .allocator = std.testing.allocator, - .file = file, - .path = try std.testing.allocator.dupe(u8, "test.lp"), - .lines = 0, - .buf = .init(std.testing.allocator), - }; + var recorder = try Recorder.init(std.testing.allocator, tmp.dir, "test2.lp"); defer recorder.deinit(); recorder.record(Command.parse("")); @@ -192,7 +181,8 @@ test "record skips empty and comment lines" { recorder.record(Command.parse("# this is a comment")); recorder.record(Command.parse("GOTO https://example.com")); - file.seekTo(0) catch unreachable; + const file = tmp.dir.openFile("test2.lp", .{}) catch unreachable; + defer file.close(); var buf: [256]u8 = undefined; const n = file.readAll(&buf) catch unreachable; const content = buf[0..n]; @@ -204,15 +194,7 @@ test "lines counter tracks successful appends" { var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); - const file = tmp.dir.createFile("count.lp", .{ .read = true }) catch unreachable; - - var recorder: Recorder = .{ - .allocator = std.testing.allocator, - .file = file, - .path = try std.testing.allocator.dupe(u8, "test.lp"), - .lines = 0, - .buf = .init(std.testing.allocator), - }; + var recorder = try Recorder.init(std.testing.allocator, tmp.dir, "count.lp"); defer recorder.deinit(); recorder.record(Command.parse("GOTO https://example.com")); // +1 @@ -234,18 +216,13 @@ test "init appends to an existing file without truncating" { _ = seed.writeAll("GOTO https://example.com\n") catch unreachable; } - // Resolve absolute path for Recorder.init (which uses std.fs.cwd()). - var path_buf: [std.fs.max_path_bytes]u8 = undefined; - const abs_path = tmp.dir.realpath("script.lp", &path_buf) catch unreachable; - - var recorder: Recorder = try .init(std.testing.allocator, abs_path); + var recorder = try Recorder.init(std.testing.allocator, tmp.dir, "script.lp"); defer recorder.deinit(); recorder.record(Command.parse("CLICK 'Login'")); try std.testing.expect(recorder.isActive()); - try std.testing.expectEqualStrings(abs_path, recorder.path); + try std.testing.expectEqualStrings("script.lp", recorder.path); - // Read back. const file = tmp.dir.openFile("script.lp", .{}) catch unreachable; defer file.close(); var buf: [256]u8 = undefined; @@ -264,21 +241,15 @@ test "recordComment splits embedded newlines into separate comment lines" { var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); - const file = tmp.dir.createFile("multi.lp", .{ .read = true }) catch unreachable; - var recorder: Recorder = .{ - .allocator = std.testing.allocator, - .file = file, - .path = try std.testing.allocator.dupe(u8, "test.lp"), - .lines = 0, - .buf = .init(std.testing.allocator), - }; + var recorder = try Recorder.init(std.testing.allocator, tmp.dir, "multi.lp"); defer recorder.deinit(); // An attacker-controlled comment trying to smuggle a command must not // produce an executable line on replay. recorder.recordComment("note\nGOTO https://attacker\r\nmore"); - file.seekTo(0) catch unreachable; + const file = tmp.dir.openFile("multi.lp", .{}) catch unreachable; + defer file.close(); var buf: [256]u8 = undefined; const n = file.readAll(&buf) catch unreachable; try std.testing.expectEqualStrings( @@ -295,6 +266,8 @@ test "record disables recorder on write failure" { defer tmp.cleanup(); // Open the file read-only so writeAll fails with `error.NotOpenForWriting`. + // Struct literal (not `init`) because only this test needs to inject a + // read-only handle to exercise the failure path. const file = blk: { _ = tmp.dir.createFile("ro.lp", .{}) catch unreachable; break :blk tmp.dir.openFile("ro.lp", .{ .mode = .read_only }) catch unreachable; @@ -306,6 +279,7 @@ test "record disables recorder on write failure" { .path = try std.testing.allocator.dupe(u8, "test.lp"), .lines = 0, .buf = .init(std.testing.allocator), + .arena = .init(std.testing.allocator), }; defer recorder.deinit(); @@ -324,12 +298,7 @@ test "init creates the file if missing" { var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); - var path_buf: [std.fs.max_path_bytes]u8 = undefined; - const dir_path = tmp.dir.realpath(".", &path_buf) catch unreachable; - var full_buf: [std.fs.max_path_bytes]u8 = undefined; - const abs_path = std.fmt.bufPrint(&full_buf, "{s}/fresh.lp", .{dir_path}) catch unreachable; - - var recorder: Recorder = try .init(std.testing.allocator, abs_path); + var recorder: Recorder = try .init(std.testing.allocator, tmp.dir, "fresh.lp"); defer recorder.deinit(); recorder.record(Command.parse("GOTO https://example.com"));