From fdad0583b95ff5ea01378de545dd52a1c2f7d373 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 6 May 2026 15:32:48 +1000 Subject: [PATCH 1/4] fix: include base branch in review prompt diff command The review prompt hardcoded `origin/main` in the example git diff command, which is wrong for branches based on other branches (e.g. develop, release/v2). Thread the actual base_branch through build_full_prompt() so the review agent gets the correct merge-base ref. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/session_commands.rs | 175 +++++++++++------- 1 file changed, 106 insertions(+), 69 deletions(-) diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 52eb20e2..b8777ca5 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -750,6 +750,7 @@ pub async fn start_branch_session( &branch_context, &session_type, launch_context.as_ref(), + Some(&branch.base_branch), ); // Create the session @@ -1150,6 +1151,7 @@ pub async fn drain_queued_sessions_for_branch( &branch_context, &session_type, launch_context.as_ref(), + Some(&branch.base_branch), ); // Atomically transition session from queued to running. @@ -1494,6 +1496,7 @@ pub async fn trigger_auto_review( &branch_context, &BranchSessionType::Review, None, + Some(&branch.base_branch), ); // Create the session @@ -2681,6 +2684,7 @@ fn build_full_prompt( branch_context: &str, session_type: &BranchSessionType, launch_context: Option<&BranchSessionLaunchContext>, + base_branch: Option<&str>, ) -> String { let action_instructions = match session_type { BranchSessionType::Note => { @@ -2724,7 +2728,7 @@ Formatting requirements: - Do not wrap the block in any additional code fences. - `---` must be on its own line, with a newline immediately before and after it. - The note content must start immediately after `---` with a markdown H1 (`# Title`). -- Do not wrap the note in code fences." +- Do not wrap the note in code fences.".to_string() } BranchSessionType::Commit => { "The user is requesting you make a commit based on the prompt below. Make the necessary \ @@ -2736,92 +2740,94 @@ Before creating the commit: - Use the user's global git identity (`git config --global user.name` and `git config --global user.email`) \ for both the author and committer. Do not use placeholder identities. - Create the commit with a DCO signoff (`git commit --signoff`) so the commit message includes a \ -matching `Signed-off-by` trailer." +matching `Signed-off-by` trailer.".to_string() } BranchSessionType::Review => { - "The user is requesting an AI code review of the current branch. - -Review the code changes on this branch by running a diff from the remote-tracking base ref, \ -for example `git diff $(git merge-base origin/main HEAD)..HEAD`. Do not compare against the \ -local base branch, which may be stale. - -Do NOT create any commits or modify any files. - -## Review philosophy - + let base = base_branch.unwrap_or("main"); + format!( + "The user is requesting an AI code review of the current branch.\n\ +\n\ +Review the code changes on this branch by running a diff from the remote-tracking base ref: \ +`git diff $(git merge-base origin/{base} HEAD)..HEAD`. Do not compare against the \ +local base branch, which may be stale.\n\ +\n\ +Do NOT create any commits or modify any files.\n\ +\n\ +## Review philosophy\n\ +\n\ Your comments should tell the story of the change — focus on the \"why\", potential issues, \ and non-obvious implications. Do NOT exhaustively document every line or restate what the code \ obviously does. It's fine to have no comments for trivial or self-explanatory files. \ -Aim for quality over quantity: a few insightful comments are better than many shallow ones. - -## Comment types - -Each comment MUST include a `type` field. Choose the type carefully: - +Aim for quality over quantity: a few insightful comments are better than many shallow ones.\n\ +\n\ +## Comment types\n\ +\n\ +Each comment MUST include a `type` field. Choose the type carefully:\n\ +\n\ - `\"information\"` — Contextual explanation, \"why\" behind a change, or architectural observation. \ Use this for comments that help a reader understand the change but don't require action. \ These are shown as subtle hold-to-reveal annotations, not inline comments. \ Examples: explaining a non-obvious design decision, noting how a change fits into the broader architecture, \ -describing what the old code was doing and why it changed. - +describing what the old code was doing and why it changed.\n\ +\n\ - `\"suggestion\"` — A recommended improvement that isn't strictly necessary. \ The code works but could be better. \ -Examples: a more idiomatic approach, better naming, a simplification. - +Examples: a more idiomatic approach, better naming, a simplification.\n\ +\n\ - `\"warning\"` — A potential issue or concern that deserves attention. \ Not a definite bug, but something that could cause problems. \ -Examples: missing edge case handling, potential performance issue, fragile assumption. - +Examples: missing edge case handling, potential performance issue, fragile assumption.\n\ +\n\ - `\"issue\"` — A bug or correctness problem that should be fixed. \ -Examples: off-by-one error, null pointer risk, logic error, security vulnerability. - +Examples: off-by-one error, null pointer risk, logic error, security vulnerability.\n\ +\n\ Most comments in a typical review should be `\"information\"` or `\"suggestion\"`. \ -Reserve `\"warning\"` and `\"issue\"` for genuine concerns. - -## Output format - +Reserve `\"warning\"` and `\"issue\"` for genuine concerns.\n\ +\n\ +## Output format\n\ +\n\ Your response must start directly with the review-title fenced block below — \ -do not output any preamble, commentary, or thinking before it. - +do not output any preamble, commentary, or thinking before it.\n\ +\n\ 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 -[ - { - \"path\": \"src/foo.ts\", - \"span\": { \"start\": 10, \"end\": 15 }, - \"type\": \"information\", - \"content\": \"This refactors the error handling from panicking to returning Results, which aligns with the broader error-handling migration across the codebase.\" - }, - { - \"path\": \"src/bar.rs\", - \"span\": { \"start\": 42, \"end\": 45 }, - \"type\": \"warning\", - \"content\": \"This unwrap() could panic if the connection pool is exhausted under load.\" - } -] -``` - -Formatting requirements: -- 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). -- Only comment on changed files. -- Be specific and actionable — reference the actual code, not generic advice." +how confident you are that they are correct and safe. Wrap it in a fenced block:\n\ +\n\ +```review-title\n\ +Looks solid overall with one minor edge case worth checking\n\ +```\n\ +\n\ +Then return your review comments as exactly one fenced JSON block:\n\ +\n\ +```review-comments\n\ +[\n\ + {{\n\ + \"path\": \"src/foo.ts\",\n\ + \"span\": {{ \"start\": 10, \"end\": 15 }},\n\ + \"type\": \"information\",\n\ + \"content\": \"This refactors the error handling from panicking to returning Results, which aligns with the broader error-handling migration across the codebase.\"\n\ + }},\n\ + {{\n\ + \"path\": \"src/bar.rs\",\n\ + \"span\": {{ \"start\": 42, \"end\": 45 }},\n\ + \"type\": \"warning\",\n\ + \"content\": \"This unwrap() could panic if the connection pool is exhausted under load.\"\n\ + }}\n\ +]\n\ +```\n\ +\n\ +Formatting requirements:\n\ +- The opening fence line for the title must be exactly: ```review-title\n\ +- The opening fence line for comments must be exactly: ```review-comments\n\ +- Each closing fence line must be exactly: ```\n\ +- Put only plain text (no markdown) inside the review-title block.\n\ +- Put only the JSON array inside the review-comments block (no prose or markdown).\n\ +- Do not wrap these blocks in any additional code fences.\n\ +\n\ +Rules:\n\ +- `span` uses 0-indexed line numbers from the \"after\" side of the diff (exclusive end).\n\ +- Only comment on changed files.\n\ +- Be specific and actionable — reference the actual code, not generic advice.") } }; @@ -3062,6 +3068,7 @@ mod tests { "branch context", &BranchSessionType::Review, None, + None, ); assert!( @@ -3078,6 +3085,33 @@ mod tests { assert!(prompt.contains("do not output any preamble, commentary, or thinking before it")); } + #[test] + fn review_prompt_includes_base_branch_in_diff_command() { + let prompt = build_full_prompt( + "user prompt", + "project info", + "branch context", + &BranchSessionType::Review, + None, + Some("develop"), + ); + assert!(prompt.contains("git merge-base origin/develop HEAD")); + assert!(!prompt.contains("origin/main")); + } + + #[test] + fn review_prompt_defaults_to_main_when_no_base_branch() { + let prompt = build_full_prompt( + "user prompt", + "project info", + "branch context", + &BranchSessionType::Review, + None, + None, + ); + assert!(prompt.contains("git merge-base origin/main HEAD")); + } + #[test] fn review_timeline_entries_exclude_information_comments() { let (store, branch) = setup_branch_store(); @@ -3272,6 +3306,7 @@ mod tests { commit_sha: "abc123".to_string(), review_id: Some("review-42".to_string()), }), + None, ); assert!(prompt.contains( @@ -3287,6 +3322,7 @@ mod tests { "branch context", &BranchSessionType::Commit, None, + None, ); assert!(prompt.contains("Use the user's global git identity")); @@ -3307,6 +3343,7 @@ mod tests { commit_sha: "abc123".to_string(), review_id: None, }), + None, ); assert!(prompt.contains("Viewed diff before starting this session: commit abc123.")); From bdc4e3fd1180665b7740978cbc7d863f953e2c5d Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 6 May 2026 17:01:40 +1000 Subject: [PATCH 2/4] fix: normalize base branch ref in review prompt diff command Use git::origin_ref_for_branch() to normalize the base branch before formatting the diff command, preventing double origin/ prefix when the stored base branch already includes origin/ (e.g. origin/main becoming origin/origin/main). Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/session_commands.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index b8777ca5..3c24e117 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -2743,12 +2743,12 @@ for both the author and committer. Do not use placeholder identities. matching `Signed-off-by` trailer.".to_string() } BranchSessionType::Review => { - let base = base_branch.unwrap_or("main"); + let base_ref = git::origin_ref_for_branch(base_branch.unwrap_or("main")); format!( "The user is requesting an AI code review of the current branch.\n\ \n\ Review the code changes on this branch by running a diff from the remote-tracking base ref: \ -`git diff $(git merge-base origin/{base} HEAD)..HEAD`. Do not compare against the \ +`git diff $(git merge-base {base_ref} HEAD)..HEAD`. Do not compare against the \ local base branch, which may be stale.\n\ \n\ Do NOT create any commits or modify any files.\n\ From 07d2c64dd832b2a45bd0fedc7bbbca780dc5f998 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 6 May 2026 17:20:19 +1000 Subject: [PATCH 3/4] fix: shell-quote base ref in review prompt diff command Single-quote the base_ref in the git merge-base command to prevent shell metacharacter injection from branch names containing special characters (e.g. ;, $(), '). Also add a test case for Some("origin/main") to pin the normalization behavior and ensure origin/main does not become origin/origin/main in the rendered prompt. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/session_commands.rs | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 3c24e117..2e9c8488 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -2748,7 +2748,7 @@ matching `Signed-off-by` trailer.".to_string() "The user is requesting an AI code review of the current branch.\n\ \n\ Review the code changes on this branch by running a diff from the remote-tracking base ref: \ -`git diff $(git merge-base {base_ref} HEAD)..HEAD`. Do not compare against the \ +`git diff $(git merge-base '{base_ref}' HEAD)..HEAD`. Do not compare against the \ local base branch, which may be stale.\n\ \n\ Do NOT create any commits or modify any files.\n\ @@ -3095,7 +3095,7 @@ mod tests { None, Some("develop"), ); - assert!(prompt.contains("git merge-base origin/develop HEAD")); + assert!(prompt.contains("git merge-base 'origin/develop' HEAD")); assert!(!prompt.contains("origin/main")); } @@ -3109,7 +3109,22 @@ mod tests { None, None, ); - assert!(prompt.contains("git merge-base origin/main HEAD")); + assert!(prompt.contains("git merge-base 'origin/main' HEAD")); + } + + #[test] + fn review_prompt_normalizes_origin_prefixed_base_branch() { + let prompt = build_full_prompt( + "user prompt", + "project info", + "branch context", + &BranchSessionType::Review, + None, + Some("origin/main"), + ); + // origin/main should NOT become origin/origin/main + assert!(prompt.contains("git merge-base 'origin/main' HEAD")); + assert!(!prompt.contains("origin/origin/main")); } #[test] From 157e3e48b1113d4f2cd77908e44099d826abf11c Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 6 May 2026 19:36:03 +1000 Subject: [PATCH 4/4] fix: shell-escape review prompt base ref Quote the normalized base ref with single-quote escaping before embedding it in the suggested git merge-base command, so branch names containing apostrophes render as a valid shell argument. Add coverage for a feature/it's-good base branch. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/session_commands.rs | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 2e9c8488..2e4debd5 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -2677,6 +2677,10 @@ fn image_timeline_entries( .collect() } +fn shell_quote_arg(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) +} + /// Assemble the full prompt from action instructions + branch context + user prompt. fn build_full_prompt( user_prompt: &str, @@ -2744,11 +2748,12 @@ matching `Signed-off-by` trailer.".to_string() } BranchSessionType::Review => { let base_ref = git::origin_ref_for_branch(base_branch.unwrap_or("main")); + let quoted_base_ref = shell_quote_arg(&base_ref); format!( "The user is requesting an AI code review of the current branch.\n\ \n\ Review the code changes on this branch by running a diff from the remote-tracking base ref: \ -`git diff $(git merge-base '{base_ref}' HEAD)..HEAD`. Do not compare against the \ +`git diff $(git merge-base {quoted_base_ref} HEAD)..HEAD`. Do not compare against the \ local base branch, which may be stale.\n\ \n\ Do NOT create any commits or modify any files.\n\ @@ -3127,6 +3132,20 @@ mod tests { assert!(!prompt.contains("origin/origin/main")); } + #[test] + fn review_prompt_shell_quotes_base_branch_with_single_quote() { + let prompt = build_full_prompt( + "user prompt", + "project info", + "branch context", + &BranchSessionType::Review, + None, + Some("feature/it's-good"), + ); + assert!(prompt.contains("git merge-base 'origin/feature/it'\\''s-good' HEAD")); + assert!(!prompt.contains("git merge-base 'origin/feature/it's-good' HEAD")); + } + #[test] fn review_timeline_entries_exclude_information_comments() { let (store, branch) = setup_branch_store();