fix(cli-push-runner): push 拒否検知の 40 行 truncate 依存を修正 (push T5) - #282
Merged
Conversation
jj は新規 bookmark の push を拒否するとき、エラー終了せず "Refusing to create new remote bookmark" を出力して exit 0 を返す。push stage はこれを 出力の部分一致で検知していたが、判定対象が run_stage_cmd (= run_cmd_shell_capped、MAX_LINES=40 の silent truncate) の出力だったため、 jj の出力が 40 行を超えて拒否行が cap の外へ落ちると拒否を見逃し、 リモート未反映のまま exit 0 で完了する。後続の cli-pr-monitor は旧 head を 監視し始める。 lib-subprocess の doc は「control flow 判定に出力を使う callsite で capped variant を使うな」と当初から明記していたが、正しい variant (unlimited) が run_cmd_shell family に存在せず、callsite は間違った variant を選んでいた。 変更: - lib-subprocess: run_cmd_shell_unlimited を追加。drain_pipe_unlimited は pipe 単体、_capped_reporting は cap が残るため、どちらも判定用には不足。 3 つ目の copy になる骨格 (spawn → drain → wait → combine) は run_cmd_shell_with に集約し、各 variant は drain 戦略の違いだけを表す。 境界判定は ADR-044 § 後続の variant 追加 に記録。 - push stage: run_push_cmd (unlimited) で全量取得し、push_was_refused は 全量出力に対して判定する。表示は成功時のみ cap_for_log (先頭 40 行 + 超過明示) を通し、失敗経路は全量表示 (診断情報を落とさない)。成功時の ログ量は従来どおり。 - runner::run_stage_cmd を削除。push stage が唯一の呼び出し元だったため 未使用になった。dead code 除去に加え、capped 経路で control flow 判定を する罠の構造的排除。MAX_LINES は表示用として残置し doc に判定禁止を明記。 contains(\"refusing to\") の厳格化は見送り (ユーザー承認済み)。誤検知は出力 表示で気付いて再実行できるが、検知漏れは本 PR が直す事故そのものでリスクが 非対称なため、ADR-043 (fail-closed) に従い部分一致を維持する。 検証: - 回帰テスト mod t5_truncated_refusal_detection 6 本 + lib-subprocess 4 本 (ADR-049 の流儀。bad = 41 行目の拒否行を検知 / good = 40 行超の正常出力を 誤検知しない / 表示 cap は判定に影響しない)。run_push_cmd を capped に 戻すと 3 本が fail することを確認済み (回帰テストが素通りしない実証)。 - サンドボックスの jj repo で配布 exe と修正後 exe を比較。拒否行を 41 行目に 置いた fake push command で before = [push] 成功 + exit 0 (silent failure 再現) / after = 拒否検知 + exit 3。成功経路 (50 行) は 40 行 + "... (10 lines truncated)" 表示で exit 0 を維持。 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesPush output handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant run_push
participant run_push_cmd
participant run_cmd_shell_unlimited
participant push_was_refused
participant cap_for_log
run_push->>run_push_cmd: pushを実行して全量出力を取得
run_push_cmd->>run_cmd_shell_unlimited: unlimited drainでコマンド実行
run_cmd_shell_unlimited-->>run_push_cmd: 全量出力を返す
run_push->>push_was_refused: 全量出力で拒否判定
run_push->>cap_for_log: 表示用出力をcap
cap_for_log-->>run_push: 表示用出力を返す
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)該当なし Applicable Findings (Medium 以下)該当なし Filtered (not applicable)該当なし Diff 概要 (軽量サマリー)
次のアクション
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/lib-subprocess/src/lib.rs`:
- Line 287: wait_with_timeout_basic を使用する旨の説明を、Err 経路で child が kill/reap される実際の
semantics に合わせて修正してください。特に kill_and_join_err による処理を反映し、Err 経路では child を kill
しないという誤った記述を削除してください。
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a89fd615-1928-4304-a088-fdd40f57794d
📒 Files selected for processing (5)
docs/adr/adr-044-subprocess-utility-extraction-boundary.mddocs/push-pipeline-fix-plan.mdsrc/cli-push-runner/src/runner.rssrc/cli-push-runner/src/stages/push.rssrc/lib-subprocess/src/lib.rs
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)該当なし Applicable Findings (Medium 以下)
Filtered (not applicable)該当なし 次のアクション
|
…h T5) CodeRabbit Minor (src/lib-subprocess/src/lib.rs): run_cmd_shell_capped の doc が「Err 経路で child を kill しない basic semantics」と書いていたが、実際は Err 経路を kill_and_join_err が受けて child を kill + reap し reader thread も join する。wait_with_timeout_basic 単体の性質としては正しい記述が、kill_and_join_err 導入 (PR #208) 以降 stale に なっていた pre-existing の不整合。child lifecycle は 3 variant 共通なので、 記述を共通骨格 run_cmd_shell_with の doc に集約し、variant 側は参照のみにした。 pre-push simplicity 警告 (非ブロッキング、採用): - cap_for_log の "... (N lines truncated)" 書式が drain_pipe_capped_reporting と重複していた。切り詰めの実装自体は共有できない (pipe を streaming しながら 数える版 vs materialize 済み文字列を切る版) が、書式片は共有できるという指摘は 妥当なので lib_subprocess::truncation_notice として切り出し、両者から使う形に した。単数/複数形の unit test 2 本を追加。 - 計画書 T5 行の「実装済 (本 PR)」を「実装済 (PR #282)」に backfill。T4 行が 「本 PR」のまま放置され PR #282 で backfill する羽目になった負債を、同じ形で 繰り返さないため。 検証: cargo test -p cli-push-runner 206 pass / -p lib-subprocess 33 pass、 cargo clippy --workspace --all-targets --all-features -- -D warnings で warning 0。 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)該当なし Applicable Findings (Medium 以下)該当なし (唯一の指摘は解決済み — 下記参照) Filtered (not applicable)該当なし 解決済み指摘 (参考)
次のアクション
|
This was referenced Jul 17, 2026
aloekun
added a commit
that referenced
this pull request
Jul 17, 2026
* docs(push-pipeline-fix-plan): T6 の PR 番号を #283 に backfill T6 の作業コミット時点では PR が未採番だったため、計画書の §4 実施結果と §8 判定記録に「PR 未採番 — 採番後に backfill」と書いて負債を明示していた。PR #283 がマージされたため採番情報のみを更新する。 変更 (3 箇所、いずれも採番情報のみ): - §4 T6 実施結果の見出し: PR 未採番 → PR #283 - §4 T6 の backlog 10 への申し送り: 「本 PR では触れず」→「PR #283 では触れず」 (T5 が §4/§8 の「本 PR」を番号へ解決した慣習に揃える) - §8 判定記録の T6 行: PR 未採番 → PR #283 由来: PR #282 (T5) の post-PR レビューで「T4 行が『本 PR』のまま放置され PR #282 で backfill する羽目になった」負債が指摘され、同じ形を繰り返さないために T6 では 未採番であることを明示していた。本コミットでその明示を回収する。 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks-stop-quality): 品質ゲートの cwd 依存を修正 (push パイプライン改善 T7) Stop hook はセッションの cwd を継承して起動されるため、cwd がリポジトリルート 以外 (例: .takt/runs に cd したまま Stop) だと 2 つの症状が黙って出ていた: 1. hooks-config.toml のルート相対 step (file-length) が「指定されたパスが 見つかりません」で失敗し、品質ゲートが誤 block する (2026-07-16 に実発火)。 pnpm / cargo 系 step は設定ファイルを上方探索するため偶然通っており、 ルート相対パスを書いた step だけが壊れる非対称が発見を遅らせていた。 2. takt subsession 判定が <cwd>/.takt/runs を探して空振りし、active run を 検出できない。ADR-004 の edit:false subsession skip が効かなくなる。 同一の根本原因なので main 冒頭で cwd を 1 度だけプロジェクトルートへ正規化する。 ルートは exe パス (<root>/.claude/<hook>.exe) から導出する — CLAUDE_PROJECT_DIR env は VSCode 拡張環境で空になることを実測確認した (ADR-005 の不安定性が現存)。 config / pipeline lock / telemetry が既に採る exe-relative 規約と同形。 ルート特定不能時は警告のみで継続 (fail-open、pipeline_is_running と同じ線引き)。 main.rs が 800 行上限に触れたため takt 判定を takt_subsession.rs へ分離した。 回帰テスト: tests/t7_cwd_independence.rs に E2E 5 本 + unit 2 本 (26 → 33)。 exe を <root>/.claude/ に staging して spawn し、exe-relative 解決を実配置で 検証する。正規化の呼び出しを外すと bad 2 本が失敗し good 3 本は通ることを確認済み。 * fix(review): apply CodeRabbit fixes for #284 Resolved findings: - [Major] src/hooks-stop-quality/tests/t7_cwd_independence.rs:96 hook の終了ステータスを確認してください * fix(review): CodeRabbit 指摘の stderr 出力を補完 (#284) auto-fix が追加した assert_hook_success は exit code assert 自体は入れたが、 指摘の「失敗時は stderr を出す」部分が未達だった: メッセージに stdout を渡しており、 かつ stderr.join() より前に呼ばれるため構造上 stderr を出せない。 本 hook の診断 (cwd 正規化の警告等) は eprintln! = stderr にしか出ないため、 指摘が想定する「非 0 exit かつ stdout が空」の失敗では stderr だけが手掛かりになる。 stderr を join してから assert に渡す形へ補正した。 guard が空振りでないことを実証済み: staged exe を where.exe (非 0 exit・stdout 空) に 差し替えると 5 本すべてが exit code Some(2) で失敗する (guard 導入前なら None を 期待する 3 本が false green で素通りしていた)。 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
push_was_refused) の判定対象を 40 行 truncate 済み出力から全量出力に変更し、jj が exit 0 で拒否した際の silent failure を塞ぐlib-subprocessにrun_cmd_shell_unlimitedを追加。3 variant の共通骨格 (spawn → drain → wait → combine) はrun_cmd_shell_withに集約runner::run_stage_cmdを削除 (capped 経路で control flow 判定する罠の構造的排除)Context
Why: jj は新規 bookmark の push を拒否するとき、エラー終了せず
Refusing to create new remote bookmarkを出力して exit 0 を返す。push stage はこの行を出力から検知していたが、判定対象がrun_stage_cmd(=run_cmd_shell_capped、MAX_LINES=40 の silent truncate) の出力だったため、jj の出力が 40 行を超えて拒否行が cap の外へ落ちると拒否を見逃し、リモート未反映のまま exit 0 で完了していた。後続のcli-pr-monitorは旧 head を監視し始める。Trigger: 2026-07-16 の push パイプライン調査 (T5)。
lib-subprocessの doc は当初から「control flow 判定に出力を使う callsite で capped variant を使うな」と明記しており、その契約違反として特定された。正しい variant (unlimited) がrun_cmd_shellfamily に存在しなかったため、callsite は間違った variant を選ぶしかなかった — この観察を ADR-044 に記録している。Scope decision:
contains("refusing to")の厳格化は見送り。誤検知 (push 成功を失敗と報告) は出力表示で気付いて再実行できるが、検知漏れは本 PR が直す事故そのもの。リスクが非対称なため ADR-043 (fail-closed) に従い部分一致を維持するcli-pr-monitorのpush_to_remoteにも拒否検知が無い同型の穴を発見したが、1 PR 1 変更のため本 PR では触れず計画の backlog に追加したValidation
cargo test -p cli-push-runner: 206 pass (T5 回帰 6 本を含む。run_stage_cmdの 2 本を削除したため 208 → 206)cargo test -p lib-subprocess: 31 pass (unlimited variant 4 本を含む)cargo clippy --workspace --all-targets --all-features -- -D warnings: warning 0run_push_cmdを capped 版に戻すと 3 本が fail することを確認 (テストが素通りしない実証)[push] 成功+ exit 0 (silent failure を再現) / after = 拒否検知 + exit 3。成功経路 (50 行・拒否なし) は 40 行 +... (10 lines truncated)表示 + exit 0 を維持pnpm pushpre-push review: verdict=APPROVE (simplicity / security とも。1 iteration / takt 313s / 合計 366s)References
contains維持の判断根拠run_cmd_shell_unlimited追加の判定を追記@時の bookmark_check 誤誘導を修正 (push T8) #280 / PR chore(push-runner-config): refute facet の dogfood を開始 (push T4) #281 — 同計画の先行タスク (push T1 / T8 / T4)docs/push-pipeline-fix-plan.md§4 T5 — 実施結果と §8 判定記録Summary by CodeRabbit
改善
ドキュメント
テスト