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
66 changes: 65 additions & 1 deletion src/cli-merge-pipeline/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,71 @@ mod feedback;
mod github;
mod pipeline;

/// `--feedback-only <PR>` を解析する。該当しない場合は None (通常 pipeline)。
///
/// ADR-030 recovery の補完: pipeline が feedback step 到達前に失敗して `.failed` marker が
/// 残らないケース (PR #267 で実観測) の手動再実行用。
fn parse_feedback_only(args: &[String]) -> Option<Result<u64, String>> {
if args.first().map(String::as_str) != Some("--feedback-only") {
return None;
}
let Some(raw) = args.get(1) else {
return Some(Err(
"usage: cli-merge-pipeline --feedback-only <PR番号>".to_string()
));
};
Some(raw.parse::<u64>().map_err(|_| {
format!(
"PR 番号が不正です: {} (usage: --feedback-only <PR番号>)",
raw
)
}))
}

fn main() {
lib_jj_helpers::inject_git_dir_for_gh(pipeline::log_info);
std::process::exit(pipeline::run_pipeline());
let args: Vec<String> = std::env::args().skip(1).collect();
let code = match parse_feedback_only(&args) {
Some(Ok(pr_number)) => pipeline::run_feedback_only(pr_number),
Some(Err(message)) => {
eprintln!("{message}");
2
}
None => pipeline::run_pipeline(),
};
std::process::exit(code);
}

#[cfg(test)]
mod tests {
use super::*;

fn args(list: &[&str]) -> Vec<String> {
list.iter().map(|s| s.to_string()).collect()
}

#[test]
fn no_args_runs_normal_pipeline() {
assert!(parse_feedback_only(&args(&[])).is_none());
}

#[test]
fn feedback_only_parses_pr_number() {
assert_eq!(
parse_feedback_only(&args(&["--feedback-only", "267"])),
Some(Ok(267))
);
}

#[test]
fn feedback_only_without_number_is_usage_error() {
let result = parse_feedback_only(&args(&["--feedback-only"])).unwrap();
assert!(result.unwrap_err().contains("usage"));
}

#[test]
fn feedback_only_with_invalid_number_is_error() {
let result = parse_feedback_only(&args(&["--feedback-only", "abc"])).unwrap();
assert!(result.unwrap_err().contains("abc"));
}
}
151 changes: 101 additions & 50 deletions src/cli-merge-pipeline/src/pipeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::github::{
should_skip_branch_delete, PrHeadInfo,
};
use lib_subprocess::run_cmd_shell_capped_reporting;
use std::path::{Path, PathBuf};

pub(crate) fn log_step(name: &str, status: &str, message: &str) {
if message.is_empty() {
Expand Down Expand Up @@ -190,30 +191,76 @@ fn run_ai_step(label: &str, ctx: Option<&PipelineContext>) {
AiStepContext::Ready {
pr_number,
owner_repo,
} => run_ai_step_for(label, pr_number, owner_repo),
} => drop(run_ai_step_for(label, pr_number, owner_repo)),
AiStepContext::SkipSilent => {}
AiStepContext::SkipWithMarker { pr_number, reason } => {
skip_with_failed_marker(label, pr_number, &reason);
}
}
}

/// 検証済みコンテキストで feedback workflow を実行する ([`run_ai_step`] の本体)。
fn run_ai_step_for(label: &str, pr_number: u64, owner_repo: &str) {
let repo_root = match std::env::current_dir() {
Ok(p) => p,
Err(e) => {
/// 手動 recovery 用エントリポイント (`cli-merge-pipeline --feedback-only <PR>`)。
///
/// merge pipeline が post_merge_feedback step の**到達前**に失敗した場合 (例: ローカル
/// 同期の concurrent checkout 中断、PR #267 で実観測)、`.failed` marker が書かれず
/// ADR-030 L2 recovery の対象にならない。本経路は marker の有無に依存せず feedback
/// workflow を単独で再実行する。マージ済み PR の番号を明示指定する前提。
///
/// 終了コード: 0 = report 生成成功、1 = 失敗 (marker は通常経路と同様に残る)。
pub(crate) fn run_feedback_only(pr_number: u64) -> i32 {
let label = "feedback-only";
let Some(owner_repo) = detect_owner_repo() else {
log_step(
label,
"FAIL",
"owner_repo を取得できませんでした (gh repo view 失敗?)",
);
return 1;
};
if !lib_pending_file::is_valid_owner_repo(&owner_repo) {
log_step(label, "FAIL", &format!("owner_repo が不正: {}", owner_repo));
return 1;
}
Comment on lines +209 to +223

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

owner_repo 検出/検証失敗時に .failed marker が書かれない — docstring の約束と不一致

Line 209 のdocstringは「1 = 失敗 (marker は通常経路と同様に残る)」と明記していますが、実際の owner_repo 失敗パス (Line 212-223) は log_step(..., "FAIL", ...) のみで、通常経路の skip_with_failed_marker (Line 157-182 相当) が行う .failed marker 書込を行っていません。PR目的の「failure-marker handling as the automatic workflow」の再利用にも反しており、L2 recovery hookがこのケースを検知できなくなります。

既存の skip_with_failed_marker を呼び出すだけで解消できます。

🐛 修正案
 pub(crate) fn run_feedback_only(pr_number: u64) -> i32 {
     let label = "feedback-only";
     let Some(owner_repo) = detect_owner_repo() else {
-        log_step(
-            label,
-            "FAIL",
-            "owner_repo を取得できませんでした (gh repo view 失敗?)",
-        );
+        skip_with_failed_marker(
+            label,
+            pr_number,
+            "owner_repo を取得できませんでした (gh repo view 失敗?)",
+        );
         return 1;
     };
     if !lib_pending_file::is_valid_owner_repo(&owner_repo) {
-        log_step(label, "FAIL", &format!("owner_repo が不正: {}", owner_repo));
+        skip_with_failed_marker(
+            label,
+            pr_number,
+            &format!("owner_repo が不正: {}", owner_repo),
+        );
         return 1;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// 終了コード: 0 = report 生成成功、1 = 失敗 (marker は通常経路と同様に残る)。
pub(crate) fn run_feedback_only(pr_number: u64) -> i32 {
let label = "feedback-only";
let Some(owner_repo) = detect_owner_repo() else {
log_step(
label,
"FAIL",
"owner_repo を取得できませんでした (gh repo view 失敗?)",
);
return 1;
};
if !lib_pending_file::is_valid_owner_repo(&owner_repo) {
log_step(label, "FAIL", &format!("owner_repo が不正: {}", owner_repo));
return 1;
}
/// 終了コード: 0 = report 生成成功、1 = 失敗 (marker は通常経路と同様に残る)。
pub(crate) fn run_feedback_only(pr_number: u64) -> i32 {
let label = "feedback-only";
let Some(owner_repo) = detect_owner_repo() else {
skip_with_failed_marker(
label,
pr_number,
"owner_repo を取得できませんでした (gh repo view 失敗?)",
);
return 1;
};
if !lib_pending_file::is_valid_owner_repo(&owner_repo) {
skip_with_failed_marker(
label,
pr_number,
&format!("owner_repo が不正: {}", owner_repo),
);
return 1;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli-merge-pipeline/src/pipeline.rs` around lines 209 - 223, Update the
owner_repo detection and validation failure branches in run_feedback_only to
call the existing skip_with_failed_marker flow, preserving the current failure
logging and return code while ensuring the .failed marker is written
consistently with the automatic workflow.


match run_ai_step_for(label, pr_number, &owner_repo) {
Ok(report) => {
log_step(
label,
"WARN",
&format!("current_dir 取得失敗: {} — feedback workflow をスキップ", e),
"PASS",
&format!("feedback report: {}", report.display()),
);
return;
0
}
};
Err(reason) => {
log_step(
label,
"FAIL",
&format!("{} (詳細は上記ログ / .failed marker)", reason),
);
1
}
Comment on lines +234 to +241

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

FAILメッセージが .failed marker の存在を暗示するが、実際には書かれないケースがある

Err(reason) は current_dir 取得失敗や trivial PR skip (ai_step_should_skip_trivial) からも返りますが、これらのケースでは marker は書かれません (marker書込があるのは feedback::run 失敗時の warn_feedback_failure 経路のみ)。汎用メッセージが常に「.failed marker」を参照すると、復旧作業時に存在しないファイルを探させてしまいます。

💬 修正案
         Err(reason) => {
             log_step(
                 label,
                 "FAIL",
-                &format!("{} (詳細は上記ログ / .failed marker)", reason),
+                &format!("{} (詳細は上記ログを参照)", reason),
             );
             1
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Err(reason) => {
log_step(
label,
"FAIL",
&format!("{} (詳細は上記ログ / .failed marker)", reason),
);
1
}
Err(reason) => {
log_step(
label,
"FAIL",
&format!("{} (詳細は上記ログを参照)", reason),
);
1
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli-merge-pipeline/src/pipeline.rs` around lines 234 - 241, Update the
Err(reason) handling in the pipeline result reporting around log_step so the
failure message no longer always references a .failed marker. Keep the reason
and existing FAIL status, but only mention the marker when the failure
originated from the feedback::run/warn_feedback_failure path; otherwise report a
generic failure without directing recovery to a nonexistent file.

}
}

/// 検証済みコンテキストで feedback workflow を実行する ([`run_ai_step`] の本体)。
///
/// 戻り値は `feedback::run` 相当の実行結果 (`Ok` = 生成された report のパス、
/// `Err` = 失敗理由)。trivial PR skip・`current_dir` 取得失敗も `Err` として返し、
/// [`run_feedback_only`] がディスク上の stale ファイルではなく今回の実行結果で
/// 判定できるようにする (SIM-NEW-pipeline-L224)。
fn run_ai_step_for(label: &str, pr_number: u64, owner_repo: &str) -> Result<PathBuf, String> {
let repo_root = std::env::current_dir().map_err(|e| {
let reason = format!("current_dir 取得失敗: {}", e);
log_step(
label,
"WARN",
&format!("{} — feedback workflow をスキップ", reason),
);
reason
})?;

if ai_step_should_skip_trivial(label, pr_number, owner_repo) {
return;
if let Some(reason) = ai_step_should_skip_trivial(label, pr_number, owner_repo) {
return Err(reason);
}

let transcript_source_dir = feedback::project_transcript_dir(&repo_root);
Expand Down Expand Up @@ -241,72 +288,76 @@ fn run_ai_step_for(label: &str, pr_number: u64, owner_repo: &str) {
),
);

run_feedback_and_report(label, &input, &repo_root, pr_number);
run_feedback_and_report(label, &input, &repo_root, pr_number)
}

/// trivial PR (#A-2) なら true を返し SKIP ログを出す。判定失敗時は WARN + false
fn ai_step_should_skip_trivial(label: &str, pr_number: u64, owner_repo: &str) -> bool {
/// trivial PR (#A-2) なら `Some(reason)` を返し SKIP ログを出す。判定失敗時は WARN + `None`
fn ai_step_should_skip_trivial(label: &str, pr_number: u64, owner_repo: &str) -> Option<String> {
match feedback::fetch_pr_diff_summary(pr_number, owner_repo) {
Ok(summary) if summary.is_trivial() => {
log_step(
label,
"SKIP",
&format!(
"trivial PR (commits={}, lines={}, all_md={}) — \
post-merge-feedback skip (#A-2)",
summary.commit_count,
summary.total_lines_changed,
summary.all_files_are_markdown,
),
let reason = format!(
"trivial PR (commits={}, lines={}, all_md={}) — post-merge-feedback skip (#A-2)",
summary.commit_count, summary.total_lines_changed, summary.all_files_are_markdown,
);
true
log_step(label, "SKIP", &reason);
Some(reason)
}
Ok(_) => false,
Ok(_) => None,
Err(e) => {
log_step(
label,
"WARN",
&format!("trivial PR 判定失敗: {} — 通常 flow で続行", e),
);
false
None
}
}
}

/// `feedback::run` を実行し、結果に応じて PASS / WARN(+marker) をログ出力する。
/// `feedback::run` 失敗時に `.failed` marker を書き込み WARN ログを出す。
fn warn_feedback_failure(label: &str, repo_root: &Path, pr_number: u64, reason: &str) {
match feedback::write_failed_marker(repo_root, pr_number, reason) {
Ok(marker) => log_step(
label,
"WARN",
&format!(
"feedback workflow 失敗: {} — marker: {} (L2 recovery が拾います)",
reason,
marker.display()
),
),
Err(marker_err) => log_step(
label,
"WARN",
&format!(
"feedback workflow 失敗: {} — marker 書込も失敗: {}",
reason, marker_err
),
),
}
}

/// `feedback::run` を実行し、結果に応じて PASS / WARN(+marker) をログ出力した上で、
/// 実行結果をそのまま返す (呼び出し元が実際の結果で判定するため。SIM-NEW-pipeline-L224)。
fn run_feedback_and_report(
label: &str,
input: &feedback::FeedbackInput,
repo_root: &std::path::Path,
repo_root: &Path,
pr_number: u64,
) {
) -> Result<PathBuf, String> {
match feedback::run(input) {
Ok(report) => {
log_step(
label,
"PASS",
&format!("feedback report 生成: {}", report.display()),
);
Ok(report)
}
Err(reason) => {
warn_feedback_failure(label, repo_root, pr_number, &reason);
Err(reason)
}
Err(reason) => match feedback::write_failed_marker(repo_root, pr_number, &reason) {
Ok(marker) => log_step(
label,
"WARN",
&format!(
"feedback workflow 失敗: {} — marker: {} (L2 recovery が拾います)",
reason,
marker.display()
),
),
Err(marker_err) => log_step(
label,
"WARN",
&format!(
"feedback workflow 失敗: {} — marker 書込も失敗: {}",
reason, marker_err
),
),
},
}
}

Expand Down
Loading