From 6c0d7a18984835eba842b8eab4b883d151b53715 Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Fri, 10 Jul 2026 12:47:13 +0000 Subject: [PATCH 1/2] feat(#247): cap review verdict at comment-only for governance docs Add governance-document check to pr-review skill step 6e and post-review.sh defense-in-depth. When ALL changed files match governance patterns (MAINTAINERS.md, GOVERNANCE.md, CODE_OF_CONDUCT.md, SECURITY.md), the review outcome is capped at comment-only with an info-level finding. PRs mixing governance docs with other files follow the normal review flow. Closes #247 Co-Authored-By: Claude Opus 4.6 --- .../fullsend-repo/scripts/post-review.sh | 56 +++++++++++++++++- .../fullsend-repo/skills/pr-review/SKILL.md | 59 ++++++++++++++++++- 2 files changed, 109 insertions(+), 6 deletions(-) diff --git a/internal/scaffold/fullsend-repo/scripts/post-review.sh b/internal/scaffold/fullsend-repo/scripts/post-review.sh index 955c64de1..d896e51bf 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 @@ -133,6 +134,55 @@ if [ "${ACTION}" = "approve" ]; then 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..447dfb4b1 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 @@ -782,10 +833,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. From 934d74b74e06967cdd7549794688828f6adcbf7a Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:46:25 +0000 Subject: [PATCH 2/2] fix(#247): address review feedback on PR #261 - Combine temp file cleanup into a single trap covering both MODIFIED_RESULT and GOV_RESULT (previously GOV_RESULT had no cleanup, inconsistent with the protected-path downgrade path) - Add governance document detection tests (is_all_governance helper with 7 test cases: single/multiple/subdir/mixed/non-gov/substring/ all-four-patterns) - Update stale comment to acknowledge both downgrade paths - Add explicit category-override note in step 6f outcome rules (protected-path and governance-document cap outcome at comment) Addresses review feedback on #261 --- .../fullsend-repo/scripts/post-review-test.sh | 93 ++++++++++++++++++- .../fullsend-repo/scripts/post-review.sh | 6 +- .../fullsend-repo/skills/pr-review/SKILL.md | 5 +- 3 files changed, 101 insertions(+), 3 deletions(-) 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 d896e51bf..0fa208394 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-review.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review.sh @@ -91,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') @@ -127,7 +132,6 @@ 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}" diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md index 447dfb4b1..893810991 100644 --- a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md @@ -691,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`.