Explains the edit-in-place approach, state transition rules, worst-wins
escalation, and inline comment lifecycle. Includes a Mermaid state
diagram for visual reference.
1. First-run escalation regression (MAJOR): Add post-posting escalation
fallback. After posting APPROVED on first run, check if a sibling
from the same user has REQUEST_CHANGES — if so, mark ours as
superseded and re-post as REQUEST_CHANGES.
2. json.Marshal error handling (MINOR): Return error from EditComment
instead of ignoring it with blank identifier.
3. Redundant condition (NIT): Remove dead assignment in reviewUnchanged
where existingEvent was assigned from r.State then compared to itself.
Replace the delete-and-repost strategy with edit-in-place:
1. No existing review → POST new (first run)
2. Same state, same body → skip entirely (threads preserved)
3. Same state, body changed → PATCH body in place via timeline API
4. State change needed → PATCH old body to "Superseded", POST new
This preserves conversation threads on inline comments. Replies to
findings are never lost. The only time a new review is posted is on
first run or when the state transitions (APPROVED ↔ REQUEST_CHANGES).
New Gitea client methods:
- EditComment: PATCH /repos/{owner}/{repo}/issues/comments/{id}
- GetTimelineReviewCommentID: finds the comment ID for a review body
by scanning the issue timeline for the sentinel
Also simplifies shouldEscalate: removes the login parameter requirement
for pre-posting scenarios (uses findOwnReview to get login from existing
review instead).
Tests: findOwnReview (4 cases), EditComment (2 cases),
GetTimelineReviewCommentID (2 cases), shouldEscalate (8 cases updated).
Before posting, compare the new review body+event against the existing
review with the same sentinel. If identical, skip entirely — this
preserves conversation threads on inline comments and avoids
re-notifying reviewers for findings they already know about.
Only re-posts when findings actually change (fixed, new, or different).
Tests: 6 cases covering identical, different body, different state,
stale reviews, and different sentinels.
- Hunk headers without comma ("@@ -1 +1 @@") now parse correctly by
splitting on comma OR space instead of comma only
- Explicit skip for "\ No newline at end of file" lines (was already
safe but now documents intent)
- Tests added for both edge cases (TDD: tests written first, confirmed
failure, then fixed)
Addresses sonnet findings #1 and #2 from PR #26 review.
Findings that reference a file+line within the diff are now posted as
inline comments directly on that line, in addition to appearing in the
summary table. Findings outside the diff range stay in the body only.
Implementation:
- gitea/diff.go: ParseDiffNewLines extracts new-file line numbers from
each hunk in the unified diff
- gitea/client.go: PostReview accepts optional []ReviewComment with
path + new_position + body (omitempty when nil)
- cmd/review-bot/main.go: maps findings → inline comments when the line
exists in the diff, passes them to PostReview
Tests:
- diff parser: multi-hunk, new files, empty diff, boundary lines
- PostReview: with comments, nil comments (omitted from payload)
Moved validateReviewerName check to right after flag parsing. Previously
it ran after the LLM request completed — wasting an expensive API call
if the name was invalid.
Sonnet review finding #1.
When reviewer-name is set, prepend "# Security Review" / "# Sonnet Review"
etc. as a top-level header. Makes it immediately obvious which role each
review represents in the Gitea UI, especially when multiple reviews come
from the same bot account.
Security (MAJOR):
- Add filepath.EvalSymlinks after Clean for system-prompt-file
- Re-validate resolved path is still within workspace
- Prevents symlink → /etc/shadow exfiltration via malicious repo
Worst-wins:
- Check BEFORE posting (not after) — no delete+repost dance
- Identify sibling bots by <!-- review-bot: prefix in body
- Only escalates for bot reviews, not human REQUEST_CHANGES
- If sibling bot has REQUEST_CHANGES and we would APPROVE → post
REQUEST_CHANGES instead
Addresses security review finding #1 (MAJOR) and sonnet finding #1.
When multiple review types share a Gitea bot account, Gitea uses the
latest review to determine the user's approval state. This creates a
race: if security finds issues but code-quality finishes last with
APPROVE, the PR appears approved.
Now before posting, each job checks if any sibling review from the same
user has REQUEST_CHANGES. If so and we would post APPROVE, we downgrade
to COMMENT instead — the review is still visible but won't override
the blocking state.
Documented in README under "Shared Token: Worst-Wins."
- system-prompt-file: reject absolute paths and paths containing ".."
Prevents reading arbitrary files outside the workspace on shared runners.
- Cleanup: cross-check r.User.Login == posted.User.Login before deletion
Defense-in-depth: only attempt to delete reviews from same author.
Flagged by both sonnet and security reviewers.
- README: fix wording (cleanup happens after posting, not before)
Issues filed for deferred work:
- #24: Consistent url.PathEscape across all client endpoints
- #25: Binary signature verification for supply-chain hardening
- Quick start example with composite action + matrix strategy
- Full action inputs table with descriptions
- How sentinel-based cleanup works (explains the reviewer-name concept)
- Custom prompt file usage with security review example
- CLI usage with all flags
- Environment variables table
- Token scopes documentation
- Setup guide for new repos
Sentinel-based cleanup:
- Reviews embed <!-- review-bot:NAME --> in body (hidden HTML comment)
- Cleanup matches by sentinel, not token identity
- Each reviewer-name is a logical identity (sonnet, gpt, security)
- Same token can run multiple review types without conflict
- No extra API scopes needed
System prompt file (--system-prompt-file / SYSTEM_PROMPT_FILE):
- Loads a local file with additional review instructions
- Appended to system base as "Additional Review Instructions"
- Enables specialized reviews (security, performance, etc.)
- Partially addresses #5
Security review:
- SECURITY_REVIEW.md prompt focused on vulnerabilities
- 3rd CI matrix entry using same token, different prompt
- Focus: injection, auth, secrets, input validation, crypto, races
CI changes:
- REVIEWER_NAME passed from matrix.name
- SYSTEM_PROMPT_FILE passed from matrix (empty for standard reviews)
- 3 reviewers: sonnet (general), gpt (general), security (focused)
- PostReview now returns *Review (id + user login from response)
- Delete flow: post first, then delete stale reviews by same user
- No read:user scope needed (identity from POST response)
- Removed GetAuthenticatedUser (requires scope we lack)
- ListReviews: full pagination (loops until partial page)
- envOrDefaultBool: case-insensitive, whitespace-trimmed
- action.yml: document accepted boolean values
- Tests updated for new PostReview signature
Before posting a review, the bot now:
1. Calls GET /api/v1/user to identify its own login
2. Lists all reviews on the PR
3. Deletes any existing reviews from itself
4. Posts the fresh review
This keeps PR threads clean — one review per bot at any time.
New Gitea client methods:
- GetAuthenticatedUser() — token self-identification
- ListReviews() — fetch reviews on a PR
- DeleteReview() — delete a review by ID
Flag: --update-existing / UPDATE_EXISTING (default true)
Set to false to preserve old behavior (stack reviews).
All delete failures are non-fatal (logged as warnings).
Closes#6
- UserMeta comment: "never trimmed" → "truncated only if base exceeds budget"
- Skip diff truncation marker when diffBudget < markerBudget (prevents
marker itself from pushing EstTokens over the limit)
- Rename filepath → filePath to avoid shadowing stdlib package name
- Account for truncation marker tokens when computing diff budget
(prevents EstTokens exceeding model limit in edge cases)
- Rune-safe truncation for both UserMeta and Diff (no split multi-byte)
- Fix misleading comment (1000 chars → ~1000 tokens/4000 chars)
- Extract marker strings as constants
- Add unit tests for BuildSystemBase and BuildUserMeta
Addresses review findings:
- Replace map-based model limits with ordered slice (longest-prefix-first)
for deterministic matching
- Truncate UserMeta when base content alone exceeds budget (keeps first
4000 chars + truncation marker)
- Remove hard minimum of 1000 tokens for diff budget — use 0 as floor
to guarantee total never exceeds limit
- Handle zero-budget edge case (diff replaced with manual-review message)
- Add tests: huge UserMeta, all-sections-huge never exceeds limit
Adds a budget package that estimates token usage and progressively
trims context to fit within model-specific limits.
Trim order (least important first):
1. Language patterns
2. Repository conventions
3. Full file context
4. Diff (truncated as last resort)
When content is trimmed, a note is appended to the user prompt so
the LLM knows context was reduced.
- New budget package with Fit(), EstimateTokens(), LimitForModel()
- Model limit table (GPT-4.1: 128K, GPT-5: 200K, Claude: 200K)
- Refactored review/prompt.go: BuildSystemBase() and BuildUserMeta()
extract non-trimmable content; old functions delegate to new ones
- main.go uses budget.Fit() instead of direct prompt assembly
- 7 unit tests covering all trim paths
Closes#19
- Add --version flag and log version on startup (closes#9)
- URL-escape ref query parameter in GetFileContentRef (closes#7)
- Add go vet to release workflow (closes#13)
Renamed local url variable to reqURL to avoid shadowing net/url package.
- Overall context timeout now derived from LLM timeout + 1 minute
(no longer hardcoded 3min that could conflict with longer LLM timeouts)
- Clarify concurrency docs: With* methods are setup-only, not concurrent
- Add ctx.Err() checks in fetchFileContext and fetchPatterns loops
(break early on cancellation instead of making unnecessary requests)
- Fix doc comments: WithTimeout and WithTemperature each get their own
- Add TestWithTimeout (verifies short timeout causes request failure)
- Log warning on directory recursion failure in GetAllFilesInPath
- Note: unexported fields is a breaking change, will document in release notes
New flag: --llm-timeout / LLM_TIMEOUT (seconds, default 300)
New builder: llmClient.WithTimeout(duration)
Composite action: new timeout input
Keeps 5 minutes as the sensible default but allows tuning for
larger repos or slower models.
- GetAllFilesInPath: log.Printf when file fetch or dir recursion fails
- integration_test: use strings.SplitN for owner/repo parsing (idiomatic)
Addresses GPT review findings #1, #2.
REVIEW.md findings 1-4, 14:
- All Gitea client methods now accept context.Context as first param
- All LLM client methods now accept context.Context as first param
- Use http.NewRequestWithContext for cancellation/timeout support
- Main uses 3-minute timeout context for all operations
- Unexport Client struct fields (baseURL, token, apiKey, etc.)
- Use bytes.NewReader instead of strings.NewReader(string(...))
patterns-repo now accepts a comma-separated list of repos:
PATTERNS_REPO="rodin/elixir-patterns,rodin/phoenix-conventions"
patterns-files accepts files AND directories:
PATTERNS_FILES="README.md,docs/"
When a path is a directory, all files within it are fetched
recursively via the Gitea contents API. Only .md, .txt, .yml,
and .yaml files are included as pattern content.
New API methods:
- ListContents: list files/dirs at a path via contents API
- GetAllFilesInPath: recursively fetch all file contents
This allows a single review action to pull idioms from multiple
pattern repos (e.g. elixir-patterns + phoenix-conventions) and
include entire directories of documentation as review criteria.
Major improvements to review quality:
1. Full file context: fetch complete content of all modified files from
the PR branch and include as reference. This eliminates false-positive
"missing import" findings since the model sees the entire file.
2. Patterns repo: new --patterns-repo / PATTERNS_REPO flag fetches
language idiom files from a separate Gitea repo (e.g. rodin/elixir-patterns)
and includes them as review criteria.
3. Multi-file patterns: --patterns-files / PATTERNS_FILES accepts
comma-separated file paths to fetch from the patterns repo.
New API methods:
- GetPullRequestFiles: list changed files in a PR
- GetFileContentRef: fetch file content from a specific branch/ref
Prompt changes:
- BuildSystemPrompt now accepts (conventions, patterns)
- BuildUserPrompt now accepts fileContext parameter
- File context displayed before diff for model reference
- Patterns presented as "review criteria" in system prompt
Composite action updated with patterns-repo and patterns-files inputs.
The LLM was treating the diff as complete file context and flagging
"missing imports" for symbols that exist in unchanged portions of
the file. Added explicit instructions to the system prompt:
- Clarify that the diff is partial; unchanged code already exists
- Explicitly instruct: do not flag missing imports/types unless the
diff introduces a new usage without a corresponding addition
- Add rule: never flag undefined symbols that could exist in the
unchanged portions
- Add temperature range validation (must be 0-2, fatal on invalid)
- release.yml: use python3 for robust JSON parsing instead of sed
- Composite action: add header comment confirming Gitea Actions compat
- All findings from review #385 addressed
Rodin
aweiker
rodin