[webhooks] add module#33
Conversation
|
Warning Review limit reached
More reviews will be available in 56 minutes and 21 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughAdds webhook configuration and bot-user seeding, lookup methods for projects, tasks, and users, and a Bitbucket push webhook flow that parses commits, verifies signatures, processes task references, and exposes the endpoint through server wiring, docs, and examples. ChangesWebhook support plumbing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/db/migrations/20260625000000_create_bot_user.sql (1)
20-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ON DUPLICATE KEY UPDATEomitsupdated_at.On re-runs the row's
name/role/statusare refreshed butupdated_atstays stale, so it won't reflect the seed change. Minor, but easy to include.♻️ Optionally refresh updated_at
) ON DUPLICATE KEY UPDATE name = VALUES(name), role = VALUES(role), status = -VALUES(status); +VALUES(status), + updated_at = NOW();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/migrations/20260625000000_create_bot_user.sql` around lines 20 - 26, The `ON DUPLICATE KEY UPDATE` clause in the bot user seed migration only refreshes `name`, `role`, and `status`, leaving `updated_at` stale on re-runs. Update the duplicate-key path in the migration so the existing row also gets a fresh `updated_at` value, alongside the current fields, in the same insert/upsert block.internal/projects/service.go (1)
102-104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider validating empty
repoURLfor consistency.
Service.Existsrejects an empty slug withErrValidationFailed, butFindByRepoURLpasses any value straight through. An emptyrepoURLwould match a project whoserepo_urlis empty, which is likely unintended. A guard here keeps validation behavior consistent across the service.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/projects/service.go` around lines 102 - 104, Add input validation to Service.FindByRepoURL so it rejects an empty repoURL with ErrValidationFailed before calling s.projects.FindByRepoURL, matching the validation behavior used by Service.Exists. Use the FindByRepoURL method in Service as the place to add the guard, and keep the existing delegate call unchanged for non-empty values.internal/projects/repository.go (1)
93-96: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider a functional index for the case-insensitive lookup.
WHERE LOWER(repo_url) = ?will not use a plain index onrepo_urland forces a full scan as the projects table grows. If this lookup is on a webhook hot path, add a functional index (e.g.CREATE INDEX ... ON projects (LOWER(repo_url))) to keep it performant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/projects/repository.go` around lines 93 - 96, The case-insensitive repo lookup in the repository query uses LOWER(repo_url), which won’t benefit from a normal index and can become slow as the projects table grows. Update the schema/migration for the projects table to add a functional index on LOWER(repo_url), and ensure the lookup in the repository query path that uses NewSelect().Model(&model).Where("LOWER(repo_url) = ?", strings.ToLower(repoURL)) can use that index. Keep the existing query shape, but make the indexed expression match the filter exactly.internal/webhooks/domain.go (1)
44-61: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist keyword registry to package scope.
keywordActionsand the verb consts are reconstructed on everyParseCommitMessagecall. Since they are constant, declaring them once at package level avoids per-call allocation and keeps the function focused on parsing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/webhooks/domain.go` around lines 44 - 61, Hoist the keyword registry and verb constants out of ParseCommitMessage-related scope into package-level declarations in domain.go, so they are initialized once instead of rebuilt per call. Keep the existing symbol names like keywordActions, verbResolved, verbClosed, and verbOnHold, and update the parser code to reference these package-scope values without changing the matching behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/db/migrations/20260625000000_create_bot_user.sql`:
- Around line 20-26: The `ON DUPLICATE KEY UPDATE` clause in the bot user seed
migration only refreshes `name`, `role`, and `status`, leaving `updated_at`
stale on re-runs. Update the duplicate-key path in the migration so the existing
row also gets a fresh `updated_at` value, alongside the current fields, in the
same insert/upsert block.
In `@internal/projects/repository.go`:
- Around line 93-96: The case-insensitive repo lookup in the repository query
uses LOWER(repo_url), which won’t benefit from a normal index and can become
slow as the projects table grows. Update the schema/migration for the projects
table to add a functional index on LOWER(repo_url), and ensure the lookup in the
repository query path that uses NewSelect().Model(&model).Where("LOWER(repo_url)
= ?", strings.ToLower(repoURL)) can use that index. Keep the existing query
shape, but make the indexed expression match the filter exactly.
In `@internal/projects/service.go`:
- Around line 102-104: Add input validation to Service.FindByRepoURL so it
rejects an empty repoURL with ErrValidationFailed before calling
s.projects.FindByRepoURL, matching the validation behavior used by
Service.Exists. Use the FindByRepoURL method in Service as the place to add the
guard, and keep the existing delegate call unchanged for non-empty values.
In `@internal/webhooks/domain.go`:
- Around line 44-61: Hoist the keyword registry and verb constants out of
ParseCommitMessage-related scope into package-level declarations in domain.go,
so they are initialized once instead of rebuilt per call. Keep the existing
symbol names like keywordActions, verbResolved, verbClosed, and verbOnHold, and
update the parser code to reference these package-scope values without changing
the matching behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85869105-80ee-4f27-9e95-672ead3efb84
📒 Files selected for processing (9)
internal/config/config.gointernal/config/module.gointernal/db/migrations/20260625000000_create_bot_user.sqlinternal/projects/repository.gointernal/projects/service.gointernal/tasks/repository.gointernal/tasks/service.gointernal/webhooks/config.gointernal/webhooks/domain.go
🤖 Pull request artifacts
|
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/webhooks/domain.go`:
- Around line 160-164: The firstLine helper currently uses strings.Index with
slicing, which triggers the stringscut lint. Update firstLine in domain.go to
use strings.Cut to split msg at the first newline, then return the trimmed
prefix from that split; keep the existing trimming behavior and preserve the
same function signature.
In `@internal/webhooks/service.go`:
- Around line 95-101: The webhook handler in the FindByRepoURL path is treating
every lookup error as a missing project, which can incorrectly return success
for transient failures. Update the error handling around
s.projectsSvc.FindByRepoURL in the service method so only a genuine not-found
case is logged as “no project found” and allows the current 202 behavior; for
any other error, log it as a failure and return an error/failed response so the
webhook is retried.
- Around line 240-244: The shortHash helper uses a hardcoded truncation length
that triggers the mnd lint and hides intent. Replace the literal length in
shortHash with a named constant defined near the helper in service.go, and use
that constant for both the length check and slice so the 7-character truncation
is explicit and easy to maintain.
- Around line 60-63: The VerifyPushEvent method in Service currently fails open
by returning nil when s.secret is empty, which allows unauthenticated webhook
writes if configuration is missing. Update VerifyPushEvent to reject requests
when the secret is unset by returning an error instead of success, and ensure
the webhook handling path that calls VerifyPushEvent treats this as a hard
failure rather than proceeding with task status updates or comment creation.
- Line 184: The partial TaskUpdate literal in tasks.TaskUpdate initialization
triggers exhaustruct, so update the TaskUpdate construction in the webhook
service to initialize every field explicitly. Use the TaskUpdate type in the
webhook handling path and fill in all remaining fields with their appropriate
zero values or data from the webhook payload, while keeping Status set from
ref.Action.Status.
- Around line 65-80: Update the signature verification path in the webhooks
service so all bad-signature branches return errors wrapped with
ErrInvalidSignature instead of plain fmt.Errorf values. In the signature check
logic that handles the sha256 prefix, hex decoding, and hmac.Equal comparison,
wrap each invalid signature failure with ErrInvalidSignature so handler.go’s
errors.Is(err, webhooks.ErrInvalidSignature) matches and maps these cases to
401.
- Around line 93-101: Normalize the repository URL used in the webhook project
lookup so it matches the stored Bitbucket repo URL format. In the `Handle` flow
where `repoURL` is built before calling `projectsSvc.FindByRepoURL`, remove the
trailing slash and ensure the URL ends with `.git` before querying. Use the
existing `repoFullName` construction point to apply the normalization so valid
Bitbucket webhooks resolve correctly.
In `@requests.http`:
- Around line 421-423: The Bitbucket webhook examples are missing the required
signature header, so they don’t reflect how `webhooks/bitbucket/push` is
actually verified. Update the request examples in `requests.http` to include a
placeholder `X-Hub-Signature-256` header alongside the existing content type,
and apply the same change to the other Bitbucket webhook example block
referenced in the comment so both examples show signed requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86381d33-b688-4791-9c92-206e3995175b
📒 Files selected for processing (10)
internal/commands/serve/serve.gointernal/server/docs/docs.gointernal/server/module.gointernal/server/webhooks/dto.gointernal/server/webhooks/handler.gointernal/users/service.gointernal/webhooks/domain.gointernal/webhooks/module.gointernal/webhooks/service.gorequests.http
✅ Files skipped from review due to trivial changes (2)
- internal/server/webhooks/dto.go
- internal/server/docs/docs.go
capcom6
force-pushed
the
webhooks/add-support
branch
from
70bc9d0 to
3d16c47
Compare
capcom6
added
ready
1 participant
Summary by CodeRabbit
POST /webhooks/bitbucket/push) with request signature verification and automated processing of referenced tasks from commit messages.