Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/mark/src-tauri/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ pub struct ReviewTimelineItem {
pub scope: String,
pub session_id: Option<String>,
pub session_status: Option<String>,
pub title: Option<String>,
pub comment_count: usize,
pub created_at: i64,
pub updated_at: i64,
Expand Down Expand Up @@ -1413,6 +1414,7 @@ fn build_branch_timeline(store: &Arc<Store>, branch_id: &str) -> Result<BranchTi
scope: r.scope.as_str().to_string(),
session_id,
session_status,
title: r.title,
comment_count,
created_at: r.created_at,
updated_at: r.updated_at,
Expand Down
56 changes: 42 additions & 14 deletions apps/mark/src-tauri/src/session_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1316,11 +1316,20 @@ fn review_timeline_entries(store: &Arc<Store>, branch_id: &str) -> Vec<TimelineE
continue;
}
let short_sha = &review.commit_sha[..review.commit_sha.len().min(7)];
let mut content = format!(
"### Code Review of {} ({} scope)\n",
short_sha,
review.scope.as_str(),
);
let heading = match review.title.as_deref() {
Some(title) => format!(
"### {} — {} ({} scope)\n",
title,
short_sha,
review.scope.as_str(),
),
None => format!(
"### Code Review of {} ({} scope)\n",
short_sha,
review.scope.as_str(),
),
};
let mut content = heading;

// Group comments by file path
let mut by_path: std::collections::BTreeMap<&str, Vec<&crate::store::models::Comment>> =
Expand Down Expand Up @@ -1425,7 +1434,18 @@ Reserve `\"warning\"` and `\"issue\"` for genuine concerns.

## Output format

Return your review as exactly one fenced JSON block with this structure:
Your response must start directly with the review-title fenced block below — \
do not output any preamble, commentary, or thinking before it.

Provide a single-sentence title (max 15 words) that conveys your overall \
confidence level in the changes. Do not describe what the changes do — instead focus on \
how confident you are that they are correct and safe. Wrap it in a fenced block:

```review-title
Looks solid overall with one minor edge case worth checking
```

Then return your review comments as exactly one fenced JSON block:

```review-comments
[
Expand All @@ -1445,10 +1465,12 @@ Return your review as exactly one fenced JSON block with this structure:
```

Formatting requirements:
- The opening fence line must be exactly: ```review-comments
- The closing fence line must be exactly: ```
- Put only the JSON array between the fences (no prose or markdown inside).
- Do not wrap this block in any additional code fences.
- The opening fence line for the title must be exactly: ```review-title
- The opening fence line for comments must be exactly: ```review-comments
- Each closing fence line must be exactly: ```
- Put only plain text (no markdown) inside the review-title block.
- Put only the JSON array inside the review-comments block (no prose or markdown).
- Do not wrap these blocks in any additional code fences.

Rules:
- `span` uses 0-indexed line numbers from the \"after\" side of the diff (exclusive end).
Expand Down Expand Up @@ -1483,11 +1505,17 @@ mod tests {
&BranchSessionType::Review,
);

assert!(
prompt.contains("Then return your review comments as exactly one fenced JSON block:")
);
assert!(prompt
.contains("Return your review as exactly one fenced JSON block with this structure:"));
assert!(prompt.contains("The opening fence line must be exactly: ```review-comments"));
assert!(prompt.contains("The closing fence line must be exactly: ```"));
.contains("The opening fence line for the title must be exactly: ```review-title"));
assert!(prompt
.contains("Put only the JSON array between the fences (no prose or markdown inside)."));
.contains("The opening fence line for comments must be exactly: ```review-comments"));
assert!(prompt.contains("Each closing fence line must be exactly: ```"));
assert!(prompt.contains(
"Put only the JSON array inside the review-comments block (no prose or markdown)."
));
assert!(prompt.contains("do not output any preamble, commentary, or thinking before it"));
}
}
177 changes: 166 additions & 11 deletions apps/mark/src-tauri/src/session_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -518,17 +518,30 @@ fn run_post_completion_hooks(
}
}

// --- Review comment extraction ---
// --- Review comment and title extraction ---
if let Ok(Some(review)) = store.get_review_by_session(session_id) {
if review.comments.is_empty() {
if let Ok(messages) = store.get_session_messages(session_id) {
let full_text: String = messages
.iter()
.filter(|m| m.role == MessageRole::Assistant)
.map(|m| m.content.as_str())
.collect::<Vec<_>>()
.join("\n");
if let Ok(messages) = store.get_session_messages(session_id) {
let full_text: String = messages
.iter()
.filter(|m| m.role == MessageRole::Assistant)
.map(|m| m.content.as_str())
.collect::<Vec<_>>()
.join("\n");

// Extract and save review title (always attempt, even if comments exist)
if review.title.is_none() {
if let Some(title) = extract_review_title(&full_text) {
log::info!("Session {session_id}: extracted review title: {title}");
if let Err(e) = store.update_review_title(&review.id, &title) {
log::error!("Failed to update review title: {e}");
}
} else {
log::warn!("Session {session_id}: review session completed but no review-title block found");
}
}

// Extract and save review comments
if review.comments.is_empty() {
let comments = extract_review_comments(&full_text);
if comments.is_empty() {
log::warn!("Session {session_id}: review session completed but no review-comments block found");
Expand Down Expand Up @@ -715,8 +728,9 @@ fn extract_review_comments(text: &str) -> Vec<Comment> {
let marker_start = "```review-comments";

let mut search_from = 0;
while let Some(start_pos) = text[search_from..].find(marker_start) {
let block_start = search_from + start_pos + marker_start.len();
while let Some(rel_pos) = find_opening_fence(&text[search_from..], marker_start) {
let start_pos = search_from + rel_pos;
let block_start = start_pos + marker_start.len();
// Skip to the next line after the opening marker
let json_start = match text[block_start..].find('\n') {
Some(pos) => block_start + pos + 1,
Expand Down Expand Up @@ -768,6 +782,42 @@ fn extract_review_comments(text: &str) -> Vec<Comment> {
comments
}

/// Extract the review title from assistant output.
///
/// Looks for a ```review-title fenced block and returns the trimmed text inside.
/// The opening fence must appear at the start of a line to avoid matching the
/// marker when it appears inside regular prose (e.g. the LLM discussing the
/// extraction logic itself).
fn extract_review_title(text: &str) -> Option<String> {
let marker = "```review-title";
let start_pos = find_opening_fence(text, marker)?;
let block_start = start_pos + marker.len();
let content_start = block_start + text[block_start..].find('\n')? + 1;
let end_pos = find_closing_fence(&text[content_start..])?;
let title = text[content_start..content_start + end_pos].trim();
if title.is_empty() {
None
} else {
Some(title.to_string())
}
}

/// Find an opening fence marker (e.g. ` ```review-title `) that appears at the
/// start of a line (position 0 or immediately after `\n`). Returns the byte
/// offset of the marker within `text`, or `None` if no line-start match exists.
fn find_opening_fence(text: &str, marker: &str) -> Option<usize> {
let mut pos = 0;
while pos < text.len() {
let candidate = text[pos..].find(marker)?;
let abs = pos + candidate;
if abs == 0 || text.as_bytes()[abs - 1] == b'\n' {
return Some(abs);
}
pos = abs + marker.len();
}
None
}

/// Find the closing ``` fence for a code block.
///
/// A closing fence must appear at the start of a line (after a newline) and
Expand Down Expand Up @@ -888,6 +938,111 @@ mod tests {
assert_eq!(find_closing_fence(text), Some(5));
}

// ── extract_review_title ──────────────────────────────────────────

#[test]
fn extract_title_simple() {
let text = "Here is my review:\n\n```review-title\nSolid refactor with one edge case\n```\n\nAnd comments...";
assert_eq!(
extract_review_title(text),
Some("Solid refactor with one edge case".to_string())
);
}

#[test]
fn extract_title_trims_whitespace() {
let text = "```review-title\n Clean changes, no concerns \n```\n";
assert_eq!(
extract_review_title(text),
Some("Clean changes, no concerns".to_string())
);
}

#[test]
fn extract_title_none_when_missing() {
let text = "Just a normal message with no review title.";
assert_eq!(extract_review_title(text), None);
}

#[test]
fn extract_title_none_when_empty() {
let text = "```review-title\n\n```\n";
assert_eq!(extract_review_title(text), None);
}

#[test]
fn extract_title_with_review_comments() {
let text = r#"Here is my review:

```review-title
Risky changes to auth flow need closer look
```

```review-comments
[
{
"path": "src/auth.rs",
"span": { "start": 10, "end": 15 },
"content": "Missing validation."
}
]
```
"#;
assert_eq!(
extract_review_title(text),
Some("Risky changes to auth flow need closer look".to_string())
);
}

#[test]
fn extract_title_ignores_llm_preamble() {
let text = r#"I now have a complete picture of the changes. Let me produce the review.

```review-title
Clean, well-tested feature addition with good backward compatibility
```

```review-comments
[
{
"path": "src/foo.rs",
"span": { "start": 10, "end": 15 },
"type": "suggestion",
"content": "Consider adding a test."
}
]
```
"#;
assert_eq!(
extract_review_title(text),
Some(
"Clean, well-tested feature addition with good backward compatibility".to_string()
)
);
}

#[test]
fn extract_title_marker_mentioned_in_preamble() {
// The LLM discusses the ```review-title marker in its preamble before
// actually producing the fenced block. The extractor must skip the
// mid-line mention and find the real opening fence.
let text = r#"Let me check whether `"```review-title"` would match inside `"```review-title-v2"`:
I now have a complete picture. Let me produce the review.

```review-title
Solid changes with minor nit
```

```review-comments
[]
```
"#;
assert_eq!(
extract_review_title(text),
Some("Solid changes with minor nit".to_string())
);
}

// ── extract_review_comments ─────────────────────────────────────────

#[test]
Expand Down
12 changes: 9 additions & 3 deletions apps/mark/src-tauri/src/store/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ impl From<rusqlite::Error> for StoreError {
///
/// Bump this whenever the schema changes.
/// Many app versions may share the same schema version.
pub const SCHEMA_VERSION: i64 = 17;
pub const SCHEMA_VERSION: i64 = 18;

/// Oldest schema version we can migrate forward from.
///
Expand Down Expand Up @@ -373,6 +373,7 @@ impl Store {
commit_sha TEXT NOT NULL,
scope TEXT NOT NULL,
session_id TEXT,
title TEXT,
created_at INTEGER NOT NULL,
updated_at INTEGER NOT NULL
);
Expand Down Expand Up @@ -518,8 +519,13 @@ impl Store {
}

// v15→v16 and v16→v17 previously added a workstation_id column that
// has been removed. The migrations are no-ops now; SCHEMA_VERSION stays
// at 17 so existing databases are simply stamped forward.
// has been removed. The migrations are no-ops now.

if db_version < 18 {
// v17 → v18: add title column to reviews
conn.execute_batch("ALTER TABLE reviews ADD COLUMN title TEXT DEFAULT NULL;")
.ok(); // Ignore error if column already exists (fresh DB)
Comment on lines +526 to +527

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fail migration if adding reviews.title does not succeed

This migration swallows every ALTER TABLE error via .ok() and then unconditionally stamps schema_version to 18, so a non-duplicate failure (for example a locked/corrupt DB) leaves the DB marked as upgraded while still missing reviews.title. After that, the new SELECT ... title ... FROM reviews queries in store/reviews.rs will fail at runtime with no such column: title, effectively breaking review loading until a reset. The migration should only ignore the specific "duplicate column" case or abort without bumping the schema version.

Useful? React with 👍 / 👎.

}

// Stamp the current schema version so future opens skip applied migrations.
conn.execute(
Expand Down
3 changes: 3 additions & 0 deletions apps/mark/src-tauri/src/store/models.rs
Original file line number Diff line number Diff line change
Expand Up @@ -822,6 +822,8 @@ pub struct Review {
pub commit_sha: String,
pub scope: ReviewScope,
pub session_id: Option<String>,
/// AI-generated one-sentence title summarising the review's confidence.
pub title: Option<String>,
/// Paths that have been marked as reviewed.
pub reviewed: Vec<String>,
/// Comments attached to specific locations.
Expand All @@ -841,6 +843,7 @@ impl Review {
commit_sha: commit_sha.to_string(),
scope,
session_id: None,
title: None,
reviewed: Vec::new(),
comments: Vec::new(),
reference_files: Vec::new(),
Expand Down
Loading