diff --git a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh index 7301542a2..3cce03f83 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh @@ -85,7 +85,8 @@ run_test "reject-label" \ "reject" "false" "rejected" # Defensive: comment + downgraded=true can't occur in production (DOWNGRADED is -# only set inside the approve branch), but verify the label logic handles it. +# only set inside the approve branch, by either the protected-path or +# governance-document downgrade path), but verify the label logic handles it. run_test "comment-with-downgrade-flag" \ "comment" "true" "requires-manual-review" @@ -99,6 +100,96 @@ run_test "failure-action-no-label" \ run_test "unknown-action-no-label" \ "banana" "false" "none" +# --------------------------------------------------------------------------- +# Governance document detection — reimplements the loop from post-review.sh +# to verify all-governance vs mixed-file classification. +# --------------------------------------------------------------------------- +GOVERNANCE_DOC_PATTERNS=( + "MAINTAINERS.md" + "GOVERNANCE.md" + "CODE_OF_CONDUCT.md" + "SECURITY.md" +) + +is_all_governance() { + local pr_files="$1" + local all_gov=true + while IFS= read -r file; do + [ -z "${file}" ] && continue + local base + base=$(basename "${file}") + local is_gov=false + for pattern in "${GOVERNANCE_DOC_PATTERNS[@]}"; do + if [ "${base}" = "${pattern}" ]; then + is_gov=true + break + fi + done + if [ "${is_gov}" = "false" ]; then + all_gov=false + break + fi + done <<< "${pr_files}" + echo "${all_gov}" +} + +run_gov_test() { + local test_name="$1" + local pr_files="$2" + local expected="$3" + + local actual + actual="$(is_all_governance "${pr_files}")" + + if [ "${actual}" != "${expected}" ]; then + echo "FAIL: ${test_name}" + echo " files: '${pr_files}'" + echo " expected: '${expected}'" + echo " actual: '${actual}'" + FAILURES=$((FAILURES + 1)) + return + fi + + echo "PASS: ${test_name}" +} + +# --- Governance detection test cases --- + +# All governance files → downgrade +run_gov_test "all-governance-single" \ + "GOVERNANCE.md" "true" + +run_gov_test "all-governance-multiple" \ + "GOVERNANCE.md +MAINTAINERS.md +CODE_OF_CONDUCT.md" "true" + +# Governance files in subdirectories → still governance +run_gov_test "governance-in-subdir" \ + "docs/GOVERNANCE.md +community/CODE_OF_CONDUCT.md" "true" + +# Mixed: governance + non-governance → no downgrade +run_gov_test "mixed-files-no-downgrade" \ + "GOVERNANCE.md +src/main.go" "false" + +# Non-governance only → no downgrade +run_gov_test "non-governance-only" \ + "src/main.go +README.md" "false" + +# Substring mismatch: MY_GOVERNANCE.md is not a governance doc +run_gov_test "substring-no-match" \ + "MY_GOVERNANCE.md" "false" + +# All four governance patterns +run_gov_test "all-four-patterns" \ + "MAINTAINERS.md +GOVERNANCE.md +CODE_OF_CONDUCT.md +SECURITY.md" "true" + # --- Summary --- echo "" diff --git a/internal/scaffold/fullsend-repo/scripts/post-review.sh b/internal/scaffold/fullsend-repo/scripts/post-review.sh index 955c64de1..0fa208394 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-review.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review.sh @@ -4,9 +4,10 @@ # Runs on the GitHub Actions runner AFTER the sandbox is destroyed. # CWD is runDir. # -# This script is the sole enforcement point for protected-path checks: -# if the PR touches sensitive paths, an "approve" action is downgraded -# to "comment" so only a human can grant approval. +# This script is the sole enforcement point for protected-path and +# governance-document checks: if the PR touches sensitive paths or +# consists solely of governance documents, an "approve" action is +# downgraded to "comment" so only a human can grant approval. # # Required environment variables: # REVIEW_TOKEN — token with pull-requests:write on the target repo @@ -90,6 +91,11 @@ REVIEW_PROTECTED_PATHS=( "skills/" ) +# Temp files used by downgrade paths below; cleaned up on exit. +MODIFIED_RESULT="" +GOV_RESULT="" +trap 'rm -f "${MODIFIED_RESULT}" "${GOV_RESULT}"' EXIT + DOWNGRADED=false if [ "${ACTION}" = "approve" ]; then PR_FILES=$(gh pr view "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" --json files --jq '.files[].path') @@ -126,13 +132,61 @@ if [ "${ACTION}" = "approve" ]; then # Rewrite the result file with downgraded action and appended notice. MODIFIED_RESULT=$(mktemp) - trap 'rm -f "${MODIFIED_RESULT}"' EXIT jq --arg notice "${PROTECTED_NOTICE}" \ '.action = "comment" | .body = (.body + $notice)' \ "${RESULT_FILE}" > "${MODIFIED_RESULT}" RESULT_FILE="${MODIFIED_RESULT}" DOWNGRADED=true fi + + # ------------------------------------------------------------------------- + # Governance-document check: if ALL changed files are governance docs, + # downgrade "approve" to "comment" (defense-in-depth, mirrors the + # protected-path downgrade above). + # Governance document patterns (kept in sync with pr-review SKILL.md): + # ------------------------------------------------------------------------- + if [ "${DOWNGRADED}" = "false" ]; then + GOVERNANCE_DOC_PATTERNS=( + "MAINTAINERS.md" + "GOVERNANCE.md" + "CODE_OF_CONDUCT.md" + "SECURITY.md" + ) + + ALL_GOVERNANCE=true + while IFS= read -r file; do + [ -z "${file}" ] && continue + BASENAME=$(basename "${file}") + IS_GOV=false + for pattern in "${GOVERNANCE_DOC_PATTERNS[@]}"; do + if [ "${BASENAME}" = "${pattern}" ]; then + IS_GOV=true + break + fi + done + if [ "${IS_GOV}" = "false" ]; then + ALL_GOVERNANCE=false + break + fi + done <<< "${PR_FILES}" + + if [ "${ALL_GOVERNANCE}" = "true" ]; then + echo "All changed files are governance documents — downgrading approve to comment" + + GOV_NOTICE=$'\n\n---\n\n' + GOV_NOTICE+=$'> **Governance documents detected** — all changed files in this PR are\n' + GOV_NOTICE+=$'> governance or process documents. The review agent cannot approve PRs that\n' + GOV_NOTICE+=$'> consist solely of governance documents.\n' + GOV_NOTICE+=$'> A human reviewer must approve this PR.\n' + + GOV_RESULT=$(mktemp) + jq --arg notice "${GOV_NOTICE}" \ + '.action = "comment" | .body = (.body + $notice)' \ + "${RESULT_FILE}" > "${GOV_RESULT}" + RESULT_FILE="${GOV_RESULT}" + DOWNGRADED=true + fi + fi fi # --------------------------------------------------------------------------- diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md index a0ecf414b..893810991 100644 --- a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md @@ -627,6 +627,57 @@ attention. If no protected files are modified, do not add a `protected-path` finding. +##### Governance document check + +Check whether the PR modifies **only** governance or process documents. +Governance documents define organizational policy — who has authority, +how it is granted, codes of conduct — whose correctness is a human +judgment call about organizational values. The agent evaluates these +documents for formatting, link validity, and consistency, but cannot +assess whether the policy itself is sound. + +Governance document patterns (kept in sync with `post-review.sh`): + +- `MAINTAINERS.md` +- `GOVERNANCE.md` +- `CODE_OF_CONDUCT.md` +- `SECURITY.md` + +These patterns match files with the given name in any directory (e.g., +`docs/GOVERNANCE.md` matches). Matching is case-sensitive and compares +exact filenames, not substrings — `MY_GOVERNANCE.md` does not match. +The governance file list may be extended per-repo via `AGENTS.md` or +`CLAUDE.md` configuration. + +If **all** changed files match governance document patterns, add an +**info**-level finding. The description MUST list the matched +governance files: + +```json +{ + "severity": "info", + "category": "governance-document", + "file": "N/A", + "description": "All changed files are governance documents (). Governance documents define organizational policy and require human review. Agent findings are informational only.", + "actionable": false +} +``` + +When a `governance-document` finding is present, the outcome MUST be +capped at `comment` — never `approve`. The agent's sub-agent findings +(formatting, links, consistency) are still included as informational +comments, but the verdict does not signal merge-readiness. + +If the PR modifies governance documents **alongside** non-governance +files (source code, tests, configuration), the governance-document +check does NOT trigger. The PR follows the normal review flow — the +governance files are reviewed as part of the full change set. + +If a governance document also falls under a protected path (e.g., +`.github/GOVERNANCE.md`), both the `governance-document` and +`protected-path` checks may fire independently. Both converge on +blocking approval, so there is no conflict. + #### 6f. Determine overall outcome Merge PR-specific findings into the challenger-adjudicated finding set @@ -640,7 +691,10 @@ and evaluate: block the PR) - **Low** or **info** findings only (no medium+) → `approve` (attach findings as comments; preserve concrete follow-up work with - `actionable: true` so the post-script can create follow-up issues) + `actionable: true` so the post-script can create follow-up issues). + **Exception:** if a `protected-path` or `governance-document` + finding is present, the outcome is capped at `comment` regardless + of severity — see the constraints below. - No findings → `approve` - The approach is fundamentally wrong — wrong design, unauthorized change, or the PR should be closed/completely rethought → `reject`. @@ -782,10 +836,12 @@ wins. `request-changes`. - **Never approve when any protected-path finding exists**, regardless of severity. +- **Never approve when a governance-document finding exists.** If all + changed files are governance documents, the outcome must be `comment`. - **PR-specific checks (step 6e) belong in the orchestrator only.** Do - not push protected-path checks, scope authorization, or PR body - injection defense into sub-agents. These require PR-level context - that sub-agents do not have. + not push protected-path checks, governance-document checks, scope + authorization, or PR body injection defense into sub-agents. These + require PR-level context that sub-agents do not have. - **All sub-agents must be dispatched simultaneously.** Include all Agent calls in a single message. Sequential dispatch defeats the architecture's purpose.