fix(release): unblock automated bump-version PR creation (#11) - #88
Closed
devops-thiago wants to merge 1 commit into
Closed
fix(release): unblock automated bump-version PR creation (#11)#88devops-thiago wants to merge 1 commit into
devops-thiago wants to merge 1 commit into
Conversation
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
============================================
- Coverage 98.48% 98.39% -0.10%
+ Complexity 1138 1137 -1
============================================
Files 51 51
Lines 3230 3230
Branches 416 416
============================================
- Hits 3181 3178 -3
- Misses 14 16 +2
- Partials 35 36 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The bump-version job pushed chore/bump-* but `gh pr create` failed with "GitHub Actions is not permitted to create or approve pull requests (createPullRequest)", and the `|| echo "PR already exists"` masked it — so the post-release pom bump PR was silently never opened. - Stop masking gh pr create failures: distinguish outcomes — success / already-exists -> done; not-permitted -> open an idempotent tracking issue so the bump is never lost; anything else -> fail loudly. - Open the PR with the default GITHUB_TOKEN, relying on the repo setting "Allow GitHub Actions to create and approve pull requests" — no stored secret or key, so nothing extra for a compromised action to exfiltrate. When the setting is off, the tracking-issue fallback kicks in. - Skip the force-push when the remote bump branch already carries the bump, to avoid churning an open PR on release re-runs. - Pass the next version via env to versions:set (defense-in-depth). - Document the setup, the close/reopen-to-run-CI step, and squash-not-rebase (signed commits) in docs/RELEASING.md, linked from CONTRIBUTING + changelog. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
devops-thiago
force-pushed
the
fix/release-bump-version-pr
branch
from
June 14, 2026 16:26
81c8cce to
4e17d6e
Compare
|
Owner
Author
|
/review |
🤖 ThrillhouseBot PR SummaryWhat this PR doesModifies the release workflow to gracefully handle gh pr create permission errors by opening a tracking issue instead of silently failing, adds idempotent push to avoid churn on re-runs, and documents the release process.
|
| Risk | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 High | 0 |
| 🟡 Medium | 0 |
| 🔵 Low | 1 |
Key Findings
- CRITICAL: NEXT and TAG variables not defined in bump PR step (
.github/workflows/release.yml:369) - LOW: Tracking issue idempotency check truncated at 100 issues (
.github/workflows/release.yml:405)
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
| git config user.name "github-actions[bot]" | ||
| git config user.email "41898282+github-actions[bot]@users.noreply.github.com" | ||
| BRANCH="chore/bump-${NEXT}" | ||
| TITLE="chore: bump version to ${NEXT}" |
There was a problem hiding this comment.
🔴 CRITICAL — NEXT and TAG variables not defined in bump PR step (low confidence — verify before acting)
The Open or update bump PR step uses ${NEXT} and ${TAG} but neither variable is assigned before use. The old step defined NEXT=${{ steps.next.outputs.version }} and TAG=${{ steps.tag.outputs.tag }} at the top of the script; the new step's run block omits these assignments, causing branch name, commit message, and PR title/body to be malformed (e.g., branch chore/bump-). The job will either fail or produce a meaningless bump PR, breaking the release automation.
| exit 1 | ||
| fi | ||
|
|
||
| echo "::warning::GitHub Actions is not permitted to open PRs; opening a tracking issue instead. Enable Settings → Actions → General → \"Allow GitHub Actions to create and approve pull requests\" to let releases open the bump PR automatically. See docs/RELEASING.md." |
There was a problem hiding this comment.
🔵 LOW — Tracking issue idempotency check truncated at 100 issues (low confidence — verify before acting)
The gh issue list --state open --limit 100 call fetches only the first 100 open issues. If more than 100 issues exist and the tracking issue is not among them, the script fails to detect it and will create a duplicate. This violates the intended idempotency. Use a higher limit (e.g., 1000) or paginate to avoid a truncation gap.
Suggested change
| echo "::warning::GitHub Actions is not permitted to open PRs; opening a tracking issue instead. Enable Settings → Actions → General → \"Allow GitHub Actions to create and approve pull requests\" to let releases open the bump PR automatically. See docs/RELEASING.md." | |
| EXISTING=$(gh issue list --state open --limit 1000 --json number,title \ | |
| -q ".[] | select(.title == \"${ISSUE_TITLE}\") | .number" | head -1) |
This was referenced Jun 20, 2026
devops-thiago
added a commit
that referenced
this pull request
Jun 20, 2026
…is outside the diff hunk (#192) (#194) ## What type of PR is this? - [x] 🐛 Bug fix ## Description The reviewer (and the verifier) see only GitHub's unified-diff **patch** — ~3 lines of context around each change. When a finding depends on an **unchanged definition that sits just outside the hunk window in the same file**, that definition is never in the reviewed material, so the model sees a *use* with no nearby *definition* and confidently flags the symbol as undefined. **Dogfood evidence — #88:** the bot posted a 🔴 **CRITICAL** *"NEXT and TAG variables not defined in bump PR step"* on `.github/workflows/release.yml`. The step's `env:` block **does** define `TAG`/`NEXT`, but those lines are unchanged context a few rows above the changed `run:` line, collapsed out of the diff. The finding is false; the workflow is correct. This PR adds two prompt-level guards — the cheap interim fix ahead of the #55 codebase-aware-context spike, mirroring how #107 frames its guard: - **Verifier** (`FindingVerifierPrompts`): rejects an *"undefined / unset / missing symbol"* finding (variable, parameter, import, function, env var, config key) when the symbol's definition is **not present in the provided material** — its absence from the diff is not proof it is undefined. Calibrated to at most `low` confidence otherwise. A `rejected` verdict drops the finding in `FindingVerificationService`, so the false positive never posts. The PR #88 case is embedded as an inline regression example. - **Generator** (`PrReviewPrompts`): generalizes the existing *"value is missing"* self-check to all undefined/unset-symbol claims and spells out the hunk-window blind spot, so the finding is less likely to be emitted in the first place. **Deliberately out of scope:** the issue's other mitigation — *widening same-file context* (pull more of each changed file / the full file when small into the prompt) — is a heavier change (extra `getFileContent` calls per file, a budget knob interacting with `max-diff-lines`) that overlaps the #55 and #53 spikes. This PR is the prompt-rule guard that fully neutralizes the demonstrated false positive. ## Related Issues Fixes #192. ## How Has This Been Tested? - [x] Unit tests Prompt-text + CHANGELOG change (no behavioral Java logic). `AiServicePromptRenderingTest` passes (5/5) — the real quarkus-langchain4j pipeline still renders both prompts with every `@V` context variable. Verified in `FindingVerificationService` that a `rejected` verdict drops the finding from the kept set, so the new reject rule has teeth. > Full suite isn't runnable locally (JaCoCo SIGABRTs file-locking its exec file on this SMB-mounted checkout); CI on `ubuntu-latest` runs it. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [ ] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors <sub>No new automated test: these are LLM prompt-rule additions with no deterministic behavior to assert; the existing prompt-rendering regression test already pins that both templates render with all context variables.</sub> ## Additional Notes The verifier rule is independent of #105 (confidence-gated posting): it **rejects** (removes) the finding rather than merely capping it to `low`, so it reduces noise regardless of whether low-confidence findings still land inline.
This was referenced Jun 21, 2026
Owner
Author
|
Closing in favor of #208. The blocker this PR worked around is resolved at the repo level: the Allow GitHub Actions to create and approve pull requests setting is now enabled ( This PR had also gone stale: its base predates the v0.2.0 CI churn ( |
devops-thiago
added a commit
that referenced
this pull request
Jun 21, 2026
## What type of PR is this? - [x] 📦 Release - [x] 📝 Documentation ## Description Prepares the **v0.2.0** release (all v0.2.0 feature/fix PRs are merged) and closes the changelog/doc gaps the post-merge audit found. ### Release - `pom.xml`: `0.2.0-SNAPSHOT` → **`0.2.0`** (the release workflow validates the pushed tag matches the pom version; it auto-bumps to the next `-SNAPSHOT` afterward). - `CHANGELOG.md`: `[Unreleased]` → **`[0.2.0] — 2026-06-21`**. ### Changelog / doc accuracy - **Added** the missing pre-v0.2.0 fixes **#175** (duplicate no-issues CI-pending message) and **#178** (ruleset-based gating), plus the **#112** runtime-crash review dimension. - **Removed** the intra-v0.2.0 *"Duplicate PR summary on busy PRs"* entry (#199 — that bug only existed in unreleased v0.2.0 code). - **Broadened** the #165 bot-identity entry (now wired through summary dedup + follow-up tracking via #201). - **Corrected** #31 (conversational replies are `@`-mention-only after #202) and the #32 `/pause` scope (also silences conversational replies, #196). - `.env.example`: added `GITHUB_BOT_LOGINS` and `MANUAL_TRIGGER_AUTH_TIMEOUT`. - `README.md`: manual App registration now lists the `pull_request_review_comment` event; the "Large diffs" limitation says hunk-boundary truncation; the Pause prose notes conversational replies are silenced too. - `docs/ARCHITECTURE.md`: added the v0.2.0 subsystems to the package table. ## After merge Tag **`v0.2.0`** on the merge commit to trigger the release workflow. (Heads-up: the automated post-release `-SNAPSHOT` bump depends on #88.) ## How Has This Been Tested? `spotless:check` and `test-compile` clean on the rebased tree; full suite runs in CI on this PR. No source logic changed.
matheusandre1
pushed a commit
to matheusandre1/ThrillhouseBot
that referenced
this pull request
Jul 27, 2026
…evops-thiago#78) ## What type of PR is this? - [x] 🐛 Bug fix - [ ] ✨ Feature - [ ] 📝 Documentation - [ ] 🔧 Refactor - [ ] 🚀 Performance - [x] ✅ Test - [x] 🔒 Security - [ ] 📦 Dependency update - [ ] 🏗️ CI/CD ## Description Manual `/review` and `@thrillhousebot review` comment triggers previously ran for **any** commenter. `WebhookController.handleIssueComment` and `TriggerDetector` only skipped the bot's own comments, so on a public repository anyone could repeatedly trigger paid AI reviews and spend the operator's API budget — a cost/abuse vector. This PR gates manual triggers on the commenter's GitHub `author_association`: - Only `OWNER`, `MEMBER`, and `COLLABORATOR` (users with write access) may run a manual review. - Everyone else (`CONTRIBUTOR`, `FIRST_TIME_CONTRIBUTOR`, `NONE`, …) gets a polite no-op: the attempt is logged and no review is dispatched. - The check **fails closed** when `author_association` is missing or unknown. - Automatic `pull_request` reviews (opened/reopened/synchronize) are **unaffected**. Changes: - `WebhookPayload.Comment` now parses the `author_association` field. - `TriggerDetector.isAuthorizedToTrigger(...)` encapsulates the write-access check (case/whitespace-insensitive, null-safe). - `WebhookController.handleIssueComment` rejects unauthorized triggers before dispatching. ## Related Issues Fixes devops-thiago#70 ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing - `TriggerDetectorTest`: authorized associations (incl. mixed case/whitespace), and rejection of non-write associations / blank / null. - `WebhookControllerTest`: added `shouldIgnoreUnauthorizedManualReviewTrigger` (a `NONE` commenter is not dispatched) and updated the existing trigger test to stub authorization. - Full suite: `./mvnw test` → 724 tests, 0 failures. `./mvnw spotless:check` passes. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Screenshots / Logs On an unauthorized attempt the bot logs and stops, e.g.: ``` INFO Ignoring unauthorized manual review trigger from @stranger (association: NONE) on PR devops-thiago#88 ``` ## Additional Notes The fix is secure-by-default and needs no configuration. If a configurable allowlist of additional usernames is desired later, it can layer on top of `isAuthorizedToTrigger` without changing the gate's call site.
matheusandre1
pushed a commit
to matheusandre1/ThrillhouseBot
that referenced
this pull request
Jul 27, 2026
…evops-thiago#85) ## What type of PR is this? - [x] 🐛 Bug fix - [ ] ✨ Feature - [ ] 📝 Documentation - [x] 🔧 Refactor - [ ] 🚀 Performance - [x] ✅ Test - [x] 🔒 Security - [ ] 📦 Dependency update - [ ] 🏗️ CI/CD ## Description Follow-up that hardens the manual-review authorization shipped in devops-thiago#78. devops-thiago#78 limited manual `/review` triggers to commenters whose GitHub `author_association` is `OWNER`/`MEMBER`/`COLLABORATOR`. A review of that change surfaced three issues: 1. **`author_association` is not a reliable proxy for write access.** `MEMBER` only means membership in the owning organization, so a **read-only org member** could still trigger paid reviews and spend the operator's API budget. It also duplicated authorization logic the project already owns (`DashboardAccessChecker` resolves real repo access via the GitHub API). 2. **No `action` filter.** `issue_comment` fires for `created`, `edited`, and `deleted`; an authorized user editing or deleting a `/review` comment re-dispatched a paid review. 3. The association set was hardcoded with no operator override. ### What this PR does - Adds **`ManualReviewAuthorizer`**, which confirms the commenter holds **admin/write** permission on the specific repository via the GitHub collaborator-permission API (`GET /repos/{owner}/{repo}/collaborators/{username}/permission`), reusing the existing installation-token flow (`GitHubAuthClient`) and REST client (`GitHubInstallationClient`). `TriggerDetector` goes back to recognizing trigger text only. - Keeps `author_association` **only as a cheap negative pre-filter**: values that provably cannot hold write access (`CONTRIBUTOR`, `NONE`, …) are rejected without an API round-trip, so public-repo trigger spam cannot exhaust the app's GitHub rate limit. It never grants access on its own. - Restricts `issue_comment` handling to the **`created`** action. - Adds a configurable **`thrillhousebot.review.manual-trigger-allowed-logins`** allowlist for explicitly trusted users. - Fails **closed** on missing permission or any API error. Automatic `pull_request` reviews are unaffected. ## Related Issues Refs devops-thiago#70, devops-thiago#78 ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing - New `ManualReviewAuthorizerTest`: read-only `MEMBER` rejected (the over-permit regression), `MEMBER`/`COLLABORATOR`/`OWNER` with write/admin authorized, `OWNER` verified via API rather than trusted on association, non-write associations and blank login rejected without any API call, allowlist match (case-insensitive), case-insensitive permission level, and fail-closed on exception / missing permission. - `WebhookControllerTest`: updated to the new authorizer; added `edited`/`deleted` action-filter cases (no dispatch, authorizer never consulted). - Full suite: `./mvnw test` → **733 tests, 0 failures**. `./mvnw spotless:check` passes. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Screenshots / Logs A read-only org member is now rejected even though their association is `MEMBER`: ``` INFO Ignoring unauthorized manual review trigger from @member on PR devops-thiago#88 ``` ## Additional Notes The write-permission check is a synchronous call on the webhook acknowledgement path; installation tokens are cached by `GitHubAuthClient`, and the negative pre-filter avoids the call entirely for non-collaborators, so the common abuse path makes no API request.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Description
The
bump-versionjob in.github/workflows/release.ymlpushes achore/bump-*branch butgh pr createfails withGitHub Actions is not permitted to create or approve pull requests (createPullRequest). The old|| echo "PR already exists"masked that real error, so the post-release pom bump PR was silently never opened.This PR:
gh pr createfailures — it branches on the outcome instead: success / "already exists" → done; "not permitted" → open an idempotent tracking issue so the bump is never lost; anything else → fail loudly.GITHUB_TOKEN, relying on the repo setting Settings → Actions → General → "Allow GitHub Actions to create and approve pull requests." This stores no token or key — nothing extra for a compromised action to exfiltrate — which is why it's preferred here over a PAT/App-token secret.versions:setvia env (defense-in-depth).docs/RELEASING.mddocumenting the setup and the twomain-ruleset caveats for merging the bump PR: re-trigger the required checks by closing/reopening the PR (the default token's PR doesn't auto-run CI), and merge via squash/merge, not rebase (the bot commit is unsigned; squash/merge produce a GitHub-signed commit). Linked fromCONTRIBUTING.mdandCHANGELOG.md.If the setting is left disabled, the job still degrades gracefully (pushes the branch, opens the tracking issue), so the bump is never silently lost.
Related Issues
Fixes #11.
How Has This Been Tested?
CI-workflow + docs change (no application code), validated statically:
release.ymlparses as YAML and thebump-versionrunblock passesbash -n(after rebasing on ci: consolidate duplicated Trivy scan+upload-SARIF steps into a composite action #87).gh pr createerror-branching was exercised against the real gh messages (already-exists / not-permitted / unexpected) — only "not permitted" falls through to the tracking issue.main-protectionruleset (required checksformat/test/frontend/trivy, signed commits, required review) — hence the close/reopen + squash-not-rebase guidance in the docs.Checklist
No automated test is added: a GitHub Actions workflow can't be exercised by the Java/JaCoCo suite. No application code changed, so existing tests are unaffected.
Additional Notes
To make it work, do one thing: enable Settings → Actions → General → Workflow permissions → "Allow GitHub Actions to create and approve pull requests." No secrets required. The bump PR still needs a human approval to merge (per the
mainruleset); seedocs/RELEASING.mdfor the close/reopen-to-run-CI and squash-not-rebase steps. If you ever want the checks to run without the manual nudge, open the PR with a GitHub App token instead — documented as an alternative, but deliberately not wired in to keep zero release secrets.