fix: remove worst-wins escalation logic #31

2026-05-02T14:07:15Z

rodin commented

2026-05-02 14:07:15 +00:00

Closes #28
Closes #29

Summary

Removes the entire worst-wins escalation system (shouldEscalate, pre-posting check, post-posting re-escalation). With separate tokens per reviewer role (security-review-bot user now exists), Gitea handles multi-reviewer blocking natively.

Changes

Remove shouldEscalate() function and all calls
Remove pre-posting escalation check
Remove post-posting escalation block (re-fetch + supersede + repost)
Remove TestShouldEscalate (8 test cases)
Keep: findOwnReview, reviewUnchanged, update-in-place/supersede logic

Deployment Note

After merging, CI workflow needs updating to use SECURITY_REVIEW_TOKEN pointing to the new security-review-bot Gitea user (already created with write access to grgl/gargoyle and rodin/review-bot).

Closes #28 Closes #29 ## Summary Removes the entire worst-wins escalation system (`shouldEscalate`, pre-posting check, post-posting re-escalation). With separate tokens per reviewer role (security-review-bot user now exists), Gitea handles multi-reviewer blocking natively. ## Changes - Remove `shouldEscalate()` function and all calls - Remove pre-posting escalation check - Remove post-posting escalation block (re-fetch + supersede + repost) - Remove `TestShouldEscalate` (8 test cases) - Keep: `findOwnReview`, `reviewUnchanged`, update-in-place/supersede logic ## Deployment Note After merging, CI workflow needs updating to use `SECURITY_REVIEW_TOKEN` pointing to the new `security-review-bot` Gitea user (already created with write access to grgl/gargoyle and rodin/review-bot).

rodin added 1 commit 2026-05-02 14:07:16 +00:00

fix: remove worst-wins escalation logic (#28 )

CI / test (pull_request) Successful in 14s

Details

CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s

Details

CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 36s

Details

CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s

Details

b78d9972ac

gpt-review-bot approved these changes 2026-05-02 14:07:53 +00:00

gpt-review-bot left a comment

First-time contributor

Gpt Review

Summary

The PR removes all worst-wins escalation logic in favor of multi-user reviewer separation and corrects the security reviewer token in CI. All changes are correct, safe, and remove significant complexity. CI passes and all relevant tests are updated.

Recommendation

APPROVE — The removal of shouldEscalate and related logic is clean, affecting only intended code paths. The update to SECURITY_REVIEW_TOKEN in the workflow is correct. All supporting tests for escalation logic have been removed, and existing positive tests for update-in-place, shared-token protection, and sentinel extraction are intact. With CI passing and no style or correctness issues identified, this PR is ready to merge.

Review by gpt

# Gpt Review ## Summary The PR removes all worst-wins escalation logic in favor of multi-user reviewer separation and corrects the security reviewer token in CI. All changes are correct, safe, and remove significant complexity. CI passes and all relevant tests are updated. ## Recommendation **APPROVE** — The removal of shouldEscalate and related logic is clean, affecting only intended code paths. The update to SECURITY_REVIEW_TOKEN in the workflow is correct. All supporting tests for escalation logic have been removed, and existing positive tests for update-in-place, shared-token protection, and sentinel extraction are intact. With CI passing and no style or correctness issues identified, this PR is ready to merge. --- *Review by gpt*

sonnet-review-bot approved these changes 2026-05-02 14:08:07 +00:00

Dismissed

sonnet-review-bot left a comment

First-time contributor

Security Review

Summary

Security posture is preserved or improved by removing the escalation logic and adding a benign misconfiguration warning. No new injection vectors, path traversal, secrets exposure, or authorization flaws are introduced, and CI has passed.

Recommendation

APPROVE — The changes remove complex escalation behavior and replace it with a simple warning for shared-token misconfiguration, relying on Gitea’s native multi-reviewer blocking. Input validation for reviewer names remains in place to protect the sentinel, and path handling for the system prompt file continues to guard against traversal and symlink attacks. Given CI passed and no security-relevant issues are introduced by this diff, proceed with merge. Ensure deployment updates the workflow to use the new SECURITY_REVIEW_TOKEN as noted.

Review by security

# Security Review ## Summary Security posture is preserved or improved by removing the escalation logic and adding a benign misconfiguration warning. No new injection vectors, path traversal, secrets exposure, or authorization flaws are introduced, and CI has passed. ## Recommendation **APPROVE** — The changes remove complex escalation behavior and replace it with a simple warning for shared-token misconfiguration, relying on Gitea’s native multi-reviewer blocking. Input validation for reviewer names remains in place to protect the sentinel, and path handling for the system prompt file continues to guard against traversal and symlink attacks. Given CI passed and no security-relevant issues are introduced by this diff, proceed with merge. Ensure deployment updates the workflow to use the new SECURITY_REVIEW_TOKEN as noted. --- *Review by security*

sonnet-review-bot approved these changes 2026-05-02 14:08:10 +00:00

Dismissed

sonnet-review-bot left a comment

First-time contributor

Sonnet Review

Summary

The PR cleanly removes the worst-wins escalation logic and replaces it with a safer warning mechanism for shared tokens. Tests were updated accordingly and CI is green. The remaining review update logic (findOwnReview, reviewUnchanged, supersede/update-in-place) remains intact and consistent.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	`cmd/review-bot/main.go`	504	warnSharedToken does not skip stale reviews when detecting shared-token usage; this could produce a warning based on an outdated sibling review. Consider ignoring r.Stale to reduce false positives.
2	[NIT]	`cmd/review-bot/main.go`	512	The sentinel prefix "<!-- review-bot:" is repeated across functions (warnSharedToken, extractSentinelName). Consider extracting a shared constant to avoid typos and ease future changes.
3	[NIT]	`cmd/review-bot/main.go`	504	Log message uses a Unicode em dash (—). While generally fine, some log aggregation systems may not preserve it. Consider using a standard hyphen-minus for maximum compatibility.

Recommendation

APPROVE — This change aligns with the goal of relying on separate reviewer identities rather than in-process escalation, simplifying logic and avoiding surprising state changes post-post. The new warnSharedToken and extractSentinelName helpers are straightforward and covered by tests. I recommend merging as-is. Optionally, refine warnSharedToken to ignore stale reviews and consolidate the sentinel prefix into a shared constant for maintainability.

Review by sonnet

# Sonnet Review ## Summary The PR cleanly removes the worst-wins escalation logic and replaces it with a safer warning mechanism for shared tokens. Tests were updated accordingly and CI is green. The remaining review update logic (findOwnReview, reviewUnchanged, supersede/update-in-place) remains intact and consistent. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 504 | warnSharedToken does not skip stale reviews when detecting shared-token usage; this could produce a warning based on an outdated sibling review. Consider ignoring r.Stale to reduce false positives. | | 2 | [NIT] | `cmd/review-bot/main.go` | 512 | The sentinel prefix "

aweiker requested changes 2026-05-02 14:09:30 +00:00

Dismissed

aweiker left a comment

what happens when someone doesn't follow instructions and uses the same token twice? can we detect and fallback into a safe mode?

rodin added 1 commit 2026-05-02 14:12:12 +00:00

fix: detect shared-token misconfiguration and warn

CI / test (pull_request) Successful in 14s

Details

CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 24s

Details

CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 42s

Details

CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 55s

Details

fd179b891b

When two review-bot roles share the same Gitea user token (misconfiguration),
log a WARNING identifying which sibling is sharing. The bot continues normally
with its own honest verdict — no escalation, no deadlock. Operators see the
warning in CI logs and can fix the token setup.

Addresses Aaron's review feedback on #28: graceful degradation when someone
doesn't follow the separate-token deployment instructions.

rodin commented

2026-05-02 14:12:19 +00:00

Good catch. Added warnSharedToken() — if two review-bot roles post under the same Gitea user, it logs:

WARNING: shared token detected — another review-bot role (security) is using the same Gitea user "sonnet-review-bot". Each role should have its own token/user for proper multi-reviewer blocking.

Behavior on misconfiguration:

Each role still posts its own honest verdict (no escalation, no deadlock)
Warning is visible in CI logs so operators know to fix it
No crash, no silent corruption

Safe degradation: worst case is two reviews from the same user (Gitea only enforces the last one posted), which is the same behavior as today minus the deadlock risk.

Good catch. Added `warnSharedToken()` — if two review-bot roles post under the same Gitea user, it logs: ``` WARNING: shared token detected — another review-bot role (security) is using the same Gitea user "sonnet-review-bot". Each role should have its own token/user for proper multi-reviewer blocking. ``` Behavior on misconfiguration: - Each role still posts its own honest verdict (no escalation, no deadlock) - Warning is visible in CI logs so operators know to fix it - No crash, no silent corruption Safe degradation: worst case is two reviews from the same user (Gitea only enforces the last one posted), which is the same behavior as today minus the deadlock risk.

aweiker reviewed 2026-05-02 14:15:04 +00:00

cmd/review-bot/main.go Outdated

						
				@@ -269,2 +269,2 @@

							// Worst-wins: escalate if a sibling blocks (need own login from existing review)

							ownLogin := ""

							// Detect shared-token misconfiguration: warn if sibling sentinel exists from same user

							warnSharedToken(existingReviews, sentinel)

aweiker commented

2026-05-02 14:15:04 +00:00

this should also make it fall into a mode where it only writes new reviews; never updates any review as we don't want to do that logic. What about review state? That might get clobbered too.

aweiker requested changes 2026-05-02 14:16:31 +00:00

Dismissed

aweiker left a comment

No new tests were added for detecting the same token was used.

rodin added 1 commit 2026-05-02 14:21:51 +00:00

fix: skip update-in-place when shared token detected

CI / test (pull_request) Successful in 14s

Details

CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 22s

Details

CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s

Details

CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m21s

Details

b1f5dd4b5f

When hasSharedToken() detects two roles sharing the same Gitea user,
the bot now skips ALL update logic (PATCH, supersede) and always POSTs
a fresh review. This prevents clobbering a sibling's review body or
state when misconfigured.

Tests now assert return values (true/false) rather than just verifying
no panic. Added additional test case for three-roles-same-user scenario.

Addresses review feedback: update logic and review state must not
interact with sibling reviews under the same user.

rodin commented

2026-05-02 14:21:58 +00:00

Addressed both findings:

Shared-token mode now skips ALL update logic — no PATCH, no supersede, just always POST fresh. This prevents clobbering a sibling's review body or state. The function is now hasSharedToken() returning bool; when true, the entire update-in-place branch is bypassed.
Tests assert actual return values — TestHasSharedToken checks true/false for each scenario (no reviews, no own review, separate users, shared token, three-roles-same-user). Plus TestExtractSentinelName covers the helper.

Addressed both findings: 1. **Shared-token mode now skips ALL update logic** — no PATCH, no supersede, just always POST fresh. This prevents clobbering a sibling's review body or state. The function is now `hasSharedToken()` returning bool; when true, the entire update-in-place branch is bypassed. 2. **Tests assert actual return values** — `TestHasSharedToken` checks `true`/`false` for each scenario (no reviews, no own review, separate users, shared token, three-roles-same-user). Plus `TestExtractSentinelName` covers the helper.

sonnet-review-bot approved these changes 2026-05-02 14:22:44 +00:00

sonnet-review-bot left a comment

First-time contributor

Security Review

Summary

The diff removes the escalation logic and adds shared-token detection without introducing security vulnerabilities. CI is passing and the changes do not affect input validation, secrets handling, or introduce injection or auth issues.

Recommendation

APPROVE — Proceed with merge. The removal of worst-wins escalation is safe given separate tokens per reviewer role and Gitea’s native handling. The new shared-token detection mitigates misconfiguration risks without exposing sensitive data. Ensure deployment notes are followed to use distinct tokens (e.g., SECURITY_REVIEW_TOKEN for the security-review-bot) so multi-reviewer blocking works as intended.

Review by security

# Security Review ## Summary The diff removes the escalation logic and adds shared-token detection without introducing security vulnerabilities. CI is passing and the changes do not affect input validation, secrets handling, or introduce injection or auth issues. ## Recommendation **APPROVE** — Proceed with merge. The removal of worst-wins escalation is safe given separate tokens per reviewer role and Gitea’s native handling. The new shared-token detection mitigates misconfiguration risks without exposing sensitive data. Ensure deployment notes are followed to use distinct tokens (e.g., SECURITY_REVIEW_TOKEN for the security-review-bot) so multi-reviewer blocking works as intended. --- *Review by security*

sonnet-review-bot approved these changes 2026-05-02 14:23:26 +00:00

sonnet-review-bot left a comment

First-time contributor

Sonnet Review

Summary

The changes cleanly remove the worst-wins escalation logic and replace it with a simpler, safer flow that relies on separate Gitea users per reviewer role. New shared-token detection is sensible, tests are updated accordingly, and CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	`cmd/review-bot/main.go`	269	When shared-token mode is detected, the code skips all update-in-place logic but also skips checking reviewUnchanged. This can lead to duplicate identical reviews being posted under the same login and sentinel. Consider still performing a reviewUnchanged check and returning early to avoid re-posting in this misconfiguration scenario.

Recommendation

APPROVE — Overall the removal of the escalation path is coherent with the new design relying on distinct Gitea users per reviewer role. The shared-token detection helper is a pragmatic safeguard to avoid clobbering another role’s review. As a small improvement, in the shared-token branch you could still check for an unchanged review and return early to prevent duplicate posts. Otherwise, the code is idiomatic, tests reflect the new behavior, and CI is green. I recommend approval.

Review by sonnet

# Sonnet Review ## Summary The changes cleanly remove the worst-wins escalation logic and replace it with a simpler, safer flow that relies on separate Gitea users per reviewer role. New shared-token detection is sensible, tests are updated accordingly, and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 269 | When shared-token mode is detected, the code skips all update-in-place logic but also skips checking reviewUnchanged. This can lead to duplicate identical reviews being posted under the same login and sentinel. Consider still performing a reviewUnchanged check and returning early to avoid re-posting in this misconfiguration scenario. | ## Recommendation **APPROVE** — Overall the removal of the escalation path is coherent with the new design relying on distinct Gitea users per reviewer role. The shared-token detection helper is a pragmatic safeguard to avoid clobbering another role’s review. As a small improvement, in the shared-token branch you could still check for an unchanged review and return early to prevent duplicate posts. Otherwise, the code is idiomatic, tests reflect the new behavior, and CI is green. I recommend approval. --- *Review by sonnet*

cmd/review-bot/main.go

						
				@@ -276,3 +269,1 @@

								log.Printf("Sibling review has REQUEST_CHANGES; escalating to REQUEST_CHANGES")

								event = "REQUEST_CHANGES"

							}

							// Detect shared-token misconfiguration: if detected, skip all

sonnet-review-bot commented

2026-05-02 14:23:26 +00:00

First-time contributor

[MINOR] When shared-token mode is detected, the code skips all update-in-place logic but also skips checking reviewUnchanged. This can lead to duplicate identical reviews being posted under the same login and sentinel. Consider still performing a reviewUnchanged check and returning early to avoid re-posting in this misconfiguration scenario.

**[MINOR]** When shared-token mode is detected, the code skips all update-in-place logic but also skips checking reviewUnchanged. This can lead to duplicate identical reviews being posted under the same login and sentinel. Consider still performing a reviewUnchanged check and returning early to avoid re-posting in this misconfiguration scenario.

aweiker dismissed aweiker's review 2026-05-02 14:24:11 +00:00

rodin added 1 commit 2026-05-02 14:25:46 +00:00

ci: use separate SECURITY_REVIEW_TOKEN for security reviewer

CI / test (pull_request) Successful in 13s

Details

CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s

Details

CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 41s

Details

CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 58s

Details

ceefa4c2e0

The security-review-bot Gitea user now has its own token. This
completes the token separation so each reviewer role posts under
its own identity, enabling native Gitea multi-reviewer blocking.

security-review-bot approved these changes 2026-05-02 14:26:43 +00:00

security-review-bot left a comment

Security Review

Summary

The changes remove the worst-wins escalation logic and adjust the CI secret for the security reviewer, replacing it with a safer shared-token detection that avoids clobbering reviews. No new security risks are introduced and existing input validations (e.g., reviewer-name, system prompt path checks) remain robust.

Recommendation

APPROVE — Proceed with merging. The removal of escalation logic simplifies behavior without weakening security, and the added shared-token detection helps prevent unintended review edits in misconfiguration scenarios. Ensure the CI environment is updated to provide SECURITY_REVIEW_TOKEN as noted in the deployment instructions.

Review by security

# Security Review ## Summary The changes remove the worst-wins escalation logic and adjust the CI secret for the security reviewer, replacing it with a safer shared-token detection that avoids clobbering reviews. No new security risks are introduced and existing input validations (e.g., reviewer-name, system prompt path checks) remain robust. ## Recommendation **APPROVE** — Proceed with merging. The removal of escalation logic simplifies behavior without weakening security, and the added shared-token detection helps prevent unintended review edits in misconfiguration scenarios. Ensure the CI environment is updated to provide SECURITY_REVIEW_TOKEN as noted in the deployment instructions. --- *Review by security*

sonnet-review-bot approved these changes 2026-05-02 14:27:00 +00:00

sonnet-review-bot left a comment

First-time contributor

Sonnet Review

Summary

The changes cleanly remove the worst-wins escalation logic and replace it with a simpler, safer shared-token detection, aligning with the new per-role token model. Tests and CI config were updated accordingly, and CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	`cmd/review-bot/main.go`	494	hasSharedToken does not filter out stale reviews when detecting shared-token misuse. If a stale sibling review from a prior misconfiguration remains under the same user, this may trigger a false positive and permanently skip update-in-place logic even after the configuration is fixed. Consider ignoring r.Stale in the detection loop.

Recommendation

APPROVE — The removal of pre/post escalation logic is coherent with the new design where each reviewer role uses a distinct Gitea user, allowing native multi-reviewer blocking. The update-in-place logic remains intact and guarded by a sensible shared-token detection. Tests have been adjusted and expanded to cover the new helpers, and CI passed. As a minor improvement, consider filtering out stale reviews in hasSharedToken to avoid false positives when old reviews from a previous shared-token misconfiguration linger. Otherwise, the PR looks solid and ready to merge.

Review by sonnet

# Sonnet Review ## Summary The changes cleanly remove the worst-wins escalation logic and replace it with a simpler, safer shared-token detection, aligning with the new per-role token model. Tests and CI config were updated accordingly, and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 494 | hasSharedToken does not filter out stale reviews when detecting shared-token misuse. If a stale sibling review from a prior misconfiguration remains under the same user, this may trigger a false positive and permanently skip update-in-place logic even after the configuration is fixed. Consider ignoring r.Stale in the detection loop. | ## Recommendation **APPROVE** — The removal of pre/post escalation logic is coherent with the new design where each reviewer role uses a distinct Gitea user, allowing native multi-reviewer blocking. The update-in-place logic remains intact and guarded by a sensible shared-token detection. Tests have been adjusted and expanded to cover the new helpers, and CI passed. As a minor improvement, consider filtering out stale reviews in hasSharedToken to avoid false positives when old reviews from a previous shared-token misconfiguration linger. Otherwise, the PR looks solid and ready to merge. --- *Review by sonnet*

cmd/review-bot/main.go

						
				@@ -539,6 +494,46 @@ func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string)

					return false

sonnet-review-bot commented

2026-05-02 14:27:00 +00:00

First-time contributor

[MINOR] hasSharedToken does not filter out stale reviews when detecting shared-token misuse. If a stale sibling review from a prior misconfiguration remains under the same user, this may trigger a false positive and permanently skip update-in-place logic even after the configuration is fixed. Consider ignoring r.Stale in the detection loop.

**[MINOR]** hasSharedToken does not filter out stale reviews when detecting shared-token misuse. If a stale sibling review from a prior misconfiguration remains under the same user, this may trigger a false positive and permanently skip update-in-place logic even after the configuration is fixed. Consider ignoring r.Stale in the detection loop.

rodin merged commit b51a19d8b9 into main

2026-05-02 16:46:06 +00:00

rodin deleted branch fix/28-remove-escalation

2026-05-02 16:46:06 +00:00

Sign in to join this conversation.

No Reviewers

gpt-review-bot

aweiker

security-review-bot

sonnet-review-bot

5 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: rodin/review-bot#31

				`@@ -539,6 +494,46 @@ func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string)`
				`return false`