Skip to content

chore(helm): activate comment-shaped Ally review gate in observe-only mode (BLO-26602) - #1346

Merged
allyblockcast[bot] merged 1 commit into
masterfrom
cto/blo-26602-activate-comment-review-gate
Aug 15, 2026
Merged

chore(helm): activate comment-shaped Ally review gate in observe-only mode (BLO-26602)#1346
allyblockcast[bot] merged 1 commit into
masterfrom
cto/blo-26602-activate-comment-review-gate

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • It enforces protected merge gates on GitHub so agent-authored PRs cannot land while a reviewer has raised a blocking finding
  • Ally reviews on two surfaces: a formal pull_request_review, and a plain PR comment headed ## Ally — Consolidated PR Review that files no review object at all
  • The merge gate was blind to the second surface, so a PR could enter the merge queue while an explicit "do not merge until X" sat unread (BLO-21907, observed live on fix(dispatch): bound worst-case queue wait for every priority tier (BLO-21792) #1022)
  • PR feat(github): gate comment-shaped Ally review findings (BLO-21907) #1262 fixed that in code, but shipped deliberately opt-in: githubApp.prCommentReviewGateStatusContext defaults to "", which makes the whole path a strict no-op
  • This pull request sets that context in the Blockcast production values so the gate starts publishing its status
  • The benefit is that the gate produces real, observable verdicts on live PRs — which is both the evidence needed to trust it and a hard prerequisite for the human step that makes it blocking

Linked Issues or Issue Description

Refs #1262
Refs BLO-26602 (activation), BLO-21907 (comment-shaped gate), BLO-17456 (formal-review gate)

What Changed

  • Set githubApp.prCommentReviewGateStatusContext: "review/ally-comment" in deploy/helm/paperclip/values.blockcast.yaml, naming it consistently with the existing review/ally-* convention used in the runbooks and shipped skills catalog.
  • Documented in-file why this is observe-only, and why it must land before the branch-protection step rather than with it.
  • Documented in-file why prReviewGateStatusContext is deliberately left unset here (see Risks).

This PR does not make anything blocking. The status only gates merges once the same context is marked required on master branch protection, which agent tokens cannot do (403 Resource not accessible by integration).

Why this must precede the branch-protection change

GitHub's required-status-checks picker only offers contexts it has observed on a recent commit. Blockcast/paperclip has never carried a legacy commit status — I checked the eight most recent open PR heads and all report <no statuses>. So the context has to be posted at least once before a human can select it in repo settings. Setting the value first is a prerequisite, not a shortcut.

Verification

Chart renders and lints locally against the production values:

$ helm template paperclip . -f values.blockcast.yaml   # exit 0
Deployment/paperclip-api  container=paperclip  PAPERCLIP_PR_COMMENT_REVIEW_GATE_STATUS_CONTEXT='review/ally-comment'
StatefulSet/paperclip     container=paperclip  PAPERCLIP_PR_COMMENT_REVIEW_GATE_STATUS_CONTEXT='review/ally-comment'

$ helm lint . -f values.blockcast.yaml
1 chart(s) linted, 0 chart(s) failed

Confirmed the key parses under githubApp (not reparented into a sibling block) and that PAPERCLIP_PR_REVIEW_GATE_STATUS_CONTEXT is correctly absent from the render.

Post-deploy, the observable signal is a review/ally-comment status appearing on PR heads — success when no unresolved Critical/Important comment finding attests to the current head, failure when one does.

Risks

Low. The status is advisory until a human marks it required; a non-required status does not block a merge or merge-queue entry.

One thing this PR deliberately does not do, and why it matters. BLO-26602 asked to bundle prReviewGateStatusContext (the formal-review gate) and mark both contexts required. Setting the value is harmless, but marking that one required on this repo would wedge every PR, so I did not set it up for that:

  • Its only writer is queueFailedPrReviewGateStatus, which posts state: "failure" on reviewer-chain exhaustion or ambiguous lifecycle end.
  • Nothing in the server ever posts success for it. github-status-delivery-outbox.ts skips delivery when a success already exists — i.e. it assumes an external writer owns the green.
  • On onprem-k8s that external writer exists (.github/scripts/require-ally-review.py). This repo has no equivalent, and no workflow here posts commit statuses at all.

So a healthy PR would simply never receive that context, and GitHub renders a required-but-never-posted context as "Expected — waiting for status" indefinitely. That is the exact wedge the values.yaml comment warns about. The comment-shaped gate does not have this problem: it posts both success and failure on every evaluated head.

Remaining risk to confirm at rollout: the App installation needs statuses: write on this repo. It has never posted a status here, so that permission is unproven empirically — if it is missing, the gate logs a post failure and stays inert rather than breaking anything.

Model Used

Claude Opus 4.5 (claude-opus-4-5), extended thinking, tool-assisted repository analysis and local Helm render/lint verification.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable — n/a, values-only config change with no source code; the gate's behavior is already covered by the tests that shipped with feat(github): gate comment-shaped Ally review findings (BLO-21907) #1262
  • If this change affects the UI, I have included before/after screenshots — n/a
  • I have updated relevant documentation to reflect my changes
  • I have considered and documented any risks above
  • All Paperclip CI gates are green — pending first run
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending
  • I will address all Greptile and reviewer comments before requesting merge

… mode (BLO-26602)

Set githubApp.prCommentReviewGateStatusContext to "review/ally-comment" in
the Blockcast production values so the server begins publishing the
comment-shaped review gate status. This does not block any merge: the status
only gates once the same context is marked required on master branch
protection, which is a human-only step (agent tokens get 403 there).

Sequencing is deliberate. GitHub's required-checks picker only offers status
contexts it has observed recently, and this repo has never carried a commit
status of any kind, so the context must be posted at least once before a human
can select it.

Deliberately does NOT set prReviewGateStatusContext. Its only writer posts
state=failure on reviewer-chain exhaustion; nothing in the server, and no
workflow or script in this repo, ever posts success for it. Requiring it here
would strand every healthy PR at "Expected - waiting for status".

Refs BLO-26602, BLO-21907, BLO-17456
@allyblockcast

allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-17456
🔗 Paperclip issue: BLO-21907
🔗 Paperclip issue: BLO-26602

@allyblockcast

allyblockcast Bot commented Aug 14, 2026

Copy link
Copy Markdown
Author

@ally please review at head c9ee47f — helm: activates the comment-shaped Ally review gate in observe-only mode. Focus on confirming observe-only cannot block a merge.

Context: the original review request on this PR was lost during the codex provider outage (BLO-27123) — codex success sat at 0/min from ~14:50Z to 17:54Z and Ally is pinned to openai/gpt-5.6-terra on that pool. Recovery does not revisit the stranded set, so this is a forward-only re-request. Codex recovered 17:56Z (~55 req/min, near-zero errors) and the path is verified working (#1329, #1341 reviewed at head in ~3 min).

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c9ee47f

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

  • The deployment-only change opts in the comment-shaped status context without configuring the separate formal-review status context.
  • The chart wiring passes the value as an environment variable; the gate implementation remains a status publisher and does not modify branch protection or merge settings.
  • The comments clearly document that making the context required is a separate follow-up operation.

Recommended Action

  1. Safe to merge from the reviewed change surface.

@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 15, 2026
Merged via the queue into master with commit 2da26b7 Aug 15, 2026
33 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants