feat(push): replace cli-push-pipeline with takt-based push-runner (ADR-015) - #33
Conversation
…R-015) cli-push-pipeline.exe を takt ベースの cli-push-runner.exe に置き換え。 AI レビューを takt ワークフローで deterministic に制御し、 お願いベースの claude -p 呼び出しを廃止。 パイプライン構成: Stage 1: quality_gate — lint/test/build を Rust スレッドで並列実行 Stage 1.5: diff — jj diff -> .takt/review-diff.txt Stage 2: takt — AI レビュー (arch+security 並列) + fix loop + supervise Stage 3: push — jj git push post-push: cli-pr-monitor (現行チェーン維持) 主な変更: - src/cli-push-runner/: takt-test-vc の tools/push-runner/ をベースに移植 - .takt/: ワークフロー (pre-push-review.yaml) + レビュー facets 4種 - push-runner-config.toml: 品質ゲート 3並列グループ + diff + takt + push - package.json: takt devDep 追加, build/push スクリプト更新, review:ai 廃止 - hooks-config.toml: [push_pipeline] セクション削除 - ADR-015: 移行の意思決定を記録 次ステップ: cli-pr-monitor の takt 化 (docs/todo.md に記録)
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughpush パイプラインをリポジトリルートの Changes
Sequence Diagram(s)sequenceDiagram
actor User as User
participant Runner as cli-push-runner
participant QG as Quality Gate
participant Diff as Diff Stage
participant Takt as Takt Executor
participant Review as Reviewers
participant Fix as Fixer
participant Supervise as Supervisor
participant Git as Git Push
User->>Runner: pnpm push
activate Runner
Runner->>Diff: run_diff() (jj diff -r @ → .takt/review-diff.txt)
Diff-->>Runner: diff file written
Runner->>QG: run_quality_gate() (lint/test/build)
QG-->>Runner: quality gate result
Runner->>Takt: run_takt(pre-push-review)
activate Takt
Takt->>Review: start arch & security reviews (read .takt/review-diff.txt)
Review-->>Takt: approved / needs_fix
alt needs_fix
Takt->>Fix: execute fix step (edit, build/test)
Fix-->>Takt: fixes applied
Takt->>Review: re-review
Review-->>Takt: approved
end
Takt->>Supervise: supervise validation
Supervise-->>Takt: ready_to_push / escalate
Takt-->>Runner: workflow complete
deactivate Takt
Runner->>Git: run_push() (jj git push)
Git-->>Runner: push result
Runner-->>User: exit code (success/failure)
deactivate Runner
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/deploy-hooks.ts (1)
22-30:⚠️ Potential issue | 🟠 Major
cli-push-runner.exe配布に対してpush-runner-config.tomlの配布導線が不足していますLine 26 で新ランナーへ切り替わりましたが、現状の deploy では設定ファイルを対象プロジェクトへ用意しないため、ランナー起動時に設定読込失敗を起こし得ます(
src/cli-push-runner/src/config.rsの探索仕様に依存)。修正例(設定ファイルのコピー/初期配置を追加)
const SETTINGS_TEMPLATE = "settings.local.json.template"; +const PUSH_RUNNER_CONFIG = "push-runner-config.toml"; function deployTo(targetDir: string): boolean { const targetClaude = join(targetDir, ".claude"); @@ for (const exe of EXE_FILES) { @@ } + + const pushRunnerConfigSrc = join(ROOT, PUSH_RUNNER_CONFIG); + const pushRunnerConfigDest = join(targetDir, PUSH_RUNNER_CONFIG); + if (existsSync(pushRunnerConfigSrc) && !existsSync(pushRunnerConfigDest)) { + copyFile(pushRunnerConfigSrc, pushRunnerConfigDest); + logger.info(` generated: ${PUSH_RUNNER_CONFIG}`); + } const configDest = join(targetClaude, "hooks-config.toml");Also applies to: 92-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deploy-hooks.ts` around lines 22 - 30, The deploy currently lists cli-push-runner.exe in EXE_FILES but does not deploy its required configuration, so add logic in scripts/deploy-hooks.ts to copy or create push-runner-config.toml alongside cli-push-runner.exe into the target project deployment location (ensure the file location matches the lookup used by src/cli-push-runner/src/config.rs); update the deployment steps that iterate EXE_FILES (and the similar block around lines 92-100) to include copying push-runner-config.toml (or an initializer that writes a default config) whenever cli-push-runner.exe is deployed.
🧹 Nitpick comments (5)
.takt/facets/instructions/review-arch.md (1)
13-16: 必読 ADR に ADR-015 を明示追加してくださいアーキレビュー手順として、push-runner 移行後の最新方針(ADR-015)を必読に含めておく方が、旧前提での判定を防げます。
✍️ 追記案
1. `CLAUDE.md` -- Project overview and ADR index 2. `docs/adr/adr-012-src-naming-convention.md` -- Naming convention for src/ directory (hooks- / cli- / lib- prefixes) 3. `docs/adr/adr-010-hooks-layout-and-build-strategy-v2.md` -- Hooks layout and build strategy +4. `docs/adr/adr-015-push-runner-takt-migration.md` -- Push pipeline migration baselineBased on learnings: Use takt-based push-runner for Push Pipeline execution (ADR-015).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.takt/facets/instructions/review-arch.md around lines 13 - 16, The must-read ADR list is missing ADR-015; update the list that currently mentions `CLAUDE.md`, `docs/adr/adr-012-src-naming-convention.md`, and `docs/adr/adr-010-hooks-layout-and-build-strategy-v2.md` to also include `docs/adr/adr-015-*.md` (ADR-015) with a short descriptor like "ADR-015 — takt-based push-runner for Push Pipeline execution" so reviewers know to read the new post-migration policy; ensure the entry appears in the same ordered list near the other ADRs and, if present, add a one-line rationale referencing "Use takt-based push-runner for Push Pipeline execution (ADR-015)"..takt/facets/instructions/fix.md (1)
52-58: テーブルの前に空行を追加してください。markdownlint (MD058) によると、テーブルは空行で囲む必要があります。
修正案
## Convergence gate + | Metric | Count | |--------|-------|🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.takt/facets/instructions/fix.md around lines 52 - 58, The Markdown table under the "Convergence gate" header needs a blank line before it to satisfy markdownlint MD058; edit the block containing the header "## Convergence gate" and the following table rows (the table starting with "| Metric | Count |") and insert a single empty line between the header and the table (and optionally ensure there's an empty line after the table) so the table is properly surrounded by blank lines.src/cli-push-runner/src/runner.rs (1)
8-34: drain_pipe の MAX_LINES 超過時にログ通知を検討してください。現在、40 行を超える出力は静かに切り捨てられます。ユーザーが出力が切り詰められたことに気づかない可能性があります。これは diff.rs が独自の
run_diff_cmdを実装した理由でもあります。切り詰め通知の追加案
pub(crate) fn drain_pipe( pipe: impl std::io::Read + Send + 'static, ) -> std::thread::JoinHandle<String> { std::thread::spawn(move || { use std::io::BufRead; let mut reader = std::io::BufReader::new(pipe); let mut collected = Vec::with_capacity(MAX_LINES); let mut buf = Vec::new(); + let mut truncated = false; loop { buf.clear(); match reader.read_until(b'\n', &mut buf) { Ok(0) => break, Ok(_) => { if collected.len() < MAX_LINES { collected.push( String::from_utf8_lossy(&buf) .trim_end_matches(&['\r', '\n'][..]) .to_string(), ); + } else { + truncated = true; } } Err(_) => break, } } - collected.join("\n") + let mut result = collected.join("\n"); + if truncated { + result.push_str(&format!("\n... (出力が {} 行で切り詰められました)", MAX_LINES)); + } + result }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-push-runner/src/runner.rs` around lines 8 - 34, The drain_pipe function silently truncates output beyond MAX_LINES (used when collecting into collected), so modify drain_pipe to detect when more lines were read than MAX_LINES and emit a clear notification (e.g., via logging or appending a sentinel line) indicating truncation; specifically, inside the loop reading from reader in drain_pipe, track whether any further Ok(_) reads occurred after collected.len() reached MAX_LINES and then (before returning collected.join("\n")) add a final line like "[output truncated]" or call the logger with context (include MAX_LINES and which stream) so callers using the JoinHandle<String> or downstream code can detect truncated output.src/cli-push-runner/src/stages/diff.rs (1)
9-21: プラットフォーム依存性: Windows 専用の実装です。
cmd /cは Windows 専用です。Linux/macOS では動作しません。現時点では問題ないかもしれませんが、将来的にクロスプラットフォーム対応が必要な場合は、#[cfg(windows)]/#[cfg(unix)]による分岐、または shell-words クレートの使用を検討してください。また、コマンド失敗時に
stderrのみを返していますが、stdoutにも有用な診断情報が含まれる可能性があります。失敗時に stdout も含める修正案
if output.status.success() { Ok(String::from_utf8_lossy(&output.stdout).into_owned()) } else { - let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); - Err(stderr) + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + let combined = if stdout.is_empty() { + stderr.into_owned() + } else if stderr.is_empty() { + stdout.into_owned() + } else { + format!("{}\n{}", stdout, stderr) + }; + Err(combined) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-push-runner/src/stages/diff.rs` around lines 9 - 21, run_diff_cmd currently invokes Windows-only "cmd /c", which breaks on Unix; change it to branch by target OS (e.g., use #[cfg(windows)] for Command::new("cmd").args(["/c", cmd]) and #[cfg(unix)] for Command::new("sh").arg("-c").arg(cmd)) or replace with a cross-platform shell helper/crate, and when returning Err include both stdout and stderr (combine output.stdout and output.stderr into the error string) so diagnostic info from both streams is preserved; update run_diff_cmd to use these platform branches and to format the error with both streams and the original command.docs/adr/adr-015-push-runner-takt-migration.md (1)
34-44: コードブロックに言語指定を追加してください。markdownlint が指摘している通り、フェンスドコードブロックには言語を指定する必要があります。ASCII ダイアグラムの場合は
textまたはplaintextを使用できます。修正案
-``` +```text pnpm push = cli-push-runner.exe && cli-pr-monitor.exe --monitor-only🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/adr-015-push-runner-takt-migration.md` around lines 34 - 44, The fenced code block containing the ASCII diagram (starting with "pnpm push = cli-push-runner.exe && cli-pr-monitor.exe --monitor-only" and the following diagram lines) lacks a language specifier; update that fence to include a language token such as "text" or "plaintext" (e.g., replace ``` with ```text) so markdownlint is satisfied and the diagram is treated as plain text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.takt/workflows/pre-push-review.yaml:
- Line 180: The workflow references missing instruction facades "fix-supervisor"
(instruction: fix-supervisor) and "loop-monitor-reviewers-fix" but only has
existing facades (fix.md, review-arch.md, review-security.md, supervise.md);
create two new instruction files named to match those symbols and implement the
expected directives used by the workflow (one providing supervisor-fix steps for
fix-supervisor and one providing the loop/monitor reviewer remediation for
loop-monitor-reviewers-fix), ensure their filenames and exported instruction
names exactly match "fix-supervisor" and "loop-monitor-reviewers-fix" so the
workflow can resolve them at runtime, and follow the same structure/format as
the existing facades (e.g., fix.md and supervise.md) so integration is
consistent.
In `@src/cli-push-runner/src/stages/push.rs`:
- Line 8: The push stage is using a fixed DEFAULT_STEP_TIMEOUT_SECS causing
spurious failures; update the call to run_stage_cmd("push", &config.command,
...) to use a push-specific timeout (e.g. a new DEFAULT_PUSH_TIMEOUT_SECS) or a
timeout value read from the config (add a push_timeout field to the config
struct and use config.push_timeout.unwrap_or(DEFAULT_PUSH_TIMEOUT_SECS)); ensure
any new constant or config field is documented and used only for the "push"
stage so other stages keep the existing default.
---
Outside diff comments:
In `@scripts/deploy-hooks.ts`:
- Around line 22-30: The deploy currently lists cli-push-runner.exe in EXE_FILES
but does not deploy its required configuration, so add logic in
scripts/deploy-hooks.ts to copy or create push-runner-config.toml alongside
cli-push-runner.exe into the target project deployment location (ensure the file
location matches the lookup used by src/cli-push-runner/src/config.rs); update
the deployment steps that iterate EXE_FILES (and the similar block around lines
92-100) to include copying push-runner-config.toml (or an initializer that
writes a default config) whenever cli-push-runner.exe is deployed.
---
Nitpick comments:
In @.takt/facets/instructions/fix.md:
- Around line 52-58: The Markdown table under the "Convergence gate" header
needs a blank line before it to satisfy markdownlint MD058; edit the block
containing the header "## Convergence gate" and the following table rows (the
table starting with "| Metric | Count |") and insert a single empty line between
the header and the table (and optionally ensure there's an empty line after the
table) so the table is properly surrounded by blank lines.
In @.takt/facets/instructions/review-arch.md:
- Around line 13-16: The must-read ADR list is missing ADR-015; update the list
that currently mentions `CLAUDE.md`,
`docs/adr/adr-012-src-naming-convention.md`, and
`docs/adr/adr-010-hooks-layout-and-build-strategy-v2.md` to also include
`docs/adr/adr-015-*.md` (ADR-015) with a short descriptor like "ADR-015 —
takt-based push-runner for Push Pipeline execution" so reviewers know to read
the new post-migration policy; ensure the entry appears in the same ordered list
near the other ADRs and, if present, add a one-line rationale referencing "Use
takt-based push-runner for Push Pipeline execution (ADR-015)".
In `@docs/adr/adr-015-push-runner-takt-migration.md`:
- Around line 34-44: The fenced code block containing the ASCII diagram
(starting with "pnpm push = cli-push-runner.exe && cli-pr-monitor.exe
--monitor-only" and the following diagram lines) lacks a language specifier;
update that fence to include a language token such as "text" or "plaintext"
(e.g., replace ``` with ```text) so markdownlint is satisfied and the diagram is
treated as plain text.
In `@src/cli-push-runner/src/runner.rs`:
- Around line 8-34: The drain_pipe function silently truncates output beyond
MAX_LINES (used when collecting into collected), so modify drain_pipe to detect
when more lines were read than MAX_LINES and emit a clear notification (e.g.,
via logging or appending a sentinel line) indicating truncation; specifically,
inside the loop reading from reader in drain_pipe, track whether any further
Ok(_) reads occurred after collected.len() reached MAX_LINES and then (before
returning collected.join("\n")) add a final line like "[output truncated]" or
call the logger with context (include MAX_LINES and which stream) so callers
using the JoinHandle<String> or downstream code can detect truncated output.
In `@src/cli-push-runner/src/stages/diff.rs`:
- Around line 9-21: run_diff_cmd currently invokes Windows-only "cmd /c", which
breaks on Unix; change it to branch by target OS (e.g., use #[cfg(windows)] for
Command::new("cmd").args(["/c", cmd]) and #[cfg(unix)] for
Command::new("sh").arg("-c").arg(cmd)) or replace with a cross-platform shell
helper/crate, and when returning Err include both stdout and stderr (combine
output.stdout and output.stderr into the error string) so diagnostic info from
both streams is preserved; update run_diff_cmd to use these platform branches
and to format the error with both streams and the original command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d55f3bd4-918e-4b96-bc13-404fd334ca1c
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/cli-push-runner/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.claude/hooks-config.toml.takt/.gitignore.takt/facets/instructions/fix.md.takt/facets/instructions/review-arch.md.takt/facets/instructions/review-security.md.takt/facets/instructions/supervise.md.takt/workflows/pre-push-review.yamlCLAUDE.mddocs/adr/adr-015-push-runner-takt-migration.mddocs/todo.mdpackage.jsonpush-runner-config.tomlscripts/deploy-hooks.tssrc/cli-push-runner/Cargo.tomlsrc/cli-push-runner/src/config.rssrc/cli-push-runner/src/log.rssrc/cli-push-runner/src/main.rssrc/cli-push-runner/src/runner.rssrc/cli-push-runner/src/stages/diff.rssrc/cli-push-runner/src/stages/mod.rssrc/cli-push-runner/src/stages/push.rssrc/cli-push-runner/src/stages/quality_gate.rssrc/cli-push-runner/src/stages/takt.rs
460bf81 to
d013f1d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.takt/facets/instructions/fix-supervisor.md (2)
22-31: 出力セクションの見出し階層を揃えると読みやすくなります
## Required output配下のWork resultsは###にすると構造が一貫します(現状は同階層)。差分案
## Required output -## Work results +### Work results - {Summary of actions taken} ### Read-only zone compliance - {Confirm no writes attempted under read-only zones} -## Changes made +### Changes made - {File paths modified} -## Build results +### Build results - {Build execution results} -## Test results +### Test results - {Test results}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.takt/facets/instructions/fix-supervisor.md around lines 22 - 31, Adjust the heading levels so sections under "## Required output" are nested consistently: change the "Work results" and "Read-only zone compliance" headings (currently plain text lines) to "### Work results" and "### Read-only zone compliance" respectively, and also make "## Changes made" a "### Changes made" if it is intended to be under "## Required output"; update the corresponding headings in the document to match this hierarchy so all subsections beneath "## Required output" use "###".
14-14:src/限定により修正不能な指摘が発生する可能性がありますLine 14 の制約だと、supervisor の指摘が
Cargo.toml・package.json・CI設定などsrc/外に出た場合、fix_supervisor が恒久的に解消できずループ化するリスクがあります。原則 src/、ただし supervisor が明示した非 read-only パスは許可の例外を入れるのが安全です。差分案
-Fixes MUST target the **source tree under review**: files under `src/`. +Fixes SHOULD target the **source tree under review**: files under `src/`. +If the supervisor explicitly points to a non-read-only file outside `src/`, +that file MAY be edited to resolve the finding.Based on learnings 「Migrate Push Pipeline to takt-based push-runner as specified in ADR-015」.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.takt/facets/instructions/fix-supervisor.md at line 14, Update the rule that currently reads "Fixes MUST target the **source tree under review**: files under `src/`." so it allows supervisor-approved exceptions: change the wording to something like "Fixes should target the source tree under review (files under `src/`) except where the supervisor explicitly identifies non-read-only paths outside `src/` that must be modified." Ensure the new text preserves the default restriction, documents the exception procedure, and references the supervisor's explicit authorization requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli-push-runner/src/config.rs`:
- Around line 65-70: load_config currently only parses TOML and returns Config,
allowing quality_gate.groups[].commands to be an empty array which makes
stages/quality_gate.rs::run_group treat it as success; after toml::from_str(...)
and before returning, validate the semantics: iterate config.quality_gate.groups
(or the equivalent field on Config) and ensure each group's commands vector is
non-empty (and optionally that command strings are non-blank); if any group has
an empty commands list return Err with a clear message indicating which group is
invalid. Update load_config to perform this check and return the parsed Config
only when validation passes.
---
Nitpick comments:
In @.takt/facets/instructions/fix-supervisor.md:
- Around line 22-31: Adjust the heading levels so sections under "## Required
output" are nested consistently: change the "Work results" and "Read-only zone
compliance" headings (currently plain text lines) to "### Work results" and "###
Read-only zone compliance" respectively, and also make "## Changes made" a "###
Changes made" if it is intended to be under "## Required output"; update the
corresponding headings in the document to match this hierarchy so all
subsections beneath "## Required output" use "###".
- Line 14: Update the rule that currently reads "Fixes MUST target the **source
tree under review**: files under `src/`." so it allows supervisor-approved
exceptions: change the wording to something like "Fixes should target the source
tree under review (files under `src/`) except where the supervisor explicitly
identifies non-read-only paths outside `src/` that must be modified." Ensure the
new text preserves the default restriction, documents the exception procedure,
and references the supervisor's explicit authorization requirement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 151462bb-5181-4c12-bc9e-516ef4ea50ef
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.takt/.gitignore.takt/facets/instructions/fix-supervisor.md.takt/facets/instructions/loop-monitor-reviewers-fix.mdpackage.jsonsrc/cli-push-runner/src/config.rssrc/cli-push-runner/src/stages/push.rs
✅ Files skipped from review due to trivial changes (3)
- .takt/.gitignore
- .takt/facets/instructions/loop-monitor-reviewers-fix.md
- src/cli-push-runner/src/stages/push.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
空 groups / 空 commands を設定エラーとして検出する validate_config() を追加。 設定ミスで品質ゲートが素通りになるリスクを防止。
d013f1d to
4792df6
Compare
Summary
Summary by CodeRabbit
New Features
Infrastructure Updates
Configuration
Documentation