feat: enforce hard approval gate for dual-core flow - #2
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4929351694
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cat "$approval_file" | ||
| echo "---------------------------------------------" | ||
|
|
||
| if ! grep -Eiq '^[[:space:]\-*]*Approved([[:space:]]*\(Y/N\))?[[:space:]]*:[[:space:]]*Y([[:space:]]|$)' "$approval_file"; then |
There was a problem hiding this comment.
The approval checks use the ERE class [[:space:]\-*] (here and in the Reason/Timestamp checks), which is an invalid bracket expression for grep -E and raises Invalid range end on GNU grep. In that case grep exits with status 2, so the script always treats even valid APPROVAL.md files as failed and never reaches phase 2.
Useful? React with 👍 / 👎.
| "${phase1_cmd[@]}" | ||
|
|
||
| mapfile -t approval_candidates < <( | ||
| find "$PWD" "$(dirname "$PWD")" -maxdepth 4 -type f -name APPROVAL.md -newer "$marker_file" 2>/dev/null |
There was a problem hiding this comment.
The discovery command searches both the current directory and its parent recursively, then selects the newest APPROVAL.md; in a workspace with sibling repos/worktrees, a newer unrelated approval file can be selected and later used to derive run_dir for phase 2. That can cause the implementation phase to run in the wrong repository context instead of the one just approved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR introduces a hard “human approval” gate for the dual-core TAKT workflow by splitting the legacy single-run piece into two distinct phases and adding a wrapper script to enforce APPROVAL.md validation + an explicit Y/n confirmation before implementation.
Changes:
- Added two new TAKT pieces: a phase-1 approval-only flow and a phase-2 implementation-only flow.
- Added a wrapper script to run phase 1, verify
APPROVAL.mdfields/approval, prompt for confirmation, then run phase 2. - Updated the setup/runbook documentation and clarified the legacy
dual-corepiece as backward-compatible.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/takt-run-approved.sh |
New wrapper to enforce the two-phase execution and hard approval gate before implementation. |
Takt_setup.md |
Updates runbook and validation scenarios to reflect the new two-phase hard-gate workflow. |
.takt/pieces/dual-core.yaml |
Rewords legacy piece description to reflect it as a single-run legacy flow. |
.takt/pieces/dual-core-approval.yaml |
New phase-1 piece (plan → audit → approval → stop). |
.takt/pieces/dual-core-apply.yaml |
New phase-2 piece (implement → fix), including a “blocked → ABORT” path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case "$arg" in | ||
| -w|--piece) | ||
| expect_piece_value=true | ||
| ;; |
There was a problem hiding this comment.
The wrapper strips -w/--piece (and later --piece=...) but silently ignores the provided value. This makes ./scripts/takt-run-approved.sh -w dual-core ... misleading and also lets typos go unnoticed. Consider rejecting these flags with a clear error, or validating the value is exactly dual-core and failing otherwise.
| if [[ ${#approval_candidates[@]} -eq 0 && -f APPROVAL.md ]]; then | ||
| approval_candidates+=("$(pwd)/APPROVAL.md") | ||
| fi |
There was a problem hiding this comment.
The fallback to use an existing APPROVAL.md when none is found newer than marker_file can allow phase 2 to proceed using a stale approval packet that wasn’t produced/updated by the phase-1 run. This undermines the “hard approval gate” guarantee. Suggest removing this fallback, or at least requiring the approval file to be updated during phase 1 (or requiring an explicit --allow-stale-approval override).
| "${phase1_cmd[@]}" | ||
|
|
||
| mapfile -t approval_candidates < <( | ||
| find "$PWD" "$(dirname "$PWD")" -maxdepth 4 -type f -name APPROVAL.md -newer "$marker_file" 2>/dev/null |
There was a problem hiding this comment.
Searching for APPROVAL.md via find "$PWD" "$(dirname "$PWD")" ... can pick up unrelated approval packets (e.g., from other worktrees under the same parent) and then drive phase 2 from the wrong directory. Consider narrowing the search to the known worktree/run directory from phase 1, or failing (with a list) when multiple candidates are found rather than guessing.
| find "$PWD" "$(dirname "$PWD")" -maxdepth 4 -type f -name APPROVAL.md -newer "$marker_file" 2>/dev/null | |
| find "$PWD" -maxdepth 4 -type f -name APPROVAL.md -newer "$marker_file" 2>/dev/null |
| exit 1 | ||
| fi | ||
|
|
||
| marker_file="$(mktemp)" |
There was a problem hiding this comment.
Because the gate relies on find ... -newer "$marker_file", files created within the same mtime resolution as marker_file can be missed on some filesystems, leading to a false "APPROVAL.md not found" failure. Consider setting the marker timestamp safely in the past (or switching to checking APPROVAL.md in an explicit run directory instead of an mtime heuristic).
| marker_file="$(mktemp)" | |
| marker_file="$(mktemp)" | |
| # Set the marker file's mtime slightly in the past so that, even on | |
| # filesystems with coarse timestamp resolution, files created during | |
| # Phase 1 are reliably "newer" than this marker. | |
| touch -d '1 second ago' "$marker_file" 2>/dev/null || true |
|
|
||
| ```bash | ||
| takt -w dual-core --create-worktree yes --auto-pr | ||
| ./scripts/takt-run-approved.sh -w dual-core --create-worktree yes --auto-pr |
There was a problem hiding this comment.
The runbook recommends ./scripts/takt-run-approved.sh -w dual-core ..., but the wrapper ignores the provided -w/--piece value. To avoid confusion, update the documented command to omit -w dual-core (or document that it’s accepted but validated/ignored).
| ./scripts/takt-run-approved.sh -w dual-core --create-worktree yes --auto-pr | |
| ./scripts/takt-run-approved.sh --create-worktree yes --auto-pr |
Summary
dual-core-approvalanddual-core-applyscripts/takt-run-approved.shwrapper to keep one-command UXY/n) and APPROVAL.md field checks before implementationdual-coreas legacy flow for backward compatibilityTakt_setup.mdrunbook and validation scenariosValidation
bash -n scripts/takt-run-approved.shtakt prompt dual-core-approval(known takt 0.13.0 preview warning remains)takt prompt dual-core-apply(known takt 0.13.0 preview warning remains)