diff --git a/.agents/prompts/reviewer.md b/.agents/prompts/reviewer.md index 6bc2c20..efc62f5 100644 --- a/.agents/prompts/reviewer.md +++ b/.agents/prompts/reviewer.md @@ -4,18 +4,22 @@ You did not implement this change. Review only. You run with read-only permissio Read `AGENTS.md`, the active plan, relevant architecture/ADRs, and `docs/evaluation.md`. The calling script supplies the GitHub Issue and the complete feature-branch diff against its base, including uncommitted and untracked working-tree changes; review that complete diff and read repository files for context. -Report findings as **Critical**, **Major**, **Minor**, or **Suggestion**. For each finding include evidence/file location, why it matters, and a recommended action. Check correctness, issue/plan compliance, architecture, security, edge cases, test coverage, reliability, and unnecessary complexity. +Check correctness, issue/plan compliance, architecture, security, edge cases, test coverage, reliability, and unnecessary complexity. -Place every finding under its matching `## Critical`, `## Major`, `## Minor`, -or `## Suggestions` section and give it a stable heading such as -`### C1. Title`, `### M1. Title`, `### Minor 1. Title`, or `### S1. Title`. -Write `None.` when a section has no findings. These identifiers are preserved -by review triage and follow-up Issues. +## Result -Return the complete review as your final message. It must contain the sections -`## Critical`, `## Major`, `## Minor`, `## Suggestions`, and `## Verdict`, in -that order. The calling script stores the review; do not write it to a file. +Return the review as JSON that matches the schema supplied by the calling script (`.agents/schemas/review.schema.json`). Do not write it to a file and do not add text around it. -Under `## Verdict`, write exactly one of `PASS`, `PASS WITH MINOR FINDINGS`, or -`CHANGES REQUIRED` on its own line. Base the verdict on the findings. State -anything you could not verify below the verdict line. +- `findings`: one entry per finding; an empty array when there are none. + - `severity`: `critical`, `major`, `minor`, or `suggestion`. + - `title`: a short, specific name for the finding. + - `evidence`: file locations and what you observed there. + - `impact`: why it matters. + - `recommendation`: the recommended action. +- `verdict`: derived from the findings only. + - `PASS`: no findings. + - `PASS_WITH_MINOR_FINDINGS`: only minor or suggestion findings. + - `CHANGES_REQUIRED`: at least one critical or major finding. +- `limitations`: anything you could not verify, such as checks you could not run. Use an empty string when there is nothing to report. A limitation is not a finding and does not change the verdict. + +Do not number the findings; the calling script assigns the identifiers that review triage and follow-up Issues use. A result that breaks these rules is rejected. diff --git a/.agents/prompts/triage-implementer.md b/.agents/prompts/triage-implementer.md index 916ad7a..7cf6160 100644 --- a/.agents/prompts/triage-implementer.md +++ b/.agents/prompts/triage-implementer.md @@ -10,8 +10,8 @@ Read: - `AGENTS.md` - the originating GitHub Issue - the matching active feature plan, when one exists -- the source independent-review artifact -- the approved triage artifact +- the source independent-review artifact (JSON) +- the approved triage artifact (JSON) - relevant architecture documentation and accepted ADRs - the current working-tree diff diff --git a/.agents/prompts/triage-reviewer.md b/.agents/prompts/triage-reviewer.md index c1e29ef..1d5b456 100644 --- a/.agents/prompts/triage-reviewer.md +++ b/.agents/prompts/triage-reviewer.md @@ -8,8 +8,7 @@ implementing fixes and you must not modify repository files. Read: - `AGENTS.md` -- the complete source review artifact supplied by the caller -- the findings manifest supplied by the caller +- the review findings supplied by the caller as JSON - the originating GitHub Issue, when identified - the matching active feature plan, when one exists - relevant architecture documentation and accepted ADRs when needed @@ -19,7 +18,7 @@ silently downgrade findings. ## Decisions -Classify every manifest entry exactly once: +Classify every finding exactly once: - `FIX_NOW`: blocks the current feature or is a clear, local, valuable fix. Critical and Major findings must use this decision. @@ -39,22 +38,22 @@ For `DEFER`, also propose: Do not create GitHub Issues. The calling script owns the human approval gate and all approved side effects. -## Output format +## Result -Return only tab-separated records, one per manifest entry, in the same order: +Return JSON that matches the schema supplied by the calling script +(`.agents/schemas/triage.schema.json`). Do not write it to a file and do not +add text around it. -```text - -``` +- `decisions`: one entry per finding of the review, and no others. + - `finding_id`: the `id` of the finding, exactly as supplied. + - `decision`: `FIX_NOW`, `DEFER`, or `ACCEPT`. + - `rationale`: the reason for the decision. + - `followup`: `null` unless the decision is `DEFER`. For `DEFER`, an object + with `title`, `recommended_action`, and at least one entry in + `acceptance_criteria`. -Rules: - -- Use exactly six fields separated by literal tab characters. -- Keep every field on one physical line and do not place tabs inside fields. -- Use only `FIX_NOW`, `DEFER`, or `ACCEPT` for the decision. -- Use `-` for the final three fields unless the decision is `DEFER`. -- Do not add Markdown fences, headings, commentary, summaries, or blank lines. -- If the findings manifest is empty, output exactly `NO_FINDINGS`. +A result that omits a finding, decides one twice, refers to an unknown finding, +or breaks the rules above is rejected. ## Boundaries diff --git a/.agents/schemas/review.schema.json b/.agents/schemas/review.schema.json new file mode 100644 index 0000000..eefa1d9 --- /dev/null +++ b/.agents/schemas/review.schema.json @@ -0,0 +1,30 @@ +{ + "type": "object", + "additionalProperties": false, + "required": ["verdict", "findings", "limitations"], + "properties": { + "verdict": { + "type": "string", + "enum": ["PASS", "PASS_WITH_MINOR_FINDINGS", "CHANGES_REQUIRED"] + }, + "findings": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["severity", "title", "evidence", "impact", "recommendation"], + "properties": { + "severity": { + "type": "string", + "enum": ["critical", "major", "minor", "suggestion"] + }, + "title": { "type": "string" }, + "evidence": { "type": "string" }, + "impact": { "type": "string" }, + "recommendation": { "type": "string" } + } + } + }, + "limitations": { "type": "string" } + } +} diff --git a/.agents/schemas/triage.schema.json b/.agents/schemas/triage.schema.json new file mode 100644 index 0000000..b02c724 --- /dev/null +++ b/.agents/schemas/triage.schema.json @@ -0,0 +1,41 @@ +{ + "type": "object", + "additionalProperties": false, + "required": ["decisions"], + "properties": { + "decisions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["finding_id", "decision", "rationale", "followup"], + "properties": { + "finding_id": { "type": "string" }, + "decision": { + "type": "string", + "enum": ["FIX_NOW", "DEFER", "ACCEPT"] + }, + "rationale": { "type": "string" }, + "followup": { + "anyOf": [ + { "type": "null" }, + { + "type": "object", + "additionalProperties": false, + "required": ["title", "recommended_action", "acceptance_criteria"], + "properties": { + "title": { "type": "string" }, + "recommended_action": { "type": "string" }, + "acceptance_criteria": { + "type": "array", + "items": { "type": "string" } + } + } + } + ] + } + } + } + } + } +} diff --git a/README.md b/README.md index d95191b..c723fd0 100644 --- a/README.md +++ b/README.md @@ -44,14 +44,14 @@ A lightweight, model-agnostic repository template for agentic software engineeri ./scripts/verify.sh ./scripts/review-feature.sh 12 ./scripts/triage-review.sh \ - .agents/reviews/feature-12-player-movement-review-01.md + .agents/reviews/feature-12-player-movement-review-01.json ``` 8. Approve the proposed triage and apply its `FIX_NOW` scope: ```bash ./scripts/apply-triage.sh \ - .agents/triage/feature-12-player-movement-review-01-triage.md + .agents/triage/feature-12-player-movement-review-01-triage.json ``` The script starts a write-capable agent only after confirmation and verifies diff --git a/docs/agentic-workflow.md b/docs/agentic-workflow.md index 479835b..6a9aa55 100644 --- a/docs/agentic-workflow.md +++ b/docs/agentic-workflow.md @@ -30,8 +30,11 @@ See `docs/development.md` for the concrete commands. - ADRs: why significant architecture decisions were made. - `.agents/plans/`: active implementation state for complex work. - `.agents/handoffs/`: compressed continuation context. -- `.agents/reviews/`: temporary independent-review artifacts. -- `.agents/triage/`: approved finding decisions and deferred-Issue traceability. +- `.agents/reviews/`: independent-review results as validated JSON, each with a + generated Markdown report. +- `.agents/triage/`: approved finding decisions and deferred-Issue traceability + as validated JSON, each with a generated Markdown report. +- `.agents/schemas/`: the schemas of those results. - `.agents/lessons/`: recurring failure lessons awaiting/promoting durable rules. - Git history: what actually changed. - PR + CI: review discussion and deterministic evidence. diff --git a/docs/development.md b/docs/development.md index af20ec7..5c4b55c 100644 --- a/docs/development.md +++ b/docs/development.md @@ -78,9 +78,13 @@ Agents run with one of two permission profiles: - `write`: an interactive session that may modify its worktree. Codex runs in a workspace-write sandbox without approval prompts; Claude accepts edits automatically. Used for planning, implementation, and applying triage. -- `read-only`: a non-interactive session that cannot modify files. Codex runs - in a read-only sandbox; Claude is limited to its read tools. The script - stores the agent's final message. Used for review and triage. +- `read-only`: a non-interactive session that cannot modify files and gets no + MCP servers, apps, or other tools from the user's configuration. Codex runs + in a read-only sandbox without the user's `config.toml`, with apps, browser + use, computer use, and web search disabled. Claude runs restricted: without user, project, or MCP + configuration, with only its Read, Glob, and Grep tools, and without asking + for any further permission. The script stores the agent's result. Used for + review and triage. A profile that a provider cannot enforce is an error; an agent is never started with broader permissions instead. @@ -208,31 +212,33 @@ It uses role `reviewer` with the `read-only` profile. The review is non-interactive: the reviewer cannot modify files and has no network access, so the script supplies the GitHub Issue and the complete diff against the base branch, including uncommitted and untracked changes. The reviewer returns its -report and the script stores it. A report without the required sections or a -valid verdict is rejected, and a reviewer that changed the working tree or +result as JSON and the script validates and stores it. An invalid result is +retried once and then rejected, and a reviewer that changed the working tree or created a commit is reported as an error; in both cases no review is stored. The review script must run from the matching feature worktree and needs an -authenticated GitHub CLI. It writes numbered artifacts without overwriting -earlier reviews: +authenticated GitHub CLI. Each round writes a numbered pair of files without +overwriting earlier reviews: ```text +.agents/reviews/feature-12-player-movement-review-01.json .agents/reviews/feature-12-player-movement-review-01.md -.agents/reviews/feature-12-player-movement-review-02.md ``` +See "Review and triage data" below for the two files. + Triage an explicit review artifact with an agent independent from the implementation: ```bash ./scripts/triage-review.sh \ - .agents/reviews/feature-12-player-movement-review-01.md + .agents/reviews/feature-12-player-movement-review-01.json ``` The full interface is: ```text -./scripts/triage-review.sh [--agent ] [--model ] +./scripts/triage-review.sh [--agent ] [--model ] ``` The triage agent (role `triage`) classifies every finding as: @@ -242,44 +248,53 @@ The triage agent (role `triage`) classifies every finding as: - `DEFER`: valid non-blocking work proposed as a separate follow-up Issue. - `ACCEPT`: consciously take no action, with an explicit rationale. +The script validates the decisions before showing them: every finding is +decided exactly once, Critical and Major findings are `FIX_NOW`, and a deferred +finding has a follow-up title, action, and acceptance criteria. Invalid +decisions are retried once and then rejected. A review without findings needs +no triage agent. + The script displays the complete proposal before side effects. Only after interactive approval does it create one GitHub Issue per `DEFER` finding and write a persistent, uniquely named artifact such as: ```text +.agents/triage/feature-12-player-movement-review-01-triage.json .agents/triage/feature-12-player-movement-review-01-triage.md ``` That artifact maps the source review findings to their decisions and any created Issue numbers. Declining the proposal creates neither an artifact nor -Issues. The source review remains unchanged. A separate `create-followups.sh` -is therefore not needed. +Issues. The source review remains unchanged. The artifact is stored only after +every follow-up Issue exists; if creating one fails, nothing is stored and a +new triage reuses the Issues created so far, which it finds by their trace +token. A re-run reuses a follow-up Issue that already exists for a finding. Deferred follow-up Issue titles include deterministic provenance: ```text -[F02][R01][S9] Concise follow-up title +[F02][R01][S2] Concise follow-up title ``` -The script obtains the feature ID from the source Issue title, the review round -from the review filename, and the finding ID from the review. When no feature -ID is available, it falls back to the source Issue number: +The script obtains the feature ID from the source Issue title and the review +round and finding ID from the review. When no feature ID is available, it falls +back to the source Issue number: ```text -[#12][R01][S9] Concise follow-up title +[#12][R01][S2] Concise follow-up title ``` Apply the approved `FIX_NOW` set from the same feature worktree: ```bash ./scripts/apply-triage.sh \ - .agents/triage/feature-12-player-movement-review-01-triage.md + .agents/triage/feature-12-player-movement-review-01-triage.json ``` The full interface is: ```text -./scripts/apply-triage.sh [--agent ] [--model ] +./scripts/apply-triage.sh [--agent ] [--model ] ``` The helper uses role `triage-implementer`. It validates the approved artifact and source review, shows the exact @@ -292,6 +307,35 @@ The helper does not commit, push, merge, deploy, or create/close Issues. Inspect the resulting diff and run another independent review and triage when fixes require confirmation. +### Review and triage data + +Results that an agent writes and a script consumes are JSON. Each review and +each triage is stored as two files with the same name: + +- `.json` is the source of truth. The scripts read only this file. +- `.md` is a report generated from the JSON for reading. It is never parsed; + editing it has no effect. + +The agent's result must match a schema in `.agents/schemas/` +(`review.schema.json`, `triage.schema.json`). The schema is passed to the +provider CLI and the script checks the result again with `jq`, including rules +a schema cannot express. For a review, the verdict must follow from the +findings: `PASS` without findings, `PASS_WITH_MINOR_FINDINGS` with only minor +or suggestion findings, and `CHANGES_REQUIRED` with at least one critical or +major finding. What a reviewer could not verify belongs in `limitations` and +does not change the verdict. + +The script, not the agent, numbers the findings: `C1` (critical), `M1` (major), +`MIN1` (minor), and `S1` (suggestion). Triage decisions and follow-up Issues +refer to those identifiers. When a result is invalid, the agent is asked once +more, with the reasons for the rejection. + +Stored artifacts are checked with the same rules as agent results, plus the +fields the scripts own, so editing a stored artifact cannot weaken a decision. + +Documents that people maintain, such as the roadmap, requirements, and +architecture, keep Markdown as their source. + Verify again after review fixes, then commit and push: ```bash diff --git a/docs/evaluation.md b/docs/evaluation.md index 4ed1de6..79a02a0 100644 --- a/docs/evaluation.md +++ b/docs/evaluation.md @@ -44,7 +44,7 @@ decline the proposed decisions. - **Suggestion:** classify explicitly; it may be deferred when worthwhile or accepted with a rationale. -The approved `.agents/triage/` artifact is the source of truth for these +The approved `.agents/triage/*.json` artifact is the source of truth for these decisions. Use `./scripts/apply-triage.sh` to hand only its `FIX_NOW` scope to a write-capable implementation agent. The helper verifies the result but does not commit it. Inspect the diff and run a new independent review/triage when needed diff --git a/scripts/apply-triage.sh b/scripts/apply-triage.sh index 9e0ffb9..c0a4b61 100755 --- a/scripts/apply-triage.sh +++ b/scripts/apply-triage.sh @@ -2,17 +2,25 @@ set -euo pipefail -source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)/lib/agent.sh" +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" +source "$script_dir/lib/agent.sh" +source "$script_dir/lib/review-data.sh" usage() { - echo "Usage: $0 [--agent ] [--model ]" + echo "Usage: $0 [--agent ] [--model ]" echo echo "The agent and model come from role 'triage-implementer' in" echo ".agents/agents.conf unless --agent and --model are given." echo echo "Examples:" - echo " $0 .agents/triage/feature-5-rendering-review-01-triage.md" - echo " $0 .agents/triage/feature-5-rendering-review-01-triage.md --agent claude --model opus" + echo " $0 .agents/triage/feature-5-rendering-review-01-triage.json" + echo " $0 .agents/triage/feature-5-rendering-review-01-triage.json --agent claude --model opus" + exit 1 +} + +report_errors() { + echo "Error: $1" >&2 + printf '%s\n' "$2" | sed 's/^/ - /' >&2 exit 1 } @@ -33,10 +41,9 @@ fi triage_dir="$(cd "$(dirname "$triage_input")" && pwd -P)" triage_path="$triage_dir/$(basename "$triage_input")" -triage_root="$root/.agents/triage" -if [[ "$triage_path" != "$triage_root/"*.md ]]; then - echo "Error: triage artifact must match .agents/triage/*.md" +if [[ "$triage_path" != "$root/.agents/triage/"*.json ]]; then + echo "Error: triage artifact must match .agents/triage/*.json (the generated .md report is not an input)." echo "Received: $triage_path" exit 1 fi @@ -46,79 +53,36 @@ if [[ ! -f "$prompt_file" ]]; then exit 1 fi +review_data_require_jq + agent_resolve "$root" triage-implementer "$AGENT_CLI_PROVIDER" "$AGENT_CLI_MODEL" agent="$AGENT_PROVIDER" model="$AGENT_MODEL" -approval_metadata_count="$(awk '/^Approved at:/ { count++ } END { print count + 0 }' "$triage_path")" -approval_valid_count="$(awk ' - /^Approved at: [0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]T[0-9][0-9]:[0-9][0-9]:[0-9][0-9]Z$/ { - count++ - } - END { - print count + 0 - } -' "$triage_path")" -if [[ "$approval_metadata_count" -ne 1 || "$approval_valid_count" -ne 1 ]]; then - echo "Error: triage artifact must contain exactly one valid UTC approval timestamp." +source_review_relative="$(triage_source_review "$triage_path")" +if [[ -z "$source_review_relative" ]]; then + echo "Error: the file is not a triage/v1 artifact with a source review: $triage_input" exit 1 fi - -source_review_count="$(awk -F '\`' '/^Source review: `[^`]+`$/ { count++ } END { print count + 0 }' "$triage_path")" -if [[ "$source_review_count" -ne 1 ]]; then - echo "Error: triage artifact must contain exactly one source review." +if [[ ! "$source_review_relative" =~ ^\.agents/reviews/[^/]+\.json$ ]]; then + echo "Error: source review must match .agents/reviews/*.json: $source_review_relative" exit 1 fi - -source_review_relative="$(awk -F '\`' '/^Source review: `[^`]+`$/ { print $2; exit }' "$triage_path")" -source_review_candidate="$root/$source_review_relative" -if [[ ! -f "$source_review_candidate" ]]; then +source_review_path="$root/$source_review_relative" +if [[ ! -f "$source_review_path" ]]; then echo "Error: source review artifact not found: $source_review_relative" exit 1 fi -source_review_dir="$(cd "$(dirname "$source_review_candidate")" && pwd -P)" -source_review_path="$source_review_dir/$(basename "$source_review_candidate")" -if [[ "$source_review_path" != "$root/.agents/reviews/"*.md ]]; then - echo "Error: source review must match .agents/reviews/*.md" - exit 1 -fi - -source_issue_metadata_count="$(awk '/^Source feature Issue:/ { count++ } END { print count + 0 }' "$triage_path")" -source_issue_valid_count="$(awk '/^Source feature Issue: #[0-9]+[[:space:]]*$/ { count++ } END { print count + 0 }' "$triage_path")" -if [[ "$source_issue_metadata_count" -gt 1 || "$source_issue_metadata_count" -ne "$source_issue_valid_count" ]]; then - echo "Error: triage artifact contains ambiguous source Issue metadata." - exit 1 -fi - -source_issue="$(awk ' - /^Source feature Issue: #[0-9]+[[:space:]]*$/ { - match($0, /#[0-9]+/) - print substr($0, RSTART + 1, RLENGTH - 1) - } -' "$triage_path")" - -review_issue_metadata_count="$(awk '/^Issue:/ { count++ } END { print count + 0 }' "$source_review_path")" -review_issue_valid_count="$(awk '/^Issue:[[:space:]]*#[0-9]+[[:space:]]*$/ { count++ } END { print count + 0 }' "$source_review_path")" -if [[ "$review_issue_metadata_count" -gt 1 || "$review_issue_metadata_count" -ne "$review_issue_valid_count" ]]; then - echo "Error: source review contains ambiguous Issue metadata." - exit 1 -fi +# Both stored artifacts are checked with the same rules as agent output, so an +# edited artifact cannot weaken a decision. +review_errors="$(review_artifact_errors "$source_review_path")" +[[ -z "$review_errors" ]] || report_errors "invalid source review artifact:" "$review_errors" -review_issue="$(awk ' - /^Issue:[[:space:]]*#[0-9]+[[:space:]]*$/ { - match($0, /#[0-9]+/) - print substr($0, RSTART + 1, RLENGTH - 1) - } -' "$source_review_path")" +triage_errors="$(triage_artifact_errors "$triage_path" "$source_review_path")" +[[ -z "$triage_errors" ]] || report_errors "invalid or unapproved triage artifact:" "$triage_errors" -if [[ -n "$source_issue" && -n "$review_issue" && "$source_issue" != "$review_issue" ]]; then - echo "Error: triage and review artifacts reference different source Issues." - exit 1 -fi -if [[ -z "$source_issue" ]]; then - source_issue="$review_issue" -fi +source_issue="$(jq -r '.issue' "$triage_path")" branch="$(git branch --show-current)" if [[ "$branch" != feature/* ]]; then @@ -127,285 +91,44 @@ if [[ "$branch" != feature/* ]]; then exit 1 fi -if [[ -z "$source_issue" && "$branch" =~ ^feature/([0-9]+)- ]]; then - source_issue="${BASH_REMATCH[1]}" -fi - -if [[ -n "$source_issue" && "$branch" != feature/${source_issue}-* ]]; then +if [[ "$branch" != feature/${source_issue}-* ]]; then echo "Error: current branch does not match source Issue #$source_issue." echo "Current branch: $branch" exit 1 fi -if [[ -n "$source_issue" ]]; then - if ! command -v gh >/dev/null 2>&1; then - echo "Error: GitHub CLI 'gh' is not installed." - exit 1 - fi - if ! gh auth status >/dev/null 2>&1; then - echo "Error: GitHub CLI is not authenticated." - echo "Run: gh auth login" - exit 1 - fi - resolved_issue="$(gh issue view "$source_issue" --json number --template '{{.number}}')" - if [[ "$resolved_issue" != "$source_issue" ]]; then - echo "Error: could not validate source Issue #$source_issue." - exit 1 - fi +if ! command -v gh >/dev/null 2>&1; then + echo "Error: GitHub CLI 'gh' is not installed." + exit 1 fi - -tmp_work="$(mktemp -d "${TMPDIR:-/tmp}/apply-triage.XXXXXX")" -trap 'rm -rf "$tmp_work"' EXIT -fix_scope_file="$tmp_work/fix-now.md" -fix_count_file="$tmp_work/fix-count" -review_findings_file="$tmp_work/review-findings.txt" -: >"$fix_scope_file" - -awk ' - function trim(value) { - sub(/^[[:space:]]+/, "", value) - sub(/[[:space:]]+$/, "", value) - return value - } - - function singular(value) { - return value == "Suggestions" ? "Suggestion" : value - } - - function emit_finding(severity, raw, id, title, token, rest, count, parts) { - raw = trim(raw) - ordinal[severity]++ - id = singular(severity) "-" ordinal[severity] - title = raw - - if (raw ~ /^(Critical|Major|Minor|Suggestion)[[:space:]]+[0-9]+[.):-]?([[:space:]]+|$)/) { - count = split(raw, parts, /[[:space:]]+/) - token = parts[2] - gsub(/[.):-]+$/, "", token) - id = parts[1] " " token - rest = raw - sub(/^(Critical|Major|Minor|Suggestion)[[:space:]]+[0-9]+[.):-]?[[:space:]]*/, "", rest) - title = trim(rest) - } else if (raw ~ /^[CMS][0-9]+[.):-]?([[:space:]]+|$)/) { - token = raw - sub(/[[:space:]].*$/, "", token) - gsub(/[.):-]+$/, "", token) - id = token - rest = raw - sub(/^[CMS][0-9]+[.):-]?[[:space:]]*/, "", rest) - title = trim(rest) - } - - if (title == "") { - title = raw - } - print id ". " title - } - - /^## Critical[[:space:]]*$/ { - section = "Critical" - next - } - /^## Major[[:space:]]*$/ { - section = "Major" - next - } - /^## Minor[[:space:]]*$/ { - section = "Minor" - next - } - /^## Suggestions[[:space:]]*$/ { - section = "Suggestions" - next - } - /^##[[:space:]]+/ { - section = "" - next - } - section != "" && /^###[[:space:]]+/ { - value = $0 - sub(/^###[[:space:]]+/, "", value) - entries++ - entry_section[entries] = section - entry_type[entries] = "heading" - entry_value[entries] = value - headings[section]++ - next - } - section != "" && /^-[[:space:]]+/ { - value = $0 - sub(/^-[[:space:]]+/, "", value) - entries++ - entry_section[entries] = section - entry_type[entries] = "bullet" - entry_value[entries] = value - } - END { - for (i = 1; i <= entries; i++) { - section = entry_section[i] - if (entry_type[i] == "heading" || (entry_type[i] == "bullet" && headings[section] == 0)) { - emit_finding(section, entry_value[i]) - } - } - } -' "$source_review_path" >"$review_findings_file" - -if ! awk -v output="$fix_scope_file" -v count_output="$fix_count_file" -v review_findings="$review_findings_file" ' - BEGIN { - while ((getline source_heading < review_findings) > 0) { - source_headings[source_heading]++ - } - close(review_findings) - } - - function expected_decision(value) { - if (value == "fix") { - return "FIX_NOW" - } - if (value == "defer") { - return "DEFER" - } - if (value == "accept") { - return "ACCEPT" - } - return "" - } - - function finish_finding( expected) { - if (!in_finding) { - return - } - - expected = expected_decision(section) - if (decision_count != 1) { - printf "Error: finding %s must contain exactly one Decision field.\n", finding_id > "/dev/stderr" - invalid = 1 - } else if (decision != expected) { - printf "Error: finding %s is under the wrong decision section.\n", finding_id > "/dev/stderr" - invalid = 1 - } - - if (seen[finding_id]++) { - printf "Error: duplicate triage finding identifier: %s\n", finding_id > "/dev/stderr" - invalid = 1 - } - - if (source_headings[finding_heading] != 1) { - printf "Error: triage finding does not map uniquely to the source review: %s\n", finding_heading > "/dev/stderr" - invalid = 1 - } - - if (section == "fix") { - printf "%s", block > output - fix_count++ - } - - in_finding = 0 - finding_id = "" - finding_heading = "" - decision = "" - decision_count = 0 - block = "" - } - - /^## Fix now[[:space:]]*$/ { - finish_finding() - section = "fix" - section_count[section]++ - next - } - /^## Deferred[[:space:]]*$/ { - finish_finding() - section = "defer" - section_count[section]++ - next - } - /^## Accepted[[:space:]]*$/ { - finish_finding() - section = "accept" - section_count[section]++ - next - } - /^## Traceability[[:space:]]*$/ { - finish_finding() - section = "trace" - section_count[section]++ - next - } - /^##[[:space:]]+/ { - finish_finding() - section = "" - next - } - - /^###[[:space:]]+/ { - finish_finding() - if (section != "fix" && section != "defer" && section != "accept") { - print "Error: finding heading appears outside a triage decision section." > "/dev/stderr" - invalid = 1 - next - } - - heading = $0 - sub(/^###[[:space:]]+/, "", heading) - separator = index(heading, ". ") - if (separator == 0) { - printf "Error: malformed finding heading: %s\n", $0 > "/dev/stderr" - invalid = 1 - next - } - - finding_id = substr(heading, 1, separator - 1) - finding_heading = heading - in_finding = 1 - block = $0 "\n" - next - } - - in_finding { - block = block $0 "\n" - if ($0 ~ /^- Decision: /) { - decision = $0 - sub(/^- Decision: /, "", decision) - decision_count++ - } - } - - END { - finish_finding() - - required[1] = "fix" - required[2] = "defer" - required[3] = "accept" - required[4] = "trace" - for (i = 1; i <= 4; i++) { - name = required[i] - if (section_count[name] != 1) { - printf "Error: triage artifact must contain exactly one %s section.\n", name > "/dev/stderr" - invalid = 1 - } - } - - close(output) - print fix_count + 0 > count_output - close(count_output) - if (invalid) { - exit 1 - } - } -' "$triage_path"; then - echo "Error: malformed or ambiguous triage artifact." +if ! gh auth status >/dev/null 2>&1; then + echo "Error: GitHub CLI is not authenticated." + echo "Run: gh auth login" + exit 1 +fi +resolved_issue="$(gh issue view "$source_issue" --json number --template '{{.number}}')" +if [[ "$resolved_issue" != "$source_issue" ]]; then + echo "Error: could not validate source Issue #$source_issue." exit 1 fi -fix_count="$(<"$fix_count_file")" +fix_count="$(jq '[.decisions[] | select(.decision == "FIX_NOW")] | length' "$triage_path")" if [[ "$fix_count" -eq 0 ]]; then echo "No FIX_NOW findings found; no implementation agent was started." exit 0 fi triage_relative="${triage_path#"$root"/}" -fix_scope="$(<"$fix_scope_file")" +fix_scope="$(jq -r --slurpfile review "$source_review_path" ' + .decisions[] | select(.decision == "FIX_NOW") | . as $d + | ($review[0].findings[] | select(.id == $d.finding_id)) as $f + | "### \($f.id). \($f.title)\n\n" + + "- Severity: \($f.severity)\n" + + "- Evidence: \($f.evidence)\n" + + "- Impact: \($f.impact)\n" + + "- Recommended action: \($f.recommendation)\n" + + "- Triage rationale: \($d.rationale)\n" +' "$triage_path")" echo "Approved FIX_NOW scope from $triage_relative:" echo diff --git a/scripts/lib/agent.sh b/scripts/lib/agent.sh index f9b1523..3be4b80 100644 --- a/scripts/lib/agent.sh +++ b/scripts/lib/agent.sh @@ -2,7 +2,7 @@ # Shared agent configuration and launcher for the workflow scripts. # Source this file; do not execute it. It uses only Bash builtins besides the -# selected agent CLI. +# selected agent CLI, and jq when an agent is asked for structured output. # # Provider and model per role come from .agents/agents.conf. The --agent and # --model options of a workflow script override that file. A requested model is @@ -133,16 +133,27 @@ agent_resolve() { AGENT_MODEL="$model" } -# agent_run [output-file] [context-file] +# agent_run [output-file] [context-file] [schema-file] # # Profiles: # write Interactive session that may modify the work directory. # read-only Non-interactive session that cannot modify files. The agent's # final message is stored in . , when -# given, is supplied to the agent on standard input. +# given, is supplied to the agent on standard input. With a +# (JSON Schema), the provider is asked for +# structured output and holds that JSON; the +# caller must still validate it. +# +# Read-only sessions are isolated from the user's configuration, so they get no +# MCP servers, apps, or other tools that act outside the sandbox. Codex runs in +# its read-only sandbox without the user's config.toml and with apps, browser +# use, computer use, and web search disabled. Claude runs restricted, without +# user, project, or MCP configuration, with only its Read, Glob, and Grep tools, +# and without asking for any further permission. # # A profile that the provider cannot enforce is an error; the agent is never -# started with broader permissions instead. Returns the agent's status. +# started with broader permissions instead. Returns the agent's status; a +# Claude session that reports an error returns 1 even when the CLI exits 0. agent_run() { local profile="$1" local provider="$2" @@ -151,6 +162,18 @@ agent_run() { local prompt="$5" local output_file="${6:-}" local context_file="${7:-/dev/null}" + local schema_file="${8:-}" + local codex_schema=() + local claude_read_only=( + --print + --restricted + --strict-mcp-config + --permission-mode dontAsk + --tools "Read,Glob,Grep" + --no-session-persistence + ) + local envelope + local status case "$profile" in write | read-only) ;; @@ -183,26 +206,59 @@ agent_run() { ) ;; codex:read-only) + if [[ -n "$schema_file" ]]; then + codex_schema=(--output-schema "$schema_file") + fi codex exec \ + --ignore-user-config \ --sandbox read-only \ + --disable apps \ + --disable browser_use \ + --disable computer_use \ + -c 'web_search="disabled"' \ --ephemeral \ --color never \ --cd "$workdir" \ --output-last-message "$output_file" \ --model "$model" \ + ${codex_schema[@]+"${codex_schema[@]}"} \ "$prompt" <"$context_file" >/dev/null ;; claude:read-only) + if [[ -z "$schema_file" ]]; then + ( + cd "$workdir" + claude "${claude_read_only[@]}" --model "$model" "$prompt" + ) <"$context_file" >"$output_file" + return + fi + + command -v jq >/dev/null 2>&1 || agent_fail "jq is required for structured agent output." + + # Claude returns structured output inside a JSON result envelope. + envelope="$output_file.envelope" + # Capture the status without toggling errexit, which belongs to the caller. + status=0 ( cd "$workdir" - claude \ - --print \ - --permission-mode plan \ - --tools "Read,Glob,Grep" \ - --no-session-persistence \ + claude "${claude_read_only[@]}" \ --model "$model" \ + --output-format json \ + --json-schema "$(<"$schema_file")" \ "$prompt" - ) <"$context_file" >"$output_file" + ) <"$context_file" >"$envelope" || status=$? + + if [[ "$status" -ne 0 ]] || jq -e '.is_error == true' "$envelope" >/dev/null 2>&1; then + jq -r '.result // empty' "$envelope" >&2 2>/dev/null || cat "$envelope" >&2 + [[ "$status" -ne 0 ]] || status=1 + return "$status" + fi + + # Without structured output, hand the raw result to the caller's + # validation, which rejects it. + jq -e '.structured_output // (.result | fromjson?)' "$envelope" >"$output_file" 2>/dev/null || + jq -r '.result // empty' "$envelope" >"$output_file" 2>/dev/null || + cp "$envelope" "$output_file" ;; *) agent_fail "agent '$provider' cannot enforce permission profile '$profile'." diff --git a/scripts/lib/review-data.sh b/scripts/lib/review-data.sh new file mode 100644 index 0000000..48c662b --- /dev/null +++ b/scripts/lib/review-data.sh @@ -0,0 +1,341 @@ +#!/usr/bin/env bash + +# Validation and rendering of review and triage data. +# Source this file; do not execute it. Requires jq. +# +# Review and triage artifacts are JSON. The Markdown next to them is rendered +# from that JSON for reading and is never parsed. +# +# Every *_errors function prints one line per violated rule and prints nothing +# when its input is valid. Agent results and stored artifacts are checked with +# the same rules, so an edited artifact cannot bypass them. + +review_data_require_jq() { + command -v jq >/dev/null 2>&1 || { + echo "Error: jq is required. Install it: 'brew install jq' or 'sudo apt-get install jq'." >&2 + exit 1 + } +} + +# review_data_is_json : succeeds when the file holds exactly one JSON value. +review_data_is_json() { + [[ -s "$1" ]] && jq -es 'length == 1' "$1" >/dev/null 2>&1 +} + +# Rules shared by agent results and stored artifacts. +_review_data_rules=' + def nonempty: type == "string" and test("\\S"); + + def positive_integer: type == "number" and . >= 1 and . == floor; + + def unexpected($allowed; $where): + (keys - $allowed) as $extra + | if ($extra | length) > 0 then "\($where) has unexpected fields: \($extra | join(", "))" else empty end; + + def missing($required; $where): + ($required - keys) as $absent + | if ($absent | length) > 0 then "\($where) lacks required fields: \($absent | join(", "))" else empty end; + + # Input: {verdict, findings, limitations}. + def review_result_rules: + if type != "object" then + "the result is not a JSON object" + else + unexpected(["findings", "limitations", "verdict"]; "the result"), + (if (.verdict | IN("PASS", "PASS_WITH_MINOR_FINDINGS", "CHANGES_REQUIRED")) then empty + else "verdict must be PASS, PASS_WITH_MINOR_FINDINGS, or CHANGES_REQUIRED" end), + (if (.limitations | type) == "string" then empty + else "limitations must be a string" end), + (if (.findings | type) != "array" then + "findings must be an array" + else + (.findings | to_entries[] | (.key + 1) as $n | .value as $f | + if ($f | type) != "object" then + "finding \($n) is not an object" + else + ($f | unexpected(["evidence", "impact", "recommendation", "severity", "title"]; "finding \($n)")), + (if ($f.severity | IN("critical", "major", "minor", "suggestion")) then empty + else "finding \($n) has an invalid severity" end), + (("title", "evidence", "impact", "recommendation") as $field | + if ($f[$field] | nonempty) then empty + else "finding \($n) has an empty \($field)" end) + end), + ([.findings[] | objects | .severity] as $severities + | ($severities | map(select(. == "critical" or . == "major")) | length) as $blocking + | if .verdict == "PASS" and ($severities | length) > 0 then + "verdict PASS requires that there are no findings" + elif .verdict == "PASS_WITH_MINOR_FINDINGS" and (($severities | length) == 0 or $blocking > 0) then + "verdict PASS_WITH_MINOR_FINDINGS requires at least one finding and no critical or major finding" + elif .verdict == "CHANGES_REQUIRED" and $blocking == 0 then + "verdict CHANGES_REQUIRED requires a critical or major finding; state anything unverified under limitations" + else empty end) + end) + end; + + # Input: {decisions}. $severity maps each finding id of the review to its severity. + def triage_decision_rules($severity): + if type != "object" or (.decisions | type) != "array" then + "the result must be an object with a decisions array" + else + unexpected(["decisions"]; "the result"), + ([.decisions[] | objects | .finding_id] as $decided + | ($severity | keys[]) as $id + | ($decided | map(select(. == $id)) | length) as $count + | if $count == 0 then "finding \($id) was not classified" + elif $count > 1 then "finding \($id) was classified more than once" + else empty end), + (.decisions | to_entries[] | (.key + 1) as $n | .value as $d | + if ($d | type) != "object" then + "decision \($n) is not an object" + elif ($d.finding_id | type) != "string" or ($severity | has($d.finding_id) | not) then + "decision \($n) refers to an unknown finding: \($d.finding_id | tostring)" + else + ($d | unexpected(["decision", "finding_id", "followup", "rationale"]; "decision \($n)")), + ($d | missing(["decision", "finding_id", "followup", "rationale"]; "decision \($n)")), + (if ($d.decision | IN("FIX_NOW", "DEFER", "ACCEPT")) then empty + else "finding \($d.finding_id) has an invalid decision" end), + (if ($d.rationale | nonempty) then empty + else "finding \($d.finding_id) has no rationale" end), + (if ($severity[$d.finding_id] | IN("critical", "major")) and $d.decision != "FIX_NOW" then + "\($severity[$d.finding_id]) finding \($d.finding_id) must be FIX_NOW" + else empty end), + (if $d.decision == "DEFER" then + (if ($d.followup | type) != "object" then + "deferred finding \($d.finding_id) needs a follow-up" + else + ($d.followup | unexpected(["acceptance_criteria", "recommended_action", "title"]; + "the follow-up of finding \($d.finding_id)")), + (if ($d.followup.title | nonempty) then empty + else "deferred finding \($d.finding_id) needs a follow-up title" end), + (if ($d.followup.recommended_action | nonempty) then empty + else "deferred finding \($d.finding_id) needs a recommended action" end), + (if (($d.followup.acceptance_criteria | type) == "array") + and (($d.followup.acceptance_criteria | length) > 0) + and ($d.followup.acceptance_criteria | all(nonempty)) then empty + else "deferred finding \($d.finding_id) needs acceptance criteria" end) + end) + elif $d.followup != null then + "finding \($d.finding_id) is not deferred and must not have a follow-up" + else empty end) + end) + end; +' + +# review_result_errors : rules for a reviewer's result. +review_result_errors() { + review_data_is_json "$1" || { + echo "the result is not exactly one JSON value" + return 0 + } + + jq -r "$_review_data_rules"' review_result_rules' "$1" +} + +# review_result_with_ids +# Prints the validated result, reduced to its known fields, with a stable +# identifier per finding: C1, M1, MIN1, S1. +review_result_with_ids() { + jq ' + def prefix: {"critical": "C", "major": "M", "minor": "MIN", "suggestion": "S"}[.]; + { + verdict, + limitations, + findings: ( + reduce .findings[] as $f ({count: {}, out: []}; + .count[$f.severity] = ((.count[$f.severity] // 0) + 1) + | .out += [{ + id: (($f.severity | prefix) + (.count[$f.severity] | tostring)), + severity: $f.severity, + title: $f.title, + evidence: $f.evidence, + impact: $f.impact, + recommendation: $f.recommendation + }] + ) | .out + ) + } + ' "$1" +} + +# review_artifact_errors : rules for a stored review artifact. +review_artifact_errors() { + review_data_is_json "$1" || { + echo "the review is not exactly one JSON value" + return 0 + } + + jq -r "$_review_data_rules"' + if type != "object" or .schema != "review/v1" then + "the file is not a review/v1 artifact" + else + unexpected(["base", "branch", "created_at", "findings", "head", "issue", "limitations", + "merge_base", "reviewed_tree", "reviewer", "round", "schema", "verdict"]; "the review"), + (if (.issue | positive_integer) then empty else "issue must be a positive integer" end), + (if (.round | positive_integer) then empty else "round must be a positive integer" end), + (("branch", "base", "merge_base", "head", "reviewed_tree", "created_at") as $field | + if (.[$field] | nonempty) then empty else "\($field) is missing" end), + (if (.reviewer | type) == "object" and (.reviewer.agent | nonempty) and (.reviewer.model | nonempty) then empty + else "reviewer must name its agent and model" end), + (if (.findings | type) == "array" and (.findings | all(type == "object")) then + (.findings[] | + if (.id | type) == "string" and (.id | test("^(C|M|MIN|S)[1-9][0-9]*$")) + and ((.id | sub("[0-9]+$"; "")) == ({"critical": "C", "major": "M", "minor": "MIN", "suggestion": "S"}[.severity])) then empty + else "finding id \(.id | tostring) does not match its severity \(.severity | tostring)" end), + ([.findings[] | .id] as $ids + | if ($ids | unique | length) != ($ids | length) then "finding ids must be unique" else empty end) + else empty end), + ({ + verdict, + limitations, + findings: (if (.findings | type) == "array" + then (.findings | map(if type == "object" then del(.id) else . end)) + else .findings end) + } | review_result_rules) + end + ' "$1" +} + +# review_render_markdown +review_render_markdown() { + jq -r --arg source "$2" ' + def section($severity; $heading): + "## \($heading)\n\n" + + ([.findings[] | select(.severity == $severity)] as $items + | if ($items | length) == 0 then "None.\n" + else ($items | map( + "### \(.id). \(.title)\n\n" + + "**Evidence:** \(.evidence)\n\n" + + "**Impact:** \(.impact)\n\n" + + "**Recommended action:** \(.recommendation)\n" + ) | join("\n")) end); + "\n\n" + + "# Independent Review — \(.branch)\n\n" + + "Issue: #\(.issue)\n\n" + + "Round: \(.round)\n\n" + + "Base: \(.base) (\(.merge_base))\n\n" + + "HEAD at review start: \(.head)\n\n" + + "Reviewed tree: \(.reviewed_tree)\n\n" + + "Reviewer: \(.reviewer.agent) (\(.reviewer.model)), read-only\n\n" + + section("critical"; "Critical") + "\n" + + section("major"; "Major") + "\n" + + section("minor"; "Minor") + "\n" + + section("suggestion"; "Suggestions") + "\n" + + "## Verdict\n\n\(.verdict | gsub("_"; " "))\n" + + (if (.limitations | test("\\S")) then "\n## Limitations\n\n\(.limitations)\n" else "" end) + ' "$1" +} + +# triage_decision_errors : rules for a triage +# agent's decisions about the findings of a review. +triage_decision_errors() { + review_data_is_json "$1" || { + echo "the decisions are not exactly one JSON value" + return 0 + } + + jq -r --slurpfile review "$2" "$_review_data_rules"' + triage_decision_rules($review[0].findings | map({key: .id, value: .severity}) | from_entries) + ' "$1" +} + +# triage_source_review +# Prints the source review path of a triage/v1 artifact; prints nothing when +# the file is not such an artifact. +triage_source_review() { + review_data_is_json "$1" || return 0 + jq -r 'if type == "object" and .schema == "triage/v1" and (.source_review | type) == "string" + then .source_review else empty end' "$1" +} + +# triage_artifact_errors : rules for a stored, +# approved triage artifact, checked against its valid source review. A triage +# is complete only when every deferred finding has its follow-up Issue. +triage_artifact_errors() { + review_data_is_json "$1" || { + echo "the triage artifact is not exactly one JSON value" + return 0 + } + + jq -r --slurpfile review "$2" "$_review_data_rules"' + 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", + "schema", "source_review", "triage"]; "the triage"), + (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), + (if .issue == $review[0].issue then empty + else "triage and review reference different source Issues" end), + (if (.triage | type) == "object" and (.triage.agent | nonempty) and (.triage.model | nonempty) then empty + else "triage must name its agent and model" end), + (if .review_verdict == $review[0].verdict and .reviewed_tree == $review[0].reviewed_tree then empty + else "the triage does not describe the stored state of its source review" end), + ($review[0].findings | map({key: .id, value: {severity, title}}) | from_entries) as $source + | (if (.decisions | type) == "array" then + (.decisions[] | objects | select($source[.finding_id] != null) + | if {severity, title} == $source[.finding_id] then empty + else "the decision for \(.finding_id) does not match the severity and title in the review" end) + else empty end), + (if (.decisions | type) == "array" then + (.decisions[] | objects | + unexpected(["decision", "finding_id", "followup", "rationale", "severity", "title"]; + "the decision for \(.finding_id | tostring)"), + missing(["decision", "finding_id", "followup", "rationale", "severity", "title"]; + "the decision for \(.finding_id | tostring)"), + (if (.followup | type) == "object" then + (.followup | + unexpected(["acceptance_criteria", "issue_number", "issue_url", "recommended_action", "title"]; + "a follow-up"), + (if (.issue_number | positive_integer) + and (.issue_url | type) == "string" + and (.issue_url | test("^https://[^ ]+/issues/[0-9]+$")) + and ((.issue_url | sub("^.*/"; "")) == (.issue_number | tostring)) then empty + else "a deferred finding has no valid follow-up Issue reference" end)) + else empty end)) + else empty end), + ({ + decisions: (if (.decisions | type) == "array" then + (.decisions | map(if type == "object" then { + finding_id, + decision, + rationale, + followup: (if (.followup | type) == "object" + then (.followup | del(.issue_number, .issue_url)) else .followup end) + } else . end)) + else .decisions end) + } | triage_decision_rules($review[0].findings | map({key: .id, value: .severity}) | from_entries)) + end + ' "$1" +} + +# triage_render_markdown +triage_render_markdown() { + jq -r --arg source "$2" ' + def group($decision; $heading): + "## \($heading)\n\n" + + ([.decisions[] | select(.decision == $decision)] as $items + | if ($items | length) == 0 then "None.\n" + else ($items | map( + "### \(.finding_id). \(.title)\n\n" + + "- Severity: \(.severity)\n" + + "- Rationale: \(.rationale)\n" + + (if .followup != null then + "- Follow-up Issue: \(.followup.title)" + + (if .followup.issue_number != null then " ([#\(.followup.issue_number)](\(.followup.issue_url)))" else "" end) + "\n" + + "- Recommended action: \(.followup.recommended_action)\n" + + "- Acceptance criteria:\n" + (.followup.acceptance_criteria | map(" - \(.)\n") | join("")) + else "" end) + ) | join("\n")) end); + "\n\n" + + "# Review Triage\n\n" + + "Source review: `\(.source_review)`\n\n" + + "Source feature Issue: #\(.issue)\n\n" + + "Reviewer verdict: \(.review_verdict | gsub("_"; " "))\n\n" + + "Triage agent: \(.triage.agent) (\(.triage.model))\n\n" + + "Approved at: \(.approved_at)\n\n" + + group("FIX_NOW"; "Fix now") + "\n" + + group("DEFER"; "Deferred") + "\n" + + group("ACCEPT"; "Accepted") + ' "$1" +} diff --git a/scripts/review-feature.sh b/scripts/review-feature.sh index a36fd80..a5a178b 100755 --- a/scripts/review-feature.sh +++ b/scripts/review-feature.sh @@ -2,7 +2,9 @@ set -euo pipefail -source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)/lib/agent.sh" +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" +source "$script_dir/lib/agent.sh" +source "$script_dir/lib/review-data.sh" fail() { echo "Error: $*" >&2 @@ -34,6 +36,7 @@ slug="${branch//\//-}" reviews_dir="$root/.agents/reviews" prompt_file="$root/.agents/prompts/reviewer.md" +schema_file="$root/.agents/schemas/review.schema.json" # Ensure we're reviewing the expected feature branch. if [[ "$branch" != feature/${issue}-* ]]; then @@ -47,6 +50,8 @@ agent="$AGENT_PROVIDER" model="$AGENT_MODEL" [[ -f "$prompt_file" ]] || fail "reviewer prompt not found: $prompt_file" +[[ -f "$schema_file" ]] || fail "review schema not found: $schema_file" +review_data_require_jq command -v gh >/dev/null 2>&1 || fail "GitHub CLI 'gh' is not installed." gh auth status >/dev/null 2>&1 || fail "GitHub CLI is not authenticated. Run: gh auth login" @@ -93,7 +98,7 @@ tree_before="$(snapshot_tree before)" review_paths=(. ":(exclude).agents/reviews" ":(exclude).agents/triage") context_file="$tmp_work/context.md" -report_file="$tmp_work/report.md" +report_file="$tmp_work/result.json" if GIT_INDEX_FILE="$tmp_work/index.before" git -C "$root" diff --cached --quiet "$merge_base" -- "${review_paths[@]}"; then fail "no changes to review between $base_ref and the working tree." @@ -119,12 +124,16 @@ fi review_number=1 previous_review="" while true; do - candidate="$reviews_dir/${slug}-review-$(printf "%02d" "$review_number").md" - if [[ ! -e "$candidate" ]]; then + candidate="$reviews_dir/${slug}-review-$(printf "%02d" "$review_number")" + if [[ ! -e "$candidate.json" && ! -e "$candidate.md" ]]; then out="$candidate" break fi - previous_review="$candidate" + # A round without JSON, such as a review from before JSON artifacts, is not + # an input for the re-review. + if [[ -f "$candidate.json" ]]; then + previous_review="$candidate.json" + fi review_number=$((review_number + 1)) done review_relative=".agents/reviews/$(basename "$out")" @@ -135,7 +144,7 @@ echo " Branch: $branch" echo " Base: $base_ref" echo " Agent: $agent" echo " Model: $model" -echo " Output: $review_relative" +echo " Output: $review_relative.json" if [[ -n "$previous_review" ]]; then echo " Previous: .agents/reviews/$(basename "$previous_review")" fi @@ -157,7 +166,7 @@ if [[ -n "$previous_review" ]]; then This is a re-review. -Read the previous review: +Read the previous review (JSON): .agents/reviews/$(basename "$previous_review") Check whether its findings have been resolved, but perform an independent review of the complete current implementation. Do not limit the review to the previous findings." @@ -166,74 +175,83 @@ fi START_PROMPT+=" You have read-only access. Do not modify, create, or delete any file. -Return the complete review as your final message, starting with the -'## Critical' section. The calling script stores it." +Return the review as JSON that matches the supplied schema. The calling +script validates and stores it." echo "Starting $agent reviewer ($model) with read-only permissions..." echo -set +e -agent_run read-only "$agent" "$model" "$root" "$START_PROMPT" "$report_file" "$context_file" -agent_status=$? -set -e - -if [[ "$(git -C "$root" rev-parse HEAD)" != "$head_before" || "$(snapshot_tree after)" != "$tree_before" ]]; then - echo "Error: the reviewer modified the working tree or created a commit. No review was stored." >&2 - git -C "$root" status --short >&2 - exit 1 -fi - -[[ "$agent_status" -eq 0 ]] || - fail "reviewer failed with status $agent_status. No review was stored." - -# Keep the report from its first section heading and require a usable verdict. -report_body="$(awk '/^## / { found = 1 } found { print }' "$report_file" 2>/dev/null || true)" -verdict="$(printf '%s\n' "$report_body" | awk ' - /^## Verdict[[:space:]]*$/ { in_verdict = 1; next } - in_verdict && /^## / { exit } - in_verdict && $0 !~ /^[[:space:]]*$/ { - gsub(/[*`]/, "") - sub(/^[[:space:]]+/, "") - sub(/[[:space:]]+$/, "") - print - exit - } -')" - -for section in "## Critical" "## Major" "## Minor" "## Suggestions" "## Verdict"; do - if ! grep -Eq "^${section}[[:space:]]*$" <<<"$report_body"; then - echo "Error: the reviewer's report has no '$section' section. No review was stored." >&2 - echo "Returned output:" >&2 - cat "$report_file" >&2 2>/dev/null || true +# Invalid output is retried once; a failed agent or a modified tree is not. +attempt_prompt="$START_PROMPT" +for attempt in 1 2; do + : >"$report_file" + set +e + agent_run read-only "$agent" "$model" "$root" "$attempt_prompt" "$report_file" "$context_file" "$schema_file" + agent_status=$? + set -e + + if [[ "$(git -C "$root" rev-parse HEAD)" != "$head_before" || "$(snapshot_tree "after-$attempt")" != "$tree_before" ]]; then + echo "Error: the reviewer modified the working tree or created a commit. No review was stored." >&2 + git -C "$root" status --short >&2 exit 1 fi -done -case "$verdict" in - PASS | "PASS WITH MINOR FINDINGS" | "CHANGES REQUIRED") ;; - *) - fail "the reviewer's verdict is missing or invalid: '${verdict}'. No review was stored." - ;; -esac + [[ "$agent_status" -eq 0 ]] || + fail "reviewer failed with status $agent_status. No review was stored." + + result_errors="$(review_result_errors "$report_file")" + [[ -n "$result_errors" ]] || break + + echo "The reviewer returned an invalid result (attempt $attempt of 2):" >&2 + printf '%s\n' "$result_errors" | sed 's/^/ - /' >&2 + [[ "$attempt" -lt 2 ]] || fail "the reviewer's result is invalid. No review was stored." + echo "Retrying once..." >&2 + attempt_prompt="$START_PROMPT + +Your previous result was rejected for these reasons: +$result_errors + +Return a corrected result." +done mkdir -p "$reviews_dir" -{ - echo "# Independent Review — $branch" - echo - echo "Issue: #$issue" - echo - echo "Base: $base_ref ($merge_base)" - echo - echo "HEAD at review start: $head_before" - echo - echo "Working tree changes included: yes" - echo - echo "Reviewed tree: $tree_before" - echo - echo "Reviewer: $agent ($model), read-only" - echo - printf '%s\n' "$report_body" -} >"$out" +review_result_with_ids "$report_file" | jq \ + --argjson issue "$issue" \ + --argjson round "$review_number" \ + --arg branch "$branch" \ + --arg base "$base_ref" \ + --arg merge_base "$merge_base" \ + --arg head "$head_before" \ + --arg tree "$tree_before" \ + --arg agent "$agent" \ + --arg model "$model" \ + --arg created_at "$(date -u +'%Y-%m-%dT%H:%M:%SZ')" \ + '{ + schema: "review/v1", + issue: $issue, + round: $round, + branch: $branch, + base: $base, + merge_base: $merge_base, + head: $head, + reviewed_tree: $tree, + reviewer: {agent: $agent, model: $model}, + created_at: $created_at, + verdict: .verdict, + limitations: .limitations, + findings: .findings + }' >"$out.json" + +artifact_errors="$(review_artifact_errors "$out.json")" +if [[ -n "$artifact_errors" ]]; then + rm -f "$out.json" + echo "Error: the stored review would be invalid; nothing was stored:" >&2 + printf '%s\n' "$artifact_errors" | sed 's/^/ - /' >&2 + exit 1 +fi +review_render_markdown "$out.json" "$(basename "$out").json" >"$out.md" -echo "Review completed: $verdict" -echo " $review_relative" +verdict="$(jq -r '.verdict | gsub("_"; " ")' "$out.json")" +echo "Review completed: $verdict ($(jq '.findings | length' "$out.json") findings)" +echo " $review_relative.json (source of truth)" +echo " $review_relative.md (generated report)" diff --git a/scripts/triage-review.sh b/scripts/triage-review.sh index 6a3f754..ead03c9 100755 --- a/scripts/triage-review.sh +++ b/scripts/triage-review.sh @@ -2,17 +2,24 @@ set -euo pipefail -source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)/lib/agent.sh" +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" +source "$script_dir/lib/agent.sh" +source "$script_dir/lib/review-data.sh" usage() { - echo "Usage: $0 [--agent ] [--model ]" + echo "Usage: $0 [--agent ] [--model ]" echo echo "The agent and model come from role 'triage' in .agents/agents.conf" echo "unless --agent and --model are given." echo echo "Examples:" - echo " $0 .agents/reviews/feature-5-rendering-review-01.md" - echo " $0 .agents/reviews/feature-5-rendering-review-01.md --agent claude --model fable" + echo " $0 .agents/reviews/feature-5-rendering-review-01.json" + echo " $0 .agents/reviews/feature-5-rendering-review-01.json --agent claude --model fable" + exit 1 +} + +fail() { + echo "Error: $*" >&2 exit 1 } @@ -25,258 +32,61 @@ review_input="${AGENT_POSITIONAL[0]}" root="$(git rev-parse --show-toplevel)" prompt_file="$root/.agents/prompts/triage-reviewer.md" +schema_file="$root/.agents/schemas/triage.schema.json" -if [[ ! -f "$review_input" ]]; then - echo "Error: review artifact not found: $review_input" - exit 1 -fi +[[ -f "$review_input" ]] || fail "review artifact not found: $review_input" review_dir="$(cd "$(dirname "$review_input")" && pwd -P)" review_path="$review_dir/$(basename "$review_input")" -reviews_root="$root/.agents/reviews" -if [[ "$review_path" != "$reviews_root/"*.md ]]; then - echo "Error: review artifact must match .agents/reviews/*.md" - echo "Received: $review_path" - exit 1 +if [[ "$review_path" != "$root/.agents/reviews/"*.json ]]; then + fail "review artifact must match .agents/reviews/*.json (the generated .md report is not an input). Received: $review_path" fi -if [[ ! -f "$prompt_file" ]]; then - echo "Error: triage prompt not found: $prompt_file" - exit 1 -fi +[[ -f "$prompt_file" ]] || fail "triage prompt not found: $prompt_file" +[[ -f "$schema_file" ]] || fail "triage schema not found: $schema_file" +review_data_require_jq agent_resolve "$root" triage "$AGENT_CLI_PROVIDER" "$AGENT_CLI_MODEL" agent="$AGENT_PROVIDER" model="$AGENT_MODEL" -if ! command -v gh >/dev/null 2>&1; then - echo "Error: GitHub CLI 'gh' is not installed." - exit 1 -fi +command -v gh >/dev/null 2>&1 || fail "GitHub CLI 'gh' is not installed." +gh auth status >/dev/null 2>&1 || fail "GitHub CLI is not authenticated. Run: gh auth login" -if ! gh auth status >/dev/null 2>&1; then - echo "Error: GitHub CLI is not authenticated." - echo "Run: gh auth login" +review_errors="$(review_artifact_errors "$review_path")" +if [[ -n "$review_errors" ]]; then + echo "Error: invalid review artifact:" >&2 + printf '%s\n' "$review_errors" | sed 's/^/ - /' >&2 exit 1 fi -tmp_work="$(mktemp -d "${TMPDIR:-/tmp}/triage-review.XXXXXX")" -trap 'rm -rf "$tmp_work"' EXIT - -findings_file="$tmp_work/findings.tsv" -decisions_file="$tmp_work/decisions.tsv" -followup_titles_file="$tmp_work/followup-titles.tsv" -mapping_file="$tmp_work/mappings.tsv" -: >"$followup_titles_file" -: >"$mapping_file" - -if ! awk -v output="$findings_file" ' - function trim(value) { - sub(/^[[:space:]]+/, "", value) - sub(/[[:space:]]+$/, "", value) - return value - } - - function singular(value) { - return value == "Suggestions" ? "Suggestion" : value - } - - function emit_finding(severity, raw, line_number, source_type, id, title, token, rest, count, parts) { - raw = trim(raw) - ordinal[severity]++ - id = singular(severity) "-" ordinal[severity] - title = raw - - if (raw ~ /^(Critical|Major|Minor|Suggestion)[[:space:]]+[0-9]+[.):-]?([[:space:]]+|$)/) { - count = split(raw, parts, /[[:space:]]+/) - token = parts[2] - gsub(/[.):-]+$/, "", token) - id = parts[1] " " token - rest = raw - sub(/^(Critical|Major|Minor|Suggestion)[[:space:]]+[0-9]+[.):-]?[[:space:]]*/, "", rest) - title = trim(rest) - } else if (raw ~ /^[CMS][0-9]+[.):-]?([[:space:]]+|$)/) { - token = raw - sub(/[[:space:]].*$/, "", token) - gsub(/[.):-]+$/, "", token) - id = token - rest = raw - sub(/^[CMS][0-9]+[.):-]?[[:space:]]*/, "", rest) - title = trim(rest) - } - - if (title == "") { - title = raw - } - - gsub(/\t/, " ", id) - gsub(/\t/, " ", title) - total++ - printf "F%03d\t%s\t%s\t%s\t%d\t%s\n", total, severity, id, title, line_number, source_type > output - } - - /^## Critical[[:space:]]*$/ { - section = "Critical" - next - } - /^## Major[[:space:]]*$/ { - section = "Major" - next - } - /^## Minor[[:space:]]*$/ { - section = "Minor" - next - } - /^## Suggestions[[:space:]]*$/ { - section = "Suggestions" - next - } - /^## Verdict[[:space:]]*$/ { - section = "" - next - } - /^##[[:space:]]+/ { - section = "" - next - } - - section != "" { - value = trim($0) - - if ($0 ~ /^###[[:space:]]+/) { - raw = $0 - sub(/^###[[:space:]]+/, "", raw) - entries++ - entry_section[entries] = section - entry_type[entries] = "heading" - entry_text[entries] = raw - entry_line[entries] = NR - headings[section]++ - next - } - - if ($0 ~ /^-[[:space:]]+/) { - raw = $0 - sub(/^-[[:space:]]+/, "", raw) - entries++ - entry_section[entries] = section - entry_type[entries] = "bullet" - entry_text[entries] = raw - entry_line[entries] = NR - bullets[section]++ - next - } - - lower = tolower(value) - empty_section = (lower == "none" || lower == "none." || lower == "none identified" || lower == "none identified." || lower == "no findings" || lower == "no findings." || lower ~ /^no (critical|major|minor|suggestion|suggestions)( findings?)?[.]?$/) - if (value != "" && value !~ /^" - echo - echo "# $issue_title" - echo - echo "## Context" - echo - if [[ -n "$source_issue" ]]; then - echo "Deferred finding from the independent review of Issue #$source_issue." - else - echo "Deferred finding from an independent review." - fi - echo - echo "Original finding: $finding_id" - echo - echo "## Finding" - echo - echo "$finding_excerpt" - echo - echo "## Evidence" - echo - echo "See \`$review_relative\`, line $source_line ($severity)." - echo - echo "## Why it matters" - echo - echo "$rationale" - echo - echo "## Recommended action" - echo - echo "$recommended_action" - echo - echo "## Acceptance criteria" - echo - echo "- $acceptance_criteria" - echo "- \`./scripts/verify.sh\` passes." - } >"$issue_body_file" + issue_title="$(jq -r --arg id "$finding_id" '.decisions[] | select(.finding_id == $id) | .followup.title' "$pending")" + issue_body_file="$tmp_work/${finding_id}-issue.md" + jq -r \ + --slurpfile review "$review_path" \ + --arg id "$finding_id" \ + --arg trace "$trace_token" \ + --arg source_review "$review_relative" ' + (.decisions[] | select(.finding_id == $id)) as $d + | ($review[0].findings[] | select(.id == $id)) as $f + | "\n\n" + + "# \($d.followup.title)\n\n" + + "## Context\n\n" + + "Deferred finding from the independent review of Issue #\(.issue).\n\n" + + "Original finding: \($f.id) (\($f.severity))\n\n" + + "## Finding\n\n\($f.title)\n\n" + + "## Evidence\n\n\($f.evidence)\n\nSource: `\($source_review)`\n\n" + + "## Why it matters\n\n\($f.impact)\n\nTriage rationale: \($d.rationale)\n\n" + + "## Recommended action\n\n\($d.followup.recommended_action)\n\n" + + "## Acceptance criteria\n\n" + + ($d.followup.acceptance_criteria | map("- \(.)\n") | join("")) + + "- `./scripts/verify.sh` passes." + ' "$pending" >"$issue_body_file" echo "Creating follow-up Issue for $finding_id..." - issue_url="$(gh issue create --title "$issue_title" --body-file "$issue_body_file")" - issue_number="${issue_url##*/}" - if [[ ! "$issue_number" =~ ^[0-9]+$ ]]; then - echo "Error: could not determine Issue number from: $issue_url" - exit 1 - fi - - printf '%s\t%s\t%s\n' "$finding_id" "$issue_number" "$issue_url" >>"$mapping_file" - echo "- $finding_id → [#$issue_number]($issue_url)" >>"$artifact" -done <"$decisions_file" - -if [[ ! -s "$mapping_file" ]]; then - echo "No follow-up Issues created." >>"$artifact" + issue_url="$(gh issue create --title "$issue_title" --body-file "$issue_body_file" &2 + printf '%s\n' "$artifact_errors" | sed 's/^/ - /' >&2 + exit 1 fi +cp "$pending" "$artifact.json" +triage_render_markdown "$artifact.json" "$(basename "$artifact").json" >"$artifact.md" + echo echo "Triage completed:" -echo " Artifact: $artifact_relative" +echo " $artifact_relative.json (source of truth)" +echo " $artifact_relative.md (generated report)" echo -render_proposal - -if [[ -s "$mapping_file" ]]; then - echo "Deferred Issue mappings:" - awk -F '\t' '{ printf "- %s → #%s\n", $1, $2 }' "$mapping_file" -fi +jq '.decisions' "$artifact.json" >"$tmp_work/final.json" +render_proposal "$tmp_work/final.json" diff --git a/tests/agent-test.sh b/tests/agent-test.sh index 8015587..3e8d1a6 100755 --- a/tests/agent-test.sh +++ b/tests/agent-test.sh @@ -150,10 +150,12 @@ done # A read-only run uses the read-only flags of each provider. : >"$MOCK_AGENT_LOG" agent_run read-only codex model-a "$workdir" "the prompt" "$tmp/report.txt" -grep -Fq -- "--sandbox read-only" "$MOCK_AGENT_LOG" || fail "codex read-only run was not sandboxed" +for flag in "--ignore-user-config" "--sandbox read-only" "--disable apps" "--disable computer_use" 'web_search="disabled"'; do + grep -Fq -- "$flag" "$MOCK_AGENT_LOG" || fail "isolated Codex read-only session lacks: $flag" +done : >"$MOCK_AGENT_LOG" agent_run read-only claude model-b "$workdir" "the prompt" "$tmp/report.txt" -grep -Fq -- "--permission-mode plan --tools Read,Glob,Grep" "$MOCK_AGENT_LOG" || +grep -Fq -- "--strict-mcp-config --permission-mode dontAsk --tools Read,Glob,Grep" "$MOCK_AGENT_LOG" || fail "claude read-only run was not restricted to read tools" # An unknown profile, or a read-only run without an output file, starts no agent. @@ -171,4 +173,45 @@ status=0 MOCK_AGENT_EXIT=7 agent_run write claude model-b "$workdir" "the prompt" >/dev/null || status=$? [[ "$status" -eq 7 ]] || fail "agent exit status was not propagated" +# Structured output from Claude arrives in a result envelope. The launcher +# extracts it, reports errors with a non-zero status, and keeps the session +# isolated from the user's configuration. +mkdir -p "$tmp/envelope-bin" +cat >"$tmp/envelope-bin/claude" <<'FAKE' +#!/usr/bin/env bash +echo "ARGS=$*" >>"$MOCK_AGENT_LOG" +printf '%s\n' "$MOCK_ENVELOPE" +exit "${MOCK_AGENT_EXIT:-0}" +FAKE +chmod +x "$tmp/envelope-bin/claude" +printf '{"type": "object"}\n' >"$tmp/schema.json" + +run_structured() { + PATH="$tmp/envelope-bin:$PATH" agent_run read-only claude model-b "$workdir" "the prompt" \ + "$tmp/structured.json" /dev/null "$tmp/schema.json" 2>/dev/null +} + +: >"$MOCK_AGENT_LOG" +MOCK_ENVELOPE='{"is_error": false, "structured_output": {"verdict": "PASS"}, "result": "ignored"}' run_structured || + fail "structured output was not accepted" +[[ "$(jq -c . "$tmp/structured.json")" == '{"verdict":"PASS"}' ]] || fail "structured output was not extracted" +for flag in "--restricted" "--strict-mcp-config" "--permission-mode dontAsk" "--tools Read,Glob,Grep" "--json-schema"; do + grep -Fq -- "$flag" "$MOCK_AGENT_LOG" || fail "isolated Claude read-only session lacks: $flag" +done +if grep -Fq -- "--permission-mode plan" "$MOCK_AGENT_LOG"; then + fail "Claude read-only session still uses plan mode" +fi + +MOCK_ENVELOPE='{"is_error": false, "result": "{\"verdict\": \"PASS\"}"}' run_structured || + fail "a JSON result without structured_output was not accepted" +[[ "$(jq -c . "$tmp/structured.json")" == '{"verdict":"PASS"}' ]] || fail "JSON in the result field was not extracted" + +status=0 +MOCK_ENVELOPE='{"is_error": true, "result": "API error"}' run_structured || status=$? +[[ "$status" -ne 0 ]] || fail "an error reported in the envelope returned success" + +status=0 +MOCK_AGENT_EXIT=5 MOCK_ENVELOPE='{"is_error": true, "result": "failed"}' run_structured || status=$? +[[ "$status" -eq 5 ]] || fail "Claude's exit status was not returned (got $status)" + echo "agent tests passed" diff --git a/tests/apply-triage-test.sh b/tests/apply-triage-test.sh index 8dd6a08..48f91a9 100755 --- a/tests/apply-triage-test.sh +++ b/tests/apply-triage-test.sh @@ -2,367 +2,241 @@ set -euo pipefail -# apply-triage.sh runs ./scripts/verify.sh; do not recurse into this suite. -if [[ "${APPLY_TRIAGE_TEST_ACTIVE:-0}" == "1" ]]; then - echo "apply-triage tests skipped inside a nested verification run" - exit 0 -fi - -root="$(git rev-parse --show-toplevel)" -script="$root/scripts/apply-triage.sh" -review_dir="$root/.agents/reviews" -triage_dir="$root/.agents/triage" -test_stem="feature-13-apply-test-$$-$RANDOM" -review="$review_dir/${test_stem}-review-01.md" -triage="$triage_dir/${test_stem}-review-01-triage.md" -decline_triage="$triage_dir/${test_stem}-review-02-triage.md" -empty_triage="$triage_dir/${test_stem}-review-03-triage.md" -malformed_triage="$triage_dir/${test_stem}-review-04-triage.md" -unapproved_triage="$triage_dir/${test_stem}-review-05-triage.md" -stale_triage="$triage_dir/${test_stem}-review-06-triage.md" -ambiguous_triage="$triage_dir/${test_stem}-review-07-triage.md" -deleted_triage="$triage_dir/${test_stem}-review-08-triage.md" +source_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd -P)" tmp="$(mktemp -d "${TMPDIR:-/tmp}/apply-triage-test.XXXXXX")" cleanup() { rm -rf "$tmp" - rm -f "$review" "$triage" "$decline_triage" "$empty_triage" "$malformed_triage" - rm -f "$unapproved_triage" "$stale_triage" "$ambiguous_triage" - rm -f "$deleted_triage" } trap cleanup EXIT -mkdir -p "$review_dir" "$triage_dir" "$tmp/bin" -for test_path in "$review" "$triage" "$decline_triage" "$empty_triage" "$malformed_triage" "$unapproved_triage" "$stale_triage" "$ambiguous_triage" "$deleted_triage"; do - if [[ -e "$test_path" ]]; then - echo "Refusing to overwrite pre-existing test path: $test_path" >&2 - exit 1 - fi -done - -cat >"$review" <<'REVIEW' -# Independent Review — feature/13-apply-test - -Issue: #13 - -## Critical - -### C1. Correctness regression - -The incorrect branch must be fixed. -- S99. Evidence detail that is not a finding - -## Minor - -### Minor 1. Deferred cleanup - -This work is outside the current scope. - -## Suggestions - -### Accepted rename - -The existing name is acceptable. - -## Verdict - -CHANGES REQUIRED -REVIEW - -write_triage() { - local target="$1" - local fix_decision="$2" - local include_approval="$3" - - { - echo "# Review Triage — $test_stem" - echo - echo "Source review: \`.agents/reviews/$(basename "$review")\`" - echo - echo "Source feature Issue: #13" - echo - echo "Reviewer verdict: CHANGES REQUIRED" - echo - echo "Triage agent: claude" - if [[ "$include_approval" == "yes" ]]; then - echo - echo "Approved at: 2026-09-11T10:00:00Z" - fi - echo - echo "## Fix now" - echo - echo "### C1. Correctness regression" - echo - echo "- Severity: Critical" - echo "- Decision: $fix_decision" - echo "- Source line: 7" - echo "- Rationale: Correctness blocks the feature." - echo - echo "## Deferred" - echo - echo "### Minor 1. Deferred cleanup" - echo - echo "- Severity: Minor" - echo "- Decision: DEFER" - echo "- Source line: 15" - echo "- Rationale: This belongs in follow-up work." - echo "- Proposed Issue: [F03][R01][Minor-1] Deferred cleanup" - echo "- Recommended action: Handle separately." - echo "- Acceptance criteria: Cleanup is complete." - echo "- Created Issue: see Traceability" - echo - echo "## Accepted" - echo - echo "### Suggestion-1. Accepted rename" - echo - echo "- Severity: Suggestions" - echo "- Decision: ACCEPT" - echo "- Source line: 23" - echo "- Rationale: The existing name is adequate." - echo - echo "## Traceability" - echo - echo "- Minor 1 → [#99](https://github.com/example/project/issues/99)" - } >"$target" +fail() { + echo "apply-triage test failed: $*" >&2 + exit 1 } -write_triage "$triage" "FIX_NOW" "yes" -cp "$triage" "$decline_triage" -write_triage "$malformed_triage" "DEFER" "yes" -write_triage "$unapproved_triage" "FIX_NOW" "no" -awk '{ gsub(/C1\. Correctness regression/, "S99. Evidence detail that is not a finding"); print }' "$triage" >"$stale_triage" -awk ' - { print } - /^Source feature Issue: #13$/ { - print "Source feature Issue: #99" - } -' "$triage" >"$ambiguous_triage" -cp "$triage" "$deleted_triage" - -cat >"$empty_triage" <"$tmp/bin/gh" <<'GH' #!/usr/bin/env bash set -euo pipefail -if [[ "${1:-} ${2:-}" == "auth status" ]]; then - exit 0 -fi -if [[ "${1:-} ${2:-}" == "issue view" ]]; then - echo "13" - exit 0 -fi +case "${1:-} ${2:-}" in + "auth status") exit 0 ;; + "issue view") echo "13"; exit 0 ;; +esac echo "Unexpected gh invocation: $*" >&2 exit 1 GH +chmod +x "$tmp/bin/gh" + +review=".agents/reviews/feature-13-apply-test-review-01.json" +triage=".agents/triage/feature-13-apply-test-review-01-triage.json" + +# Creates a repository on feature/13-apply-test with a stored review and its +# approved triage. Its verification passes unless a check is replaced. +setup_repo() { + local repo="$tmp/$1" + + mkdir -p "$repo/.agents/reviews" "$repo/.agents/triage" + copy_workflow "$repo" + printf 'triage-implementer: codex model-i\n' >"$repo/.agents/agents.conf" + printf 'marker: test -f AGENTS.md\n' >"$repo/scripts/verify.conf" + printf '# Agents\n' >"$repo/AGENTS.md" + + jq -n '{ + schema: "review/v1", issue: 13, round: 1, branch: "feature/13-apply-test", base: "main", + merge_base: "aaaa", head: "bbbb", reviewed_tree: "cccc", + reviewer: {agent: "claude", model: "model-r"}, created_at: "2026-01-01T00:00:00Z", + verdict: "CHANGES_REQUIRED", limitations: "", + findings: [ + {id: "C1", severity: "critical", title: "Correctness regression", evidence: "src/a.sh:3", impact: "Wrong result.", recommendation: "Fix the branch."}, + {id: "MIN1", severity: "minor", title: "Deferred cleanup", evidence: "src/a.sh:9", impact: "Clutter.", recommendation: "Clean up."}, + {id: "S1", severity: "suggestion", title: "Accepted rename", evidence: "src/a.sh:8", impact: "Readability.", recommendation: "Rename."} + ] + }' >"$repo/$review" + + jq -n --arg review "$review" '{ + schema: "triage/v1", source_review: $review, issue: 13, reviewed_tree: "cccc", + review_verdict: "CHANGES_REQUIRED", triage: {agent: "claude", model: "model-t"}, + approved_at: "2026-01-01T10:00:00Z", + decisions: [ + {finding_id: "C1", severity: "critical", title: "Correctness regression", decision: "FIX_NOW", rationale: "Correctness blocks the feature.", followup: null}, + {finding_id: "MIN1", severity: "minor", title: "Deferred cleanup", decision: "DEFER", rationale: "Follow-up work.", + followup: {title: "[#13][R01][MIN1] Deferred cleanup", recommended_action: "Handle separately.", acceptance_criteria: ["Done."], issue_number: 99, issue_url: "https://github.com/example/project/issues/99"}}, + {finding_id: "S1", severity: "suggestion", title: "Accepted rename", decision: "ACCEPT", rationale: "Adequate.", followup: null} + ] + }' >"$repo/$triage" + + git -C "$repo" init -q -b main + git -C "$repo" config user.name "Apply Test" + git -C "$repo" config user.email "apply-test@example.com" + git -C "$repo" add . + git -C "$repo" commit -qm "Seed project" + git -C "$repo" switch -q -c feature/13-apply-test + + printf '%s\n' "$repo" +} -cat >"$tmp/bin/codex" <<'CODEX' -#!/usr/bin/env bash -set -euo pipefail -printf 'codex %s\n' "$*" >>"$MOCK_AGENT_LOG" -if [[ -n "${MOCK_CHMOD_PATH:-}" ]]; then - chmod 600 "$MOCK_CHMOD_PATH" -fi -if [[ -n "${MOCK_DELETE_PATH:-}" ]]; then - rm -f "$MOCK_DELETE_PATH" -fi -exit "${MOCK_AGENT_EXIT:-0}" -CODEX - -cat >"$tmp/bin/claude" <<'CLAUDE' -#!/usr/bin/env bash -set -euo pipefail -printf 'claude %s\n' "$*" >>"$MOCK_AGENT_LOG" -if [[ -n "${MOCK_CHMOD_PATH:-}" ]]; then - chmod 600 "$MOCK_CHMOD_PATH" -fi -if [[ -n "${MOCK_DELETE_PATH:-}" ]]; then - rm -f "$MOCK_DELETE_PATH" -fi -exit "${MOCK_AGENT_EXIT:-0}" -CLAUDE - -real_git="$(command -v git)" -printf '%s\n' \ - '#!/usr/bin/env bash' \ - 'set -euo pipefail' \ - 'if [[ "${1:-} ${2:-}" == "branch --show-current" ]]; then' \ - ' echo "feature/13-apply-test"' \ - ' exit 0' \ - 'fi' \ - "exec \"$real_git\" \"\$@\"" >"$tmp/bin/git" - -chmod +x "$tmp/bin/gh" "$tmp/bin/git" "$tmp/bin/codex" "$tmp/bin/claude" - -review_hash_before="$(git hash-object "$review")" -triage_hash_before="$(git hash-object "$triage")" - -output="$( - printf 'y\n' | - PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - APPLY_TRIAGE_TEST_ACTIVE=1 \ - "$script" "$triage" --agent codex --model test-model -)" - -[[ "$output" == *"Approved FIX_NOW scope"* ]] -[[ "$output" == *"### C1. Correctness regression"* ]] -[[ "$output" == *"verification passed"* ]] -[[ "$(git hash-object "$review")" == "$review_hash_before" ]] -[[ "$(git hash-object "$triage")" == "$triage_hash_before" ]] -grep -Fq "codex --sandbox workspace-write --ask-for-approval never --model test-model" "$tmp/agent.log" -grep -Fq "### C1. Correctness regression" "$tmp/agent.log" -if grep -Fq "Deferred cleanup" "$tmp/agent.log" || grep -Fq "Accepted rename" "$tmp/agent.log"; then - echo "Implementation agent received a non-FIX_NOW finding." >&2 - exit 1 -fi - -agent_calls_before="$(grep -c '^[a-z]' "$tmp/agent.log")" -decline_output="$( - printf 'n\n' | - PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - APPLY_TRIAGE_TEST_ACTIVE=1 \ - "$script" "$decline_triage" --agent claude --model sonnet -)" -[[ "$decline_output" == *"Apply triage declined"* ]] -[[ "$(grep -c '^[a-z]' "$tmp/agent.log")" -eq "$agent_calls_before" ]] - -empty_output="$( - PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - APPLY_TRIAGE_TEST_ACTIVE=1 \ - "$script" "$empty_triage" --agent claude --model sonnet -)" -[[ "$empty_output" == *"No FIX_NOW findings found"* ]] -[[ "$(grep -c '^[a-z]' "$tmp/agent.log")" -eq "$agent_calls_before" ]] - -claude_output="$( - printf 'y\n' | - PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - APPLY_TRIAGE_TEST_ACTIVE=1 \ - "$script" "$triage" --agent claude --model opus -)" -[[ "$claude_output" == *"verification passed"* ]] -grep -Fq "claude --permission-mode acceptEdits --model opus" "$tmp/agent.log" - -if PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - APPLY_TRIAGE_TEST_ACTIVE=1 \ - "$script" "$malformed_triage" --agent claude --model sonnet "$tmp/malformed.out" 2>&1; then - echo "Expected malformed triage validation to fail." >&2 - exit 1 -fi -grep -Fq "wrong decision section" "$tmp/malformed.out" +edit_triage() { + local repo="$1" + local filter="$2" -if PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - APPLY_TRIAGE_TEST_ACTIVE=1 \ - "$script" "$unapproved_triage" --agent claude --model sonnet >"$tmp/unapproved.out" 2>&1; then - echo "Expected unapproved triage validation to fail." >&2 - exit 1 -fi -grep -Fq "approval timestamp" "$tmp/unapproved.out" + jq "$filter" "$repo/$triage" >"$repo/$triage.tmp" + mv "$repo/$triage.tmp" "$repo/$triage" +} -if PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - APPLY_TRIAGE_TEST_ACTIVE=1 \ - "$script" "$stale_triage" --agent claude --model sonnet >"$tmp/stale.out" 2>&1; then - echo "Expected stale triage validation to fail." >&2 - exit 1 -fi -if ! grep -Fq "does not map uniquely to the source review" "$tmp/stale.out"; then - cat "$tmp/stale.out" >&2 - exit 1 -fi +run_apply() { + local repo="$1" + local answer="$2" + shift 2 -if PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - APPLY_TRIAGE_TEST_ACTIVE=1 \ - "$script" "$ambiguous_triage" --agent claude --model sonnet >"$tmp/ambiguous.out" 2>&1; then - echo "Expected ambiguous Issue metadata validation to fail." >&2 - exit 1 -fi -grep -Fq "ambiguous source Issue metadata" "$tmp/ambiguous.out" - -if printf 'y\n' | PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - MOCK_AGENT_EXIT=7 \ - APPLY_TRIAGE_TEST_ACTIVE=1 \ - "$script" "$triage" --agent claude --model sonnet >"$tmp/failed-agent.out" 2>&1; then - echo "Expected failed implementation agent to propagate failure." >&2 - exit 1 -fi -grep -Fq "Running repository verification" "$tmp/failed-agent.out" -grep -Fq "implementation agent exited with status 7" "$tmp/failed-agent.out" + ( + cd "$repo" + printf '%s\n' "$answer" | + PATH="$tmp/bin:/usr/bin:/bin" MOCK_AGENT_LOG="$repo.log" ./scripts/apply-triage.sh "$@" + ) >"$repo.out" 2>&1 +} -if [[ "$(uname -s)" == "Darwin" ]]; then - original_mode="$(stat -f '%Lp' "$triage")" -else - original_mode="$(stat -c '%a' "$triage")" -fi -if printf 'y\n' | PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - MOCK_CHMOD_PATH="$triage" \ - APPLY_TRIAGE_TEST_ACTIVE=1 \ - "$script" "$triage" --agent claude --model sonnet >"$tmp/mode.out" 2>&1; then - echo "Expected protected artifact mode change to fail." >&2 - exit 1 -fi -chmod "$original_mode" "$triage" -grep -Fq "modified the approved triage artifact" "$tmp/mode.out" -grep -Fq "Running repository verification" "$tmp/mode.out" - -if printf 'y\n' | PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - MOCK_DELETE_PATH="$deleted_triage" \ - APPLY_TRIAGE_TEST_ACTIVE=1 \ - "$script" "$deleted_triage" --agent claude --model sonnet >"$tmp/deleted.out" 2>&1; then - echo "Expected protected artifact deletion to fail." >&2 - exit 1 -fi -grep -Fq "modified the approved triage artifact" "$tmp/deleted.out" -grep -Fq "Running repository verification" "$tmp/deleted.out" +expect_rejected() { + local repo="$1" + local description="$2" + shift 2 -if PATH="$tmp/bin:/usr/bin:/bin" "$script" "$triage_dir/missing.md" --agent claude --model sonnet >"$tmp/missing.out" 2>&1; then - echo "Expected missing triage validation to fail." >&2 - exit 1 -fi -grep -Fq "triage artifact not found" "$tmp/missing.out" + if run_apply "$repo" y "$@"; then + cat "$repo.out" >&2 + fail "expected failure: $description" + fi + [[ ! -e "$repo.log" ]] || fail "an implementation agent was started although: $description" +} -rm "$tmp/bin/codex" -if PATH="$tmp/bin:/usr/bin:/bin" "$script" "$triage" --agent codex --model test-model >"$tmp/agent.out" 2>&1; then - echo "Expected missing agent validation to fail." >&2 - exit 1 +# After confirmation, a write-capable agent receives exactly the FIX_NOW +# findings, and the result is verified. +repo="$(setup_repo apply)" +run_apply "$repo" y "$triage" || { + cat "$repo.out" >&2 + fail "applying an approved triage failed" +} +grep -Fq -- "--sandbox workspace-write --ask-for-approval never --model model-i" "$repo.log" || + fail "implementation agent was not started write-capable with the configured model" +grep -Fq "C1. Correctness regression" "$repo.log" || fail "the FIX_NOW finding was not passed to the agent" +if grep -Fq "Deferred cleanup" "$repo.log" || grep -Fq "Accepted rename" "$repo.log"; then + fail "the implementation agent received a finding that is not FIX_NOW" +fi +grep -Fq "Verification passed." "$repo.out" || fail "the result was not verified" + +# Declining, and a triage without FIX_NOW findings, start no agent. +repo="$(setup_repo decline)" +run_apply "$repo" n "$triage" || fail "declined apply returned an error" +[[ ! -e "$repo.log" ]] || fail "an agent was started after declining" + +repo="$(setup_repo nothing-to-fix)" +jq '.verdict = "PASS_WITH_MINOR_FINDINGS" | del(.findings[0])' "$repo/$review" >"$repo/$review.tmp" +mv "$repo/$review.tmp" "$repo/$review" +edit_triage "$repo" '.review_verdict = "PASS_WITH_MINOR_FINDINGS" | del(.decisions[0])' +run_apply "$repo" y "$triage" || fail "a triage without FIX_NOW findings returned an error" +[[ ! -e "$repo.log" ]] || fail "an agent was started without FIX_NOW findings" + +# Unapproved, mismatching, or misplaced triage artifacts are rejected. +repo="$(setup_repo unapproved)" +edit_triage "$repo" 'del(.approved_at)' +expect_rejected "$repo" "the triage is not approved" "$triage" + +# An edited artifact cannot weaken a decision: the stored triage is checked +# with the same rules as the triage agent's output. +repo="$(setup_repo critical-accepted)" +edit_triage "$repo" '.decisions[0].decision = "ACCEPT"' +expect_rejected "$repo" "a Critical finding is not FIX_NOW" "$triage" + +repo="$(setup_repo no-followup-issue)" +edit_triage "$repo" '.decisions[1].followup.issue_number = null | .decisions[1].followup.issue_url = null' +expect_rejected "$repo" "a deferred finding has no follow-up Issue" "$triage" + +repo="$(setup_repo no-rationale)" +edit_triage "$repo" '.decisions[0].rationale = ""' +expect_rejected "$repo" "a decision has no rationale" "$triage" + +repo="$(setup_repo invalid-review)" +jq '.findings[0].evidence = ""' "$repo/$review" >"$repo/$review.tmp" +mv "$repo/$review.tmp" "$repo/$review" +expect_rejected "$repo" "the source review is invalid" "$triage" + +# Script-owned fields of stored artifacts are constrained, so they cannot be +# used as paths or contradict the review. +repo="$(setup_repo unsafe-id)" +jq '.findings[2].id = "../S1"' "$repo/$review" >"$repo/$review.tmp" +mv "$repo/$review.tmp" "$repo/$review" +expect_rejected "$repo" "a finding id is not a script-assigned identifier" "$triage" + +repo="$(setup_repo bad-round)" +jq '.round = 1.5' "$repo/$review" >"$repo/$review.tmp" +mv "$repo/$review.tmp" "$repo/$review" +expect_rejected "$repo" "the review round is not a positive integer" "$triage" + +repo="$(setup_repo bad-issue-reference)" +edit_triage "$repo" '.decisions[1].followup.issue_number = -0.5 | .decisions[1].followup.issue_url = "not an issue"' +expect_rejected "$repo" "a follow-up Issue reference is invalid" "$triage" + +repo="$(setup_repo missing-metadata)" +edit_triage "$repo" 'del(.triage) | del(.decisions[2].followup)' +expect_rejected "$repo" "triage metadata and a required field are missing" "$triage" + +repo="$(setup_repo copied-title)" +edit_triage "$repo" '.decisions[0].title = "Something else"' +expect_rejected "$repo" "the triage misstates a finding of the review" "$triage" + +repo="$(setup_repo unknown-finding)" +edit_triage "$repo" '.decisions[0].finding_id = "C9"' +expect_rejected "$repo" "a decision does not map to the review" "$triage" + +repo="$(setup_repo undecided-finding)" +edit_triage "$repo" 'del(.decisions[2])' +expect_rejected "$repo" "a finding of the review has no decision" "$triage" + +repo="$(setup_repo other-issue)" +edit_triage "$repo" '.issue = 99' +expect_rejected "$repo" "triage and review reference different Issues" "$triage" + +repo="$(setup_repo missing-review)" +rm "$repo/$review" +expect_rejected "$repo" "the source review is missing" "$triage" + +repo="$(setup_repo wrong-branch)" +git -C "$repo" switch -q -c feature/14-other +expect_rejected "$repo" "the branch belongs to another Issue" "$triage" + +repo="$(setup_repo preconditions)" +printf '# report\n' >"$repo/.agents/triage/report.md" +expect_rejected "$repo" "the input is the generated report" ".agents/triage/report.md" +expect_rejected "$repo" "the triage does not exist" ".agents/triage/missing.json" +expect_rejected "$repo" "the agent is unsupported" "$triage" --agent copilot --model model-x + +# A failing agent is reported after verification still ran. +repo="$(setup_repo agent-fails)" +if MOCK_AGENT_EXIT=7 run_apply "$repo" y "$triage"; then + fail "a failed implementation agent returned success" +fi +grep -Fq "== Verification summary ==" "$repo.out" || fail "verification did not run after a failed agent" + +# An agent that changes or removes the protected artifacts is rejected. +repo="$(setup_repo edits-triage)" +if MOCK_AGENT_ACTION="chmod 600 '$repo/$triage'" run_apply "$repo" y "$triage" --agent claude --model model-c; then + fail "a changed triage artifact returned success" +fi +grep -Fq -- "--permission-mode acceptEdits --model model-c" "$repo.log" || + fail "overridden Claude agent was not started write-capable with its model" + +repo="$(setup_repo deletes-review)" +if MOCK_AGENT_ACTION="rm '$repo/$review'" run_apply "$repo" y "$triage"; then + fail "a deleted review artifact returned success" +fi + +# A failing verification fails the run. +repo="$(setup_repo verification-fails)" +printf 'broken: false\n' >"$repo/scripts/verify.conf" +git -C "$repo" commit -qam "Break verification" +if run_apply "$repo" y "$triage"; then + fail "a failed verification returned success" fi -grep -Fq "'codex' command not found" "$tmp/agent.out" echo "apply-triage tests passed" diff --git a/tests/lib-fakes.sh b/tests/lib-fakes.sh new file mode 100644 index 0000000..538df95 --- /dev/null +++ b/tests/lib-fakes.sh @@ -0,0 +1,90 @@ +#!/usr/bin/env bash + +# Shared fakes for the workflow tests. Source this file. +# +# make_fake_agents +# Creates fake 'codex' and 'claude' executables that log their arguments and +# standard input and return MOCK_OUTPUT. With MOCK_OUTPUT_FIRST set, the first +# call of a test returns that value instead. MOCK_AGENT_ACTION is evaluated in +# the agent's working directory; MOCK_AGENT_EXIT sets the exit status. +make_fake_agents() { + local bin="$1" + local jq_path + + mkdir -p "$bin" + + # Tests restrict PATH to this directory and the system directories, so make + # jq available wherever it is installed. + jq_path="$(command -v jq)" || { + echo "jq is required to run the workflow tests." >&2 + exit 1 + } + ln -sf "$jq_path" "$bin/jq" + cat >"$bin/claude" <<'AGENT' +#!/usr/bin/env bash +set -euo pipefail + +agent="$(basename "$0")" +{ + echo "AGENT=$agent" + echo "ARGS=$*" +} >>"$MOCK_AGENT_LOG" +if [[ ! -t 0 ]]; then + cat >"$MOCK_AGENT_LOG.stdin" +fi + +output="${MOCK_OUTPUT:-}" +if [[ -n "${MOCK_OUTPUT_FIRST:-}" && ! -e "$MOCK_AGENT_LOG.called" ]]; then + output="$MOCK_OUTPUT_FIRST" +fi +: >"$MOCK_AGENT_LOG.called" + +output_file="" +structured=0 +args=("$@") +for ((i = 0; i < ${#args[@]}; i++)); do + case "${args[$i]}" in + --output-last-message) output_file="${args[$((i + 1))]}" ;; + --json-schema | --output-schema) structured=1 ;; + esac +done + +if [[ -n "${MOCK_AGENT_ACTION:-}" ]]; then + eval "$MOCK_AGENT_ACTION" +fi + +if [[ "$agent" == "codex" ]]; then + if [[ -n "$output_file" ]]; then + printf '%s\n' "$output" >"$output_file" + fi +elif [[ "$structured" -eq 1 ]]; then + # Claude wraps structured output in a result envelope. + if printf '%s' "$output" | jq -e . >/dev/null 2>&1; then + printf '{"is_error": false, "structured_output": %s}\n' "$output" + else + jq -n --arg result "$output" '{is_error: false, result: $result}' + fi +else + printf '%s\n' "$output" +fi + +exit "${MOCK_AGENT_EXIT:-0}" +AGENT + cp "$bin/claude" "$bin/codex" + chmod +x "$bin/claude" "$bin/codex" +} + +# copy_workflow +# Copies the workflow scripts, libraries, prompts, and schemas into a test +# repository directory. +copy_workflow() { + local repo="$1" + local source_root + + source_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd -P)" + mkdir -p "$repo/scripts/lib" "$repo/.agents/prompts" "$repo/.agents/schemas" + cp "$source_root"/scripts/*.sh "$repo/scripts/" + cp "$source_root"/scripts/lib/*.sh "$repo/scripts/lib/" + cp "$source_root"/.agents/prompts/*.md "$repo/.agents/prompts/" + cp "$source_root"/.agents/schemas/*.json "$repo/.agents/schemas/" +} diff --git a/tests/review-feature-test.sh b/tests/review-feature-test.sh index 1eb0e40..c2b7bdb 100755 --- a/tests/review-feature-test.sh +++ b/tests/review-feature-test.sh @@ -31,99 +31,25 @@ echo "Unexpected gh invocation: $*" >&2 exit 1 GH -# The fake reviewer records its arguments and standard input, then behaves -# according to MOCK_REVIEW_MODE. -cat >"$tmp/bin/claude" <<'AGENT' -#!/usr/bin/env bash -set -euo pipefail - -agent="$(basename "$0")" -{ - echo "AGENT=$agent" - echo "ARGS=$*" -} >>"$MOCK_AGENT_LOG" -cat >"$MOCK_AGENT_LOG.stdin" - -output_file="" -while [[ $# -gt 0 ]]; do - if [[ "$1" == "--output-last-message" ]]; then - output_file="$2" - fi - shift -done +source "$source_root/tests/lib-fakes.sh" +make_fake_agents "$tmp/bin" +chmod +x "$tmp/bin/gh" -report() { - if [[ -n "$output_file" ]]; then - cat >"$output_file" - else - cat - fi +finding() { + printf '{"severity": "%s", "title": "%s", "evidence": "marker.txt:1", "impact": "The marker is never read.", "recommendation": "Read the marker."}' "$1" "$2" } - -case "${MOCK_REVIEW_MODE:-success}" in - fail) - exit 43 - ;; - large) - { - printf '## Critical\n\nNone.\n\n## Major\n\nNone.\n\n## Minor\n\nNone.\n\n## Suggestions\n\n' - for ((line = 0; line < 6000; line++)); do - printf -- '- Suggestion detail line %s that pads the report beyond a pipe buffer.\n' "$line" - done - printf '\n## Verdict\n\nPASS WITH MINOR FINDINGS\n' - } | report - exit 0 - ;; - no-verdict) - printf '## Critical\n\nNone.\n\n## Major\n\nNone.\n' | report - exit 0 - ;; - modify) - printf 'changed by the reviewer\n' >>feature.txt - ;; - create) - printf 'created by the reviewer\n' >reviewer-note.txt - ;; -esac - -report <<'REPORT' -Preface that is not part of the review. - -## Critical - -None. - -## Major - -### M1. Marker content is unchecked - -The marker file is created but never read. - -## Minor - -None. - -## Suggestions - -None. - -## Verdict - -**CHANGES REQUIRED** -REPORT -AGENT -cp "$tmp/bin/claude" "$tmp/bin/codex" -chmod +x "$tmp/bin/gh" "$tmp/bin/claude" "$tmp/bin/codex" +valid_result="{\"verdict\": \"CHANGES_REQUIRED\", \"limitations\": \"Tests were not run.\", \"findings\": [$(finding minor "Marker name is vague"), $(finding major "Marker content is unchecked")]}" +# A verdict that does not follow from the findings. +inconsistent_result="{\"verdict\": \"CHANGES_REQUIRED\", \"limitations\": \"\", \"findings\": [$(finding minor "Marker name is vague")]}" +export MOCK_OUTPUT="$valid_result" # Creates a repository on feature/7-marker with one committed, one modified, # and one untracked file relative to main. setup_repo() { local repo="$tmp/$1" - mkdir -p "$repo/scripts/lib" "$repo/.agents/prompts" - cp "$source_root/scripts/review-feature.sh" "$repo/scripts/review-feature.sh" - cp "$source_root/scripts/lib/agent.sh" "$repo/scripts/lib/agent.sh" - cp "$source_root/.agents/prompts/reviewer.md" "$repo/.agents/prompts/reviewer.md" + mkdir -p "$repo" + copy_workflow "$repo" printf 'reviewer: claude model-r\n' >"$repo/.agents/agents.conf" printf 'base content\n' >"$repo/feature.txt" mkdir -p "$repo/docs" @@ -165,34 +91,33 @@ expect_no_review() { cat "$repo.out" >&2 fail "expected failure: $description" fi - if compgen -G "$repo/.agents/reviews/*.md" >/dev/null; then + if compgen -G "$repo/.agents/reviews/*" >/dev/null; then fail "a review was stored although: $description" fi } -# The script stores the returned report; the reviewer runs read-only and -# receives the Issue and the complete diff on standard input. +# The script stores the validated result as JSON with script-assigned finding +# identifiers and a generated report; the reviewer runs read-only and receives +# the Issue and the complete diff on standard input. repo="$(setup_repo claude)" run_review "$repo" 7 || { cat "$repo.out" >&2 fail "review with the configured agent failed" } -artifact="$repo/.agents/reviews/feature-7-marker-review-01.md" -[[ -f "$artifact" ]] || fail "the script did not store the review" -grep -Fqx "Issue: #7" "$artifact" || fail "stored review lacks the Issue reference" -grep -Fqx "### M1. Marker content is unchecked" "$artifact" || fail "stored review lacks the returned finding" -if grep -Fq "Preface that is not part of the review" "$artifact"; then - fail "text before the first section was stored" -fi -grep -Fq -- "--permission-mode plan --tools Read,Glob,Grep" "$repo.log" || +artifact="$repo/.agents/reviews/feature-7-marker-review-01" +[[ -f "$artifact.json" && -f "$artifact.md" ]] || fail "the script did not store the review and its report" +[[ "$(jq -c '[.issue, .round, .verdict, (.findings | map(.id))]' "$artifact.json")" == '[7,1,"CHANGES_REQUIRED",["MIN1","M1"]]' ]] || + fail "stored review lacks the Issue, round, verdict, or finding identifiers" +[[ "$(jq -r '.reviewed_tree | length' "$artifact.json")" -eq 40 ]] || fail "stored review lacks the reviewed tree" +grep -Fq "M1. Marker content is unchecked" "$artifact.md" || fail "generated report lacks a finding" +grep -Fq -- "--strict-mcp-config --permission-mode dontAsk --tools Read,Glob,Grep" "$repo.log" || fail "Claude reviewer was not restricted to read tools" +grep -Fq -- "--json-schema" "$repo.log" || fail "Claude reviewer was not given the schema" grep -Fq -- "--model model-r" "$repo.log" || fail "configured model was not passed to the reviewer" grep -Fq "The marker file must exist." "$repo.log.stdin" || fail "Issue was not supplied to the reviewer" for change in "+committed change" "+uncommitted change" "+untracked marker"; do grep -Fqx -- "$change" "$repo.log.stdin" || fail "diff supplied to the reviewer lacks: $change" done -[[ -z "$(git -C "$repo" status --porcelain -- feature.txt marker.txt | grep -v '^ M feature.txt$' | grep -v '^?? marker.txt$')" ]] || - fail "the review changed the state of the reviewed files" # A second round is numbered and points the reviewer at the previous review, # which is not part of the reviewed diff. @@ -200,27 +125,74 @@ run_review "$repo" 7 --agent codex --model model-c || { cat "$repo.out" >&2 fail "re-review with an overridden agent failed" } -[[ -f "$repo/.agents/reviews/feature-7-marker-review-02.md" ]] || fail "re-review was not numbered" -grep -Fq "feature-7-marker-review-01.md" "$repo.log" || fail "re-review did not reference the previous review" -grep -Fq -- "exec --sandbox read-only" "$repo.log" || fail "Codex reviewer was not sandboxed read-only" +[[ "$(jq '.round' "$repo/.agents/reviews/feature-7-marker-review-02.json")" -eq 2 ]] || fail "re-review was not numbered" +grep -Fq "feature-7-marker-review-01.json" "$repo.log" || fail "re-review did not reference the previous review" +grep -Fq -- "--sandbox read-only" "$repo.log" || fail "Codex reviewer was not sandboxed read-only" +grep -Fq -- "--output-schema" "$repo.log" || fail "Codex reviewer was not given the schema" if grep -Fq "Marker content is unchecked" "$repo.log.stdin"; then fail "previous review artifact was included in the reviewed diff" fi +# A review without findings is valid. +repo="$(setup_repo no-findings)" +MOCK_OUTPUT='{"verdict": "PASS", "limitations": "", "findings": []}' run_review "$repo" 7 || { + cat "$repo.out" >&2 + fail "a review without findings was rejected" +} +[[ "$(jq -c '[.verdict, (.findings | length)]' "$repo/.agents/reviews/feature-7-marker-review-01.json")" == '["PASS",0]' ]] || + fail "a review without findings was not stored as PASS" + # A reviewer that changes or creates files is detected. repo="$(setup_repo modify)" -MOCK_REVIEW_MODE=modify expect_no_review "$repo" "the reviewer modified a file" 7 +MOCK_AGENT_ACTION="printf 'changed by the reviewer\n' >>'$repo/feature.txt'" \ + expect_no_review "$repo" "the reviewer modified a file" 7 grep -Fq "modified the working tree" "$repo.out" || fail "modification was not reported" repo="$(setup_repo create)" -MOCK_REVIEW_MODE=create expect_no_review "$repo" "the reviewer created a file" 7 +MOCK_AGENT_ACTION="printf 'created by the reviewer\n' >'$repo/reviewer-note.txt'" \ + expect_no_review "$repo" "the reviewer created a file" 7 + +# A failed reviewer stores nothing. +repo="$(setup_repo agent-fails)" +MOCK_AGENT_EXIT=43 expect_no_review "$repo" "the reviewer failed" 7 -# A large valid report is stored. -repo="$(setup_repo large)" -MOCK_REVIEW_MODE=large run_review "$repo" 7 || { +# An invalid result is retried once: it is accepted when the retry is valid +# and rejected when it is not. +repo="$(setup_repo retry)" +MOCK_OUTPUT_FIRST="$inconsistent_result" run_review "$repo" 7 || { cat "$repo.out" >&2 - fail "a large valid report was rejected" + fail "a valid result after one invalid result was rejected" } +[[ "$(grep -c '^AGENT=' "$repo.log")" -eq 2 ]] || fail "an invalid result was not retried exactly once" +[[ "$(grep -c 'previous result was rejected' "$repo.log")" -eq 1 ]] || + fail "the retry did not tell the reviewer why its result was rejected" +grep -Fq "requires a critical or major finding" "$repo.log" || fail "the retry prompt lacks the rejection reason" + +# The reviewer may not supply script-owned fields or more than one result. +owned_fields_result="$(jq '. + {issue: 999} | .findings[0].id = "agent-id"' <<<"$valid_result")" +pass_result='{"verdict": "PASS", "limitations": "", "findings": []}' +minor_only_result="{\"verdict\": \"PASS_WITH_MINOR_FINDINGS\", \"limitations\": \"\", \"findings\": [$(finding minor "Marker name is vague")]}" + +# One invalid result per rule, each derived from a valid result. +invalid_results=( + "not json" + "$pass_result $pass_result" + '{"verdict": "PASS"}' + "$owned_fields_result" + "$inconsistent_result" + "$(jq '.verdict = "APPROVED"' <<<"$valid_result")" + "$(jq '.findings[0].severity = "blocker"' <<<"$valid_result")" + "$(jq '.findings[1].evidence = " "' <<<"$valid_result")" + "$(jq '.verdict = "PASS"' <<<"$minor_only_result")" + "$(jq '.verdict = "PASS_WITH_MINOR_FINDINGS"' <<<"$valid_result")" + "$(jq '.verdict = "PASS_WITH_MINOR_FINDINGS"' <<<"$pass_result")" +) +for index in "${!invalid_results[@]}"; do + invalid_result="${invalid_results[$index]}" + repo="$(setup_repo "invalid-$index")" + MOCK_OUTPUT="$invalid_result" expect_no_review "$repo" "the result is invalid: $invalid_result" 7 + [[ "$(grep -c '^AGENT=' "$repo.log")" -eq 2 ]] || fail "an invalid result was not retried exactly once" +done # From a subdirectory, the snapshot still contains tracked files that match an # ignore rule, so they are not reviewed as deletions. @@ -236,13 +208,6 @@ if grep -Fq "tracked.log" "$repo.log.stdin"; then fail "an unchanged tracked file matching an ignore rule appeared in the reviewed diff" fi -# A failed reviewer or an unusable report stores nothing. -repo="$(setup_repo agent-fails)" -MOCK_REVIEW_MODE=fail expect_no_review "$repo" "the reviewer failed" 7 - -repo="$(setup_repo no-verdict)" -MOCK_REVIEW_MODE=no-verdict expect_no_review "$repo" "the report has no verdict" 7 - # Invalid invocations fail before a reviewer starts. repo="$(setup_repo preconditions)" expect_no_review "$repo" "the branch belongs to another Issue" 8 diff --git a/tests/start-planning-test.sh b/tests/start-planning-test.sh index 921556b..b4cf92e 100755 --- a/tests/start-planning-test.sh +++ b/tests/start-planning-test.sh @@ -2,12 +2,6 @@ set -euo pipefail -# Keep the verification runs nested inside the apply-triage suite fast. -if [[ "${APPLY_TRIAGE_TEST_ACTIVE:-0}" == "1" ]]; then - echo "start-planning tests skipped inside a nested verification run" - exit 0 -fi - root="$(git rev-parse --show-toplevel)" script_source="$root/scripts/start-planning.sh" tmp="$(mktemp -d "${TMPDIR:-/tmp}/start-planning-test.XXXXXX")" diff --git a/tests/triage-review-test.sh b/tests/triage-review-test.sh index 6915ae4..e55503d 100755 --- a/tests/triage-review-test.sh +++ b/tests/triage-review-test.sh @@ -2,263 +2,284 @@ set -euo pipefail -root="$(git rev-parse --show-toplevel)" -script="$root/scripts/triage-review.sh" -review_dir="$root/.agents/reviews" -triage_dir="$root/.agents/triage" -test_stem="feature-99999-triage-test-$$-$RANDOM" -review="$review_dir/${test_stem}-review-01.md" -decline_review="$review_dir/${test_stem}-review-02.md" -fallback_review="$review_dir/${test_stem}-review-07.md" -artifact="$triage_dir/${test_stem}-review-01-triage.md" -rerun_artifact="$triage_dir/${test_stem}-review-01-triage-02.md" -decline_artifact="$triage_dir/${test_stem}-review-02-triage.md" -fallback_artifact="$triage_dir/${test_stem}-review-07-triage.md" +source_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd -P)" tmp="$(mktemp -d "${TMPDIR:-/tmp}/triage-review-test.XXXXXX")" cleanup() { rm -rf "$tmp" - rm -f "$review" "$decline_review" "$fallback_review" - rm -f "$artifact" "$rerun_artifact" "$decline_artifact" "$fallback_artifact" } trap cleanup EXIT -mkdir -p "$review_dir" "$triage_dir" "$tmp/bin" -for test_path in "$review" "$decline_review" "$fallback_review" "$artifact" "$rerun_artifact" "$decline_artifact" "$fallback_artifact"; do - if [[ -e "$test_path" ]]; then - echo "Refusing to overwrite pre-existing test path: $test_path" >&2 - exit 1 - fi -done - -cat >"$review" <<'REVIEW' -# Independent Review — feature/5-test - -Issue: #5 - -## Critical - -### C1. Correctness regression - -The primary path returns an incorrect result. - -## Major - -None. - -## Minor - -- Minor 1. Missing edge-case coverage -- Minor 2. Simplify fixture setup - -## Suggestions - -### S1. Rename local helper - -The current name is less explicit. - -## Verdict - -CHANGES REQUIRED -REVIEW - -cp "$review" "$decline_review" -cp "$review" "$fallback_review" +fail() { + echo "triage-review test failed: $*" >&2 + exit 1 +} -cat >"$tmp/bin/claude" <<'AGENT' -#!/usr/bin/env bash -set -euo pipefail -printf '%s\n' "$*" >>"$MOCK_AGENT_LOG" -printf '%s\n' "$MOCK_TRIAGE_OUTPUT" -AGENT +source "$source_root/tests/lib-fakes.sh" +make_fake_agents "$tmp/bin" +# The fake GitHub CLI logs created Issues and finds them again by trace token. cat >"$tmp/bin/gh" <<'GH' #!/usr/bin/env bash set -euo pipefail -if [[ "${1:-} ${2:-}" == "auth status" ]]; then - exit 0 -fi - -if [[ "${1:-} ${2:-}" == "issue view" ]]; then - printf '%s' "${MOCK_ISSUE_CONTEXT:-Title: F02 — Test feature +case "${1:-} ${2:-}" in + "auth status") + exit 0 + ;; + "issue view") + printf '%s' "${MOCK_ISSUE_CONTEXT:-Title: F02 — Test feature Test acceptance criteria.}" - exit 0 -fi - -if [[ "${1:-} ${2:-}" == "issue list" ]]; then - exit 0 -fi - -if [[ "${1:-} ${2:-}" == "issue create" ]]; then - shift 2 - title="" - body_file="" - while [[ $# -gt 0 ]]; do - case "$1" in - --title) - title="$2" - shift 2 - ;; - --body-file) - body_file="$2" - shift 2 - ;; - *) - shift - ;; - esac - done - - { - echo "TITLE: $title" - cat "$body_file" - } >>"$MOCK_GH_LOG" - echo "https://github.com/example/project/issues/123" - exit 0 -fi - + exit 0 + ;; + "issue list") + if [[ -n "${MOCK_EXISTING_ISSUE:-}" ]]; then + echo "$MOCK_EXISTING_ISSUE" + fi + exit 0 + ;; + "issue create") + if [[ -n "${MOCK_GH_CREATE_EXIT:-}" ]]; then + echo "simulated Issue creation failure" >&2 + exit "$MOCK_GH_CREATE_EXIT" + fi + shift 2 + title="" + body_file="" + while [[ $# -gt 0 ]]; do + case "$1" in + --title) title="$2"; shift 2 ;; + --body-file) body_file="$2"; shift 2 ;; + *) shift ;; + esac + done + { + echo "TITLE: $title" + cat "$body_file" + } >>"$MOCK_GH_LOG" + echo "https://github.com/example/project/issues/123" + exit 0 + ;; +esac echo "Unexpected gh invocation: $*" >&2 exit 1 GH +chmod +x "$tmp/bin/gh" -cat >"$tmp/bin/codex" <<'CODEX' -#!/usr/bin/env bash -set -euo pipefail -printf '%s\n' "$*" >>"$MOCK_AGENT_LOG" +# Creates a repository with a stored review of Issue #5 in the given round. +setup_repo() { + local repo="$tmp/$1" + local round="${2:-1}" + local findings="${3:-default}" -output_file="" -while [[ $# -gt 0 ]]; do - if [[ "$1" == "--output-last-message" ]]; then - output_file="$2" - shift 2 + mkdir -p "$repo/.agents/reviews" + copy_workflow "$repo" + printf 'triage: claude model-t\n' >"$repo/.agents/agents.conf" + + if [[ "$findings" == "none" ]]; then + findings='[]' + verdict="PASS" else - shift + verdict="CHANGES_REQUIRED" + findings='[ + {"id": "C1", "severity": "critical", "title": "Correctness regression", "evidence": "src/a.sh:3", "impact": "Wrong result.", "recommendation": "Fix the branch."}, + {"id": "MIN1", "severity": "minor", "title": "Missing edge-case coverage", "evidence": "tests/a.sh:9\nSecond line.", "impact": "A regression would go unnoticed.", "recommendation": "Add a test."}, + {"id": "S1", "severity": "suggestion", "title": "Rename local helper", "evidence": "src/a.sh:8", "impact": "Readability.", "recommendation": "Rename it."} + ]' + fi + jq -n --argjson round "$round" --argjson findings "$findings" --arg verdict "$verdict" '{ + schema: "review/v1", issue: 5, round: $round, branch: "feature/5-test", base: "main", + merge_base: "aaaa", head: "bbbb", reviewed_tree: "cccc", + reviewer: {agent: "codex", model: "model-r"}, created_at: "2026-01-01T00:00:00Z", + verdict: $verdict, limitations: "", findings: $findings + }' >"$repo/.agents/reviews/feature-5-test-review-$(printf '%02d' "$round").json" + + git -C "$repo" init -q -b main + git -C "$repo" config user.name "Triage Test" + git -C "$repo" config user.email "triage-test@example.com" + git -C "$repo" add . + git -C "$repo" commit -qm "Seed project" + + printf '%s\n' "$repo" +} + +run_triage() { + local repo="$1" + local answer="$2" + shift 2 + + ( + cd "$repo" + printf '%s\n' "$answer" | + PATH="$tmp/bin:/usr/bin:/bin" MOCK_AGENT_LOG="$repo.log" MOCK_GH_LOG="$repo.gh" \ + ./scripts/triage-review.sh "$@" + ) >"$repo.out" 2>&1 +} + +expect_no_triage() { + local repo="$1" + local description="$2" + shift 2 + + if run_triage "$repo" y "$@"; then + cat "$repo.out" >&2 + fail "expected failure: $description" fi + if compgen -G "$repo/.agents/triage/*" >/dev/null; then + fail "a triage artifact was stored although: $description" + fi + [[ ! -e "$repo.gh" ]] || fail "a follow-up Issue was created although: $description" +} + +review=".agents/reviews/feature-5-test-review-01.json" +valid_decisions='{"decisions": [ + {"finding_id": "C1", "decision": "FIX_NOW", "rationale": "Correctness blocks the feature.", "followup": null}, + {"finding_id": "MIN1", "decision": "DEFER", "rationale": "Valuable but not blocking.", "followup": {"title": "Add boundary-condition coverage", "recommended_action": "Add focused tests.", "acceptance_criteria": ["The boundary is covered by a test."]}}, + {"finding_id": "S1", "decision": "ACCEPT", "rationale": "The name is adequate.", "followup": null} +]}' +# A Critical finding may not be deferred. +downgraded_decisions="$(jq '.decisions[0] = {"finding_id": "C1", "decision": "DEFER", "rationale": "Later.", "followup": {"title": "Fix it later", "recommended_action": "Fix.", "acceptance_criteria": ["Fixed."]}}' <<<"$valid_decisions")" +# A finding of the review is missing. +incomplete_decisions="$(jq 'del(.decisions[2])' <<<"$valid_decisions")" +export MOCK_OUTPUT="$valid_decisions" + +# Approval stores validated decisions and creates exactly the deferred +# follow-up Issue with a provenance-prefixed title. +repo="$(setup_repo approve)" +run_triage "$repo" y "$review" || { + cat "$repo.out" >&2 + fail "approved triage failed" +} +artifact="$repo/.agents/triage/feature-5-test-review-01-triage" +[[ -f "$artifact.json" && -f "$artifact.md" ]] || fail "triage artifact and report were not stored" +[[ "$(jq -c '[.schema, .issue, .source_review, (.decisions | map(.decision))]' "$artifact.json")" == \ + "[\"triage/v1\",5,\"$review\",[\"FIX_NOW\",\"DEFER\",\"ACCEPT\"]]" ]] || fail "stored triage is incomplete" +[[ "$(jq -r '.decisions[1].followup | "\(.title) #\(.issue_number)"' "$artifact.json")" == \ + "[F02][R01][MIN1] Add boundary-condition coverage #123" ]] || fail "follow-up Issue was not recorded with its provenance" +[[ "$(grep -c '^TITLE:' "$repo.gh")" -eq 1 ]] || fail "exactly one follow-up Issue was expected" +grep -Fqx "TITLE: [F02][R01][MIN1] Add boundary-condition coverage" "$repo.gh" || fail "follow-up title lacks provenance" +grep -Fq "triage-source:$review#MIN1" "$repo.gh" || fail "follow-up Issue lacks its trace token" +grep -Fqx "Second line." "$repo.gh" || fail "follow-up Issue lacks the multi-line evidence" +grep -Fq -- "--strict-mcp-config --permission-mode dontAsk --tools Read,Glob,Grep" "$repo.log" || fail "triage agent was not read-only" +grep -Fq -- "--json-schema" "$repo.log" || fail "triage agent was not given the schema" +grep -Fq "Correctness regression" "$repo.log.stdin" || fail "findings were not supplied to the triage agent" + +# The complete follow-up proposal is shown before the approval question. +proposal="$(sed '/Proceed with this triage/,$d' "$repo.out")" +for shown in "Add focused tests." "The boundary is covered by a test."; do + [[ "$proposal" == *"$shown"* ]] || fail "the proposal did not show before approval: $shown" done -[[ -n "$output_file" ]] -printf '%s\n' "$MOCK_TRIAGE_OUTPUT" >"$output_file" -CODEX - -chmod +x "$tmp/bin/claude" "$tmp/bin/codex" "$tmp/bin/gh" - -review_hash_before="$(git hash-object "$review")" -decisions=$'F001\tFIX_NOW\tCorrectness blocks the feature.\t-\t-\t-\nF002\tDEFER\tCoverage is valuable but non-blocking.\tAdd boundary-condition coverage\tAdd focused tests for the uncovered boundary.\tBoundary behavior is covered by an automated test.\nF003\tACCEPT\tThe fixture setup is adequate for this scope.\t-\t-\t-\nF004\tACCEPT\tThe existing local name is adequate.\t-\t-\t-' - -output="$( - printf 'y\n' | - PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_TRIAGE_OUTPUT="$decisions" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - MOCK_GH_LOG="$tmp/gh.log" \ - "$script" "$review" --agent claude --model sonnet -)" - -[[ -f "$artifact" ]] -[[ "$(git hash-object "$review")" == "$review_hash_before" ]] -[[ "$output" == *"Proposed triage:"* ]] -[[ "$output" == *"C1 Correctness regression"* ]] -[[ "$output" == *"Minor 1 Missing edge-case coverage"* ]] -[[ "$output" == *"S1 Rename local helper"* ]] -[[ "$output" == *"Minor 1 → #123"* ]] -[[ "$output" == *"[F02][R01][Minor-1] Add boundary-condition coverage"* ]] - -grep -Fq "Source review: \`.agents/reviews/$(basename "$review")\`" "$artifact" -grep -Fq "Source feature Issue: #5" "$artifact" -grep -Fq "Reviewer verdict: CHANGES REQUIRED" "$artifact" -grep -Fq "### C1. Correctness regression" "$artifact" -grep -Fq "### Minor 1. Missing edge-case coverage" "$artifact" -grep -Fq "### Minor 2. Simplify fixture setup" "$artifact" -grep -Fq "### S1. Rename local helper" "$artifact" -grep -Fq "Minor 1 → [#123]" "$artifact" - -[[ "$(grep -c '^TITLE:' "$tmp/gh.log")" -eq 1 ]] -grep -Fq "TITLE: [F02][R01][Minor-1] Add boundary-condition coverage" "$tmp/gh.log" -grep -Fq "Original finding: Minor 1" "$tmp/gh.log" -grep -Fq "Issue #5" "$tmp/gh.log" -grep -Fq ".agents/reviews/$(basename "$review")" "$tmp/gh.log" -grep -Fq -- "- Minor 1. Missing edge-case coverage" "$tmp/gh.log" -if grep -Fq -- "- Minor 2. Simplify fixture setup" "$tmp/gh.log"; then - echo "Deferred bullet excerpt included the next finding." >&2 - exit 1 +# A second approved round reuses the recorded Issue instead of creating one. +run_triage "$repo" y "$review" || { + cat "$repo.out" >&2 + fail "repeated triage failed" +} +[[ "$(jq -r '.decisions[1].followup.issue_number' "$artifact-02.json")" == "123" ]] || + fail "repeated triage did not reuse the existing follow-up Issue" +[[ "$(grep -c '^TITLE:' "$repo.gh")" -eq 1 ]] || fail "repeated triage created a duplicate follow-up Issue" + +# Without a local record, an Issue found by its trace token is reused. +repo="$(setup_repo found-remotely)" +MOCK_EXISTING_ISSUE="https://github.com/example/project/issues/77" run_triage "$repo" y "$review" || { + cat "$repo.out" >&2 + fail "triage with an existing remote Issue failed" +} +[[ "$(jq -r '.decisions[1].followup.issue_number' "$repo/.agents/triage/feature-5-test-review-01-triage.json")" == "77" ]] || + fail "an existing remote follow-up Issue was not reused" +[[ ! -e "$repo.gh" ]] || fail "a duplicate of an existing remote follow-up Issue was created" + +# When Issue creation fails, no triage artifact is stored. +repo="$(setup_repo create-fails)" +if MOCK_GH_CREATE_EXIT=1 run_triage "$repo" y "$review"; then + fail "a failed follow-up Issue creation returned success" fi -grep -Fq "triage-source:.agents/reviews/$(basename "$review")#F002" "$tmp/gh.log" -grep -Fq -- "--print --permission-mode plan --tools Read,Glob,Grep --no-session-persistence" "$tmp/agent.log" - -fallback_output="$( - printf 'y\n' | - PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_ISSUE_CONTEXT=$'Title: Test feature without roadmap ID\n\nTest acceptance criteria.' \ - MOCK_TRIAGE_OUTPUT="$decisions" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - MOCK_GH_LOG="$tmp/fallback-gh.log" \ - "$script" "$fallback_review" --agent claude --model sonnet -)" -[[ "$fallback_output" == *"[#5][R07][Minor-1] Add boundary-condition coverage"* ]] -grep -Fq "TITLE: [#5][R07][Minor-1] Add boundary-condition coverage" "$tmp/fallback-gh.log" -grep -Fq "Minor 1 → [#123]" "$fallback_artifact" - -decline_output="$( - printf 'n\n' | - PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_TRIAGE_OUTPUT="$decisions" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - MOCK_GH_LOG="$tmp/gh.log" \ - "$script" "$decline_review" --agent claude --model sonnet -)" -[[ "$decline_output" == *"Triage declined"* ]] -[[ ! -e "$decline_artifact" ]] -[[ "$(grep -c '^TITLE:' "$tmp/gh.log")" -eq 1 ]] - -codex_output="$( - printf 'n\n' | - PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_TRIAGE_OUTPUT="$decisions" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - MOCK_GH_LOG="$tmp/gh.log" \ - "$script" "$decline_review" --agent codex --model test-model -)" -[[ "$codex_output" == *"Triage declined"* ]] -grep -Fq -- "exec --sandbox read-only --ephemeral --color never" "$tmp/agent.log" -grep -Fq -- "--model test-model" "$tmp/agent.log" - -rerun_output="$( - printf 'y\n' | - PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_TRIAGE_OUTPUT="$decisions" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - MOCK_GH_LOG="$tmp/gh.log" \ - "$script" "$review" --agent claude --model sonnet -)" -[[ "$rerun_output" == *"Reusing existing follow-up Issue #123"* ]] -[[ "$(grep -c '^TITLE:' "$tmp/gh.log")" -eq 1 ]] -grep -Fq "Minor 1 → [#123]" "$rerun_artifact" - -invalid_decisions=$'F001\tDEFER\tShould not downgrade.\tBad follow-up\tDo something.\tSomething changes.\nF002\tDEFER\tCoverage is useful.\tAdd coverage\tAdd tests.\tTests pass.\nF003\tACCEPT\tKeep fixture.\t-\t-\t-\nF004\tACCEPT\tSkip rename.\t-\t-\t-' -if PATH="$tmp/bin:/usr/bin:/bin" \ - MOCK_TRIAGE_OUTPUT="$invalid_decisions" \ - MOCK_AGENT_LOG="$tmp/agent.log" \ - MOCK_GH_LOG="$tmp/gh.log" \ - "$script" "$review" --agent claude --model sonnet "$tmp/invalid.out" 2>&1; then - echo "Expected Critical downgrade validation to fail." >&2 - exit 1 +if compgen -G "$repo/.agents/triage/*" >/dev/null; then + fail "a triage artifact was stored although a follow-up Issue could not be created" fi -grep -Fq "Critical finding F001 must be FIX_NOW" "$tmp/invalid.out" -[[ "$(grep -c '^TITLE:' "$tmp/gh.log")" -eq 1 ]] -if PATH="$tmp/bin:/usr/bin:/bin" "$script" "$review_dir/missing.md" --agent claude --model sonnet >"$tmp/missing.out" 2>&1; then - echo "Expected missing review validation to fail." >&2 - exit 1 -fi -grep -Fq "review artifact not found" "$tmp/missing.out" +# Without a feature ID in the Issue title, the Issue number is the provenance. +repo="$(setup_repo fallback 7)" +MOCK_ISSUE_CONTEXT=$'Title: Test feature without roadmap ID\n\nBody.' \ + run_triage "$repo" y ".agents/reviews/feature-5-test-review-07.json" || { + cat "$repo.out" >&2 + fail "triage without a feature ID failed" +} +grep -Fqx "TITLE: [#5][R07][MIN1] Add boundary-condition coverage" "$repo.gh" || fail "fallback provenance is wrong" -rm "$tmp/bin/codex" -if PATH="$tmp/bin:/usr/bin:/bin" "$script" "$review" --agent codex --model test-model >"$tmp/agent.out" 2>&1; then - echo "Expected missing agent validation to fail." >&2 - exit 1 +# Declining has no side effects. +repo="$(setup_repo decline)" +run_triage "$repo" n "$review" --agent codex --model model-c || fail "declined triage returned an error" +if compgen -G "$repo/.agents/triage/*" >/dev/null || [[ -e "$repo.gh" ]]; then + fail "declined triage had side effects" fi -grep -Fq "'codex' command not found" "$tmp/agent.out" +grep -Fq -- "--sandbox read-only" "$repo.log" || fail "Codex triage agent was not sandboxed read-only" +grep -Fq -- "--output-schema" "$repo.log" || fail "Codex triage agent was not given the schema" + +# Invalid decisions are retried once and then rejected before any side effect. +# The triage agent may not supply script-owned fields or more than one result. +owned_fields_decisions="$(jq '.decisions[0].severity = "minor"' <<<"$valid_decisions")" +# A required field is absent rather than null. +missing_field_decisions="$(jq 'del(.decisions[2].followup)' <<<"$valid_decisions")" + +# One invalid result per rule, each derived from the valid decisions. +invalid_decisions=( + "not json" + "$valid_decisions $valid_decisions" + "$downgraded_decisions" + "$incomplete_decisions" + "$owned_fields_decisions" + "$missing_field_decisions" + "$(jq '.decisions += [.decisions[2]]' <<<"$valid_decisions")" + "$(jq '.decisions[2].finding_id = "S9"' <<<"$valid_decisions")" + "$(jq '.decisions[2].decision = "LATER"' <<<"$valid_decisions")" + "$(jq '.decisions[2].rationale = ""' <<<"$valid_decisions")" + "$(jq '.decisions[1].followup = null' <<<"$valid_decisions")" + "$(jq '.decisions[1].followup.title = ""' <<<"$valid_decisions")" + "$(jq '.decisions[1].followup.recommended_action = ""' <<<"$valid_decisions")" + "$(jq '.decisions[1].followup.acceptance_criteria = []' <<<"$valid_decisions")" + "$(jq '.decisions[2].followup = .decisions[1].followup' <<<"$valid_decisions")" +) +for index in "${!invalid_decisions[@]}"; do + invalid="${invalid_decisions[$index]}" + repo="$(setup_repo "invalid-$index")" + MOCK_OUTPUT="$invalid" expect_no_triage "$repo" "the decisions are invalid" "$review" + [[ "$(grep -c '^AGENT=' "$repo.log")" -eq 2 ]] || fail "invalid decisions were not retried exactly once" +done + +repo="$(setup_repo retry)" +MOCK_OUTPUT_FIRST="$incomplete_decisions" run_triage "$repo" y "$review" || { + cat "$repo.out" >&2 + fail "valid decisions after one invalid result were rejected" +} +grep -Fq "finding S1 was not classified" "$repo.log" || fail "the retry prompt lacks the rejection reason" + +# A triage agent that fails or changes the working tree is rejected. +repo="$(setup_repo agent-fails)" +MOCK_AGENT_EXIT=9 expect_no_triage "$repo" "the triage agent failed" "$review" + +repo="$(setup_repo modifies)" +MOCK_AGENT_ACTION="printf 'x\n' >'$repo/note.txt'" expect_no_triage "$repo" "the triage agent changed the working tree" "$review" + +# A review without findings needs no agent and still records an approved triage. +repo="$(setup_repo none 1 none)" +run_triage "$repo" y "$review" || { + cat "$repo.out" >&2 + fail "triage of a review without findings failed" +} +[[ "$(jq '.decisions | length' "$repo/.agents/triage/feature-5-test-review-01-triage.json")" -eq 0 ]] || + fail "triage of a review without findings is not empty" +[[ ! -e "$repo.log" ]] || fail "a triage agent was started for a review without findings" + +# Invalid inputs fail before an agent starts. +repo="$(setup_repo preconditions)" +printf '# report\n' >"$repo/.agents/reviews/feature-5-test-review-01.md" +expect_no_triage "$repo" "the input is the generated report" ".agents/reviews/feature-5-test-review-01.md" +expect_no_triage "$repo" "the review does not exist" ".agents/reviews/missing.json" +printf '{"schema": "other"}\n' >"$repo/.agents/reviews/other.json" +expect_no_triage "$repo" "the file is not a review artifact" ".agents/reviews/other.json" +expect_no_triage "$repo" "the agent is unsupported" "$review" --agent copilot --model model-x +[[ ! -e "$repo.log" ]] || fail "a triage agent was started despite failed preconditions" echo "triage-review tests passed"