From 1ef60ba876c952877d8c23310393f6696c6f276e Mon Sep 17 00:00:00 2001 From: aloekun Date: Fri, 17 Jul 2026 04:54:44 +0900 Subject: [PATCH 1/2] =?UTF-8?q?fix(cli-push-runner):=20push=20=E6=8B=92?= =?UTF-8?q?=E5=90=A6=E6=A4=9C=E7=9F=A5=E3=81=AE=2040=20=E8=A1=8C=20truncat?= =?UTF-8?q?e=20=E4=BE=9D=E5=AD=98=E3=82=92=E4=BF=AE=E6=AD=A3=20(push=20?= =?UTF-8?q?=E3=83=91=E3=82=A4=E3=83=97=E3=83=A9=E3=82=A4=E3=83=B3=E6=94=B9?= =?UTF-8?q?=E5=96=84=20T5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- ...-subprocess-utility-extraction-boundary.md | 20 ++ docs/push-pipeline-fix-plan.md | 69 ++++++- src/cli-push-runner/src/runner.rs | 35 +--- src/cli-push-runner/src/stages/push.rs | 177 ++++++++++++++++-- src/lib-subprocess/src/lib.rs | 154 +++++++++------ 5 files changed, 347 insertions(+), 108 deletions(-) diff --git a/docs/adr/adr-044-subprocess-utility-extraction-boundary.md b/docs/adr/adr-044-subprocess-utility-extraction-boundary.md index abd1a096..1ca12da5 100644 --- a/docs/adr/adr-044-subprocess-utility-extraction-boundary.md +++ b/docs/adr/adr-044-subprocess-utility-extraction-boundary.md @@ -30,6 +30,9 @@ PR #205 (173a) - #208 (173d) で以下を `lib-subprocess` に集約: | `run_cmd_shell` | `_capped` / `_capped_reporting` | drain variant 違い (上記に従う) | | `kill_and_join_err` | 1 (内部 helper) | Err 経路で child kill + reader thread join (PR #208 CR Major 対応) | +> 本表は順位 173 完了時点 (2026-06-15) の状態。`run_cmd_shell` はその後 3 variant に増えた +> (下記「後続の variant 追加」)。 + 5 callsite (cli-push-runner / cli-push-pipeline / cli-merge-pipeline / hooks-stop-quality / hooks-post-tool-linter) が `lib_subprocess::*` 経由で utility を共有。 ### 173e で判定した境界 @@ -108,6 +111,23 @@ PR #205 (173a) - #208 (173d) で以下を `lib-subprocess` に集約: | `run_cmd_direct` (cli-pr-monitor) | ❌ 各 crate 残置 | shell vs direct args で signature 非互換 (層 1) | | `run_cmd_inherit` (2 crate) | ❌ 各 crate 残置 | timeout 有無の policy 差が意図的、signature 異なる (層 1) | +### 後続の variant 追加 (2026-07-17: `run_cmd_shell_unlimited`) + +push パイプライン改善 T5 (push 拒否検知の truncate 依存修正) で `run_cmd_shell_unlimited` を追加し、 +`run_cmd_shell` は 3 variant になった。本 ADR の境界基準を実適用した最初の事例なので判定を記録する。 + +| 論点 | 判定 | 根拠 | +|---|---|---| +| lib に入れるか / callsite に置くか | ✅ lib に variant 追加 | 層 1 の「1 crate でしか使われていない → extract せず」は**重複除去のための extract 判定**であり、確立済み family への variant 追加には適用しない。代替 (callsite に spawn→drain→wait→combine の骨格を複製) は重複を**増やす**。`drain_pipe` が既に `_unlimited` を持ち `run_cmd_shell` だけ欠けていた非対称の解消でもある | +| 単一関数に merge するか | ✅ 3 variant 維持 | 層 2「callsite ごとに intent が異なる」。しかも本件は intent の取り違えが silent failure を生んだ実例そのもの (下記) で、variant 名で意図を表明させる価値が実証された | +| 3 variant の body 重複 | ✅ 共通骨格を `run_cmd_shell_with` に集約 | 3 つ目の copy が出た時点で rule of three。public API と self-documenting な variant 名は不変で、各 variant は drain 戦略の違いのみを表す | + +T5 の由来は本 ADR にとって示唆的である: `run_cmd_shell_capped` の doc は「control flow 判定に +出力を使う callsite では capped を使うな」と当初から明記していたが、`cli-push-runner` の push stage は +まさにそれをやっていた (40 行 cap の外に jj の拒否行が落ちると、リモート未反映のまま exit 0)。 +**doc の契約だけでは守られず、正しい variant が存在しないと callsite は間違った variant を選ぶ**。 +層 2 の「callsite が variant 名で intent 表明」は、選択肢が揃っていて初めて機能する。 + ## 影響 ### 良い影響 diff --git a/docs/push-pipeline-fix-plan.md b/docs/push-pipeline-fix-plan.md index 49c15e14..787349d6 100644 --- a/docs/push-pipeline-fix-plan.md +++ b/docs/push-pipeline-fix-plan.md @@ -108,6 +108,55 @@ T1 を最優先とする理由: 以降の全 PR の dogfood push が速くなり - **テスト**: 41 行以上の出力の末尾に `Refusing to ...` を含む fixture で 「拒否が検知されること」の回帰テスト。既存の push stage テスト群に追加。 - **リスク**: 低。出力保持量が増えるだけ。 +- **実施結果 (2026-07-17, 実装済み / 本 PR)**: + - **実装**: 方針どおり「判定は全量・cap は表示側のみ」。 + - `lib-subprocess` に `run_cmd_shell_unlimited` を追加した。既存 asset のうち + `drain_pipe_unlimited` は pipe 単体、`run_cmd_shell_capped_reporting` は truncate を + 明示するだけで**判定用には依然不足** (cap は残る) のため、`run_cmd_shell` family に + 欠けていた unlimited variant を足す形にした。3 variant の共通骨格 + (spawn → drain → wait → combine) は 3 つ目の copy が出た時点で `run_cmd_shell_with` に + 集約し、各 variant は drain 戦略の違いだけを表す。境界判定は ADR-044 §「後続の + variant 追加」に記録した。 + - push stage は `run_push_cmd` (unlimited) で全量取得し、`push_was_refused` は + **全量出力**に対して判定する。表示は成功時のみ `cap_for_log` (先頭 40 行 + + `... (N lines truncated)`) を通し、**失敗経路 (拒否 / Err) は全量表示**する + — 失敗時こそ診断情報を落としてはならないため (§6 backlog 1 と同じ理由)。 + 成功時のログ量は従来どおり 40 行で、増えない。 + - **副産物: `runner::run_stage_cmd` を削除**した。push stage が唯一の呼び出し元だったため + 未使用になり clippy が検出。dead code を残さない方針 (T2 と同じ) に加え、 + 「capped 経路で control flow 判定する」罠を構造的に排除する意味がある。 + `MAX_LINES` は表示用として残置し (quality_gate / scratch_file_warning / lint_screen が使用)、 + doc に「判定に使う出力を本値で cap してはならない」を明記した。 + - **`contains` 厳格化は不採用 (ユーザー承認済み)**: 方針の「行頭マッチ等に厳格化するか検討」は + **見送り**、`contains("refusing to")` を維持した。理由はリスクの非対称性: 誤検知 + (push 成功を失敗と報告) は出力もそのまま表示されるため気付いて再実行できるのに対し、 + 検知漏れは**リモート未反映のまま exit 0** = T5 が直そうとしている事故そのもの。 + jj のメッセージ書式変更で検知漏れ側に倒れる厳格化は ADR-043 (fail-closed) に反する。 + 判断根拠はコード doc (`push_was_refused`) にも残した。§6 backlog 8 は却下扱い。 + - **回帰テスト (ADR-049 の流儀)**: `mod t5_truncated_refusal_detection` に 6 本 + (cli-push-runner 206 passed。`run_stage_cmd` の 2 本を削除したため 208 → 206)、 + `lib-subprocess` に unlimited variant の 4 本 (31 passed)。 + bad = 41 行目の拒否行を検知すること、good = 40 行超の正常出力を誤検知しないこと、 + および「表示 cap は判定に影響しない」ことを固定した。 + **修正前の挙動に対して失敗することを確認済み**: `run_push_cmd` を capped 版に戻すと + 上記 3 本が fail する (「run_push_cmd が 40 行に切り詰めている = T5 の不具合」)。 + 回帰テストが素通りしないことの実証。 + - **サンドボックス実機検証 (before/after)**: 短いパス (`C:\t5\repo`) に jj repo を張り、 + `[push] command` を「40 行の正常出力 + 末尾に拒否行」の fake command に、 + `[diff] command` を空出力にして takt を skip、quality_gate を noop にして + push stage まで到達させ、配布 exe (修正前) と修正後 exe を比較した。 + + | | before (現行配布 exe) | after (修正後) | + |---|---|---| + | 41 行目の拒否行 | 見逃し → `[push] 成功` | `[push] 失敗: リモートに反映されませんでした (jj が push を拒否)` | + | exit code | **0 (silent failure が再現)** | 3 (EXIT_PUSH_FAILURE) | + | 成功時 (50 行・拒否なし) の表示 | 40 行で silent truncate | 40 行 + `... (10 lines truncated)`、exit 0 | + + before が**「リモート未反映のまま exit 0」を逐語で再現**することを確認した上で修正を当てている。 + この状態で本番なら pr-monitor が旧 head を監視し始める。 + - **発見 (本タスク外)**: `cli-pr-monitor` の `push_to_remote` は exit code のみを見ており + **拒否検知が無い**。post-PR の re-push で同型の silent failure が起き得る。 + §2 原則 4 (1 PR 1 変更) に従い本 PR では触れず、§6 backlog 9 に追加した。 ### T6: diff stage の timeout 欠落 @@ -424,7 +473,7 @@ T1 を最優先とする理由: 以降の全 PR の dogfood push が速くなり コメントに明記した。あわせて ADR-047 の「本 PR (導入 PR) は OFF とする」という 導入 PR 時点の記述を、dogfood 開始済みの現状に合わせて過去形へ更新した。 - **有効化前に確認したこと** (dogfood のブートストラップ注意 = §2 原則 4 の適用。 - 有効化した瞬間から本 PR 自身の push が refute workflow を通るため、事前に静的確認した): + 有効化した瞬間から PR #281 自身の push が refute workflow を通るため、事前に静的確認した): - refute 側の資産が揃っている: `.takt/workflows/pre-push-review-refute.yaml` / `.takt/facets/instructions/refute-finding.md` / `.takt/facets/output-contracts/refutation-report.md`。 @@ -435,7 +484,7 @@ T1 を最優先とする理由: 以降の全 PR の dogfood push が速くなり cwd の `push-runner-config.toml` から読まれる (`config_path()`)。 - 同じ config を読む他 exe への波及なし: `cli-pr-monitor` の `stages/gate.rs` は `[quality_gate]` のみ参照し `[pre_push_review]` を見ない。 - - **初回 dogfood push の実測 (本 PR 自身の push、2026-07-17)**: + - **初回 dogfood push の実測 (PR #281 自身の push、2026-07-17)**: | stage | 実測 | |---|---| @@ -455,7 +504,7 @@ T1 を最優先とする理由: 以降の全 PR の dogfood push が速くなり `all("approved") → COMPLETE` に抜けたため、`any("needs_fix") → verify` の経路に 入らなかった。よって **verify 実動の観測は次に findings が出る run に持ち越す** (完了条件の「verify step が動くことの確認」は「有効化が効いていることの確認」までを - 本 PR の成果として読む。ユーザー承認済みの範囲)。 + PR #281 の成果として読む。ユーザー承認済みの範囲)。 - 参考: T0 の PR #278 (コード変更・fix なし) は quality_gate 93.9s / takt 149.4s / 合計 247s。本 run は docs+config の小 diff かつ T1 適用後のため単純比較はできないが、 同じ「fix なし run」帯に収まっている。 @@ -595,7 +644,16 @@ T1 を最優先とする理由: 以降の全 PR の dogfood push が速くなり 6. `advance_jj_bookmarks` の二重実行 (stage 1 と stage 8) の統合検討。 7. 同一 checkout での `pnpm push` 並走ガード (pipeline lock は advisory のまま、 push 同士のみ相互排他にするか検討。ADR-025/ADR-045 との整合を確認)。 -8. `push_was_refused` の `contains` 誤爆厳格化 (T5 に含めなかった場合)。 +8. ~~`push_was_refused` の `contains` 誤爆厳格化 (T5 に含めなかった場合)。~~ + **却下 (2026-07-17, T5 で判定)**: リスクが非対称なため厳格化しない。誤検知は出力表示で + 気付いて再実行できるが、検知漏れは「リモート未反映のまま exit 0」= T5 が防ぐ事故そのもの。 + ADR-043 (fail-closed) に従い `contains` を維持する。判断根拠は `push_was_refused` の + doc コメントに恒久化済み (§4 T5 実施結果)。 +9. `cli-pr-monitor` の `push_to_remote` (`src/cli-pr-monitor/src/stages/push.rs`) に + 拒否検知を追加する (T5 の調査で発見、2026-07-17)。jj は新規 bookmark 拒否時に exit 0 を + 返すが、同関数は exit code のみを見ているため post-PR の re-push が無言で失敗し得る。 + 出力取得は `run_cmd_direct` (unlimited) なので**判定の追加だけ**で済む + (T5 と違い truncate 問題は無い)。規模 XS。 ## 7. スコープ外 (本計画では実施しない) @@ -630,4 +688,5 @@ T1 を最優先とする理由: 以降の全 PR の dogfood push が速くなり | T0 | 実装・マージ済 (PR #278) | 2026-07-16 | stage 別ログ `stage= elapsed=<秒>s` を追加 (§5 T0 実施結果)。before 値は §1 表を使用。初回実測で T1 の前提に疑義 → §5 T1 の申し送り参照 | | T1 | 実装済 (PR #279) | 2026-07-16 | `LINT_SCREEN_EVALS` env opt-in で eval を gate から除外。`--ignored` 63s → 21s。step_timeout 600 → 300 (実測 right-size)。**判断根拠**: 着手前実測で (b) 41.3s が (a) 63s の 65% を占め、申し送りの判定基準「前提は生きている」に該当したため実施。ただし絶対値が 269s の約 1/4 だったため期待効果を -2〜4.5 分 → -42s に下方修正し、§1 結論の (1)「主犯」認定も修正した。実行の本丸は (2)(3) = T10/T12 | | T8 | 実装済 (PR #280) | 2026-07-17 | 「`@` 空 + bookmark が `@-`」を `NoBookmarks` から切り分け、`jj edit @-` を案内するよう修正。`working_copy_is_empty()` を advance と共有して規則の二重定義を解消。`main.rs` 側の重複案内も撤去。回帰テスト 12 本 + サンドボックス実機で before/after 比較 (§4 T8 実施結果)。**post-PR 修正 (CodeRabbit Major 2 件 + simplicity 警告 2 件を採用)**: (a) 判定順を反転し、bookmark が空の `@` にある場合も中断する — 続行すると `jj diff -r @` が空になり祖先の未 push 変更が AI レビューを経ずに push される (方針 2 を却下した理由と同じ穴が bookmark の位置違いで残っていた)。(b) `@-` 照会の失敗を `unwrap_or_default()` で「親はあるが bookmark 無し」に潰していたのを `ParentState::Unavailable` として保持し、親を確認できない場合は実行不能な `jj edit @-` を案内しない (T8 が直したはずの誤誘導の再生産だった)。あわせて simplicity-review の非ブロッキング警告 2 件も採用: (c) `query_parent_state()` の jj 失敗を log する (他の jj 失敗処理の慣習と揃える)。(d) `@-` に bookmark が無い場合は `jj edit @-` だけでは次に `NoBookmarks` で止まるため、bookmark 作成まで含めて案内する。Minor 1 件のうち日付指摘は CodeRabbit が UTC 基準のため不採用 (本 repo の記録は JST 基準で 2026-07-17 が正)。**dogfood 実証**: 本修正の push 作業中に、私自身が `jj new` で空コミットを作ってしまい T8 の incident 状態を再現したが、修正後の bookmark_check が破壊的な `jj bookmark create -r @` ではなく正しい `jj edit @-` を案内し、案内どおりの操作で復旧できた (修正が in the wild で機能することの実証)。**方針変更**: 方針 2 (`@-` を検査対象にして続行) は `[diff] command = "jj diff -r @"` により **takt レビューを無言 skip して push する**ことが判明したため不採用。方針 3 の「push すべき新変更がない」も再現記録の事実 4 (実際に push 成功 = 変更はあった) と矛盾するため不採用。exit 7 は維持し**案内文のみ正す**方針をユーザー承認のうえ採用した。**実施順**: T4-T7 を飛ばして T1 の次に実施 (T1 の dogfood push で再現が取れたタイミングを優先。T8 は他タスクと独立のため順序入替は無害) | -| T4 | 実装済 (本 PR) | 2026-07-17 | `push-runner-config.toml` の `refute_enabled = false → true` で dogfood 開始 (変更は方針どおり 1 行、templates は OFF 据え置き)。**dogfood 開始日を同 PR で固定**: ADR-039 bounded lifetime の起点が無いと 2 週間の期限が判定不能になるため、開始 2026-07-17 → **判定期限 2026-07-31** を ADR-047 (ステータス行 / Config opt-in / Bounded lifetime の 3 箇所) + config コメントに明記した。採否判定自体は本計画と独立に ADR-047 で進行する (§8 完了条件 4. の引き継ぎ先)。**初回 dogfood push で切替を実証** (本 PR 自身の push): 起動ログ `takt (pre-push-review-refute)` + takt の `ワークフロー 'pre-push-review-refute' を起動` → 完走を確認。合計 151s (pre_checks 1.3s / quality_gate 49.7s / diff 0.1s / takt 97.8s / push 2.2s)。**verify は予告どおり未発火**: reviewers 2 本とも APPROVE で `all("approved") → COMPLETE` に抜けたため `any("needs_fix") → verify` に入らず、verify 実動の観測は次の findings 発生 run に持ち越し (完了条件は「有効化が効いていることの確認」までで読む)。**副産物: 計測手順の誤りを発見・修正**。ADR-047 §dogfood 計測項目の `.takt/runs/*-pre-push-review-refute/trace.md` は **1 件もマッチしない** — run ディレクトリ名は workflow 名でなく task 名から作られ (`runSlug` = `-pre-push-review`)、refute run でも `20260716-182505-pre-push-review` になる (timestamp も UTC で JST の日付と 1 日ずれ得る)。放置すると 2026-07-31 の採否判定で run 0 件 →「データなし」誤読の恐れがあったため、ADR-047 と §5 T4 実施結果を `meta.json` の `piece` フィールド基準 (`grep -l '"piece": "pre-push-review-refute"' .takt/runs/*/meta.json`) に修正した。設計時の計測手順が実運用開始まで未検証だった例。有効化前の静的確認 (refute workflow / facet 群の存在、`resolve_takt_workflow` の unit test 4 本、`cli-pr-monitor` への波及なし、Rust 変更ゼロのため exe 再ビルド不要) は §5 T4 実施結果に記載。**実施順**: T5-T7 を飛ばして T8 の次に実施 (T4 は他タスクと独立の XS で、依存なし) | +| T5 | 実装済 (本 PR) | 2026-07-17 | push 拒否検知を 40 行 truncate 済み出力から**全量出力**に切替。`lib-subprocess` に `run_cmd_shell_unlimited` を追加 (`drain_pipe_unlimited` は pipe 単体、`_capped_reporting` は cap が残るためどちらも判定用には不足だった) し、3 variant の共通骨格を `run_cmd_shell_with` に集約。境界判定は ADR-044 §「後続の variant 追加」に記録。表示は成功時のみ `cap_for_log` で 40 行 + 超過明示に絞り、**失敗経路は全量表示** (診断情報を落とさない)。**副産物**: 唯一の呼び出し元が消えた `runner::run_stage_cmd` を削除 — dead code 除去に加え「capped 経路で control flow 判定する」罠の構造的排除。`MAX_LINES` は表示用として残置し doc に判定禁止を明記。**厳格化は不採用 (ユーザー承認済み)**: `contains` 誤爆の厳格化はリスクが非対称 (誤検知は出力表示で気付けるが、検知漏れは「リモート未反映のまま exit 0」= 本タスクが防ぐ事故そのもの) で ADR-043 (fail-closed) に反するため見送り、§6 backlog 8 を却下記録に変更。**回帰テスト**: `mod t5_truncated_refusal_detection` 6 本 + lib-subprocess 4 本。`run_push_cmd` を capped に戻すと 3 本が fail することを確認済み (回帰テストが素通りしないことの実証)。**サンドボックス実機で before/after 比較**: 拒否行を 41 行目に置いた fake push command で、before = `[push] 成功` + **exit 0 (silent failure 再現)** / after = 拒否検知 + exit 3。成功経路 (50 行) は 40 行 + `... (10 lines truncated)` 表示で exit 0 を維持。**発見 (本タスク外)**: `cli-pr-monitor` の `push_to_remote` は拒否検知が無く同型の穴 → §6 backlog 9 に追加 (1 PR 1 変更のため別 PR)。**実施順**: 計画の推奨順どおり T4 の次に実施 | +| T4 | 実装済 (PR #281) | 2026-07-17 | `push-runner-config.toml` の `refute_enabled = false → true` で dogfood 開始 (変更は方針どおり 1 行、templates は OFF 据え置き)。**dogfood 開始日を同 PR で固定**: ADR-039 bounded lifetime の起点が無いと 2 週間の期限が判定不能になるため、開始 2026-07-17 → **判定期限 2026-07-31** を ADR-047 (ステータス行 / Config opt-in / Bounded lifetime の 3 箇所) + config コメントに明記した。採否判定自体は本計画と独立に ADR-047 で進行する (§8 完了条件 4. の引き継ぎ先)。**初回 dogfood push で切替を実証** (PR #281 自身の push): 起動ログ `takt (pre-push-review-refute)` + takt の `ワークフロー 'pre-push-review-refute' を起動` → 完走を確認。合計 151s (pre_checks 1.3s / quality_gate 49.7s / diff 0.1s / takt 97.8s / push 2.2s)。**verify は予告どおり未発火**: reviewers 2 本とも APPROVE で `all("approved") → COMPLETE` に抜けたため `any("needs_fix") → verify` に入らず、verify 実動の観測は次の findings 発生 run に持ち越し (完了条件は「有効化が効いていることの確認」までで読む)。**副産物: 計測手順の誤りを発見・修正**。ADR-047 §dogfood 計測項目の `.takt/runs/*-pre-push-review-refute/trace.md` は **1 件もマッチしない** — run ディレクトリ名は workflow 名でなく task 名から作られ (`runSlug` = `-pre-push-review`)、refute run でも `20260716-182505-pre-push-review` になる (timestamp も UTC で JST の日付と 1 日ずれ得る)。放置すると 2026-07-31 の採否判定で run 0 件 →「データなし」誤読の恐れがあったため、ADR-047 と §5 T4 実施結果を `meta.json` の `piece` フィールド基準 (`grep -l '"piece": "pre-push-review-refute"' .takt/runs/*/meta.json`) に修正した。設計時の計測手順が実運用開始まで未検証だった例。有効化前の静的確認 (refute workflow / facet 群の存在、`resolve_takt_workflow` の unit test 4 本、`cli-pr-monitor` への波及なし、Rust 変更ゼロのため exe 再ビルド不要) は §5 T4 実施結果に記載。**実施順**: T5-T7 を飛ばして T8 の次に実施 (T4 は他タスクと独立の XS で、依存なし) | diff --git a/src/cli-push-runner/src/runner.rs b/src/cli-push-runner/src/runner.rs index 83f23471..0908a35a 100644 --- a/src/cli-push-runner/src/runner.rs +++ b/src/cli-push-runner/src/runner.rs @@ -1,21 +1,15 @@ use std::process::Command; -use lib_subprocess::run_cmd_shell_capped; - use crate::log::log_info; +/// stage コマンドの出力をログ表示用に切り詰める行数の既定値。 +/// +/// **判定に使う出力を本値で cap してはならない** (T5): push stage は cap の外に落ちた +/// jj の拒否行を見逃して silent-failure push を起こしていた。出力を control flow に +/// 使う callsite は `lib_subprocess::run_cmd_shell_unlimited` で全量を取得し、 +/// cap は表示側にのみ掛ける (`stages/push.rs` の `run_push_cmd` / `cap_for_log`)。 pub(crate) const MAX_LINES: usize = 40; -/// コマンドを実行し、成功時は出力を `Ok`、失敗時はエラー出力を `Err` で返す。 -pub(crate) fn run_stage_cmd(label: &str, cmd: &str, timeout: u64) -> Result { - let (success, output) = run_cmd_shell_capped(label, cmd, timeout, MAX_LINES); - if success { - Ok(output) - } else { - Err(output) - } -} - pub(crate) fn run_cmd_inherit(label: &str, program: &str, args: &[&str]) -> bool { log_info(&format!("{}: {} {}", label, program, args.join(" "))); match Command::new(program) @@ -33,20 +27,3 @@ pub(crate) fn run_cmd_inherit(label: &str, program: &str, args: &[&str]) -> bool } } -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn run_stage_cmd_returns_ok_on_success() { - let result = run_stage_cmd("test", "echo hello", 10); - assert!(result.is_ok(), "successful command should return Ok"); - } - - #[test] - fn run_stage_cmd_returns_err_on_failure() { - let result = run_stage_cmd("test", "exit 1", 10); - assert!(result.is_err(), "failed command should return Err"); - } - -} diff --git a/src/cli-push-runner/src/stages/push.rs b/src/cli-push-runner/src/stages/push.rs index 58eb7b75..1b2c7c9b 100644 --- a/src/cli-push-runner/src/stages/push.rs +++ b/src/cli-push-runner/src/stages/push.rs @@ -1,7 +1,9 @@ +use lib_subprocess::run_cmd_shell_unlimited; + use super::push_jj_bookmark::advance_jj_bookmarks; use crate::config::{PushConfig, DEFAULT_PUSH_TIMEOUT_SECS}; use crate::log::log_stage; -use crate::runner::run_stage_cmd; +use crate::runner::MAX_LINES; pub(crate) fn run_push(config: &PushConfig, detected_bookmarks: &[String]) -> bool { // NOTE: takt fix や手動 jj describe で @ が進んでも bookmark が旧コミットのまま残る問題の対策 @@ -18,34 +20,74 @@ pub(crate) fn run_push(config: &PushConfig, detected_bookmarks: &[String]) -> bo let command = build_push_command(&config.command, detected_bookmarks); log_stage("push", &command); - match run_stage_cmd("push", &command, timeout) { + match run_push_cmd(&command, timeout) { Ok(output) => { if push_was_refused(&output) { log_stage( "push", "失敗: リモートに反映されませんでした (jj が push を拒否)", ); - if !output.is_empty() { - eprintln!("{}", output); - } + print_output(&output); return false; } log_stage("push", "成功"); - if !output.is_empty() { - eprintln!("{}", output); - } + print_output(&cap_for_log(&output)); true } Err(output) => { log_stage("push", "失敗"); - if !output.is_empty() { - eprintln!("{}", output); - } + print_output(&output); false } } } +/// push コマンド専用: 出力を切り詰めずに全行を取得する。 +/// +/// capped variant (`run_cmd_shell_capped`、`MAX_LINES` 行で silent truncate) を使うと、 +/// jj の出力が cap を超えて拒否行が外に落ちた場合に `push_was_refused` が拒否を見逃し、 +/// **リモート未反映のまま exit 0** になる (後続の pr-monitor が旧 head を監視する)。 +/// `lib-subprocess` の doc が定める「出力を control flow 判定に使う callsite で capped +/// variant を使ってはならない」契約に従い、判定は全量出力に対して行う。 +fn run_push_cmd(cmd: &str, timeout: u64) -> Result { + let (success, output) = run_cmd_shell_unlimited("push", cmd, timeout); + if success { + Ok(output) + } else { + Err(output) + } +} + +fn print_output(output: &str) { + if !output.is_empty() { + eprintln!("{}", output); + } +} + +/// 成功時のログ表示用に先頭 `MAX_LINES` 行へ切り詰め、超過分は行数を明示する。 +/// +/// 判定 (`push_was_refused`) は全量出力に対して行い、cap は表示にのみ掛ける +/// (= 従来のログ量を維持しつつ、判定は truncate の影響を受けない)。失敗経路では +/// 診断情報を落とさないため本関数を通さず全量を出す。 +/// +/// truncate 表記は `lib_subprocess::drain_pipe_capped_reporting` に合わせる (ログ上の +/// 見え方を統一する)。あちらは pipe を streaming しながら数えるため実装は共有できない。 +fn cap_for_log(output: &str) -> String { + let mut lines = output.lines(); + let head: Vec<&str> = lines.by_ref().take(MAX_LINES).collect(); + let truncated = lines.count(); + if truncated == 0 { + return head.join("\n"); + } + let suffix = if truncated == 1 { "" } else { "s" }; + format!( + "{}\n... ({} line{} truncated)", + head.join("\n"), + truncated, + suffix + ) +} + /// 検出済み bookmark から push コマンドを組み立てる (ADR-045 事故 follow-up)。 /// /// 旧実装は config の `jj git push --all` を無条件実行しており、並列 workspace 運用で @@ -99,7 +141,7 @@ fn has_explicit_push_target(base: &str) -> bool { }) } -/// bookmark 名が shell 経由実行 (`run_stage_cmd`) で安全な文字だけで構成されるか。 +/// bookmark 名が shell 経由実行 (`run_push_cmd`) で安全な文字だけで構成されるか。 /// `jj bookmark list` 出力由来とはいえ shell に渡す文字列のため、許可リストで検証する。 fn is_shell_safe_bookmark_name(name: &str) -> bool { !name.is_empty() @@ -115,6 +157,14 @@ fn is_shell_safe_bookmark_name(name: &str) -> bool { /// この無言失敗を成功と誤報告しないための検知。`-b` 明示 (jj 0.42 で自動 track) 時は /// 通常発生しないが、fail-open で bare push になった場合や他の "Refusing to ..." /// ガード条件を捕捉する安全網として残す。 +/// +/// **入力は `run_push_cmd` の全量出力であること**。cap 済み出力を渡すと拒否行が +/// 落ちて silent-failure push を見逃す (本関数が防ぐべき事故そのもの)。 +/// +/// 単純な部分一致に留めるのは fail-closed (ADR-043) の判断による。行頭マッチ等への +/// 厳格化は誤検知を減らすが、jj のメッセージ書式変更で検知漏れ側に倒れる。両者のリスクは +/// 非対称で、誤検知 (push 成功を失敗と報告) は出力もそのまま表示されるため気付いて +/// 再実行できるのに対し、検知漏れはリモート未反映のまま exit 0 で先へ進む。 fn push_was_refused(output: &str) -> bool { output.to_lowercase().contains("refusing to") } @@ -209,4 +259,107 @@ mod tests { assert!(!is_shell_safe_bookmark_name("a\"b")); assert!(!is_shell_safe_bookmark_name("a|b")); } + + /// T5 回帰テスト群: push 拒否検知が 40 行 truncate 済み出力に依存していた不具合 + /// (ADR-049 の流儀: 1 test = 1 failure mode + good/bad)。 + /// + /// 由来: 2026-07-16 の push パイプライン調査 (コード監査で発見。in the wild の + /// 発火記録は無く、`lib-subprocess` の doc 契約違反として特定された)。 + /// + /// 事故の形: `run_push` は当時の `runner::run_stage_cmd` (= `run_cmd_shell_capped`、 + /// `MAX_LINES` 行の silent truncate) の出力に `push_was_refused` を掛けていた。jj の出力が + /// cap を超えて拒否行が外へ落ちると、拒否を見逃して **リモート未反映のまま exit 0** となり、 + /// 後続の pr-monitor が旧 head を監視する。 + /// + /// 修正の核心は「判定は全量出力 (`run_push_cmd`)、cap は表示側 (`cap_for_log`) にのみ」。 + /// bad / good とも cap を超える長さの実出力で固定し、判定と表示の分離を seal する。 + mod t5_truncated_refusal_detection { + use super::*; + + /// 40 行の正常出力の後に拒否行が来る = 拒否行が cap の外に落ちる状況の再現。 + const REFUSAL_BEYOND_CAP: &str = "(for /L %i in (1,1,40) do @echo Changes to push to origin) \ + & echo Warning: Refusing to create new remote bookmark feat/x@origin"; + + /// 40 行を超える正常な push 出力 (拒否なし)。 + const SUCCESS_BEYOND_CAP: &str = + "(for /L %i in (1,1,50) do @echo Add bookmark feat/x to 3000737e)"; + + /// incident 再現 (bad): cap の外にある拒否行を検知できること。 + /// jj は拒否時も exit 0 を返すため、この検知が唯一の防波堤になる。 + #[test] + fn refusal_beyond_the_cap_is_detected() { + let output = run_push_cmd(REFUSAL_BEYOND_CAP, 30) + .expect("jj の拒否は exit 0 なので Ok 経路で返る"); + assert!( + output.lines().count() > MAX_LINES, + "run_push_cmd が {} 行に切り詰めている ({} 行の fixture を投入) = T5 の不具合。\ + 判定に使う出力は truncate してはならない", + output.lines().count(), + MAX_LINES + 1, + ); + assert!( + push_was_refused(&output), + "cap の外にある拒否行を検知できること: {:?}", + output, + ); + } + + /// good: cap を超える正常な push 出力を拒否と誤判定しないこと。 + #[test] + fn long_successful_output_is_not_refused() { + let output = run_push_cmd(SUCCESS_BEYOND_CAP, 30).expect("成功コマンドは Ok"); + assert!( + output.lines().count() > MAX_LINES, + "run_push_cmd が {} 行に切り詰めている = T5 の不具合 (good 側も全量で判定する)", + output.lines().count(), + ); + assert!(!push_was_refused(&output), "誤検知しないこと: {:?}", output); + } + + /// 表示 cap は判定に影響しない: `cap_for_log` は超過分を明示して切り詰めるが、 + /// `push_was_refused` に渡すのは常に全量出力である。 + #[test] + fn cap_for_log_truncates_display_but_not_the_verdict() { + let output = run_push_cmd(REFUSAL_BEYOND_CAP, 30).expect("拒否出力は exit 0"); + let displayed = cap_for_log(&output); + assert!( + displayed.contains("truncated"), + "表示側は超過を明示して切り詰めること: {:?}", + displayed, + ); + assert!( + !push_was_refused(&displayed), + "前提の確認: 表示用に cap すると拒否行が落ちる (だから判定は全量で行う)", + ); + assert!(push_was_refused(&output), "判定は全量出力に対して真であること"); + } + + #[test] + fn cap_for_log_keeps_short_output_unchanged() { + let output = "Changes to push to origin:\n Add bookmark feat/x to 3000737e"; + assert_eq!(cap_for_log(output), output); + } + + #[test] + fn cap_for_log_reports_truncated_line_count() { + let output: String = (0..MAX_LINES + 3) + .map(|i| format!("line {}\n", i)) + .collect(); + let displayed = cap_for_log(&output); + assert!( + displayed.ends_with("... (3 lines truncated)"), + "超過行数を明示すること: {:?}", + displayed, + ); + } + + #[test] + fn cap_for_log_uses_singular_form_for_one_truncated_line() { + let output: String = (0..MAX_LINES + 1).map(|i| format!("line {}\n", i)).collect(); + assert!( + cap_for_log(&output).ends_with("... (1 line truncated)"), + "1 行超過は単数形", + ); + } + } } diff --git a/src/lib-subprocess/src/lib.rs b/src/lib-subprocess/src/lib.rs index 8da219fe..aeda3140 100644 --- a/src/lib-subprocess/src/lib.rs +++ b/src/lib-subprocess/src/lib.rs @@ -4,6 +4,10 @@ //! 順位 173b: wait_with_timeout を polling 系 2 variant (`_safe` / `_basic`) として抽出。 //! 順位 173c: drain_pipe を 3 variant (`_unlimited` / `_capped` / `_capped_reporting`) として抽出。 //! 順位 173d: run_cmd_shell を 2 variant (`_capped` / `_capped_reporting`) として抽出。 +//! 2026-07-17: run_cmd_shell に `_unlimited` variant を追加 (出力を control flow 判定に +//! 使う callsite 用。ADR-044 層 2 = callsite ごとに intent が異なるため variant 維持)。 +//! 3 variant の共通骨格 (spawn → drain → wait → combine) は `run_cmd_shell_with` に集約し、 +//! 各 variant は drain 戦略の違いのみを表す。 //! ADR-026 Cargo workspace + ADR-012 lib-* naming に整合。 //! //! 順位 173e で variant merge を検討予定。 @@ -224,24 +228,16 @@ fn kill_and_join_err( (false, error) } -/// `cmd /c ` で shell コマンドを実行し timeout 付きで結果を返す **silent capped** variant。 -/// -/// 戻り値: `(success, combined_output)`。 -/// - 起動失敗 / try_wait 失敗 → `(false, error_message)` -/// - timeout → `(false, "timed out after Ns\n")` -/// - exit 正常 → `(status.success(), combined)` +/// `run_cmd_shell_*` 3 variant の共通骨格 (spawn → drain → wait → combine)。 /// -/// 内部で `drain_pipe_capped(max_lines)` を使用するため stdout / stderr は `max_lines` 行で -/// silent truncate。control flow 判定に出力を使う callsite では別途 `drain_pipe_unlimited` -/// を直接組み立てるか、`max_lines` を十分大きく取ること。 -/// -/// 内部で `wait_with_timeout_basic` を使用 (= Err 経路で child を kill しない basic semantics)。 -pub fn run_cmd_shell_capped( - label: &str, - cmd: &str, - timeout_secs: u64, - max_lines: usize, -) -> (bool, String) { +/// variant 間の差は `drain` (= どの `drain_pipe_*` を使うか) だけで、それ以外の +/// semantics — timeout メッセージ書式 / Err 経路の child 扱い / 戻り値の意味 — は +/// 3 variant で共通。stdout と stderr は型が異なる (`ChildStdout` / `ChildStderr`) ため、 +/// 同一の drain 戦略を両方へ適用できるよう `Box` に統一して渡す。 +fn run_cmd_shell_with(label: &str, cmd: &str, timeout_secs: u64, drain: F) -> (bool, String) +where + F: Fn(Box) -> JoinHandle, +{ let mut child = match Command::new("cmd") .args(["/c", cmd]) .stdout(Stdio::piped()) @@ -252,14 +248,8 @@ pub fn run_cmd_shell_capped( Err(e) => return (false, format!("Failed to execute {}: {}", cmd, e)), }; - let stdout_handle = drain_pipe_capped( - child.stdout.take().expect("stdout must be piped"), - max_lines, - ); - let stderr_handle = drain_pipe_capped( - child.stderr.take().expect("stderr must be piped"), - max_lines, - ); + let stdout_handle = drain(Box::new(child.stdout.take().expect("stdout must be piped"))); + let stderr_handle = drain(Box::new(child.stderr.take().expect("stderr must be piped"))); let exit_status = match wait_with_timeout_basic(label, &mut child, timeout_secs) { Ok(status) => status, @@ -282,6 +272,30 @@ pub fn run_cmd_shell_capped( } } +/// `cmd /c ` で shell コマンドを実行し timeout 付きで結果を返す **silent capped** variant。 +/// +/// 戻り値: `(success, combined_output)`。 +/// - 起動失敗 / try_wait 失敗 → `(false, error_message)` +/// - timeout → `(false, "timed out after Ns\n")` +/// - exit 正常 → `(status.success(), combined)` +/// +/// 内部で `drain_pipe_capped(max_lines)` を使用するため stdout / stderr は `max_lines` 行で +/// silent truncate。**出力を control flow 判定に使う callsite で本 variant を使ってはならない** +/// (判定対象の行が cap の外に落ちても戻り値からは判別できず、無言で誤判定する)。 +/// その場合は `run_cmd_shell_unlimited` を使う。 +/// +/// 内部で `wait_with_timeout_basic` を使用 (= Err 経路で child を kill しない basic semantics)。 +pub fn run_cmd_shell_capped( + label: &str, + cmd: &str, + timeout_secs: u64, + max_lines: usize, +) -> (bool, String) { + run_cmd_shell_with(label, cmd, timeout_secs, |pipe| { + drain_pipe_capped(pipe, max_lines) + }) +} + /// `cmd /c ` で shell コマンドを実行し timeout 付きで結果を返す **reporting capped** variant。 /// /// `run_cmd_shell_capped` と同 signature だが内部で `drain_pipe_capped_reporting(max_lines)` @@ -294,44 +308,22 @@ pub fn run_cmd_shell_capped_reporting( timeout_secs: u64, max_lines: usize, ) -> (bool, String) { - let mut child = match Command::new("cmd") - .args(["/c", cmd]) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - { - Ok(c) => c, - Err(e) => return (false, format!("Failed to execute {}: {}", cmd, e)), - }; - - let stdout_handle = drain_pipe_capped_reporting( - child.stdout.take().expect("stdout must be piped"), - max_lines, - ); - let stderr_handle = drain_pipe_capped_reporting( - child.stderr.take().expect("stderr must be piped"), - max_lines, - ); - - let exit_status = match wait_with_timeout_basic(label, &mut child, timeout_secs) { - Ok(status) => status, - Err(e) => return kill_and_join_err(&mut child, stdout_handle, stderr_handle, e), - }; - - let stdout = stdout_handle.join().unwrap_or_default(); - let stderr = stderr_handle.join().unwrap_or_default(); - let combined = combine_output(&stdout, &stderr); + run_cmd_shell_with(label, cmd, timeout_secs, |pipe| { + drain_pipe_capped_reporting(pipe, max_lines) + }) +} - match exit_status { - None => { - let mut msg = format!("timed out after {}s", timeout_secs); - if !combined.is_empty() { - msg = format!("{}\n{}", msg, combined); - } - (false, msg) - } - Some(status) => (status.success(), combined), - } +/// `cmd /c ` で shell コマンドを実行し timeout 付きで結果を返す **unlimited** variant。 +/// +/// `run_cmd_shell_capped` から `max_lines` を除いたもので、内部で `drain_pipe_unlimited` +/// を使用するため出力は truncate されない。**出力を control flow 判定に使う callsite** +/// (例: cli-push-runner の push stage が jj の "Refusing to ..." を検知する) はこの variant を +/// 使うこと。capped variant では判定対象の行が cap の外に落ちて silent failure になる。 +/// +/// 出力が過大な場合にメモリ消費が線形成長する点は `drain_pipe_unlimited` と同じ trade-off。 +/// ログ表示量を絞りたい場合は、判定は全量に対して行い cap は表示側にのみ掛ける。 +pub fn run_cmd_shell_unlimited(label: &str, cmd: &str, timeout_secs: u64) -> (bool, String) { + run_cmd_shell_with(label, cmd, timeout_secs, drain_pipe_unlimited) } #[cfg(test)] @@ -565,4 +557,42 @@ mod tests { output, ); } + + #[test] + fn run_cmd_shell_unlimited_returns_true_on_exit_zero() { + let (ok, _output) = run_cmd_shell_unlimited("test", "exit 0", 10); + assert!(ok, "exit 0 should report success"); + } + + #[test] + fn run_cmd_shell_unlimited_returns_false_on_exit_nonzero() { + let (ok, _output) = run_cmd_shell_unlimited("test", "exit 1", 10); + assert!(!ok, "exit 1 should report failure"); + } + + /// 本 variant の存在理由: capped variant が silent truncate する行数を超えても + /// 全行が戻り値に残ること (= control flow 判定に使える)。 + #[test] + fn run_cmd_shell_unlimited_preserves_output_beyond_the_capped_variant_cap() { + let cmd = "(for /L %i in (1,1,60) do @echo line %i)"; + let (ok, output) = run_cmd_shell_unlimited("test", cmd, 30); + assert!(ok, "command should succeed: {:?}", output); + assert_eq!( + output.lines().count(), + 60, + "all lines must survive; unlimited variant must not truncate: {:?}", + output, + ); + } + + #[test] + fn run_cmd_shell_unlimited_reports_timeout_with_message() { + let (ok, output) = run_cmd_shell_unlimited("test", "ping 127.0.0.1 -n 10", 1); + assert!(!ok, "timeout should report failure"); + assert!( + output.starts_with("timed out after 1s"), + "timeout message expected: {:?}", + output, + ); + } } From 22e5fc6615e04f35bb86e76c8e787b474fd11f72 Mon Sep 17 00:00:00 2001 From: aloekun Date: Fri, 17 Jul 2026 12:40:28 +0900 Subject: [PATCH 2/2] =?UTF-8?q?fix(review):=20CodeRabbit=20Minor=201=20?= =?UTF-8?q?=E4=BB=B6=20+=20pre-push=20=E8=AD=A6=E5=91=8A=202=20=E4=BB=B6?= =?UTF-8?q?=E3=82=92=E5=8F=8D=E6=98=A0=20(PR=20#282=20/=20push=20T5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/push-pipeline-fix-plan.md | 20 ++++++++++++--- src/cli-push-runner/src/stages/push.rs | 14 +++-------- src/lib-subprocess/src/lib.rs | 34 +++++++++++++++++++++++--- 3 files changed, 51 insertions(+), 17 deletions(-) diff --git a/docs/push-pipeline-fix-plan.md b/docs/push-pipeline-fix-plan.md index 787349d6..e5e898c0 100644 --- a/docs/push-pipeline-fix-plan.md +++ b/docs/push-pipeline-fix-plan.md @@ -108,7 +108,7 @@ T1 を最優先とする理由: 以降の全 PR の dogfood push が速くなり - **テスト**: 41 行以上の出力の末尾に `Refusing to ...` を含む fixture で 「拒否が検知されること」の回帰テスト。既存の push stage テスト群に追加。 - **リスク**: 低。出力保持量が増えるだけ。 -- **実施結果 (2026-07-17, 実装済み / 本 PR)**: +- **実施結果 (2026-07-17, 実装済み / PR #282)**: - **実装**: 方針どおり「判定は全量・cap は表示側のみ」。 - `lib-subprocess` に `run_cmd_shell_unlimited` を追加した。既存 asset のうち `drain_pipe_unlimited` は pipe 単体、`run_cmd_shell_capped_reporting` は truncate を @@ -156,7 +156,21 @@ T1 を最優先とする理由: 以降の全 PR の dogfood push が速くなり この状態で本番なら pr-monitor が旧 head を監視し始める。 - **発見 (本タスク外)**: `cli-pr-monitor` の `push_to_remote` は exit code のみを見ており **拒否検知が無い**。post-PR の re-push で同型の silent failure が起き得る。 - §2 原則 4 (1 PR 1 変更) に従い本 PR では触れず、§6 backlog 9 に追加した。 + §2 原則 4 (1 PR 1 変更) に従い PR #282 では触れず、§6 backlog 9 に追加した。 + - **post-PR レビュー指摘の採用 (3 件、ユーザー承認済み)**: + - **CodeRabbit Minor**: `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 warning (書式重複)**: `cap_for_log` の `... (N lines truncated)` 書式が + `drain_pipe_capped_reporting` と重複していた。切り詰めの実装自体は共有できない + (streaming vs materialize 済み文字列) が、**書式片は共有できる**という指摘は妥当なので + `lib_subprocess::truncation_notice` として切り出し、両者から使う形にした。 + - **pre-push warning (表記)**: 本節と §8 の T5 行の「本 PR」を PR 番号採番後に backfill。 + T4 行が「本 PR」のまま放置され本 PR で backfill する羽目になった負債を、同じ形で + 繰り返さないため。 ### T6: diff stage の timeout 欠落 @@ -688,5 +702,5 @@ T1 を最優先とする理由: 以降の全 PR の dogfood push が速くなり | T0 | 実装・マージ済 (PR #278) | 2026-07-16 | stage 別ログ `stage= elapsed=<秒>s` を追加 (§5 T0 実施結果)。before 値は §1 表を使用。初回実測で T1 の前提に疑義 → §5 T1 の申し送り参照 | | T1 | 実装済 (PR #279) | 2026-07-16 | `LINT_SCREEN_EVALS` env opt-in で eval を gate から除外。`--ignored` 63s → 21s。step_timeout 600 → 300 (実測 right-size)。**判断根拠**: 着手前実測で (b) 41.3s が (a) 63s の 65% を占め、申し送りの判定基準「前提は生きている」に該当したため実施。ただし絶対値が 269s の約 1/4 だったため期待効果を -2〜4.5 分 → -42s に下方修正し、§1 結論の (1)「主犯」認定も修正した。実行の本丸は (2)(3) = T10/T12 | | T8 | 実装済 (PR #280) | 2026-07-17 | 「`@` 空 + bookmark が `@-`」を `NoBookmarks` から切り分け、`jj edit @-` を案内するよう修正。`working_copy_is_empty()` を advance と共有して規則の二重定義を解消。`main.rs` 側の重複案内も撤去。回帰テスト 12 本 + サンドボックス実機で before/after 比較 (§4 T8 実施結果)。**post-PR 修正 (CodeRabbit Major 2 件 + simplicity 警告 2 件を採用)**: (a) 判定順を反転し、bookmark が空の `@` にある場合も中断する — 続行すると `jj diff -r @` が空になり祖先の未 push 変更が AI レビューを経ずに push される (方針 2 を却下した理由と同じ穴が bookmark の位置違いで残っていた)。(b) `@-` 照会の失敗を `unwrap_or_default()` で「親はあるが bookmark 無し」に潰していたのを `ParentState::Unavailable` として保持し、親を確認できない場合は実行不能な `jj edit @-` を案内しない (T8 が直したはずの誤誘導の再生産だった)。あわせて simplicity-review の非ブロッキング警告 2 件も採用: (c) `query_parent_state()` の jj 失敗を log する (他の jj 失敗処理の慣習と揃える)。(d) `@-` に bookmark が無い場合は `jj edit @-` だけでは次に `NoBookmarks` で止まるため、bookmark 作成まで含めて案内する。Minor 1 件のうち日付指摘は CodeRabbit が UTC 基準のため不採用 (本 repo の記録は JST 基準で 2026-07-17 が正)。**dogfood 実証**: 本修正の push 作業中に、私自身が `jj new` で空コミットを作ってしまい T8 の incident 状態を再現したが、修正後の bookmark_check が破壊的な `jj bookmark create -r @` ではなく正しい `jj edit @-` を案内し、案内どおりの操作で復旧できた (修正が in the wild で機能することの実証)。**方針変更**: 方針 2 (`@-` を検査対象にして続行) は `[diff] command = "jj diff -r @"` により **takt レビューを無言 skip して push する**ことが判明したため不採用。方針 3 の「push すべき新変更がない」も再現記録の事実 4 (実際に push 成功 = 変更はあった) と矛盾するため不採用。exit 7 は維持し**案内文のみ正す**方針をユーザー承認のうえ採用した。**実施順**: T4-T7 を飛ばして T1 の次に実施 (T1 の dogfood push で再現が取れたタイミングを優先。T8 は他タスクと独立のため順序入替は無害) | -| T5 | 実装済 (本 PR) | 2026-07-17 | push 拒否検知を 40 行 truncate 済み出力から**全量出力**に切替。`lib-subprocess` に `run_cmd_shell_unlimited` を追加 (`drain_pipe_unlimited` は pipe 単体、`_capped_reporting` は cap が残るためどちらも判定用には不足だった) し、3 variant の共通骨格を `run_cmd_shell_with` に集約。境界判定は ADR-044 §「後続の variant 追加」に記録。表示は成功時のみ `cap_for_log` で 40 行 + 超過明示に絞り、**失敗経路は全量表示** (診断情報を落とさない)。**副産物**: 唯一の呼び出し元が消えた `runner::run_stage_cmd` を削除 — dead code 除去に加え「capped 経路で control flow 判定する」罠の構造的排除。`MAX_LINES` は表示用として残置し doc に判定禁止を明記。**厳格化は不採用 (ユーザー承認済み)**: `contains` 誤爆の厳格化はリスクが非対称 (誤検知は出力表示で気付けるが、検知漏れは「リモート未反映のまま exit 0」= 本タスクが防ぐ事故そのもの) で ADR-043 (fail-closed) に反するため見送り、§6 backlog 8 を却下記録に変更。**回帰テスト**: `mod t5_truncated_refusal_detection` 6 本 + lib-subprocess 4 本。`run_push_cmd` を capped に戻すと 3 本が fail することを確認済み (回帰テストが素通りしないことの実証)。**サンドボックス実機で before/after 比較**: 拒否行を 41 行目に置いた fake push command で、before = `[push] 成功` + **exit 0 (silent failure 再現)** / after = 拒否検知 + exit 3。成功経路 (50 行) は 40 行 + `... (10 lines truncated)` 表示で exit 0 を維持。**発見 (本タスク外)**: `cli-pr-monitor` の `push_to_remote` は拒否検知が無く同型の穴 → §6 backlog 9 に追加 (1 PR 1 変更のため別 PR)。**実施順**: 計画の推奨順どおり T4 の次に実施 | +| T5 | 実装済 (PR #282) | 2026-07-17 | push 拒否検知を 40 行 truncate 済み出力から**全量出力**に切替。`lib-subprocess` に `run_cmd_shell_unlimited` を追加 (`drain_pipe_unlimited` は pipe 単体、`_capped_reporting` は cap が残るためどちらも判定用には不足だった) し、3 variant の共通骨格を `run_cmd_shell_with` に集約。境界判定は ADR-044 §「後続の variant 追加」に記録。表示は成功時のみ `cap_for_log` で 40 行 + 超過明示に絞り、**失敗経路は全量表示** (診断情報を落とさない)。**副産物**: 唯一の呼び出し元が消えた `runner::run_stage_cmd` を削除 — dead code 除去に加え「capped 経路で control flow 判定する」罠の構造的排除。`MAX_LINES` は表示用として残置し doc に判定禁止を明記。**厳格化は不採用 (ユーザー承認済み)**: `contains` 誤爆の厳格化はリスクが非対称 (誤検知は出力表示で気付けるが、検知漏れは「リモート未反映のまま exit 0」= 本タスクが防ぐ事故そのもの) で ADR-043 (fail-closed) に反するため見送り、§6 backlog 8 を却下記録に変更。**回帰テスト**: `mod t5_truncated_refusal_detection` 6 本 + lib-subprocess 4 本。`run_push_cmd` を capped に戻すと 3 本が fail することを確認済み (回帰テストが素通りしないことの実証)。**サンドボックス実機で before/after 比較**: 拒否行を 41 行目に置いた fake push command で、before = `[push] 成功` + **exit 0 (silent failure 再現)** / after = 拒否検知 + exit 3。成功経路 (50 行) は 40 行 + `... (10 lines truncated)` 表示で exit 0 を維持。**発見 (本タスク外)**: `cli-pr-monitor` の `push_to_remote` は拒否検知が無く同型の穴 → §6 backlog 9 に追加 (1 PR 1 変更のため別 PR)。**post-PR 修正 (CodeRabbit Minor 1 件 + pre-push 非ブロッキング警告 2 件を採用)**: (a) `run_cmd_shell_capped` の doc「Err 経路で child を kill しない」は **pre-existing の stale 記述** (`kill_and_join_err` 導入 = PR #208 以降、実際は kill + reap + reader thread join している) だったため、child lifecycle の記述を 3 variant 共通の骨格 `run_cmd_shell_with` に集約し variant 側は参照のみにした。(b) `cap_for_log` の truncate 書式重複を `lib_subprocess::truncation_notice` として切り出し (実装は streaming vs materialize で共有できないが書式片は共有できる、という指摘は妥当)。(c) T5 行 / §4 の「本 PR」を PR #282 に backfill (T4 行が放置され本 PR で backfill する羽目になった負債を繰り返さないため)。**実施順**: 計画の推奨順どおり T4 の次に実施 | | T4 | 実装済 (PR #281) | 2026-07-17 | `push-runner-config.toml` の `refute_enabled = false → true` で dogfood 開始 (変更は方針どおり 1 行、templates は OFF 据え置き)。**dogfood 開始日を同 PR で固定**: ADR-039 bounded lifetime の起点が無いと 2 週間の期限が判定不能になるため、開始 2026-07-17 → **判定期限 2026-07-31** を ADR-047 (ステータス行 / Config opt-in / Bounded lifetime の 3 箇所) + config コメントに明記した。採否判定自体は本計画と独立に ADR-047 で進行する (§8 完了条件 4. の引き継ぎ先)。**初回 dogfood push で切替を実証** (PR #281 自身の push): 起動ログ `takt (pre-push-review-refute)` + takt の `ワークフロー 'pre-push-review-refute' を起動` → 完走を確認。合計 151s (pre_checks 1.3s / quality_gate 49.7s / diff 0.1s / takt 97.8s / push 2.2s)。**verify は予告どおり未発火**: reviewers 2 本とも APPROVE で `all("approved") → COMPLETE` に抜けたため `any("needs_fix") → verify` に入らず、verify 実動の観測は次の findings 発生 run に持ち越し (完了条件は「有効化が効いていることの確認」までで読む)。**副産物: 計測手順の誤りを発見・修正**。ADR-047 §dogfood 計測項目の `.takt/runs/*-pre-push-review-refute/trace.md` は **1 件もマッチしない** — run ディレクトリ名は workflow 名でなく task 名から作られ (`runSlug` = `-pre-push-review`)、refute run でも `20260716-182505-pre-push-review` になる (timestamp も UTC で JST の日付と 1 日ずれ得る)。放置すると 2026-07-31 の採否判定で run 0 件 →「データなし」誤読の恐れがあったため、ADR-047 と §5 T4 実施結果を `meta.json` の `piece` フィールド基準 (`grep -l '"piece": "pre-push-review-refute"' .takt/runs/*/meta.json`) に修正した。設計時の計測手順が実運用開始まで未検証だった例。有効化前の静的確認 (refute workflow / facet 群の存在、`resolve_takt_workflow` の unit test 4 本、`cli-pr-monitor` への波及なし、Rust 変更ゼロのため exe 再ビルド不要) は §5 T4 実施結果に記載。**実施順**: T5-T7 を飛ばして T8 の次に実施 (T4 は他タスクと独立の XS で、依存なし) | diff --git a/src/cli-push-runner/src/stages/push.rs b/src/cli-push-runner/src/stages/push.rs index 1b2c7c9b..97c5c63d 100644 --- a/src/cli-push-runner/src/stages/push.rs +++ b/src/cli-push-runner/src/stages/push.rs @@ -1,4 +1,4 @@ -use lib_subprocess::run_cmd_shell_unlimited; +use lib_subprocess::{run_cmd_shell_unlimited, truncation_notice}; use super::push_jj_bookmark::advance_jj_bookmarks; use crate::config::{PushConfig, DEFAULT_PUSH_TIMEOUT_SECS}; @@ -70,8 +70,8 @@ fn print_output(output: &str) { /// (= 従来のログ量を維持しつつ、判定は truncate の影響を受けない)。失敗経路では /// 診断情報を落とさないため本関数を通さず全量を出す。 /// -/// truncate 表記は `lib_subprocess::drain_pipe_capped_reporting` に合わせる (ログ上の -/// 見え方を統一する)。あちらは pipe を streaming しながら数えるため実装は共有できない。 +/// truncate 表記は `lib_subprocess::truncation_notice` を共有し、`drain_pipe_capped_reporting` +/// (pipe を streaming しながら数える版) とログ上の見え方を揃える。 fn cap_for_log(output: &str) -> String { let mut lines = output.lines(); let head: Vec<&str> = lines.by_ref().take(MAX_LINES).collect(); @@ -79,13 +79,7 @@ fn cap_for_log(output: &str) -> String { if truncated == 0 { return head.join("\n"); } - let suffix = if truncated == 1 { "" } else { "s" }; - format!( - "{}\n... ({} line{} truncated)", - head.join("\n"), - truncated, - suffix - ) + format!("{}\n{}", head.join("\n"), truncation_notice(truncated)) } /// 検出済み bookmark から push コマンドを組み立てる (ADR-045 事故 follow-up)。 diff --git a/src/lib-subprocess/src/lib.rs b/src/lib-subprocess/src/lib.rs index aeda3140..9ccacbfa 100644 --- a/src/lib-subprocess/src/lib.rs +++ b/src/lib-subprocess/src/lib.rs @@ -168,6 +168,17 @@ pub fn drain_pipe_capped( }) } +/// 切り捨てた行数を報告する注記行を作る (`"... (N lines truncated)"`)。 +/// +/// 出力を「先頭 N 行 + 超過の明示」に切り詰める箇所で表記を揃えるための共有点。 +/// 切り詰めの実装自体は共有できない (pipe を streaming しながら数える +/// `drain_pipe_capped_reporting` と、materialize 済み文字列を切る callsite では +/// 構造が異なる) が、**ログ上の見え方は一致させる**。 +pub fn truncation_notice(truncated_lines: usize) -> String { + let suffix = if truncated_lines == 1 { "" } else { "s" }; + format!("... ({} line{} truncated)", truncated_lines, suffix) +} + /// 子プロセスの stdout / stderr パイプを別スレッドで最大 `max_lines` 行まで読込し、 /// 超過があれば末尾に `"... (N lines truncated)"` を付与する **reporting capped** variant。 /// @@ -202,8 +213,7 @@ pub fn drain_pipe_capped_reporting( } } if truncated > 0 { - let suffix = if truncated == 1 { "" } else { "s" }; - collected.push(format!("... ({} line{} truncated)", truncated, suffix)); + collected.push(truncation_notice(truncated)); } collected.join("\n") }) @@ -231,9 +241,15 @@ fn kill_and_join_err( /// `run_cmd_shell_*` 3 variant の共通骨格 (spawn → drain → wait → combine)。 /// /// variant 間の差は `drain` (= どの `drain_pipe_*` を使うか) だけで、それ以外の -/// semantics — timeout メッセージ書式 / Err 経路の child 扱い / 戻り値の意味 — は +/// semantics — timeout メッセージ書式 / child の lifecycle / 戻り値の意味 — は /// 3 variant で共通。stdout と stderr は型が異なる (`ChildStdout` / `ChildStderr`) ため、 /// 同一の drain 戦略を両方へ適用できるよう `Box` に統一して渡す。 +/// +/// child の lifecycle: 待機は `wait_with_timeout_basic` (= try_wait 失敗時に child を +/// kill しない basic semantics) を使うが、その Err 経路は本関数が `kill_and_join_err` で +/// 受け、child の kill + reap と reader thread の join を行う。timeout 経路は +/// `wait_with_timeout_basic` 自身が kill + reap する。結果として **どの失敗経路でも +/// child は残らない**。 fn run_cmd_shell_with(label: &str, cmd: &str, timeout_secs: u64, drain: F) -> (bool, String) where F: Fn(Box) -> JoinHandle, @@ -284,7 +300,7 @@ where /// (判定対象の行が cap の外に落ちても戻り値からは判別できず、無言で誤判定する)。 /// その場合は `run_cmd_shell_unlimited` を使う。 /// -/// 内部で `wait_with_timeout_basic` を使用 (= Err 経路で child を kill しない basic semantics)。 +/// child の lifecycle は 3 variant 共通 (`run_cmd_shell_with` の doc を参照)。 pub fn run_cmd_shell_capped( label: &str, cmd: &str, @@ -447,6 +463,16 @@ mod tests { assert_eq!(handle.join().unwrap(), "only\ntwo"); } + #[test] + fn truncation_notice_uses_plural_form_for_multiple_lines() { + assert_eq!(truncation_notice(2), "... (2 lines truncated)"); + } + + #[test] + fn truncation_notice_uses_singular_form_for_one_line() { + assert_eq!(truncation_notice(1), "... (1 line truncated)"); + } + #[test] fn drain_pipe_capped_reporting_appends_truncation_summary_when_over_cap() { let input = Cursor::new(b"a\nb\nc\nd\ne\n".to_vec());