feat(scripts): gate triage-triggered code/fix PRs behind write+ approval - #523
feat(scripts): gate triage-triggered code/fix PRs behind write+ approval#523waynesun09 wants to merge 1 commit into
Conversation
PR Summary by QodoGate triage-triggered code/fix PRs behind needs-write-approval label
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
|
🤖 Finished Review · ✅ Success · Started 12:37 PM UTC · Completed 12:54 PM UTC |
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. TRIGGER_ROLE malformed skips gate
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| echo "Trigger role is 'triage' — applying needs-write-approval gate to PR #${target_pr}" | ||
| gh label create "needs-write-approval" --repo "${REPO_FULL_NAME}" \ | ||
| --description "Triggered by a triage-role user; needs write+ approval before merge" \ | ||
| --color "B60205" --force 2>/dev/null || true | ||
| gh pr edit "${target_pr}" --repo "${REPO_FULL_NAME}" \ | ||
| --add-label "needs-write-approval" 2>/dev/null || \ | ||
| _write_approval_gate_warn "Failed to apply needs-write-approval label to PR #${target_pr}" |
There was a problem hiding this comment.
1. Gate label application fail-open 📜 Skill insight ☼ Reliability
When TRIGGER_ROLE=triage, apply_write_approval_gate_if_needed treats gh pr edit failures as warnings and returns success, so post-code/post-fix can continue and a triage-triggered PR may proceed without the required needs-write-approval merge gate label. This creates a fail-open authorization path where labeling errors (permissions/auth/network/API) can silently bypass the intended enforcement signal.
Agent Prompt
## Issue description
`apply_write_approval_gate_if_needed` is intended to enforce an explicit merge-authorization gate for triage-triggered PRs by applying the `needs-write-approval` label, but it currently fails open: when `gh pr edit` cannot apply the label (permissions/auth/network/API errors, invalid PR input, missing label), the helper suppresses the error and returns success, allowing post-code/post-fix to complete and leaving a triage PR potentially mergeable without the required gate marker.
## Issue Context
The gate is described as “informational-but-load-bearing” and as a mandatory explicit merge gate for triage-triggered PRs; therefore, failure to apply the label must be treated as a hard failure (or otherwise enforced) when `TRIGGER_ROLE=triage`. Today the behavior is effectively best-effort (warning-only, `2>/dev/null`, `|| true` style suppression), and both post-code and post-fix call the helper and then continue/exit 0, meaning labeling failure is not surfaced or enforced.
## Fix Focus Areas
- scripts/lib/write-approval-gate.lib.sh[29-51]
- scripts/lib/write-approval-gate.lib.sh[37-51]
- scripts/post-code.src.sh[521-533]
- scripts/post-code.src.sh[522-532]
- scripts/post-fix.src.sh[436-456]
- scripts/post-fix.src.sh[446-456]
- scripts/post-code-test.sh[1187-1214]
- scripts/post-fix-test.sh[403-430]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if [[ "${TRIGGER_ROLE:-}" != "triage" ]]; then | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
2. trigger_role malformed skips gate 📜 Skill insight ⛨ Security
apply_write_approval_gate_if_needed treats any TRIGGER_ROLE value other than exactly triage as a no-op, so a malformed/unexpected value bypasses the gate instead of failing closed. This can result in triage-triggered PRs missing the required merge gate if configuration is absent or malformed.
Agent Prompt
## Issue description
The gate predicate `[[ "${TRIGGER_ROLE:-}" != "triage" ]] && return 0` fails open for malformed/unexpected `TRIGGER_ROLE` values.
## Issue Context
For auth/validation gates, absence/malformed config must not broaden access. Here, an unexpected value (e.g. typo) should not silently disable the gate.
## Fix Focus Areas
- scripts/lib/write-approval-gate.lib.sh[37-43]
- scripts/post-code-test.sh[1187-1214]
- scripts/post-fix-test.sh[403-430]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| source "${SCRIPT_DIR_POST}/lib/gitleaks-install.lib.sh" | ||
| # shellcheck source=lib/pr-assignee.lib.sh | ||
| source "${SCRIPT_DIR_POST}/lib/pr-assignee.lib.sh" | ||
| # shellcheck source=lib/write-approval-gate.lib.sh | ||
| source "${SCRIPT_DIR_POST}/lib/write-approval-gate.lib.sh" |
There was a problem hiding this comment.
3. Protected scripts/ files modified 📜 Skill insight § Compliance
This PR modifies multiple files under the protected scripts/ path, which requires explicit human review and must not be auto-approved. Ensure governance/infrastructure review expectations are followed for these changes.
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: PR modifies post-code and post-fix script infrastructure to add triage-role authorization gate |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
0aed7e1 to
cc91a3c
Compare
|
🤖 Review · |
fullsend-ai/fullsend#5687 will allow the GitHub triage role to trigger /fs-code and /fs-fix. post-code.sh/post-fix.sh apply a needs-write-approval label to the resulting PR when TRIGGER_ROLE is triage (normalized case-insensitively; unrecognized values warn and default to no-gate). Enforcement lives in skills/merge-queue/scripts/lib/write-approval-check.lib.sh, used by both enqueue-pr.sh (the documented primary enqueue entry point) and await-and-enqueue.sh (its polling wrapper), so there is no bypass via either documented path: - Whether a PR ever required write-approval is derived from the immutable issue-events timeline (a labeled event), not the label's current presence — GitHub's triage role includes repo-wide label management, so a mutable "is it currently labeled" check is removable by the very user it constrains. - The approving reviewer's permission is re-checked live against the collaborator-permission API, not trusted from GitHub's reviewDecision alone — reviewDecision does not distinguish reviewer permission level, and since the PR is bot-authored (not authored by the triage trigger), that user is free to review and approve their own triage-triggered PR. - The approval must be on the PR's current head commit, so a later /fs-fix push cannot ride on a stale approval from before those commits existed. - Bot/App reviewers are excluded from counting as write+ approvers. This does not prevent a write+ collaborator from merging directly via GitHub's native UI or `gh pr merge`, which are unaware of this label — documented as a known residual limitation in docs/code.md and docs/fix.md. Assisted-by: Claude (fix, review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
cc91a3c to
0638a35
Compare
|
🤖 Review · ❌ Terminated · Started 2:39 PM UTC · Ended 2:58 PM UTC |
Status: draft, paused pending a design revisitThis went through three independent review rounds (Claude ×2 + Grok per round, 9 agent-reviews total), each of which fixed the previous round's specific gap and surfaced a new one. Converting to draft rather than continuing to patch — the pattern below suggests the underlying approach needs a design rethink, not another quick fix. Round 1 — label-only gateShipped: Round 2 — live-permission check, gated on current label stateShipped: Round 3 — immutable-timeline check, commit-pinned, both entry pointsShipped: "Was write-approval ever required" derived from the issue-events timeline (a
Recommendation for whoever picks this back upThe recurring failure mode is relying on signals GitHub's own Related: fullsend-ai/fullsend#5687 (the authorization-side change this PR was meant to support). |
| # in the caller rather than a legitimate write+ trigger. | ||
| # Requires REPO_FULL_NAME. Best-effort: never fails the calling script. | ||
| # Note: parameter is target_pr (not pr_number) to avoid SC2153 against | ||
| # PR_NUMBER from post-failure-report.lib.sh once both libs are bundled into |
There was a problem hiding this comment.
[medium] fail-open
The merge gate for triage-role PRs is best-effort only: if gh pr edit --add-label fails, the PR proceeds without the needs-write-approval label. The enforcement side (write_approval_ever_required) checks the issue-events timeline for a labeled event — a failed label application means the enforcement gate sees ever_required == false and returns 0, making the triage-triggered PR indistinguishable from a write-triggered PR.
Suggested fix: Make the label application a hard failure (exit non-zero), or use a second enforcement signal so enforcement does not depend solely on the label.
| if declare -F gha_echo >/dev/null 2>&1; then | ||
| gha_echo warning "$*" | ||
| else | ||
| echo "warning: $*" >&2 |
There was a problem hiding this comment.
[low] authorization-bypass
Unrecognized TRIGGER_ROLE values are treated as write (no gate applied). If a new role is introduced and this script is not updated, PRs triggered by that role will bypass the write-approval gate.
Suggested fix: Invert the logic: only allow known safe roles to skip the gate.
| run_ever_required_test "currently-labeled" \ | ||
| '[{"event": "labeled", "label": {"name": "needs-write-approval"}}]' \ | ||
| "true" | ||
|
|
There was a problem hiding this comment.
[low] test-inadequate
No test for the case where a reviewer's most recent action is a COMMENT review on the head commit with a prior APPROVED. The jq filter's max_by(submitted_at) would pick the COMMENT and fail the APPROVED check — blocking enqueue when a valid approval exists. Fail-closed, not fail-open.
|
🤖 Finished Review · ✅ Success · Started 2:39 PM UTC · Completed 2:58 PM UTC |
1 participant
Summary
fullsend-ai/fullsend#5687 proposes letting the GitHub
triagerole trigger/fs-codeand/fs-fix(currently write+ only), on the condition that the resulting PR is explicitly gated behind write+ approval before merge — independent of whatever branch protection a given repo happens to have configured. That fullsend-side authorization change is a companion PR; this PR implements the gate.Changes
scripts/lib/write-approval-gate.lib.sh:apply_write_approval_gate_if_needed(target_pr)applies aneeds-write-approvallabel whenTRIGGER_ROLEnormalizes (case-insensitive, trimmed) to"triage". Warns (without gating) on any other unrecognized non-empty value. Wired intopost-code.src.sh(beforeready-for-review, so downstream automation never races ahead of it) andpost-fix.src.sh.skills/merge-queue/scripts/lib/write-approval-check.lib.sh: the enforcement, used by bothenqueue-pr.sh(the documented primary enqueue entry point) andawait-and-enqueue.sh(its polling wrapper) — see "Design history" below for why both needed it.needs-write-approvalandTRIGGER_ROLEdocumented indocs/code.md/docs/fix.md;skills/merge-queue/SKILL.mddocuments the new refusal behavior.post-code-test.sh/post-fix-test.sh; a newwrite-approval-check-test.shexercising the actual jq filters used for timeline-based detection, commit-pinning, and bot-exclusion.Design history — this took two review rounds to get right
Round 1 shipped a label-only design. Review (3 agents: Claude ×2, Grok) found it didn't work: GitHub's
triagerole includes repo-wide label management (removable by the constrained user), and nothing read the label at all —post-review.shnever submits a realgh pr review(confirmed by grep), soreviewDecision=APPROVEDcan only come from an actual review by some collaborator, and GitHub doesn't distinguish reviewer permission level. Since the PR is bot-authored (not authored by the triage trigger), that user could approve their own triggered PR themselves.Round 2 fixed enforcement but gated it on the label's current presence — reintroducing the exact removability problem one layer down — and only wired it into
await-and-enqueue.sh, missing thatenqueue-pr.shis the documented primary path and bypasses it entirely. Also missing: nothing pinned an approval to the PR's current head commit, so a stale approval survives a later/fs-fixpush.What's actually shipped now:
labeledevent), not current label state.enqueue-pr.shandawait-and-enqueue.shenforce this independently (the latter also delegates to the former at the end, so there's no path that skips it).Known residual limitation (documented in
docs/code.md/docs/fix.md): a write+ collaborator merging directly via GitHub's native UI orgh pr mergebypasses this entirely — it's unaware of the label. This PR only gates the merge-queue skill's own scripts, not GitHub itself.Test plan
make check-bundle— bundled scripts matchmake script-buildoutputbash scripts/post-code-test.sh/--bundled,bash scripts/post-fix-test.sh/--bundled— all passbash skills/merge-queue/scripts/write-approval-check-test.sh— all pass (timeline-immutability, commit-pinning, bot-exclusion, latest-review-wins dedup)pre-commit run— shellcheck clean across all changed/new scriptsRelated