[#761] add-command-gate#763
Open
nrslib wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughワークフローの品質ゲートに type: command を追加し、設定スキーマ／型、YAML ローダーとポリシー検証、コマンド実行ランナー、失敗メッセージ整形、ワークフロー実行ループへの統合、worktree 同期、ドキュメントと広範なテスト群を導入しました。 ChangesCommand Quality Gates
🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/infra/config/loaders/qualityGateOverrides.ts (1)
111-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCommand gate の重複除去が参照同一性になっています。
Line 112 の
new Set(gates)は object を値比較しないため、同内容の command gate がレイヤ間で重複実行されます。非冪等コマンドだと副作用を二重化します。文字列/command で正規化キーを作って重複判定してください。修正例
- // Deduplicate gates (same text = same gate) - const uniqueGates = Array.from(new Set(gates)); + // Deduplicate gates by semantic identity + const seen = new Set<string>(); + const uniqueGates: QualityGate[] = []; + for (const gate of gates) { + const key = + typeof gate === 'string' + ? `s:${gate}` + : `c:${gate.type}:${gate.name ?? ''}:${gate.command}:${gate.cwd ?? ''}:${gate.timeoutMs ?? ''}`; + if (seen.has(key)) { + continue; + } + seen.add(key); + uniqueGates.push(gate); + }🤖 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/infra/config/loaders/qualityGateOverrides.ts` around lines 111 - 113, The current deduplication uses new Set(gates) which compares by reference so identical command gate objects across layers aren’t deduped; change deduplication to build a normalized key for each gate (e.g., serialize its command string and relevant fields such as args/env or a joined "command|args" token) and use a Map or object to keep the first seen gate per key, then produce uniqueGates from the Map.values(); update the logic where gates is read and where uniqueGates is produced to use this key-based de-duplication so semantically-equal command gates aren’t executed twice.src/infra/task/projectLocalTaktSync.ts (1)
85-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
quality-gates/logsが同期時に削除されてしまいます
sourceEntriesからlogsを除外している一方、Line 86 の削除フェーズで除外考慮がないため、targetDir/logsが「未存在扱い」で消されます。生成物ディレクトリを同期対象外にする意図と逆挙動です。修正案
- for (const entry of fs.readdirSync(targetDir)) { + for (const entry of fs.readdirSync(targetDir)) { + if (shouldSkipTaktSyncEntry(sourceDir, entry)) { + continue; + } if (!sourceEntries.has(entry)) { removePath(path.join(targetDir, entry)); } }🤖 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/infra/task/projectLocalTaktSync.ts` around lines 85 - 89, The deletion loop mistakenly removes excluded directories because it iterates fs.readdirSync(targetDir) without applying shouldSkipTaktSyncEntry; update the loop in projectLocalTaktSync.ts to skip entries that should be excluded (call shouldSkipTaktSyncEntry(targetDir, entry) or reuse the same exclusion logic) before calling removePath on path.join(targetDir, entry) so that excluded names like "logs" are not treated as missing and deleted.src/__tests__/system-workflow-schema.test.ts (1)
1037-1060: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick wincommand gate オブジェクトの受理ケースも固定してください。
このテストで通している
quality_gatesは従来の文字列だけなので、新規の{ type: 'command', ... }形式が step schema 側で壊れてもここでは検出できません。少なくとも 1 件は文字列 gate と command gate を混在させたケースを追加しておきたいです。🤖 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/__tests__/system-workflow-schema.test.ts` around lines 1037 - 1060, The test currently validates only string quality gates so add a case that mixes a string gate and a command gate object to ensure the schema accepts both forms; update the test named "structured_output.schema_ref と delay_before_ms を受け付ける" to include quality_gates: ['Review before finishing', { type: 'command', name: 'run_lint', args: { fix: true } }] (or similar valid command gate shape) and assert the parsed step. Use the existing WorkflowStepRawSchema.safeParse call and the step variable assertions (e.g., expect(step.quality_gates)).
🤖 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/__tests__/engine-rate-limit-fallback.test.ts`:
- Around line 683-745: The test may pass without the command gate ever running;
after awaiting engine.run() add an assertion that the gate marker file was
created to guarantee the command gate executed — check
fs.existsSync(gateMarkerPath) (or require/import fs and use fs.existsSync) and
assert it is true (e.g., expect(fs.existsSync(gateMarkerPath)).toBe(true)) so
the test cannot be a false positive; reference the existing gateMarkerPath
variable and the engine.run() call when adding this assertion.
In `@src/__tests__/it-workflow-loader.test.ts`:
- Around line 1298-1441: Add tests in the same suite to cover
workflow_command_gates precedence by exercising loadWorkflowConfig with
conflicting global vs project settings: create one test where global config
enables command gates (global workflow_command_gates: { custom_scripts: true })
but the project's .takt/config.yaml explicitly sets
workflow_command_gates.custom_scripts: false and assert loadWorkflowConfig
rejects command gates (throws /workflow_command_gates\.custom_scripts/ or
step-specific rejection); add another test where global enables but project has
an empty object (workflow_command_gates: {}) and assert the effective behavior
(allow or reject) matches policy by checking loaded config and step. Use the
existing patterns in the file (mkdirSync, writeFileSync, loadWorkflowConfig,
expect(...).toThrow / expect(...).not.toBeNull) and the same YAML templates
(command-gate.yaml / parallel-command-gate-allowed.yaml) to implement these two
cases.
In `@src/core/workflow/quality-gates/commandGateRunner.ts`:
- Around line 202-229: The stdout and stderr handlers currently track output
size separately using stdoutBytes and stderrBytes, allowing each stream to reach
the limit independently; change to a single shared counter (e.g., totalBytes)
used by both handlers and by appendOutputWithinLimit so any stream pushing the
cumulative total over the limit sets outputLimitExceeded and calls
terminateProcess immediately; update references to stdoutBytes/stderrBytes to
use totalBytes and ensure both child.stdout?.on('data', ...) and
child.stderr?.on('data', ...) call appendOutputWithinLimit with the shared total
and update that shared variable from the returned bytes.
---
Outside diff comments:
In `@src/__tests__/system-workflow-schema.test.ts`:
- Around line 1037-1060: The test currently validates only string quality gates
so add a case that mixes a string gate and a command gate object to ensure the
schema accepts both forms; update the test named "structured_output.schema_ref と
delay_before_ms を受け付ける" to include quality_gates: ['Review before finishing', {
type: 'command', name: 'run_lint', args: { fix: true } }] (or similar valid
command gate shape) and assert the parsed step. Use the existing
WorkflowStepRawSchema.safeParse call and the step variable assertions (e.g.,
expect(step.quality_gates)).
In `@src/infra/config/loaders/qualityGateOverrides.ts`:
- Around line 111-113: The current deduplication uses new Set(gates) which
compares by reference so identical command gate objects across layers aren’t
deduped; change deduplication to build a normalized key for each gate (e.g.,
serialize its command string and relevant fields such as args/env or a joined
"command|args" token) and use a Map or object to keep the first seen gate per
key, then produce uniqueGates from the Map.values(); update the logic where
gates is read and where uniqueGates is produced to use this key-based
de-duplication so semantically-equal command gates aren’t executed twice.
In `@src/infra/task/projectLocalTaktSync.ts`:
- Around line 85-89: The deletion loop mistakenly removes excluded directories
because it iterates fs.readdirSync(targetDir) without applying
shouldSkipTaktSyncEntry; update the loop in projectLocalTaktSync.ts to skip
entries that should be excluded (call shouldSkipTaktSyncEntry(targetDir, entry)
or reuse the same exclusion logic) before calling removePath on
path.join(targetDir, entry) so that excluded names like "logs" are not treated
as missing and deleted.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: d9356dd1-44c7-40e7-9742-7122d3a94e22
⛔ Files ignored due to path filters (1)
.takt/config.yamlis excluded by!.takt/**
📒 Files selected for processing (55)
builtins/en/config.yamlbuiltins/ja/config.yamlbuiltins/skill-codex/references/yaml-schema.mdbuiltins/skill/references/yaml-schema.mddocs/configuration.ja.mddocs/configuration.mddocs/workflows.ja.mddocs/workflows.mdsrc/__tests__/clone.test.tssrc/__tests__/commandQualityGateRunner.test.tssrc/__tests__/config-env-overrides.test.tssrc/__tests__/engine-parallel.test.tssrc/__tests__/engine-rate-limit-fallback.test.tssrc/__tests__/globalConfig-defaults.test.tssrc/__tests__/globalConfig.test.tssrc/__tests__/it-instruction-builder.test.tssrc/__tests__/it-workflow-loader.test.tssrc/__tests__/models.test.tssrc/__tests__/projectConfig.test.tssrc/__tests__/projectLocalTaktSync.test.tssrc/__tests__/qualityGateOverrides.test.tssrc/__tests__/system-workflow-schema.test.tssrc/__tests__/workflow-call-schema.test.tssrc/__tests__/workflow-run-loop-command-gates.test.tssrc/core/models/config-schemas.tssrc/core/models/config-types.tssrc/core/models/index.tssrc/core/models/schema-base.tssrc/core/models/types.tssrc/core/models/workflow-types.tssrc/core/workflow/engine/ParallelRunner.tssrc/core/workflow/engine/WorkflowEngine.tssrc/core/workflow/engine/WorkflowEngineSetup.tssrc/core/workflow/engine/WorkflowRunLoop.tssrc/core/workflow/instruction/InstructionBuilder.tssrc/core/workflow/quality-gates/commandGateMessage.tssrc/core/workflow/quality-gates/commandGateRunner.tssrc/core/workflow/quality-gates/qualityGateRunner.tssrc/core/workflow/quality-gates/types.tssrc/core/workflow/types.tssrc/infra/config/configNormalizers.tssrc/infra/config/env/global-current-env-specs.tssrc/infra/config/env/project-current-env-specs.tssrc/infra/config/global/globalConfigCore.tssrc/infra/config/global/globalConfigSerializer.tssrc/infra/config/loaders/qualityGateOverrides.tssrc/infra/config/loaders/workflowFileLoader.tssrc/infra/config/loaders/workflowNormalizationPolicies.tssrc/infra/config/loaders/workflowParser.tssrc/infra/config/loaders/workflowStepNormalizer.tssrc/infra/config/project/projectConfig.tssrc/infra/config/project/projectConfigTransforms.tssrc/infra/config/traced/tracedConfigSchema.tssrc/infra/task/clone.tssrc/infra/task/projectLocalTaktSync.ts
Owner
Author
|
レビューしました。GitHub 上は mergeable で CI も通っていますが、現時点ではまだマージしない方がよいです。 主な理由:
npm test -- --run src/__tests__/commandQualityGateRunner.test.ts src/__tests__/workflow-run-loop-command-gates.test.ts src/__tests__/projectLocalTaktSync.test.ts src/__tests__/it-workflow-loader.test.ts失敗箇所は
CodeRabbit の 結論: 少なくとも |
Owner
Author
Summaryタスク指示書: PR #763 command gate のレビュー指摘対応目的
参照資料最初に以下を確認すること。
関連箇所の特定には以下も使用すること。 rg "isPathInside|isRealPathInside|realpath|quality_gates|qualityGate|workflow_command_gates|stdoutBytes|stderrBytes|outputLimitExceeded"優先度: 高1. command gate の
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/workflow/quality-gates/commandGateRunner.ts (1)
60-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick win出力上限判定が「超過」ではなく「到達」で失敗しています。
Line 60 の条件が
< MAX_OUTPUT_BYTESになっており、合計出力がちょうど上限のケースでもoutputLimitExceededになります。要件どおり「超過時のみ失敗」に合わせるなら<=にしてください（境界テスト側の期待値も合わせて更新が必要です）。修正案
- if (outputBytes + chunkBytes < MAX_OUTPUT_BYTES) { + if (outputBytes + chunkBytes <= MAX_OUTPUT_BYTES) { return { output: `${current}${chunk}`, bytes: outputBytes + chunkBytes, exceeded: false, }; }🤖 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/core/workflow/quality-gates/commandGateRunner.ts` around lines 60 - 73, The slice that decides whether adding chunk triggers the output limit uses the condition `if (outputBytes + chunkBytes < MAX_OUTPUT_BYTES)` which treats an exact-equal-to limit as "exceeded"; change that condition to `<=` so an exact match is allowed (i.e., `if (outputBytes + chunkBytes <= MAX_OUTPUT_BYTES)`), ensure when returning the success branch you return bytes as `outputBytes + chunkBytes`, and keep the overflow branch logic using `MAX_OUTPUT_BYTES`, `boundedChunk`, and `OUTPUT_LIMIT_MARKER` unchanged; update the related boundary tests that expect "exceeded" to instead expect success on exact-equality.
🤖 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.
Outside diff comments:
In `@src/core/workflow/quality-gates/commandGateRunner.ts`:
- Around line 60-73: The slice that decides whether adding chunk triggers the
output limit uses the condition `if (outputBytes + chunkBytes <
MAX_OUTPUT_BYTES)` which treats an exact-equal-to limit as "exceeded"; change
that condition to `<=` so an exact match is allowed (i.e., `if (outputBytes +
chunkBytes <= MAX_OUTPUT_BYTES)`), ensure when returning the success branch you
return bytes as `outputBytes + chunkBytes`, and keep the overflow branch logic
using `MAX_OUTPUT_BYTES`, `boundedChunk`, and `OUTPUT_LIMIT_MARKER` unchanged;
update the related boundary tests that expect "exceeded" to instead expect
success on exact-equality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a64ee529-0a79-42a3-9236-67978fe9c6c7
📒 Files selected for processing (9)
src/__tests__/commandQualityGateRunner.test.tssrc/__tests__/engine-rate-limit-fallback.test.tssrc/__tests__/it-workflow-loader.test.tssrc/__tests__/qualityGateOverrides.test.tssrc/__tests__/system-workflow-schema.test.tssrc/core/models/quality-gate-defaults.tssrc/core/workflow/quality-gates/commandGateMessage.tssrc/core/workflow/quality-gates/commandGateRunner.tssrc/infra/config/loaders/qualityGateOverrides.ts
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
タスク指示書:
quality_gatesに command gate を追加する目的
TAKT の既存
quality_gatesに、ステップ完了時に shell command を機械的に実行するtype: commandgate を追加する。commandgate は exit code0の場合のみ成功とし、exit code0以外の場合は、その実行結果を AI への差し戻し入力として渡す。既存の文字列quality_gatesはそのまま AI へのプロンプト指示として維持し、同じ配列内で command gate と同居できるようにする。参照資料
以下を最初に確認すること。
.takt/config.yamlworkflow_overrides.steps.*.quality_gatesの実例src/infra/task/projectLocalTaktSync.ts.takt/config.yamlなど、worktree 再利用時に同期される.takt資産の定義src/infra/task/clone-exec.tsquality_gates読み込み・プロンプト生成・ステップ完了処理に関係するモジュールrg "quality_gates|qualityGate|quality gate|quality_gate"で実装箇所を特定すること優先度: 高
1.
quality_gatesの型・設定読み込みを拡張する対象モジュール:
quality_gatesを扱う型定義実装内容:
quality_gates配列に以下の object gate を混在可能にする。type: commandの gate を command gate として扱う。type: command以外の既存挙動を壊さない。command gate の最低限の設定項目:
type: "command"name?: stringcommand: stringcwd?: stringtimeout_ms?: number2. ステップ完了時に command gate を実行する
対象モジュール:
実装内容:
type: commandgate を、対象ステップ完了時に機械的に実行する。quality_gates配列の定義順に従う。0の場合のみ成功とする。0以外の場合は gate 失敗として扱い、AI に差し戻す。commandには shell script ファイルパスも指定できるようにする。cwd省略時は worktree のプロジェクトルートで実行する。cwd指定時は、そのcwd上でcommandを実行する。差し戻しに含める情報:
3.
.takt/quality-gates/を worktree に持っていく対象モジュール:
src/infra/task/projectLocalTaktSync.ts.takt資産同期処理実装内容:
.takt/quality-gates/を.takt/config.yamlと同じく worktree 側へ持っていく対象にする。quality-gatesディレクトリを追加する。command: "./.takt/quality-gates/check.sh"は、元リポジトリではなく worktree 内のファイルを指すようにする。.takt/config.yamlの同期挙動は維持する。優先度: 中
4. 失敗ログの整形を追加する
対象モジュール:
実装内容:
5. 設定例を更新する
対象モジュール:
.takt/config.yaml実装内容:
quality_gatesと command gate が同居する例を追加する。.takt/quality-gates/check.shを使う例を示す。type: commandがステップ完了時の機械実行 gate であることが分かる記述にする。優先度: 低
6. サンプル shell 配置の扱いを整理する
対象モジュール:
.takt/quality-gates/実装内容:
.takt/quality-gates/check.shのサンプルを追加する。例:
テスト方針
優先度: 高
追加・更新するテスト:
quality_gatesに文字列と{ type: "command", ... }が混在できること0なら成功1など non-zero なら失敗.takt/quality-gates/が再利用・リトライ時の同期対象になること.takt/config.yamlの既存同期が維持されること必要に応じて追加する統合テスト:
workflow_overrides.steps.implement.quality_gatesに command gate を設定し、ステップ完了後に command が実行されること確認方法
最低限、以下を実行して確認する。
npm run build npm run lint npm testworktree / workflow 実行に関係する変更が入るため、可能であれば以下も実行する。
手動確認例:
.takt/quality-gates/check.shを作成する。.takt/config.yamlのworkflow_overrides.steps.implement.quality_gatesに以下を追加する。check.shが exit code0を返す場合、次の処理へ進むことを確認する。check.shが exit code1を返す場合、AI への差し戻しに command 実行結果が含まれることを確認する。.takt/quality-gates/check.shが worktree 内に存在することを確認する。Execution Report
Workflow
takt-defaultcompleted successfully.Closes #761
Summary by CodeRabbit
新機能
設定
ドキュメント
テスト
動作変更