From 0ac7e8d55bbac64c0062162573f507d185f5207a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 13 May 2025 12:46:21 -0700 Subject: [PATCH 1/2] fix: tweak the label for citations for better rendering (#919) Adds a space so that sequential citations have some more breathing room. As I had to update the tests for this change, I also introduced a `toDiffableString()` helper to make the test easier to update as we make formatting changes to the output. --- .../chat/terminal-chat-response-item.tsx | 7 ++++++- codex-cli/tests/markdown.test.tsx | 17 ++++++++++++++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/codex-cli/src/components/chat/terminal-chat-response-item.tsx b/codex-cli/src/components/chat/terminal-chat-response-item.tsx index 888547a66c..a81d541406 100644 --- a/codex-cli/src/components/chat/terminal-chat-response-item.tsx +++ b/codex-cli/src/components/chat/terminal-chat-response-item.tsx @@ -334,6 +334,11 @@ function rewriteFileCitations( return markdown.replace(citationRegex, (_match, file, start, _end) => { const absPath = path.resolve(cwd, file); const uri = `${fileOpener}://file${absPath}:${start}`; - return `[${file}](${uri})`; + const label = `${file}:${start}`; + // In practice, sometimes multiple citations for the same file, but with a + // different line number, are shown sequentially, so we: + // - include the line number in the label to disambiguate them + // - add a space after the link to make it easier to read + return `[${label}](${uri}) `; }); } diff --git a/codex-cli/tests/markdown.test.tsx b/codex-cli/tests/markdown.test.tsx index 79c12bb9d5..f4c7fb428a 100644 --- a/codex-cli/tests/markdown.test.tsx +++ b/codex-cli/tests/markdown.test.tsx @@ -137,14 +137,16 @@ orchestration is: ${GREEN}${BOLD}### ${YELLOW}src/cli.tsx${COLOR_OFF}${BOLD_OFF} * Uses ${BOLD}meow${BOLD_OFF} for flags/subcommands and prints the built-in help/usage: - ${BLUE}src/cli.tsx (${LINK_ON}vscode://file/home/user/codex/src/cli.tsx:49${LINK_OFF})src/cli.tsx ${COLOR_OFF} + ${BLUE}src/cli.tsx:49 (${LINK_ON}vscode://file/home/user/codex/src/cli.tsx:49${LINK_OFF})${COLOR_OFF} ${BLUE}src/cli.tsx:55 ${COLOR_OFF} ${BLUE}(${LINK_ON}vscode://file/home/user/codex/src/cli.tsx:55${LINK_OFF})${COLOR_OFF} * Handles special subcommands (e.g. ${YELLOW}codex completion …${COLOR_OFF}), ${YELLOW}--config${COLOR_OFF}, API-key validation, then either: * Spawns the ${BOLD}AgentLoop${BOLD_OFF} for the normal multi-step prompting/edits flow, or * Runs ${BOLD}single-pass${BOLD_OFF} mode if ${YELLOW}--full-context${COLOR_OFF} is set.`; - expect(outputWithAnsi).toBe(expectedNestedList); + expect(toDiffableString(outputWithAnsi)).toBe( + toDiffableString(expectedNestedList), + ); }); it("citations should get converted to hyperlinks when stdout supports them", () => { @@ -154,8 +156,17 @@ either: , ); - const expected = `File with TODO: ${BLUE}src/approvals.ts (${LINK_ON}vscode://file/foo/bar/src/approvals.ts:40${LINK_OFF})${COLOR_OFF}`; + const expected = `File with TODO: ${BLUE}src/approvals.ts:40 (${LINK_ON}vscode://file/foo/bar/src/approvals.ts:40${LINK_OFF})${COLOR_OFF}`; const outputWithAnsi = lastFrame(); expect(outputWithAnsi).toBe(expected); }); }); + +function toDiffableString(str: string) { + // The test harness is not able to handle ANSI codes, so we need to escape + // them, but still give it line-based input so that it can diff the output. + return str + .split("\n") + .map((line) => JSON.stringify(line)) + .join("\n"); +} From a2fa531c14f6b77fa4b365a733eb1c79a51ab6d3 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 13 May 2025 13:04:18 -0700 Subject: [PATCH 2/2] feat: auto-approve nl and support piping to sed --- codex-cli/src/approvals.ts | 13 ++++++++-- codex-cli/tests/approvals.test.ts | 43 +++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/codex-cli/src/approvals.ts b/codex-cli/src/approvals.ts index 032acec026..e626da7fa5 100644 --- a/codex-cli/src/approvals.ts +++ b/codex-cli/src/approvals.ts @@ -365,6 +365,11 @@ export function isSafeCommand( reason: "View file contents", group: "Reading files", }; + case "nl": + return { + reason: "View file with line numbers", + group: "Reading files", + }; case "rg": return { reason: "Ripgrep search", @@ -448,11 +453,15 @@ export function isSafeCommand( } break; case "sed": + // We allow two types of sed invocations: + // 1. `sed -n 1,200p FILE` + // 2. `sed -n 1,200p` because the file is passed via stdin, e.g., + // `nl -ba README.md | sed -n '1,200p'` if ( cmd1 === "-n" && isValidSedNArg(cmd2) && - typeof cmd3 === "string" && - command.length === 4 + (command.length === 3 || + (typeof cmd3 === "string" && command.length === 4)) ) { return { reason: "Sed print subset", diff --git a/codex-cli/tests/approvals.test.ts b/codex-cli/tests/approvals.test.ts index 94daacce00..c592c39525 100644 --- a/codex-cli/tests/approvals.test.ts +++ b/codex-cli/tests/approvals.test.ts @@ -32,6 +32,12 @@ describe("canAutoApprove()", () => { group: "Reading files", runInSandbox: false, }); + expect(check(["nl", "-ba", "README.md"])).toEqual({ + type: "auto-approve", + reason: "View file with line numbers", + group: "Reading files", + runInSandbox: false, + }); expect(check(["pwd"])).toEqual({ type: "auto-approve", reason: "Print working directory", @@ -147,4 +153,41 @@ describe("canAutoApprove()", () => { type: "ask-user", }); }); + + test("sed", () => { + // `sed` used to read lines from a file. + expect(check(["sed", "-n", "1,200p", "filename.txt"])).toEqual({ + type: "auto-approve", + reason: "Sed print subset", + group: "Reading files", + runInSandbox: false, + }); + // Bad quoting! The model is doing the wrong thing here, so this should not + // be auto-approved. + expect(check(["sed", "-n", "'1,200p'", "filename.txt"])).toEqual({ + type: "ask-user", + }); + // Extra arg: here we are extra conservative, we do not auto-approve. + expect(check(["sed", "-n", "1,200p", "file1.txt", "file2.txt"])).toEqual({ + type: "ask-user", + }); + + // `sed` used to read lines from a file with a shell command. + expect(check(["bash", "-lc", "sed -n '1,200p' filename.txt"])).toEqual({ + type: "auto-approve", + reason: "Sed print subset", + group: "Reading files", + runInSandbox: false, + }); + + // Pipe the output of `nl` to `sed`. + expect( + check(["bash", "-lc", "nl -ba README.md | sed -n '1,200p'"]), + ).toEqual({ + type: "auto-approve", + reason: "View file with line numbers", + group: "Reading files", + runInSandbox: false, + }); + }); });