mirror of
https://github.com/openai/codex.git
synced 2026-11-07 08:34:29 +00:00
Render code comment directives in TUI replay (#26554)
## Summary
Resumed Codex App or VS Code review sessions can contain
`::code-comment` directives that the TUI previously displayed verbatim
because only rich clients interpret them.
This change rewrites valid line-start directives into readable Markdown
during assistant-message parsing, using the session working directory
for relative file paths. The fallback is applied consistently to live
messages, replayed transcripts, and resume previews while preserving
malformed directives and existing `::git-*` parsing.
## Before
The TUI exposed the raw client directive:
```text
::code-comment{title="Fix body= parsing" body="Keep role=\"tab\", ::git-stage{cwd=/tmp}, file=, and \n literal." file="/repo/src/app.ts" start=10 end=12 priority="P2"}
```
## After
The same directive is rendered as readable review feedback:
```text
- [P2] Fix body= parsing — src/app.ts:10-12
Keep role="tab", ::git-stage{cwd=/tmp}, file=, and \n literal.
```
Fixes #25658
This commit is contained in:
1 parent
fb0993dd3b
commit
e5af672d73
6 files changed
+167
-11
No files matched your search
@@ -38,7 +38,8 @@ impl ChatWidget {
|
|||||||
// Consolidate the run of streaming AgentMessageCells into a single AgentMarkdownCell
|
// Consolidate the run of streaming AgentMessageCells into a single AgentMarkdownCell
|
||||||
// that can re-render from source on resize.
|
// that can re-render from source on resize.
|
||||||
if let Some(source) = source {
|
if let Some(source) = source {
|
||||||
let source = parse_assistant_markdown(&source).visible_markdown;
|
let source =
|
||||||
|
parse_assistant_markdown(&source, self.config.cwd.as_path()).visible_markdown;
|
||||||
self.app_event_tx.send(AppEvent::ConsolidateAgentMessage {
|
self.app_event_tx.send(AppEvent::ConsolidateAgentMessage {
|
||||||
source,
|
source,
|
||||||
cwd: self.config.cwd.to_path_buf(),
|
cwd: self.config.cwd.to_path_buf(),
|
||||||
@@ -261,7 +262,7 @@ impl ChatWidget {
|
|||||||
AgentMessageContent::Text { text } => message.push_str(text),
|
AgentMessageContent::Text { text } => message.push_str(text),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
let parsed = parse_assistant_markdown(&message);
|
let parsed = parse_assistant_markdown(&message, self.config.cwd.as_path());
|
||||||
self.finalize_completed_assistant_message(
|
self.finalize_completed_assistant_message(
|
||||||
(!parsed.visible_markdown.is_empty()).then_some(parsed.visible_markdown.as_str()),
|
(!parsed.visible_markdown.is_empty()).then_some(parsed.visible_markdown.as_str()),
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -89,9 +89,9 @@ impl ChatWidget {
|
|||||||
// source only when no earlier item-level event (AgentMessageItem, plan
|
// source only when no earlier item-level event (AgentMessageItem, plan
|
||||||
// commit, review output) already recorded markdown for this turn. This
|
// commit, review output) already recorded markdown for this turn. This
|
||||||
// prevents the final summary from overwriting a more specific source.
|
// prevents the final summary from overwriting a more specific source.
|
||||||
let sanitized_last_agent_message = last_agent_message
|
let sanitized_last_agent_message = last_agent_message.as_deref().map(|message| {
|
||||||
.as_deref()
|
parse_assistant_markdown(message, self.config.cwd.as_path()).visible_markdown
|
||||||
.map(|message| parse_assistant_markdown(message).visible_markdown);
|
});
|
||||||
if let Some(message) = sanitized_last_agent_message
|
if let Some(message) = sanitized_last_agent_message
|
||||||
.as_ref()
|
.as_ref()
|
||||||
.filter(|message| !message.is_empty())
|
.filter(|message| !message.is_empty())
|
||||||
|
|||||||
@@ -1,6 +1,8 @@
|
|||||||
//! Codex App git action directives embedded in assistant markdown.
|
//! Codex App directives embedded in assistant markdown.
|
||||||
|
|
||||||
|
use std::collections::HashMap;
|
||||||
use std::collections::HashSet;
|
use std::collections::HashSet;
|
||||||
|
use std::path::Path;
|
||||||
|
|
||||||
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
|
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
|
||||||
pub(crate) enum GitActionDirective {
|
pub(crate) enum GitActionDirective {
|
||||||
@@ -50,12 +52,16 @@ impl ParsedAssistantMarkdown {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
pub(crate) fn parse_assistant_markdown(markdown: &str) -> ParsedAssistantMarkdown {
|
pub(crate) fn parse_assistant_markdown(markdown: &str, cwd: &Path) -> ParsedAssistantMarkdown {
|
||||||
let mut git_actions = Vec::new();
|
let mut git_actions = Vec::new();
|
||||||
let mut seen = HashSet::new();
|
let mut seen = HashSet::new();
|
||||||
let mut visible_lines = Vec::new();
|
let mut visible_lines = Vec::new();
|
||||||
|
|
||||||
for line in markdown.lines() {
|
for line in markdown.lines() {
|
||||||
|
if let Some(rewritten) = rewrite_code_comment_line(line, cwd) {
|
||||||
|
visible_lines.push(rewritten.trim_end().to_string());
|
||||||
|
continue;
|
||||||
|
}
|
||||||
let (visible_line, line_actions) = strip_line_directives(line);
|
let (visible_line, line_actions) = strip_line_directives(line);
|
||||||
for action in line_actions {
|
for action in line_actions {
|
||||||
if seen.insert(action.clone()) {
|
if seen.insert(action.clone()) {
|
||||||
@@ -78,6 +84,53 @@ pub(crate) fn parse_assistant_markdown(markdown: &str) -> ParsedAssistantMarkdow
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn rewrite_code_comment_line(line: &str, cwd: &Path) -> Option<String> {
|
||||||
|
let content = line.trim_start_matches([' ', '\t']);
|
||||||
|
let indent = &line[..line.len() - content.len()];
|
||||||
|
let marker_length = content.bytes().take_while(|byte| *byte == b':').count();
|
||||||
|
if !(1..=3).contains(&marker_length) {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
|
|
||||||
|
let directive = content[marker_length..].strip_prefix("code-comment{")?;
|
||||||
|
let (attributes, suffix) = directive.rsplit_once('}')?;
|
||||||
|
let attributes = parse_code_comment_attributes(attributes)?;
|
||||||
|
let title = attributes.get("title")?;
|
||||||
|
let body = attributes.get("body")?;
|
||||||
|
let file = attributes.get("file")?;
|
||||||
|
let title = title.trim();
|
||||||
|
let body = body.trim();
|
||||||
|
let file = file.trim();
|
||||||
|
(!title.is_empty() && !body.is_empty() && !file.is_empty()).then_some(())?;
|
||||||
|
|
||||||
|
let start = directive_integer(&attributes, "start").unwrap_or(1).max(1);
|
||||||
|
let end = directive_integer(&attributes, "end")
|
||||||
|
.unwrap_or(start)
|
||||||
|
.max(start);
|
||||||
|
let title = if title_has_priority(title) {
|
||||||
|
title.to_string()
|
||||||
|
} else if let Some(priority @ 0..=3) = directive_integer(&attributes, "priority") {
|
||||||
|
format!("[P{priority}] {title}")
|
||||||
|
} else {
|
||||||
|
title.to_string()
|
||||||
|
};
|
||||||
|
let file_path = Path::new(file);
|
||||||
|
let file = file_path
|
||||||
|
.strip_prefix(cwd)
|
||||||
|
.unwrap_or(file_path)
|
||||||
|
.to_string_lossy()
|
||||||
|
.replace('\\', "/");
|
||||||
|
let location = if start == end {
|
||||||
|
format!("{file}:{start}")
|
||||||
|
} else {
|
||||||
|
format!("{file}:{start}-{end}")
|
||||||
|
};
|
||||||
|
|
||||||
|
Some(format!(
|
||||||
|
"{indent}- {title} — {location}\n{indent} {body}{suffix}"
|
||||||
|
))
|
||||||
|
}
|
||||||
|
|
||||||
fn strip_line_directives(line: &str) -> (String, Vec<GitActionDirective>) {
|
fn strip_line_directives(line: &str) -> (String, Vec<GitActionDirective>) {
|
||||||
let mut visible = String::new();
|
let mut visible = String::new();
|
||||||
let mut actions = Vec::new();
|
let mut actions = Vec::new();
|
||||||
@@ -106,6 +159,46 @@ fn strip_line_directives(line: &str) -> (String, Vec<GitActionDirective>) {
|
|||||||
(visible, actions)
|
(visible, actions)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn directive_integer(attributes: &HashMap<String, String>, name: &str) -> Option<i64> {
|
||||||
|
attributes
|
||||||
|
.get(name)?
|
||||||
|
.trim()
|
||||||
|
.trim_start_matches(['P', 'p'])
|
||||||
|
.parse()
|
||||||
|
.ok()
|
||||||
|
}
|
||||||
|
|
||||||
|
fn title_has_priority(title: &str) -> bool {
|
||||||
|
let bytes = title.trim_start().as_bytes();
|
||||||
|
bytes.len() >= 4
|
||||||
|
&& bytes[0] == b'['
|
||||||
|
&& matches!(bytes[1], b'P' | b'p')
|
||||||
|
&& bytes[2].is_ascii_digit()
|
||||||
|
&& bytes[3] == b']'
|
||||||
|
}
|
||||||
|
|
||||||
|
fn parse_code_comment_attributes(input: &str) -> Option<HashMap<String, String>> {
|
||||||
|
let mut attributes = HashMap::new();
|
||||||
|
let mut rest = input.trim();
|
||||||
|
while !rest.is_empty() {
|
||||||
|
let equals = rest.find('=')?;
|
||||||
|
let name = rest[..equals].trim();
|
||||||
|
if name.is_empty() {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
|
rest = rest[equals + 1..].trim_start();
|
||||||
|
let (value, next) = if let Some(quoted) = rest.strip_prefix('"') {
|
||||||
|
parse_quoted_value(quoted)?
|
||||||
|
} else {
|
||||||
|
let end = rest.find(char::is_whitespace).unwrap_or(rest.len());
|
||||||
|
(rest[..end].to_string(), &rest[end..])
|
||||||
|
};
|
||||||
|
attributes.insert(name.to_string(), value);
|
||||||
|
rest = next.trim_start();
|
||||||
|
}
|
||||||
|
Some(attributes)
|
||||||
|
}
|
||||||
|
|
||||||
fn parse_git_action(name: &str, attributes: &str) -> Option<GitActionDirective> {
|
fn parse_git_action(name: &str, attributes: &str) -> Option<GitActionDirective> {
|
||||||
let attrs = parse_attributes(attributes)?;
|
let attrs = parse_attributes(attributes)?;
|
||||||
let cwd = attrs.get("cwd")?.clone();
|
let cwd = attrs.get("cwd")?.clone();
|
||||||
@@ -153,14 +246,36 @@ fn parse_attributes(input: &str) -> Option<std::collections::HashMap<String, Str
|
|||||||
Some(attrs)
|
Some(attrs)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn parse_quoted_value(input: &str) -> Option<(String, &str)> {
|
||||||
|
let mut value = String::new();
|
||||||
|
let mut characters = input.char_indices().peekable();
|
||||||
|
|
||||||
|
while let Some((index, character)) = characters.next() {
|
||||||
|
if character == '"' {
|
||||||
|
return Some((value, &input[index + 1..]));
|
||||||
|
}
|
||||||
|
match character {
|
||||||
|
'\\' if characters.peek().is_some_and(|(_, next)| *next == '"') => {
|
||||||
|
value.push('"');
|
||||||
|
characters.next();
|
||||||
|
}
|
||||||
|
_ => value.push(character),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
None
|
||||||
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
use pretty_assertions::assert_eq;
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn strips_and_parses_git_action_directives() {
|
fn strips_and_parses_git_action_directives() {
|
||||||
let parsed = parse_assistant_markdown(
|
let parsed = parse_assistant_markdown(
|
||||||
"Done\n\n::git-stage{cwd=\"/repo\"} ::git-push{cwd=\"/repo\" branch=\"feat/x\"}",
|
"Done\n\n::git-stage{cwd=\"/repo\"} ::git-push{cwd=\"/repo\" branch=\"feat/x\"} ::git-stage{cwd=\"C:\\repo\\\"}",
|
||||||
|
Path::new("/repo"),
|
||||||
);
|
);
|
||||||
|
|
||||||
assert_eq!(parsed.visible_markdown, "Done");
|
assert_eq!(parsed.visible_markdown, "Done");
|
||||||
@@ -174,22 +289,50 @@ mod tests {
|
|||||||
cwd: "/repo".to_string(),
|
cwd: "/repo".to_string(),
|
||||||
branch: "feat/x".to_string(),
|
branch: "feat/x".to_string(),
|
||||||
},
|
},
|
||||||
|
GitActionDirective::Stage {
|
||||||
|
cwd: "C:\\repo\\".to_string(),
|
||||||
|
},
|
||||||
]
|
]
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn hides_malformed_directives_without_materializing_rows() {
|
fn hides_malformed_directives_without_materializing_rows() {
|
||||||
let parsed = parse_assistant_markdown("Done ::git-push{cwd=\"/repo\"}");
|
let parsed = parse_assistant_markdown("Done ::git-push{cwd=\"/repo\"}", Path::new("/repo"));
|
||||||
|
|
||||||
assert_eq!(parsed.visible_markdown, "Done");
|
assert_eq!(parsed.visible_markdown, "Done");
|
||||||
assert!(parsed.git_actions.is_empty());
|
assert!(parsed.git_actions.is_empty());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn renders_code_comment_directives_as_markdown() {
|
||||||
|
let parsed = parse_assistant_markdown(
|
||||||
|
concat!(
|
||||||
|
"Found two issues.\n\n",
|
||||||
|
r#"::code-comment{title="Fix body= parsing" body="Keep role=\"tab\", ::git-stage{cwd=/tmp}, file=, and \n literal." file="/repo/src/app.ts" start=10 end=12 priority="P2"}"#,
|
||||||
|
"\n\n",
|
||||||
|
r#":::code-comment{title="[P1] Clamp the range" body="The line range should match the App." file="codex/src/range.ts" start=8 end=2 priority=3}"#,
|
||||||
|
),
|
||||||
|
Path::new("/repo"),
|
||||||
|
);
|
||||||
|
|
||||||
|
insta::assert_snapshot!("code_comment_directive_fallback", parsed.visible_markdown);
|
||||||
|
assert!(parsed.git_actions.is_empty());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn preserves_non_directive_and_malformed_code_comment_text() {
|
||||||
|
let markdown = "Mention `::code-comment{title=\"Example\"}` inline.\n::code-comment{title=\"Missing body\" file=\"/repo/src/app.ts\"}";
|
||||||
|
let parsed = parse_assistant_markdown(markdown, Path::new("/repo"));
|
||||||
|
|
||||||
|
assert_eq!(parsed.visible_markdown, markdown);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn last_created_branch_cwd_uses_the_last_matching_directive() {
|
fn last_created_branch_cwd_uses_the_last_matching_directive() {
|
||||||
let parsed = parse_assistant_markdown(
|
let parsed = parse_assistant_markdown(
|
||||||
"::git-create-branch{cwd=\"/first\" branch=\"first\"}\n::git-push{cwd=\"/repo\" branch=\"first\"}\n::git-create-branch{cwd=\"/second\" branch=\"second\"}",
|
"::git-create-branch{cwd=\"/first\" branch=\"first\"}\n::git-push{cwd=\"/repo\" branch=\"first\"}\n::git-create-branch{cwd=\"/second\" branch=\"second\"}",
|
||||||
|
Path::new("/repo"),
|
||||||
);
|
);
|
||||||
|
|
||||||
assert_eq!(parsed.last_created_branch_cwd(), Some("/second"));
|
assert_eq!(parsed.last_created_branch_cwd(), Some("/second"));
|
||||||
|
|||||||
@@ -774,6 +774,7 @@ async fn load_transcript_preview(
|
|||||||
.thread_read(thread_id, /*include_turns*/ true)
|
.thread_read(thread_id, /*include_turns*/ true)
|
||||||
.await
|
.await
|
||||||
.map_err(std::io::Error::other)?;
|
.map_err(std::io::Error::other)?;
|
||||||
|
let cwd = thread.cwd.as_path();
|
||||||
let mut lines = thread
|
let mut lines = thread
|
||||||
.turns
|
.turns
|
||||||
.iter()
|
.iter()
|
||||||
@@ -794,7 +795,7 @@ async fn load_transcript_preview(
|
|||||||
}),
|
}),
|
||||||
ThreadItem::AgentMessage { text, .. } => Some(TranscriptPreviewLine {
|
ThreadItem::AgentMessage { text, .. } => Some(TranscriptPreviewLine {
|
||||||
speaker: TranscriptPreviewSpeaker::Assistant,
|
speaker: TranscriptPreviewSpeaker::Assistant,
|
||||||
text: parse_assistant_markdown(text).visible_markdown,
|
text: parse_assistant_markdown(text, cwd).visible_markdown,
|
||||||
}),
|
}),
|
||||||
_ => None,
|
_ => None,
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -67,7 +67,7 @@ pub(crate) fn thread_to_transcript_cells(
|
|||||||
}));
|
}));
|
||||||
}
|
}
|
||||||
ThreadItem::AgentMessage { text, .. } => {
|
ThreadItem::AgentMessage { text, .. } => {
|
||||||
let parsed = parse_assistant_markdown(text);
|
let parsed = parse_assistant_markdown(text, cwd);
|
||||||
if !parsed.visible_markdown.trim().is_empty() {
|
if !parsed.visible_markdown.trim().is_empty() {
|
||||||
cells.push(Arc::new(AgentMarkdownCell::new(
|
cells.push(Arc::new(AgentMarkdownCell::new(
|
||||||
parsed.visible_markdown,
|
parsed.visible_markdown,
|
||||||
|
|||||||
+11
@@ -0,0 +1,11 @@
|
|||||||
|
---
|
||||||
|
source: tui/src/git_action_directives.rs
|
||||||
|
expression: parsed.visible_markdown
|
||||||
|
---
|
||||||
|
Found two issues.
|
||||||
|
|
||||||
|
- [P2] Fix body= parsing — src/app.ts:10-12
|
||||||
|
Keep role="tab", ::git-stage{cwd=/tmp}, file=, and \n literal.
|
||||||
|
|
||||||
|
- [P1] Clamp the range — codex/src/range.ts:8
|
||||||
|
The line range should match the App.
|
||||||
Reference in new issue
Block a user