feat(#247): cap review verdict at comment-only for governance docs

[fullsend-ai-coder]

Summary

• Add governance-document check to pr-review skill step 6e: when ALL changed files match governance patterns (MAINTAINERS.md, GOVERNANCE.md, CODE_OF_CONDUCT.md, SECURITY.md), cap the review outcome at comment-only with an info-level finding
• Add matching defense-in-depth check in post-review.sh to downgrade approve→comment for governance-only PRs
• PRs mixing governance docs with other files follow the normal review flow

Extends the ADR comment-only pattern (fullsend-ai#1659) to governance/process documents. Complements fullsend-ai#1772 by addressing the highest-risk document subset first.

Note: pre-commit hooks could not run in sandbox (shellcheck-py download blocked by network policy). Post-script runs authoritative pre-commit on the runner.

---

Closes #247

Post-script verification

• Branch is not main/master (agent/247-governance-doc-comment-only)
• Secret scan passed (scan-secrets --staged)
• Pre-commit hooks: shellcheck-py unavailable in sandbox (network restriction); authoritative run deferred to runner

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 12:54 PM UTC · Completed 12:54 PM UTC
Commit: c776c24 · View workflow run →

[guyoron1]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:57 PM UTC · Completed 4:10 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [logic-error] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The governance-document finding uses info severity, which under normal 6f rules would produce an approve verdict. The diff compensates with three explicit override rules (governance-document check section, 6f exception, and constraints) that cap the outcome at comment. This works but differs from the protected-path pattern, which uses high/medium severity to prevent approval via the severity-based logic directly. In interactive mode (no post-review.sh), enforcement relies entirely on the agent following these text-based override instructions. Current behavior is correct; this is a design observation about defense-in-depth consistency.

• [edge-case] internal/scaffold/fullsend-repo/scripts/post-review-test.sh — The is_all_governance() test helper returns true for empty input (vacuous truth). In production this is harmless because post-review.sh exits with error on empty PR_FILES before reaching the governance loop. Adding a documenting test case (e.g., run_gov_test "empty-input-vacuous-true" "" "true") with a comment about the upstream guard would make this explicit.

• [edge-case] internal/scaffold/fullsend-repo/scripts/post-review.sh — The governance check is guarded by DOWNGRADED=false, so it's skipped when the protected-path check already fired. This is correct — both paths produce comment action with DOWNGRADED=true. The user sees only the protected-path explanation, but enforcement behavior is correct.

• [scope-exceeded] internal/scaffold/fullsend-repo/scripts/post-review.sh — The governance-document check and protected-path check are separate enforcement paths with different trigger semantics: protected-path fires when ANY protected file is touched; governance-document fires only when ALL files are governance docs. This architectural distinction is self-evident from the code and documented in SKILL.md, but no comment in post-review.sh explains why these are separate rather than unified.

• [missing-documentation] docs/agents/review.md — This high-level doc does not mention governance-document verdict capping. However, it also does not mention protected-path enforcement — both are delegated to post-review.sh and the SKILL.md. No gap relative to existing content.

• [missing-documentation] internal/scaffold/fullsend-repo/agents/code.md — Mentions protected paths at lines 88-89 but not governance-document verdict caps. This is expected — the code agent implements issues and does not need to know about review-side governance enforcement. The governance check does not restrict which files the code agent can modify.

Previous run

Review

Findings

Medium

• [edge-case] internal/scaffold/fullsend-repo/scripts/post-review.sh:170 — The GOV_RESULT temp file created by mktemp has no cleanup trap, unlike MODIFIED_RESULT at line 128–129 which has trap 'rm -f ...' EXIT. The DOWNGRADED guard currently prevents both paths from executing in the same run, so the trap-overwrite risk is latent. However, the inconsistency is real — if the guard is ever relaxed, adding a second trap ... EXIT would silently replace the first.
  Remediation: Add cleanup for GOV_RESULT, either by extending the existing trap or combining both temp file cleanups into a single trap.

• [missing-test] internal/scaffold/fullsend-repo/scripts/post-review-test.sh:87 — The test file tests label-selection logic (determine_outcome_label) but has no coverage for the governance document detection logic itself. No test verifies that a PR consisting entirely of governance files triggers the downgrade, or that a mixed PR does not. The comment on line 87–88 ("DOWNGRADED is only set inside the approve branch") remains technically true but is now stale — DOWNGRADED is set by two separate code paths within the approve branch.
  Remediation: Add test cases for the governance detection logic and update the comment to acknowledge both downgrade paths.

Low

• [logic-error] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:660 — The governance-document finding is defined as info-level, but step 6f's outcome rules map "info findings only" to "approve". The new constraint "Never approve when a governance-document finding exists" overrides step 6f, but this override is implicit — an implementer reading step 6f in isolation would not know to check for governance-document before selecting "approve". This follows the same pattern as protected-path, so the interaction is correct, but step 6f could benefit from an explicit note about category-based overrides.

• [edge-case] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:650 — The governance-document category is not added to the triage category-to-dimension table in step 3a. On re-reviews, a prior governance-document finding would be routed to the correctness sub-agent as a fallback per the "unrecognized categories go to correctness" rule. This is harmless since the finding is info-level and non-actionable, and adding it to the table would also be conceptually wrong (no sub-agent should own this orchestrator-only category).

[guyoron1]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 5:38 PM UTC · Completed 5:49 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed 3 of 4 review findings. Fixed GOV_RESULT temp file cleanup by combining both downgrade paths into a single trap. Added 7 governance detection test cases and updated a stale comment. Added explicit category-override note in step 6f. Disagreed with 1 finding (adding governance-document to the triage table) per the reviewer's own analysis that it would be conceptually wrong.

Fixed (3):

1. GOV_RESULT temp file has no cleanup trap (internal/scaffold/fullsend-repo/scripts/post-review.sh): Combined temp file cleanup into a single trap declared before the approve block, covering both MODIFIED_RESULT and GOV_RESULT. Removed the per-variable trap from the protected-path path. Both downgrade paths now share consistent cleanup.
2. Missing test coverage for governance document detection logic (internal/scaffold/fullsend-repo/scripts/post-review-test.sh): Added is_all_governance helper function and 7 test cases covering: single governance file, multiple governance files, governance files in subdirectories, mixed governance+non-governance files, non-governance only, substring mismatch (MY_GOVERNANCE.md), and all four governance patterns. Updated stale comment on line 87-88 to acknowledge both downgrade paths (protected-path and governance-document).
3. governance-document finding logic not explicit in step 6f (internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md): Added explicit exception note to the info-only-approve rule in step 6f: protected-path or governance-document findings cap the outcome at comment regardless of severity, with cross-reference to the constraints section.

Disagreed (1):

1. governance-document category missing from triage table in step 3a: The reviewer's own analysis concludes that adding governance-document to the triage table would be conceptually wrong because no sub-agent should own this orchestrator-only category. The current fallback (unrecognized categories route to correctness) is harmless since the finding is info-level and non-actionable. No change needed.

Tests: passed

_{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:51 PM UTC · Completed 6:05 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-review]

[low] edge-case

The is_all_governance() test helper returns true for empty input (vacuous truth). In production this is guarded by PR_FILES non-empty check. Adding a documenting test case would make the edge case explicit.

Suggested fix: Add: run_gov_test "empty-input-vacuous-true" "" "true" with a comment noting the upstream guard.

[fullsend-ai-review]

[low] edge-case

Governance check is skipped when protected-path already fired (DOWNGRADED=false guard). Both paths produce the same result (comment action with DOWNGRADED=true), so enforcement is correct. User sees only the protected-path explanation.