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{/^