From 6bba2f72f8c847006dca0d2ee153d39ea81b34bd Mon Sep 17 00:00:00 2001 From: Ahmet Taspinar Date: Sat, 3 Oct 2026 00:12:42 +0200 Subject: [PATCH] Redesign finish-feature.sh as the closing step of a feature finish-feature.sh "" runs in the feature worktree, runs verification, and checks that the latest review is current and has no critical or major finding, that every round with findings has an approved triage published on the Issue, and that the latest round has no FIX_NOW findings left. It then stages the changes and opens a structured, editable commit message. --no-review records the reason for a low-risk change. It never pushes or merges. triage-review.sh records the publication in the triage artifact and can publish a stored triage again with --publish. Closes #48 Co-Authored-By: Claude Opus 5.5 --- README.md | 8 +- docs/agentic-workflow.md | 2 +- docs/development.md | 45 ++++++-- scripts/finish-feature.sh | 167 +++++++++++++++++++++++++-- scripts/lib/review-data.sh | 5 +- scripts/triage-review.sh | 184 ++++++++++++++++++----------- scripts/verify.conf | 1 + tests/finish-feature-test.sh | 216 +++++++++++++++++++++++++++++++++++ tests/triage-review-test.sh | 18 ++- 9 files changed, 558 insertions(+), 88 deletions(-) create mode 100755 tests/finish-feature-test.sh diff --git a/README.md b/README.md index bde7b06..c29257f 100644 --- a/README.md +++ b/README.md @@ -74,6 +74,12 @@ A lightweight, model-agnostic repository template for agentic software engineeri the resulting implementation. Approved `DEFER` findings become linked follow-up Issues; `ACCEPT` findings retain their rationale in the triage artifact. -9. Commit, push, and open a PR containing `Closes #12`. After CI and required gates pass, merge the PR and clean up the worktree. +9. After a new review round confirms the fixes, commit with the closing checks: + + ```bash + ./scripts/finish-feature.sh 12 "Implement player movement" + ``` + + Then push and open a PR containing `Closes #12`. After CI and required gates pass, merge the PR and clean up the worktree. See `docs/development.md` for commands, `docs/agentic-workflow.md` for the lifecycle, and `.agents/policies/` for boundaries. diff --git a/docs/agentic-workflow.md b/docs/agentic-workflow.md index 9a08740..8cb24fc 100644 --- a/docs/agentic-workflow.md +++ b/docs/agentic-workflow.md @@ -9,7 +9,7 @@ ready work. Roadmap item → GitHub Issue → feature plan (when warranted) → isolated branch/worktree → implementation → local verification → independent review when required → review triage → approved fix-now application → -verification/re-review when needed → commit → push/PR → CI → human gate where +verification/re-review when needed → `finish-feature.sh` → push/PR → CI → human gate where required → merge → automatic Issue closure → cleanup. Independent review happens before the implementation commit so it can include diff --git a/docs/development.md b/docs/development.md index b30c009..f40358c 100644 --- a/docs/development.md +++ b/docs/development.md @@ -481,12 +481,42 @@ Check a review yourself with: It exits 0 when the review is current, 1 when it is stale, and 2 on an error. -Verify again after review fixes, then commit and push: +### Finishing a feature + +When the latest review round is resolved, finish the feature from its +worktree: + +```bash +./scripts/finish-feature.sh 12 "Implement player movement" +``` + +The script refuses another branch than `feature/12-*` or a tree without +changes, runs `./scripts/verify.sh`, and checks the latest review of the +feature: it must be current and have no critical or major finding. Every +review round with findings needs an approved triage that was published on the +Issue (the newest triage of that round counts), and the latest round may have +no `FIX_NOW` findings left to apply. After fixes, run a new review round first; +a passed newer round confirms the fixes of earlier rounds. A failed +publication can be repeated with `./scripts/triage-review.sh --publish +`. + +It then stages all changes (review and triage files are ignored) and opens a +structured commit message in your editor: the summary, the Issue, a list of +changes to fill in, the verification, the review round and verdict, and +`Refs #12`. Emptying the message aborts the commit and leaves the changes +staged. The script never pushes, opens a PR, or merges. + +A change that `.agents/policies/autonomy.md` classifies as low risk may be +finished without an independent review; the reason is recorded in the commit +message: + +```bash +./scripts/finish-feature.sh 12 "Fix a typo in the README" --no-review "documentation only" +``` + +Then push and open the pull request: ```bash -./scripts/verify.sh -git add . -git commit -m "Implement player movement" git push -u origin feature/12-player-movement ``` @@ -515,8 +545,5 @@ repository-relative plan path; the detailed plan remains in `.agents/plans/`. Roadmap item → GitHub Issue → optional implementation plan → isolated feature worktree → implementation → verification → independent review when required → triage → apply approved `FIX_NOW` findings → verification/re-review when needed -→ commit → push/PR → CI and gates → merge → automatic Issue closure → worktree -cleanup. - -Do not use `scripts/finish-feature.sh` as part of this flow until it has been -redesigned or deprecated; its interface predates the current review script. +→ `finish-feature.sh` (checks and commit) → push/PR → CI and gates → merge → +automatic Issue closure → worktree cleanup. diff --git a/scripts/finish-feature.sh b/scripts/finish-feature.sh index 60f1263..4e9f8be 100755 --- a/scripts/finish-feature.sh +++ b/scripts/finish-feature.sh @@ -1,10 +1,163 @@ #!/usr/bin/env bash + set -euo pipefail -./scripts/verify.sh -if [[ -n "$(git status --porcelain)" ]]; then - echo "Working tree has changes. Review them before committing." - git status --short - exit 2 + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" +source "$script_dir/lib/review-data.sh" +source "$script_dir/lib/fingerprint.sh" + +usage() { + echo "Usage: $0 \"\" [--no-review \"\"]" + echo + echo "Run in the feature worktree. Checks that verification passes and that the" + echo "latest independent review is current and resolved, then commits the feature" + echo "with an editable, structured message. It never pushes or merges." + echo + echo "--no-review is only for changes that .agents/policies/autonomy.md classifies" + echo "as low risk; the reason is recorded in the commit message." + exit 1 +} + +fail() { + echo "Error: $*" >&2 + exit 1 +} + +# latest_triage : prints the newest triage of a review. The first +# triage has no number (-triage.json); later ones are numbered (-triage-02.json). +latest_triage() { + local base + local number + local best="" + local best_number=0 + local candidate + + base="$root/.agents/triage/$(basename "$1" .json)" + [[ ! -f "$base-triage.json" ]] || { best="$base-triage.json"; best_number=1; } + for candidate in "$base-triage-"[0-9][0-9].json; do + [[ -f "$candidate" ]] || continue + number="${candidate%.json}" + number="${number##*-}" + if ((10#$number > best_number)); then + best="$candidate" + best_number=$((10#$number)) + fi + done + printf '%s' "$best" +} + +[[ $# -eq 2 || ( $# -eq 4 && "$3" == "--no-review" ) ]] || usage +issue="$1" +summary="$2" +no_review_reason="${4:-}" + +[[ "$issue" =~ ^[0-9]+$ ]] || fail "issue number must be numeric: $issue" +[[ "$summary" =~ [^[:space:]] ]] || fail "the commit summary is empty." +[[ $# -eq 2 || "$no_review_reason" =~ [^[:space:]] ]] || fail "--no-review requires a reason." + +root="$(git rev-parse --show-toplevel)" +branch="$(git branch --show-current)" +[[ "$branch" == feature/${issue}-* ]] || + fail "finish-feature.sh must run on feature/${issue}-*. Current branch: $branch" + +[[ -n "$(git -C "$root" status --porcelain)" ]] || fail "there are no changes to commit." +review_data_require_jq + +# 1. Verification passes. +echo "Running repository verification..." +(cd "$root" && ./scripts/verify.sh) || fail "verification failed; nothing was committed." +echo + +# 2. The latest review is current and resolved, unless explicitly skipped. +if [[ -n "$no_review_reason" ]]; then + review_line="Review: no independent review (low risk: $no_review_reason)" +else + slug="${branch//\//-}" + shopt -s nullglob + reviews=("$root/.agents/reviews/${slug}-review-"[0-9][0-9].json) + shopt -u nullglob + [[ "${#reviews[@]}" -gt 0 ]] || + fail "no review of this feature exists. Run ./scripts/review-feature.sh $issue, or use --no-review for a low-risk change." + latest="${reviews[${#reviews[@]}-1]}" + latest_relative="${latest#"$root"/}" + + errors="$(review_artifact_errors "$latest")" + [[ -z "$errors" ]] || fail "the latest review is invalid: ${errors//$'\n'/; }" + [[ "$(jq -r '.kind // "feature"' "$latest")" == "feature" && "$(jq -r '.issue' "$latest")" == "$issue" ]] || + fail "$latest_relative is not a review of Issue #$issue." + + tmp_work="$(mktemp -d "${TMPDIR:-/tmp}/finish-feature.XXXXXX")" + trap 'rm -rf "$tmp_work"' EXIT + current=0 + review_is_current "$root" "$tmp_work" "$latest" || current=$? + case "$current" in + 0) ;; + 1) fail "the latest review ($latest_relative) is stale: the code changed after it. Run ./scripts/review-feature.sh $issue again." ;; + *) fail "could not compute the fingerprint of the working tree." ;; + esac + + round="$(jq -r '.round' "$latest")" + verdict="$(jq -r '.verdict | gsub("_"; " ")' "$latest")" + blocking="$(jq '[.findings[] | select(.severity == "critical" or .severity == "major")] | length' "$latest")" + [[ "$blocking" -eq 0 ]] || + fail "the latest review (round $round) has $blocking critical or major finding(s). Triage, fix, and review again." + + # Every round with findings needs an approved triage that was published on + # the Issue; only the latest round may not have FIX_NOW findings left, since + # a newer, current review confirms the fixes of earlier rounds. + triage_note="" + for review in "${reviews[@]}"; do + [[ "$(jq '.findings | length' "$review")" -gt 0 ]] || continue + review_round="$(jq -r '.round' "$review")" + triage="$(latest_triage "$review")" + [[ -n "$triage" ]] || + fail "the findings of round $review_round are not triaged. Run ./scripts/triage-review.sh ${review#"$root"/}." + triage_relative="${triage#"$root"/}" + errors="$(triage_artifact_errors "$triage" "$review")" + [[ -z "$errors" ]] || fail "the triage of round $review_round is invalid: ${errors//$'\n'/; }" + [[ "$(jq -r '.published_at // empty' "$triage")" != "" ]] || + fail "the triage of round $review_round was not published on Issue #$issue. Run ./scripts/triage-review.sh --publish $triage_relative." + if [[ "$review" == "$latest" ]]; then + [[ "$(jq '[.decisions[] | select(.decision == "FIX_NOW")] | length' "$triage")" -eq 0 ]] || + fail "round $review_round has FIX_NOW findings. Apply them with ./scripts/apply-triage.sh $triage_relative and review again." + fi + triage_note="; triage published on #$issue" + done + review_line="Review: round $round, $verdict, by $(jq -r '"\(.reviewer.agent) (\(.reviewer.model))"' "$latest")$triage_note" fi -./scripts/review-feature.sh "${1:-main}" -echo "Stable commit verified. Complete independent review before push/PR." + +# 3. Stage everything and commit with an editable, structured message. +git -C "$root" add -A +echo "Changes to commit:" +git -C "$root" status --short +echo + +message_file="$(mktemp "${TMPDIR:-/tmp}/finish-feature-message.XXXXXX")" +trap 'rm -f "$message_file"; [[ -z "${tmp_work:-}" ]] || rm -rf "$tmp_work"' EXIT +cat >"$message_file" <&2 + exit 1 +fi + +echo +echo "Committed $(git -C "$root" rev-parse --short HEAD) on $branch." +echo +echo "Next steps:" +echo " git push -u origin $branch" +echo " Open a pull request whose description contains: Closes #$issue" diff --git a/scripts/lib/review-data.sh b/scripts/lib/review-data.sh index 48dc0ae..d630882 100644 --- a/scripts/lib/review-data.sh +++ b/scripts/lib/review-data.sh @@ -301,8 +301,11 @@ triage_artifact_errors() { if type != "object" or .schema != "triage/v1" then "the file is not a triage/v1 artifact" else - unexpected(["approved_at", "decisions", "issue", "review_verdict", "reviewed_tree", + unexpected(["approved_at", "decisions", "issue", "published_at", "review_verdict", "reviewed_tree", "schema", "source_review", "triage"]; "the triage"), + (if .published_at == null or ((.published_at | type) == "string" + and (.published_at | test("^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$"))) then empty + else "published_at must be a UTC timestamp" end), (if (.approved_at | type) == "string" and (.approved_at | test("^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$")) then empty else "the triage has no valid UTC approval timestamp" end), diff --git a/scripts/triage-review.sh b/scripts/triage-review.sh index e9cdacd..cf7bbbb 100755 --- a/scripts/triage-review.sh +++ b/scripts/triage-review.sh @@ -9,6 +9,7 @@ source "$script_dir/lib/fingerprint.sh" usage() { echo "Usage: $0 [--agent ] [--model ]" + echo " $0 --publish [--mark-only]" echo echo "The agent and model come from role 'triage' in .agents/agents.conf" echo "unless --agent and --model are given." @@ -24,7 +25,123 @@ fail() { exit 1 } +# publish_triage: publishes the stored triage .json and its source +# review as a comment on the source Issue and records the publication. +# The reports are published on the Issue instead of being committed, rendered +# from the validated JSON. A comment holds at most 65,536 characters: when both +# reports do not fit in one comment, they are published in numbered parts, so +# the record is never shortened. +publish_triage() { + round="$(jq -r '.round' "$review_path")" + comment_base="$artifact-comment" + rm -f "$comment_base"*.md + heading="## Independent review and triage — round $round" + { + echo "Reviewer verdict: $(jq -r '.verdict | gsub("_"; " ")' "$review_path")" + echo + echo "
" + echo "Review report" + echo + review_render_markdown "$review_path" "$review_stem.json" | sed '1{/^