diff --git a/offline/README.md b/offline/README.md index 78b2815..c0e10d4 100644 --- a/offline/README.md +++ b/offline/README.md @@ -24,6 +24,7 @@ Open replication of the code review benchmark used by companies like [Augment](h | [Kodus](https://kodus.io/) | AI code review | | [Macroscope](https://www.macroscope.com/) | AI code review | | [Qodo](https://www.qodo.ai/) | AI code review | +| [Shipwright](https://github.com/kpuru88/shipwright-agent) | AI code review agent | | [Sourcery](https://sourcery.ai/) | AI code review | Adding a new tool requires forking the benchmark PRs and collecting the tool's reviews — see Steps 0 and 1 below. diff --git a/offline/results/benchmark_data.json b/offline/results/benchmark_data.json index f6a1028..736d2c3 100644 --- a/offline/results/benchmark_data.json +++ b/offline/results/benchmark_data.json @@ -1789,6 +1789,31 @@ "created_at": "2026-06-28T23:12:51Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "keycloak__keycloak__pull__37429__shipwright__4701de31", + "pr_url": "https://github.com/keycloak/keycloak/pull/37429", + "review_comments": [ + { + "path": "themes/src/main/resources-community/theme/base/account/messages/messages_lt.properties", + "line": 101, + "body": "Lithuanian account translation for 'totpStep1' was replaced with Italian text ('Installa una delle seguenti applicazioni sul tuo cellulare:') instead of a proper Lithuanian translation, so Lithuanian-locale users will see Italian text in the account TOTP setup step. Location: themes/src/main/resources-community/theme/base/account/messages/messages_lt.properties:101", + "created_at": null + }, + { + "path": "themes/src/main/resources-community/theme/base/login/messages/messages_lt.properties", + "line": 71, + "body": "Lithuanian login translation for 'loginTotpStep1' was replaced with the same Italian text ('Installa una delle seguenti applicazioni sul tuo cellulare:') instead of a Lithuanian translation, causing Italian text to appear on the login TOTP configuration screen for Lithuanian users. Location: themes/src/main/resources-community/theme/base/login/messages/messages_lt.properties:71", + "created_at": null + }, + { + "path": "themes/src/main/resources-community/theme/base/account/messages/messages_zh_CN.properties", + "line": 112, + "body": "The Simplified Chinese (zh_CN) 'totpStep1' string was replaced with text using Traditional Chinese characters (手機, 安裝, 應用程式) instead of Simplified Chinese (手机, 安装, 应用程序), causing a locale mismatch for zh_CN users. Location: themes/src/main/resources-community/theme/base/account/messages/messages_zh_CN.properties:112", + "created_at": null + } + ] } ] }, @@ -3247,6 +3272,31 @@ "created_at": "2026-06-28T21:20:12Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "keycloak__keycloak__pull__37634__shipwright__4701de31", + "pr_url": "https://github.com/keycloak/keycloak/pull/37634", + "review_comments": [ + { + "path": "services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java", + "line": 73, + "body": "In the AccessTokenContext constructor, the null-check for rawTokenId is a copy-paste bug: `Objects.requireNonNull(grantType, \"Null rawTokenId not allowed\")` re-checks grantType (already checked on the previous line) instead of rawTokenId, and uses a misleading message. As a result, a null rawTokenId is never rejected; it is silently stored and later concatenated into the token id string in encodeTokenId (`... + ':' + tokenContext.getRawTokenId()`), producing a malformed token id like \"onltac:null\" instead of failing fast. Location: services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java:73", + "created_at": null + }, + { + "path": "testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java", + "line": 479, + "body": "The new isAccessTokenId matcher has both wrong substring bounds and inverted boolean logic. The encoded context is 6 chars: sessionType(0-2)+tokenType(2-4)+grantType(4-6), so the grant shortcut must be extracted with substring(4,6), not substring(3,5). Additionally the condition `if (items[0].substring(3, 5).equals(expectedGrantShortcut)) return false;` returns false (no match) exactly when the shortcut equals the expected value, and falls through to the UUID check (returning true) when it does NOT match. This inverts the intended semantics of the matcher used by expectCodeToToken, expectDeviceCodeToToken, expectRefresh and expectAuthReqIdToToken, causing correctly-encoded token ids to fail matching and incorrectly-encoded ones to potentially pass. Location: testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java:479", + "created_at": null + }, + { + "path": "server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java", + "line": 31, + "body": "The new getShortcut() Javadoc states shortcuts are \"usually like 3-letters shortcut\", but the encoding implementation (DefaultTokenContextEncoderProvider) strictly requires each of sessionType/tokenType/grantType shortcuts to be exactly 2 characters (encodedContext.length() != 6 check, fixed 2-char substrings). All actual implementations use 2-letter shortcuts (ac, cc, pg, pc, rt, ro, te, ci, dg), contradicting the docstring and risking a broken implementation if a future grant type factory follows the documented \"3-letters\" guidance. Location: server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java:31", + "created_at": null + } + ] } ] }, @@ -4705,6 +4755,31 @@ "created_at": "2026-06-28T23:26:03Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "keycloak__keycloak__pull__38446__shipwright__4701de31", + "pr_url": "https://github.com/keycloak/keycloak/pull/38446", + "review_comments": [ + { + "path": "server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java", + "line": 114, + "body": "CredentialHelper.createRecoveryCodesCredential looks up the recovery-codes credential provider using a hardcoded string literal \"keycloak-recovery-authn-codes\" instead of the existing RecoveryAuthnCodesCredentialProviderFactory.PROVIDER_ID constant (which was previously imported/used in RecoveryAuthnCodesAction and is now removed from that file). If this literal ever diverges from the actual factory id, session.getProvider(...) silently returns null and the subsequent recoveryCodeCredentialProvider.createCredential(...) call throws an NPE. Location: server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java:114", + "created_at": null + }, + { + "path": "testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java", + "line": 114, + "body": "In supportsCredentialType, the new recovery-codes check uses `credentialType.equals(RecoveryAuthnCodesCredentialModel.TYPE)` instead of `RecoveryAuthnCodesCredentialModel.TYPE.equals(credentialType)`, unlike the sibling condition `CredentialModel.PASSWORD.equals(credentialType)` on the line above. If credentialType is null this throws a NullPointerException instead of returning false as the other null-safe checks do. Location: testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java:114", + "created_at": null + }, + { + "path": "testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java", + "line": 275, + "body": "Same reversed-order equals pattern is repeated in isConfiguredFor: `credentialType.equals(RecoveryAuthnCodesCredentialModel.TYPE) && myUser.recoveryCodes != null` calls equals on the possibly-null parameter instead of on the constant, risking an NPE where the OTP branch above it is null-safe. Location: testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java:275", + "created_at": null + } + ] } ] }, @@ -5863,6 +5938,19 @@ "created_at": "2026-06-28T22:40:43Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "keycloak__keycloak__pull__36882__shipwright__4701de31", + "pr_url": "https://github.com/keycloak/keycloak/pull/36882", + "review_comments": [ + { + "path": "operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java", + "line": 115, + "body": "In getTestKeycloakDeployment/build-CR helper, the code null-checks getFeatureSpec() before creating a new FeatureSpec (implying it may already exist, e.g. set up elsewhere on the CR), but then unconditionally calls setEnabledFeatures(List.of(ROLLING_UPDATES)) which replaces the entire enabled-features list rather than merging with any features that may already be enabled on that spec. If a FeatureSpec with other enabled features already exists on the CR when this helper runs, those features are silently dropped, changing test behavior in a way not obvious from the code. Location: operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java:115", + "created_at": null + } + ] } ] }, @@ -7384,6 +7472,19 @@ "created_at": "2026-06-28T22:50:30Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "keycloak__keycloak__pull__36880__shipwright__4701de31", + "pr_url": "https://github.com/keycloak/keycloak/pull/36880", + "review_comments": [ + { + "path": "services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java", + "line": 91, + "body": "The javadoc for `requireView()` and `requireViewClientScopes()` (both void methods that throw ForbiddenException, per the interface's naming convention) was copy-pasted from the corresponding boolean-returning `canView()`/`canViewClientScopes()` methods. It now incorrectly states \"Returns {@code true} if ... returns {@code true}\" instead of \"Throws ForbiddenException if ... returns {@code false}\" like all other sibling `require*` methods in this same file (e.g. requireList(), requireManage(), requireManageClientScopes()), misleading callers/implementers about the actual contract of these void methods. Location: services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java:91", + "created_at": null + } + ] } ] }, @@ -8963,6 +9064,31 @@ "created_at": "2026-06-28T22:57:21Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "keycloak__keycloak__pull__37038__shipwright__4701de31", + "pr_url": "https://github.com/keycloak/keycloak/pull/37038", + "review_comments": [ + { + "path": "services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java", + "line": 70, + "body": "In GroupPermissionsV2.canManage() (the no-arg, \"all groups\" check), the fallback ABAC check calls hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE) — identical to canView()'s check. This means a caller with only a VIEW scope permission on the Groups resource type (no MANAGE) will satisfy canManage(), granting group-management capabilities (e.g., creating groups) to a view-only admin. The per-group overload canManage(GroupModel) correctly checks only AdminPermissionsSchema.MANAGE, confirming this is an inconsistent/incorrect duplication of canView()'s logic rather than intended behavior. Location: services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java:70", + "created_at": null + }, + { + "path": "services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java", + "line": 122, + "body": "getGroupIdsWithViewPermission() in GroupPermissionsV2 iterates authorization Resource objects via resourceStore.findByType and uses groupResource.getId() (the authorization Resource's internal UUID) both as the argument to hasPermission(...) and as the value stored into the returned set. However hasPermission(String groupId, ...) expects the actual GroupModel id (it looks the resource up via resourceStore.findByName(server, groupId), and resources for groups are created/named using the GroupModel id, e.g. resolveGroup returns group.getId() as the resource name). Using the resource's internal id instead of its name means findByName inside hasPermission will not match the resource, and the ids added to the returned set are not actual group ids. This breaks the downstream group-based filtering (session.setAttribute(UserModel.GROUPS, groupIds) in UsersResource/BruteForceUsersResource), causing FGAP V2 admins with narrow per-group VIEW_MEMBERS/MANAGE_MEMBERS permissions to get incorrect (likely empty) user search results. Location: services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java:122", + "created_at": null + }, + { + "path": "services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java", + "line": 453, + "body": "In UsersResource.searchForUser, the per-user authorization filter `.filter(usersEvaluator::canView)` that used to be applied to the search stream was removed entirely. The remaining group-based restriction (session.setAttribute(UserModel.GROUPS, groupIds)) only narrows results when the caller has a group-based view grant; if the caller instead only has a narrow individual user-level VIEW permission (not a group-based one) and getGroupIdsWithViewPermission() returns an empty set, no group restriction is applied and no per-item permission check happens either, so the search will return every matching user with no authorization filtering, exposing users the caller should not be able to view. Location: services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java:453", + "created_at": null + } + ] } ] }, @@ -10427,6 +10553,19 @@ "created_at": "2026-06-28T22:36:15Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "keycloak__keycloak__pull__33832__shipwright__4701de31", + "pr_url": "https://github.com/keycloak/keycloak/pull/33832", + "review_comments": [ + { + "path": "common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java", + "line": 68, + "body": "In CryptoIntegration.detectProvider(), the new 'ignored crypto providers' message is built with a StringBuilder that appends \", \" after every provider name including the last one, producing a malformed trailing separator (e.g. \"Ignored crypto providers: com.foo.Bar, \"). Additionally the pre-built string is passed to logger.debugf(builder.toString()) instead of logger.debug(...), which treats the message as a printf-style format string unnecessarily. Location: common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java:68", + "created_at": null + } + ] } ] }, @@ -11653,6 +11792,19 @@ "created_at": "2026-06-28T23:30:25Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "keycloak__keycloak__pull__40940__shipwright__4701de31", + "pr_url": "https://github.com/keycloak/keycloak/pull/40940", + "review_comments": [ + { + "path": "tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java", + "line": 156, + "body": "The new concurrency test starts a background reader thread and, after setting deletedAll to true, immediately asserts caughtExceptions without waiting for the thread to actually finish its current iteration (e.g., via Thread.join()). Since the reader thread only checks the flag after completing its current groups() call, an exception raised concurrently with the delete loop can be added to caughtExceptions after the assertion has already executed, making the test non-deterministic and able to falsely pass even when the regression (NPE during concurrent access) actually occurs. Location: tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java:156", + "created_at": null + } + ] } ] }, @@ -13015,6 +13167,25 @@ "created_at": "2026-06-28T22:13:01Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "keycloak__keycloak__pull__41249__shipwright__4701de31", + "pr_url": "https://github.com/keycloak/keycloak/pull/41249", + "review_comments": [ + { + "path": "services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java", + "line": 47, + "body": "UsernameForm.authenticate() calls `isConditionalPasskeysEnabled()` with no arguments, but the only method with that name added in this PR (in UsernamePasswordForm.java, which UsernameForm extends) is `protected boolean isConditionalPasskeysEnabled(UserModel user)` requiring a UserModel parameter. There is no zero-arg overload defined anywhere in the diff, so this call site will not compile, breaking the build for this authenticator. Location: services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java:47", + "created_at": null + }, + { + "path": "services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java", + "line": 113, + "body": "The guard for filling the WebAuthn/passkeys form data in UsernamePasswordForm.authenticate() and challenge() was changed from requiring `context.getUser() == null` (i.e. fill webauthn data during the normal/initial login when no user is yet selected) to `isConditionalPasskeysEnabled(context.getUser())`, which requires `context.getUser() != null`. This inverts rather than extends the original condition, so webauthn conditional-UI data is now only populated during re-authentication and is never populated during the standard first-time login flow. This directly conflicts with the pre-existing (unmodified) assertions in PasskeysUsernamePasswordFormTest#webauthnLoginWithExternalKey, which still expect `//form[@id='webauth']` to be present right after `oauth.openLoginForm()` when no user/session context exists yet. Location: services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java:113", + "created_at": null + } + ] } ] }, @@ -14441,6 +14612,31 @@ "created_at": "2026-06-28T23:04:21Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "getsentry__sentry__pull__93824__shipwright__4701de31", + "pr_url": "https://github.com/getsentry/sentry/pull/93824", + "review_comments": [ + { + "path": "src/sentry/spans/consumers/process/flusher.py", + "line": 347, + "body": "In SpanFlusher.join(), when timeout is set and iterating over multiple flusher processes, if the deadline expires while checking a given process, the code does `break` on the outer for-loop before ever calling `process.terminate()` for that process and for all subsequent processes in the dict. The previous single-process implementation always called `self.process.terminate()` unconditionally after the wait loop regardless of whether the deadline had passed. This regression leaves daemon flusher processes running/orphaned on consumer shutdown when a timeout is hit with multiple processes. Location: src/sentry/spans/consumers/process/flusher.py:347", + "created_at": null + }, + { + "path": "src/sentry/spans/consumers/process/flusher.py", + "line": 195, + "body": "Metric tag key is inconsistent across the newly-sharded metrics: 'spans.buffer.flusher.produce' and 'spans.buffer.segment_size_bytes' use tags={\"shard\": shard_tag} while 'spans.buffer.flusher.wait_produce' uses tags={\"shards\": shard_tag} (plural). This breaks correlation/aggregation of these related metrics by the same dimension in dashboards/alerts. Location: src/sentry/spans/consumers/process/flusher.py:195", + "created_at": null + }, + { + "path": "src/sentry/spans/consumers/process/flusher.py", + "line": 30, + "body": "Updated class docstring states the flusher \"Creates one process per shard for parallel processing,\" but the implementation actually distributes shards across at most `max_processes` processes (default 1 via the new --flusher-processes click option), meaning multiple shards commonly share a single process. This directly misdescribes the sharding behavior introduced in this diff. Location: src/sentry/spans/consumers/process/flusher.py:30", + "created_at": null + } + ] } ] }, @@ -16412,6 +16608,31 @@ "created_at": "2026-06-28T22:58:37Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "ai-code-review-evaluation__sentry-greptile__pull__5__shipwright__4701de31", + "pr_url": "https://github.com/ai-code-review-evaluation/sentry-greptile/pull/5", + "review_comments": [ + { + "path": "src/sentry/preprod/api/endpoints/organization_preprod_artifact_assemble.py", + "line": 80, + "body": "analytics.record(\"preprod_artifact.api.assemble\", ...) is called unconditionally at the top of post() before the `features.has(\"organizations:preprod-artifact-assemble\", ...)` check that can return early (403/404). This records an analytics event even for requests that are rejected because the feature is disabled or the user lacks access, polluting analytics with invalid/unauthorized attempts. Location: src/sentry/preprod/api/endpoints/organization_preprod_artifact_assemble.py:80", + "created_at": null + }, + { + "path": "src/sentry/issues/endpoints/browser_reporting_collector.py", + "line": 45, + "body": "BrowserReportSerializer.validate_timestamp/validate_age use `self.initial_data.get(\"age\")`/`self.initial_data.get(\"timestamp\")` as truthiness checks. Since 0 is a valid legitimate value for `age` (e.g. \"report generated 0ms ago\") and `timestamp` has `min_value=0`, a payload with `age=0` and `timestamp` both present will bypass the mutual-exclusion validation because `0` is falsy, allowing invalid/ambiguous reports (both age and timestamp set) to pass validation silently. Location: src/sentry/issues/endpoints/browser_reporting_collector.py:45", + "created_at": null + }, + { + "path": "src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py", + "line": 103, + "body": "fetch_error_details() zips `error_ids` positionally with `events.values()` (the dict returned by `nodestore.backend.get_multi(node_ids)`), assuming the returned dict preserves the exact order of the requested node_ids. If the backend's get_multi implementation does not guarantee this ordering, error titles/messages will be mismatched with the wrong error_id in the resulting ErrorEvent list, producing incorrect error summaries sent to Seer. Location: src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py:103", + "created_at": null + } + ] } ] }, @@ -18067,6 +18288,43 @@ "created_at": "2026-06-28T22:12:55Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "ai-code-review-evaluation__sentry-greptile__pull__1__shipwright__4701de31", + "pr_url": "https://github.com/ai-code-review-evaluation/sentry-greptile/pull/1", + "review_comments": [ + { + "path": "src/sentry/api/paginator.py", + "line": 852, + "body": "OptimizedCursorPaginator.get_item_key() applies math.floor()/math.ceil() directly to the ordering field value (`getattr(item, self.key)`), assuming a numeric key. The audit log endpoint calls this paginator with order_by=\"-datetime\", so the key value is a datetime object; math.floor/ceil on a datetime raises TypeError, meaning any request with optimized_pagination=true will crash instead of returning results. Location: src/sentry/api/paginator.py:852", + "created_at": null + }, + { + "path": "src/sentry/api/paginator.py", + "line": 889, + "body": "The 'advanced feature' branch builds queryset[start_offset:stop] with a negative start_offset taken directly from cursor.offset. Django QuerySet slicing does not support negative indices (raises AssertionError: 'Negative indexing is not supported'), so this code path will throw an exception rather than the 'safe' bidirectional pagination the comments describe, contradicting the stated intent and crashing any request that triggers it. Location: src/sentry/api/paginator.py:889", + "created_at": null + }, + { + "path": "src/sentry/api/endpoints/organization_auditlogs.py", + "line": 73, + "body": "`enable_advanced = request.user.is_superuser or organization_context.member.has_global_access` unconditionally dereferences `organization_context.member` when the user is not a superuser. In this control-silo endpoint `member` may be None (e.g., staff/superuser-adjacent accounts without a regional membership record), causing an AttributeError instead of a graceful permission failure. Location: src/sentry/api/endpoints/organization_auditlogs.py:73", + "created_at": null + }, + { + "path": "src/sentry/api/paginator.py", + "line": 179, + "body": "Comment states this change is meant to 'allow negative offsets to enable efficient bidirectional pagination', but the actual code clamps the offset to zero via `max(0, offset)` for the non-prev case, which is the opposite of what the comment claims — misleading documentation about the change's behavior. Location: src/sentry/api/paginator.py:179", + "created_at": null + }, + { + "path": "src/sentry/api/endpoints/organization_auditlogs.py", + "line": 68, + "body": "A client-controlled query parameter (`optimized_pagination=true`) is used to switch pagination implementation and gate an 'advanced features' flag whose own comments describe 'enabl[ing] access to data beyond normal pagination bounds', with the authorization check embedded ad hoc in the view rather than expressed via the endpoint's permission classes. This pattern is a red flag for a hidden/undocumented privilege-widening code path that should be reviewed and likely rejected rather than merged as-is. Location: src/sentry/api/endpoints/organization_auditlogs.py:68", + "created_at": null + } + ] } ] }, @@ -19332,6 +19590,25 @@ "created_at": "2026-06-29T00:07:24Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "grafana__grafana__pull__97529__shipwright__4701de31", + "pr_url": "https://github.com/grafana/grafana/pull/97529", + "review_comments": [ + { + "path": "pkg/storage/unified/search/bleve.go", + "line": 134, + "body": "BuildIndex() previously held b.cacheMu for the entire function, serializing full index construction. Now the lock only wraps the final `b.cache[key] = idx` assignment, so two concurrent calls for the same key (e.g. two requests triggering getOrCreateIndex before a cache entry exists) can both open/build a bleve index for the same key. For file-backed indexes this means two goroutines write into the same directory concurrently (corruption risk), and whichever build loses the race to acquire the lock last has its bleve.Index silently discarded without being closed, leaking file handles/resources. Location: pkg/storage/unified/search/bleve.go:134", + "created_at": null + }, + { + "path": "pkg/storage/unified/resource/server.go", + "line": 303, + "body": "In server.Init(), the order of starting the change watcher and building the search index was swapped: search index initialization now runs before initWatcher() is called (previously the watcher started first). This introduces a window where writes occurring while the search index snapshot is being built are not yet observed by the watcher, since the watcher only starts listening after the (potentially long-running) index build completes. Events written in that window can be missed entirely by the search index, causing it to silently miss documents. Location: pkg/storage/unified/resource/server.go:303", + "created_at": null + } + ] } ] }, @@ -20621,6 +20898,25 @@ "created_at": "2026-06-28T23:46:01Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "getsentry__sentry__pull__80168__shipwright__4701de31", + "pr_url": "https://github.com/getsentry/sentry/pull/80168", + "review_comments": [ + { + "path": "src/sentry/incidents/grouptype.py", + "line": 9, + "body": "`MetricAlertDetectorHandler` was changed from a concrete `DetectorHandler` implementation to `class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): pass`, but it does not implement the abstract methods required by `StatefulDetectorHandler` (`get_dedupe_value`, `get_group_key_values`, and the newly-added `build_occurrence_and_event_data`). Instantiating this handler (e.g. via `Detector.detector_handler`) will now raise `TypeError: Can't instantiate abstract class MetricAlertDetectorHandler with abstract methods ...`, whereas previously it was a valid (stub) concrete class. Location: src/sentry/incidents/grouptype.py:9", + "created_at": null + }, + { + "path": "src/sentry/workflow_engine/processors/detector.py", + "line": 226, + "body": "The docstring for `StatefulDetectorHandler.evaluate` still states \"returns a list of `DetectorEvaluationResult`\" even though the method's return type and implementation were changed in this diff to return `dict[DetectorGroupKey, DetectorEvaluationResult]`, making the documentation inconsistent with the actual behavior. Location: src/sentry/workflow_engine/processors/detector.py:226", + "created_at": null + } + ] } ] }, @@ -21792,6 +22088,12 @@ "created_at": "2026-06-28T23:21:58Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "getsentry__sentry__pull__80528__shipwright__4701de31", + "pr_url": "https://github.com/getsentry/sentry/pull/80528", + "review_comments": [] } ] }, @@ -23038,6 +23340,31 @@ "created_at": "2026-06-28T23:18:05Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "getsentry__sentry__pull__77754__shipwright__4701de31", + "pr_url": "https://github.com/getsentry/sentry/pull/77754", + "review_comments": [ + { + "path": "src/sentry/integrations/services/assignment_source.py", + "line": 16, + "body": "AssignmentSource.queued has a class-level default `datetime = timezone.now()`, which is evaluated once at class/module definition time rather than per-instance. Every AssignmentSource created without explicitly passing `queued` (e.g. via `from_integration`) will get the exact same, stale timestamp (the time the module was imported), not the actual time of creation/queueing, defeating the purpose of the field. Location: src/sentry/integrations/services/assignment_source.py:16", + "created_at": null + }, + { + "path": "src/sentry/integrations/utils/sync.py", + "line": 140, + "body": "`AssignmentSource.to_dict()` uses `dataclasses.asdict()`, which leaves the `queued` field as a raw `datetime` object rather than a serializable type. This dict is then passed directly as Celery task kwargs via `sync_assignee_outbound.apply_async(kwargs={\"assignment_source_dict\": assignment_source.to_dict(), ...})`. Since Celery task kwargs must be serializable (JSON by default), embedding a raw `datetime` object will fail to serialize when the task is actually enqueued through a real broker, breaking outbound assignee sync whenever an assignment_source is supplied. Location: src/sentry/integrations/utils/sync.py:140", + "created_at": null + }, + { + "path": "tests/sentry/integrations/services/test_assignment_source.py", + "line": 8, + "body": "Test method name `test_from_dict_empty_array` is misleading: the test passes an empty dict (`{}`), not an array, mischaracterizing the scenario under test. Location: tests/sentry/integrations/services/test_assignment_source.py:8", + "created_at": null + } + ] } ] }, @@ -24429,6 +24756,37 @@ "created_at": "2026-06-28T23:34:19Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "getsentry__sentry__pull__95633__shipwright__4701de31", + "pr_url": "https://github.com/getsentry/sentry/pull/95633", + "review_comments": [ + { + "path": "src/sentry/remote_subscriptions/consumers/queue_consumer.py", + "line": 195, + "body": "OrderedQueueWorker.run() always calls offset_tracker.complete_offset() in the `finally` block regardless of whether result_processor raised an exception. This means a message that failed processing (exception logged but swallowed) is still marked as \"done\" and its offset becomes eligible for commit via get_committable_offsets(). Uptime results that fail processing are silently dropped and never retried, while their Kafka offsets get committed as if they succeeded, causing silent data loss. Location: src/sentry/remote_subscriptions/consumers/queue_consumer.py:195", + "created_at": null + }, + { + "path": "src/sentry/remote_subscriptions/consumers/queue_consumer.py", + "line": 320, + "body": "SimpleQueueProcessingStrategy.submit() catches all exceptions from decode/grouping and, in the except block, immediately calls add_offset()+complete_offset() to mark the message done, discarding it without retry or DLQ handling. Combined with the same pattern in OrderedQueueWorker.run(), this means any processing/decoding failure results in the offset being committed as if the message succeeded, causing silent message/result loss. Location: src/sentry/remote_subscriptions/consumers/queue_consumer.py:320", + "created_at": null + }, + { + "path": "src/sentry/remote_subscriptions/consumers/queue_consumer.py", + "line": 132, + "body": "FixedQueuePool creates each per-queue worker queue with `queue.Queue()` (unbounded, maxsize=0), yet the class docstring for SimpleQueueProcessingStrategy explicitly claims \"Natural backpressure when queues fill up.\" Since the queues have no maxsize, submit() never blocks and queues can grow unboundedly under load, contradicting the documented backpressure guarantee and risking unbounded memory growth. Location: src/sentry/remote_subscriptions/consumers/queue_consumer.py:132", + "created_at": null + }, + { + "path": "tests/sentry/uptime/consumers/test_results_consumer.py", + "line": 1850, + "body": "test_thread_queue_parallel_error_handling is documented as testing \"that errors in processing don't block offset commits for other messages,\" but the actual assertion checks that nothing was committed for test_partition (`assert len(committed_offsets) == 0 or test_partition not in committed_offsets`). Given the implementation always marks offsets complete in the worker's `finally` block even on exception, both offset 100 (failed) and 101 (succeeded) become contiguously committable, so the assertion contradicts both the stated test intent and the actual commit semantics being exercised, making the test misleading/incorrect. Location: tests/sentry/uptime/consumers/test_results_consumer.py:1850", + "created_at": null + } + ] } ] }, @@ -26196,6 +26554,43 @@ "created_at": "2026-06-28T22:21:53Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "getsentry__sentry__pull__92393__shipwright__4701de31", + "pr_url": "https://github.com/getsentry/sentry/pull/92393", + "review_comments": [ + { + "path": "src/sentry/api/paginator.py", + "line": 880, + "body": "OptimizedCursorPaginator.get_result adds a branch that slices `queryset[start_offset:stop]` with a negative `start_offset` when `enable_advanced_features and cursor.offset < 0`. Django QuerySets do not support negative indexing (raises AssertionError at runtime), so this new 'advanced pagination' path will always crash instead of working, directly contradicting the added comment claiming 'the underlying Django ORM properly handles negative slicing automatically'. Location: src/sentry/api/paginator.py:880", + "created_at": null + }, + { + "path": "src/sentry/api/paginator.py", + "line": 181, + "body": "BasePaginator.get_result now clamps negative offsets to 0 only when `cursor.is_prev` is False (`start_offset = max(0, offset) if not cursor.is_prev else offset`), silently changing pagination behavior for every subclass of BasePaginator used across the codebase. Instead of surfacing an error for an invalid/negative offset, requests will silently be served from offset 0, which can mask bugs and return incorrect page results without any indication to the caller. Location: src/sentry/api/paginator.py:181", + "created_at": null + }, + { + "path": "src/sentry/api/endpoints/organization_auditlogs.py", + "line": 73, + "body": "The audit log endpoint now enables a whole new pagination code path (OptimizedCursorPaginator with `enable_advanced_features=True`) purely based on a client-supplied GET parameter (`optimized_pagination=true`). This exposes the broken negative-offset feature (see paginator.py) to end users of a sensitive audit-log API, and the surrounding comments ('This is safe because permissions are checked at the queryset level') are misleading since the feature does not function correctly and was added with no relation to the stated PR purpose. Location: src/sentry/api/endpoints/organization_auditlogs.py:73", + "created_at": null + }, + { + "path": "src/sentry/spans/buffer.py", + "line": 446, + "body": "The application-level guard that dropped segments exceeding `self.max_segment_spans` was removed from flush_segments, leaving only the fixed eviction threshold of 1000 hardcoded in add-buffer.lua as protection. If `max_segment_spans` is configured to a value other than 1000, this configured limit is now silently unenforced, changing observable behavior for oversized segments. Location: src/sentry/spans/buffer.py:446", + "created_at": null + }, + { + "path": "src/sentry/scripts/spans/add-buffer.lua", + "line": 56, + "body": "add-buffer.lua hardcodes the eviction threshold as the literal `1000` in two places (`span_count > 1000` and `span_count - 1000`) with no connection to any configurable buffer/segment-size setting from the Python side, making the limit a magic number that must be kept in sync manually with `max_segment_spans`/related config if it changes. Location: src/sentry/scripts/spans/add-buffer.lua:56", + "created_at": null + } + ] } ] }, @@ -27930,6 +28325,31 @@ "created_at": "2026-06-28T22:35:45Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "getsentry__sentry__pull__94376__shipwright__4701de31", + "pr_url": "https://github.com/getsentry/sentry/pull/94376", + "review_comments": [ + { + "path": "src/sentry/testutils/factories.py", + "line": 354, + "body": "In `_set_sample_rate_from_error_sampling`, the check `if client_sample_rate:` treats a valid `client_sample_rate` of `0` (or `0.0`) as falsy and skips setting `normalized_data[\"sample_rate\"]`. A test event with `contexts.error_sampling.client_sample_rate = 0` will silently fail to get `sample_rate` set, even though 0 is a legitimate sample-rate value. Location: src/sentry/testutils/factories.py:354", + "created_at": null + }, + { + "path": "src/sentry/api/helpers/error_upsampling.py", + "line": 36, + "body": "New helper docstring claims `_are_all_projects_error_upsampled` \"reads the allowlist configuration fresh each time\" and this staleness is described as intentional, but the same PR wraps every call to this function through `is_errors_query_for_error_upsampled_projects` with a 60-second cache (`cache.set(cache_key, is_eligible, 60)`), so callers actually get a stale allowlist result for up to 60 seconds. The docstring is misleading about the real observed behavior. Location: src/sentry/api/helpers/error_upsampling.py:36", + "created_at": null + }, + { + "path": "tests/snuba/api/endpoints/test_organization_events_stats.py", + "line": 3612, + "body": "Test comments say \"First bucket has 1 event\" / \"Second bucket has 1 event\" but the assertions actually check for the upsampled value 10 (1 event / 0.1 sample rate). The comments were copied from a non-upsampled test and do not describe what the test is actually verifying, making the intent of the assertions confusing/misleading for reviewers and future maintainers. Location: tests/snuba/api/endpoints/test_organization_events_stats.py:3612", + "created_at": null + } + ] } ] }, @@ -29189,6 +29609,25 @@ "created_at": "2026-06-28T22:40:53Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "grafana__grafana__pull__103633__shipwright__4701de31", + "pr_url": "https://github.com/grafana/grafana/pull/103633", + "review_comments": [ + { + "path": "pkg/services/authz/rbac/cache.go", + "line": 30, + "body": "The new userPermDenialCacheKey builds the cache key by naive string concatenation of five dynamic segments: namespace + \".perm_\" + userUID + \"_\" + action + \"_\" + name + \"_\" + parent. Since Grafana resource/folder UIDs (name, parent) can contain underscores, different (action,name,parent) combinations can produce identical concatenated keys (e.g. action=\"a_b\",name=\"c\",parent=\"\" collides with action=\"a\",name=\"b_c\",parent=\"\"). Because this key is used both to look up and to set an explicit permission-denial entry in Check(), a collision causes the denial cached for one resource to be incorrectly served (as Allowed:false) for an unrelated resource/action for the duration of shortCacheTTL, causing spurious access denials. Location: pkg/services/authz/rbac/cache.go:30", + "created_at": null + }, + { + "path": "pkg/services/authz/rbac.go", + "line": 244, + "body": "NoopCache's methods use receiver name `lc` (short for LocalCache, copied from the local-cache implementation), which is misleading now that the type is a no-op cache used specifically to disable client-side caching for the embedded/local authz client. Location: pkg/services/authz/rbac.go:244", + "created_at": null + } + ] } ] }, @@ -30712,6 +31151,25 @@ "created_at": "2026-06-28T22:48:17Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "getsentry__sentry__pull__67876__shipwright__4701de31", + "pr_url": "https://github.com/getsentry/sentry/pull/67876", + "review_comments": [ + { + "path": "src/sentry/integrations/github/integration.py", + "line": 505, + "body": "In `GitHubInstallation.dispatch`, the new authenticity check directly indexes `integration.metadata[\"sender\"][\"login\"]` without checking that the `sender` key exists in metadata. Integrations created/updated before this security check was introduced (or through other code paths that don't populate `sender` in metadata) will raise an unhandled `KeyError`, causing a 500 error instead of a graceful failure page during what should be a security validation step. Location: src/sentry/integrations/github/integration.py:505", + "created_at": null + }, + { + "path": "src/sentry/integrations/github/integration.py", + "line": 416, + "body": "The OAuth authorize URL is built by directly interpolating `client_id`, `state` (pipeline.signature), and `redirect_uri` into an f-string query string without URL-encoding (`urlencode`). If any of these values (especially the signed `state`) contain characters like `+`, `/`, `=`, or `&`, the resulting query string will be malformed or truncated, potentially breaking the OAuth handshake or allowing parameter injection into the redirect URL. Location: src/sentry/integrations/github/integration.py:416", + "created_at": null + } + ] } ] }, @@ -31966,6 +32424,25 @@ "created_at": "2026-06-28T22:22:01Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "keycloak__keycloak__pull__32918__shipwright__4701de31", + "pr_url": "https://github.com/keycloak/keycloak/pull/32918", + "review_comments": [ + { + "path": "model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java", + "line": 249, + "body": "In the cache-hit path of the new getForLogin(), models are rebuilt via `session.identityProviders().getById(id)` and added directly to the result set, without being wrapped through `createOrganizationAwareIdentityProviderModel(...)` as done in the cache-miss/invalid branches. This means the models returned when the login cache is already populated are plain (non-organization-aware) IdentityProviderModel instances, while models returned on a cache miss/invalidate are wrapped. Callers such as OrganizationAwareIdentityProviderBean that rely on org-aware wrapping behavior will see inconsistent model semantics depending on cache state. Location: model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java:249", + "created_at": null + }, + { + "path": "testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java", + "line": 381, + "body": "The new testCacheIDPForLogin test registers cleanup callbacks using the hard-coded alias \"alias\" (`testRealm().identityProviders().get(\"alias\")::remove`) instead of the actual alias created in the loop (\"idp-alias-\" + i / \"idp-alias-20\"). None of the 21 identity providers created by this test are actually cleaned up, and the cleanup call will attempt to remove a non-existent IdP, likely throwing a NotFoundException during teardown and leaking test data across subsequent tests. Location: testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java:381", + "created_at": null + } + ] } ] }, @@ -33117,6 +33594,25 @@ "created_at": "2026-06-28T23:57:04Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "grafana__grafana__pull__94942__shipwright__4701de31", + "pr_url": "https://github.com/grafana/grafana/pull/94942", + "review_comments": [ + { + "path": "pkg/expr/reader.go", + "line": 193, + "body": "enableSqlExpressions() always returns false regardless of the feature flag state: it computes `enabled := !IsEnabledGlobally(...)` but then unconditionally returns `false` in both the `if enabled` branch and the final return, discarding the flag check entirely. As a result, ReadQuery() for QueryTypeSQL will always fail with \"sqlExpressions is not implemented\" even when the FlagSqlExpressions feature is enabled globally, making the SQL expression feature completely unusable. Location: pkg/expr/reader.go:193", + "created_at": null + }, + { + "path": "pkg/expr/sql/db.go", + "line": 11, + "body": "The new DB type's methods (TablesList, RunCommands, QueryFramesInto) are stub implementations that unconditionally return `errors.New(\"not implemented\")`. Since parser.go's TablesList() and sql_command.go's Execute() now call these stubbed methods instead of the removed go-duck implementation, all SQL parsing and query execution paths will always fail at runtime, breaking the core SQL analytics functionality this PR claims to introduce. Location: pkg/expr/sql/db.go:11", + "created_at": null + } + ] } ] }, @@ -34108,6 +34604,19 @@ "created_at": "2026-06-29T00:05:52Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "grafana__grafana__pull__90939__shipwright__4701de31", + "pr_url": "https://github.com/grafana/grafana/pull/90939", + "review_comments": [ + { + "path": "pkg/api/webassets/webassets.go", + "line": 44, + "body": "The double-checked locking pattern is incomplete: after acquiring the write lock (entryPointAssetsCacheMu.Lock()), the code does not re-check whether entryPointAssetsCache was already populated by a concurrent goroutine that raced through the initial RLock check while cache was nil. This means multiple concurrent requests hitting GetWebAssets during startup (when cache is nil) will each serially rebuild the asset manifest under the write lock instead of reusing the value the first goroutine just cached, defeating the purpose of the cache and causing repeated expensive file reads/computation for every blocked caller. Location: pkg/api/webassets/webassets.go:44", + "created_at": null + } + ] } ] }, @@ -35513,6 +36022,25 @@ "created_at": "2026-06-29T00:00:06Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "grafana__grafana__pull__80329__shipwright__4701de31", + "pr_url": "https://github.com/grafana/grafana/pull/80329", + "review_comments": [ + { + "path": "pkg/services/annotations/annotationsimpl/xorm_store.go", + "line": 533, + "body": "Multiple new logging calls in CleanAnnotations/CleanOrphanedAnnotationTags use r.log.Error() for routine, successful cleanup operations (e.g. 'Annotations to clean by time', 'cleaned annotations by time', 'Tags to clean', 'cleaned tags') even though no error occurred (the err value logged is guaranteed nil at that point, having already been checked). This misuses the Error log level for informational events, which will pollute error logs and can trigger false error-rate alerts on every cleanup cycle run (now every minute after the ticker change in cleanup.go). Location: pkg/services/annotations/annotationsimpl/xorm_store.go:533", + "created_at": null + }, + { + "path": "pkg/services/annotations/annotationsimpl/xorm_store.go", + "line": 528, + "body": "The new 'Annotations to clean by time'/'by count'/'Tags to clean' log statements log the full slice of fetched IDs ('ids', ids) at Error level. With the newly introduced batch size of up to 32767 (as exercised by the new SQLite test case with 40003 annotations), each cleanup cycle can emit log lines containing tens of thousands of IDs, causing excessive log volume/size on every run of the cleanup job (now scheduled every 1 minute instead of every 10 minutes). Location: pkg/services/annotations/annotationsimpl/xorm_store.go:528", + "created_at": null + } + ] } ] }, @@ -37355,6 +37883,55 @@ "created_at": "2026-06-28T23:53:28Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "grafana__grafana__pull__90045__shipwright__4701de31", + "pr_url": "https://github.com/grafana/grafana/pull/90045", + "review_comments": [ + { + "path": "pkg/apiserver/rest/dualwriter_mode3.go", + "line": 142, + "body": "In DualWriterMode3.Delete, the enriched logger `log` (with name/kind/method fields) is created but the context is populated with the original `d.Log` instead: `ctx = klog.NewContext(ctx, d.Log)`. All other methods (Create, Get, List, Update, DeleteCollection) correctly pass the enriched `log` into the context. This causes downstream code retrieving the logger from context in Delete to lose the added structured fields. Location: pkg/apiserver/rest/dualwriter_mode3.go:142", + "created_at": null + }, + { + "path": "pkg/apiserver/rest/dualwriter_mode3.go", + "line": 45, + "body": "In Create, when d.Storage.Create fails, the failure duration is recorded via d.recordLegacyDuration(true, ...) instead of d.recordStorageDuration(true, ...), mislabeling storage errors under the legacy-store metric. Location: pkg/apiserver/rest/dualwriter_mode3.go:45", + "created_at": null + }, + { + "path": "pkg/apiserver/rest/dualwriter_mode3.go", + "line": 141, + "body": "In Update, when d.Storage.Update fails, the failure duration is recorded via d.recordLegacyDuration(true, ...) instead of d.recordStorageDuration(true, ...), same mislabeling as Create, causing storage failures to be counted against legacy-store metrics. Location: pkg/apiserver/rest/dualwriter_mode3.go:141", + "created_at": null + }, + { + "path": "pkg/apiserver/rest/dualwriter_mode3.go", + "line": 151, + "body": "In Delete, the successful-storage-delete branch calls d.recordStorageDuration(false, mode3Str, name, method, startStorage), passing the object `name` where every other call site passes `options.Kind`. This produces inconsistent, high-cardinality metric labels (per-object name instead of resource kind) for the same metric. Location: pkg/apiserver/rest/dualwriter_mode3.go:151", + "created_at": null + }, + { + "path": "pkg/apiserver/rest/dualwriter_mode3.go", + "line": 198, + "body": "In DeleteCollection's background goroutine, the legacy-store deletion duration is recorded with d.recordStorageDuration(err != nil, ...) instead of d.recordLegacyDuration(...), unlike the analogous goroutines in Create/Delete/Update which correctly call recordLegacyDuration for the legacy operation. Location: pkg/apiserver/rest/dualwriter_mode3.go:198", + "created_at": null + }, + { + "path": "pkg/apiserver/rest/dualwriter_mode3.go", + "line": 52, + "body": "Create, Delete, Update, and DeleteCollection now perform the Legacy write asynchronously via `go func(){...}()`, deriving the timeout context from the same request-scoped `ctx` that is returned to the caller immediately afterward. Since the caller (and typically the underlying HTTP request) may cancel/close that context as soon as the handler returns the response, the background legacy write can be canceled before completing, silently dropping legacy-store writes with only a metric recorded and no error surfaced. Location: pkg/apiserver/rest/dualwriter_mode3.go:52", + "created_at": null + }, + { + "path": "pkg/apiserver/rest/dualwriter_mode3.go", + "line": 146, + "body": "Delete previously called Legacy.Delete for best-effort cleanup even when Storage.Delete returned a NotFound error (via `if !apierrors.IsNotFound(err)`), removed in this diff along with the apierrors import. Now Delete returns immediately on ANY Storage.Delete error, including NotFound, and never attempts the legacy delete cleanup goroutine, changing the dual-write consistency guarantee and potentially leaving orphaned legacy records. Location: pkg/apiserver/rest/dualwriter_mode3.go:146", + "created_at": null + } + ] } ] }, @@ -38856,6 +39433,37 @@ "created_at": "2026-06-28T23:38:08Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "grafana__grafana__pull__106778__shipwright__4701de31", + "pr_url": "https://github.com/grafana/grafana/pull/106778", + "review_comments": [ + { + "path": "public/app/features/alerting/unified/hooks/useAbilities.ts", + "line": 352, + "body": "New function `useIsGrafanaPromRuleEditable` calls `ctx.hasPermissionInMetadata(...)` twice, but `ctx` is never imported or defined anywhere in this diff (only `contextSrv`-style helpers like `isAdmin` are imported). This will throw a ReferenceError at runtime whenever this code path executes (e.g. when rendering the rule-list menu for a Grafana Prometheus rule), crashing the component. Location: public/app/features/alerting/unified/hooks/useAbilities.ts:352", + "created_at": null + }, + { + "path": "public/app/features/alerting/unified/hooks/useAbilities.ts", + "line": 259, + "body": "In the renamed `useAllRulerRuleAbilities` (formerly `useAllAlertRuleAbilities`), the previous federated-rule-group check `isFederatedRuleGroup(rule.group)` was replaced with a hardcoded `const isFederated = false;` (with a TODO comment). This removes the immutability guard for federated rule groups, so Edit/Delete/Duplicate/Pause actions that should be blocked for federated (read-only) groups will now be allowed. Location: public/app/features/alerting/unified/hooks/useAbilities.ts:259", + "created_at": null + }, + { + "path": "public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx", + "line": 52, + "body": "`RuleActionsButtons` now accepts `rule?: RulerRuleDTO` (optional via `RequireAtLeastOne`), but it still passes `rule` directly into `useRulerRuleAbility(rule, groupIdentifier, AlertRuleAction.Update)`, whose signature (unchanged in this diff) requires a non-optional `RulerRuleDTO`. This is a signature/type mismatch introduced by widening the Props type without updating all callees, and will fail type-checking or behave incorrectly for promRule-only invocations (e.g. from the new GrafanaRuleListItem where `rule` is not supplied). Location: public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx:52", + "created_at": null + }, + { + "path": "public/app/features/alerting/unified/rule-list/GrafanaRuleListItem.tsx", + "line": 36, + "body": "The new `GrafanaRuleListItem` constructs the rule view link as `createRelativeUrl('/alerting/grafana/${uid}/view')` without the `returnTo` parameter that the deleted `GrafanaRuleLoader.tsx` used to include (`createRelativeUrl(..., { returnTo })` via `createReturnTo()`). This silently drops return-to navigation state when users click through to view a rule from the rule list. Location: public/app/features/alerting/unified/rule-list/GrafanaRuleListItem.tsx:36", + "created_at": null + } + ] } ] }, @@ -39992,6 +40600,19 @@ "created_at": "2026-06-28T23:15:44Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "grafana__grafana__pull__107534__shipwright__4701de31", + "pr_url": "https://github.com/grafana/grafana/pull/107534", + "review_comments": [ + { + "path": "public/app/plugins/datasource/loki/shardQuerySplitting.test.ts", + "line": 112, + "body": "The test previously asserted the top-level interpolation helper was invoked exactly once (`toHaveBeenCalledTimes(1)`), which matched the single call made in `runShardSplitQuery`'s `.map()` over `request.targets` (which contains one query). The diff only swaps the bulk `interpolateVariablesInQueries` call for a per-query `.map(query => datasource.applyTemplateVariables(...))` call over the same single-element `request.targets` array — the underlying shard-splitting logic that generates sub-requests is unchanged. The updated assertion now expects `toHaveBeenCalledTimes(5)`, which is inconsistent with the single call performed in the shown code path and appears to be a copy/paste mistake (likely conflated with the '5 shards' count from an unrelated test), making the test defective/likely to fail against the real call count. Location: public/app/plugins/datasource/loki/shardQuerySplitting.test.ts:112", + "created_at": null + } + ] } ] }, @@ -41466,6 +42087,31 @@ "created_at": "2026-06-28T22:20:35Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "grafana__grafana__pull__79265__shipwright__4701de31", + "pr_url": "https://github.com/grafana/grafana/pull/79265", + "review_comments": [ + { + "path": "pkg/services/anonymous/anonimpl/anonstore/database.go", + "line": 97, + "body": "CreateOrUpdateDevice enforces the device limit via a separate CountDevices query followed by a conditional insert/update, with no transaction or locking between the two steps. Under concurrent anonymous authentications, multiple requests for distinct new devices can all read a count below the limit and then all succeed in inserting, allowing the configured anonymous device limit to be exceeded (TOCTOU race). Location: pkg/services/anonymous/anonimpl/anonstore/database.go:97", + "created_at": null + }, + { + "path": "pkg/services/anonymous/anonimpl/client.go", + "line": 45, + "body": "Authenticate now returns the raw anonstore sentinel error (anonstore.ErrDeviceLimitReached) directly as the authn error when TagDevice fails due to device limit, instead of wrapping it in an authn-specific error. This couples the authn layer to an internal storage-layer sentinel and may not satisfy expectations of authn.ContextAwareClient callers/error-handling machinery designed around authn package errors. Location: pkg/services/anonymous/anonimpl/client.go:45", + "created_at": null + }, + { + "path": "pkg/services/anonymous/anonimpl/anonstore/database.go", + "line": 16, + "body": "The 30-day device expiration window constant was duplicated as `anonymousDeviceExpiration` independently in both anonstore/database.go and api/api.go (renamed from the old shared `thirtyDays`), instead of being defined once and reused. This duplication risks the two values drifting out of sync if one is changed without the other. Location: pkg/services/anonymous/anonimpl/anonstore/database.go:16", + "created_at": null + } + ] } ] }, @@ -42570,6 +43216,25 @@ "created_at": "2026-06-28T23:57:17Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "discourse__discourse__commit__ecfa17b5a79dfdc91e7a4d50b42ae78a35d0a293__shipwright__4701de31", + "pr_url": "https://github.com/discourse/discourse/commit/ecfa17b5a79dfdc91e7a4d50b42ae78a35d0a293", + "review_comments": [ + { + "path": "config/initializers/i18n.rb", + "line": 8, + "body": "config/initializers/i18n.rb includes I18n::Backend::Fallbacks into I18n::Backend::Simple to enable the new fallback behavior, but lib/freedom_patches/translate_accelerator.rb reopens I18n::Backend::Simple and defines its own `translate(key, *args)` method directly on the class (unchanged context in the diff). In Ruby, a method defined directly on a class always takes precedence over a method of the same name provided by a later-included module, so I18n::Backend::Fallbacks#translate will never actually run for normal translation lookups performed through the accelerator's cached `translate` method. As a result, the whole point of this PR (server-side locale fallback) is effectively neutralized for the primary translation path, since the fallback logic is only reachable if the accelerator explicitly calls `super`, which is not shown/guaranteed here. Location: config/initializers/i18n.rb:8", + "created_at": null + }, + { + "path": "config/initializers/i18n.rb", + "line": 15, + "body": "FallbackLocaleList#[] builds the fallback chain as `[locale, SiteSetting.default_locale.to_sym, :en].uniq.compact`, mixing the raw `locale` argument (whose type, String or Symbol, is not normalized) with a symbol-coerced default locale. If `locale` is passed as a String (e.g. \"en\") while the default locale symbol is `:en`, `.uniq` will not treat them as duplicates, producing a fallback list with both `\"en\"` and `:en\"`, which is then iterated in `ensure_loaded!` to call `I18n.ensure_loaded!` twice for what is effectively the same locale. Location: config/initializers/i18n.rb:15", + "created_at": null + } + ] } ] }, @@ -43733,6 +44398,25 @@ "created_at": "2026-06-28T22:55:56Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "grafana__grafana__pull__76186__shipwright__4701de31", + "pr_url": "https://github.com/grafana/grafana/pull/76186", + "review_comments": [ + { + "path": "pkg/plugins/log/fake.go", + "line": 46, + "body": "TestLogger.FromContext always returns a brand new TestLogger via NewTestLogger(), completely disconnected from the receiver. Since production code now calls `m.logger.FromContext(ctx).Info(...)` (see logger_middleware.go), any test that injects a TestLogger and later asserts on its InfoLogs/WarnLogs/etc. fields will never see the recorded call, because the call is made against a throwaway logger instance rather than the original fake. Location: pkg/plugins/log/fake.go:46", + "created_at": null + }, + { + "path": "pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go", + "line": 50, + "body": "The previous logRequest implementation added a `traceID` field (via tracing.TraceIDFromContext) to the \"Plugin Request Completed\" log line. This code, along with the tracing import, was removed and no equivalent traceID attribute was added to the new ContextualLoggerMiddleware's instrumentContext (which only adds endpoint, pluginId, dsName, dsUID, uname). As a result, the trace ID is silently dropped from plugin request completion logs, regressing log-to-trace correlation. Location: pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go:50", + "created_at": null + } + ] } ] }, @@ -45852,6 +46536,43 @@ "created_at": "2026-06-28T23:54:24Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "discourse__discourse__commit__d1c69189f3c90ecf56013a8da904da9bff9a8e19__shipwright__4701de31", + "pr_url": "https://github.com/discourse/discourse/commit/d1c69189f3c90ecf56013a8da904da9bff9a8e19", + "review_comments": [ + { + "path": "spec/fabricators/category_fabricator.rb", + "line": 1, + "body": "The contents of category_fabricator.rb and embeddable_host_fabricator.rb were swapped: category_fabricator.rb now only defines Fabricator(:embeddable_host), while embeddable_host_fabricator.rb defines the :category, :diff_category, :happy_category, :private_category fabricators. Any spec relying on Fabricate(:category) etc. from category_fabricator.rb will break unless the misnamed file happens to be loaded, and the file names no longer match their contents. Location: spec/fabricators/category_fabricator.rb:1", + "created_at": null + }, + { + "path": "app/models/embeddable_host.rb", + "line": 5, + "body": "before_validation callback unconditionally calls `self.host.sub!` twice without checking for nil. If host is not supplied (e.g. params[:embeddable_host][:host] is nil, as happens via Admin::EmbeddableHostsController#create/#update when the param is missing), this raises NoMethodError instead of failing validates_format_of gracefully, producing an unhandled 500 error. Location: app/models/embeddable_host.rb:5", + "created_at": null + }, + { + "path": "app/controllers/admin/embeddable_hosts_controller.rb", + "line": 9, + "body": "update and destroy fetch the record with `EmbeddableHost.where(id: params[:id]).first` but never check for nil before calling methods on it (`save_host(host)` sets `host.host = ...`, and `host.destroy` is called directly). Requesting an update/destroy for a non-existent id causes an unhandled NoMethodError instead of a proper 404 response. Location: app/controllers/admin/embeddable_hosts_controller.rb:9", + "created_at": null + }, + { + "path": "app/models/embeddable_host.rb", + "line": 19, + "body": "record_for_host lowercases the `host` column in the SQL (`lower(host) = ?`) but compares it against the un-downcased `host` variable derived from `URI(host).host`, which preserves original case. A referer/URL with mixed-case hostname (e.g. https://EvilTrout.com) will fail to match a stored lowercase host, causing host_allowed? to incorrectly return false for otherwise-valid hosts. Location: app/models/embeddable_host.rb:19", + "created_at": null + }, + { + "path": "db/migrate/20150818190757_create_embeddable_hosts.rb", + "line": 26, + "body": "The migration builds INSERT statements via raw string interpolation of `h` (`VALUES ('#{h}', ...)`), and inserts the raw site_settings value directly without going through the model's before_validation normalization that strips scheme/path from the host (e.g. `http://foo.com` or `foo.com/path` values in the old setting are inserted verbatim). Later lookups via EmbeddableHost.record_for_host (which compares against a stripped host) will not match these un-normalized migrated rows, silently breaking embedding for hosts migrated from the old site setting. Location: db/migrate/20150818190757_create_embeddable_hosts.rb:26", + "created_at": null + } + ] } ] }, @@ -47379,6 +48100,43 @@ "created_at": "2026-06-28T23:37:06Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "discourse__discourse__commit__d38c4d5f7443223c81529c22470293771baf9f38__shipwright__4701de31", + "pr_url": "https://github.com/discourse/discourse/commit/d38c4d5f7443223c81529c22470293771baf9f38", + "review_comments": [ + { + "path": "app/assets/stylesheets/desktop/topic-post.scss", + "line": 291, + "body": "The `.reply-to-tab`/discussion link color was changed from `scale-color($primary, $lightness: 30%)` to `dark-light-choose(scale-color($primary, $lightness: 70%), scale-color($secondary, $lightness: 30%))`. Unlike every other conversion in this PR (which preserves the original light-theme lightness value and only adds the complementary dark-theme fallback), this one silently changes the primary/light-theme lightness from 30% to 70%, altering the light-theme appearance unintentionally. Location: app/assets/stylesheets/desktop/topic-post.scss:291", + "created_at": null + }, + { + "path": "app/assets/stylesheets/mobile/modal.scss", + "line": 102, + "body": "`.custom-message-length` color changed from `scale-color($primary, $lightness: 70%)` to `dark-light-choose(scale-color($primary, $lightness: 30%), scale-color($secondary, $lightness: 70%))`. The light-theme value was altered from 70% to 30%, inconsistent with the mechanical pattern used elsewhere in the PR, causing an unintended visual regression for light theme. Location: app/assets/stylesheets/mobile/modal.scss:102", + "created_at": null + }, + { + "path": "app/assets/stylesheets/mobile/topic-post.scss", + "line": 182, + "body": "The `h3` color was changed from `scale-color($primary, $lightness: 20%)` to `dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%))`. The original light-theme lightness (20%) was replaced with 50%, deviating from the pattern used elsewhere and unintentionally changing the light-theme heading color. Location: app/assets/stylesheets/mobile/topic-post.scss:182", + "created_at": null + }, + { + "path": "app/assets/stylesheets/desktop/user.scss", + "line": 522, + "body": "`.name` color changed from `scale-color($primary, $lightness: 30%)` to `dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%))`. The light-theme value was changed from 30% to 50%, inconsistent with the rest of the PR's mechanical transformation, causing an unintended color shift in light theme. Location: app/assets/stylesheets/desktop/user.scss:522", + "created_at": null + }, + { + "path": "app/assets/stylesheets/mobile/user.scss", + "line": 497, + "body": "Same defect as in desktop/user.scss: `.name` color changed from `scale-color($primary, $lightness: 30%)` to `dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%))`, silently altering the light-theme lightness value instead of preserving it. Location: app/assets/stylesheets/mobile/user.scss:497", + "created_at": null + } + ] } ] }, @@ -49374,6 +50132,31 @@ "created_at": "2026-06-28T23:44:35Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "discourse__discourse__commit__060cda77729cb1c4a827560e09e89a7b22078ba9__shipwright__4701de31", + "pr_url": "https://github.com/discourse/discourse/commit/060cda77729cb1c4a827560e09e89a7b22078ba9", + "review_comments": [ + { + "path": "config/routes.rb", + "line": 49, + "body": "The admin groups routes for adding/removing members are declared as bare (unwrapped) routes inside `resources :groups do ... end`, which Rails maps as member routes keyed by the default `:id` parameter (i.e. `/admin/groups/:id/members`). However, `Admin::GroupsController#add_members` and `#remove_member` call `params.require(:group_id)` instead of `params[:id]`. Real requests generated by the JS model (`/admin/groups/' + this.get('id') + '/members.json'`) will therefore raise `ActionController::ParameterMissing` for the missing `:group_id` key, breaking the add/remove-member feature entirely. The controller specs mask this bug because they invoke the actions directly with an explicit `group_id:` param, bypassing the actual route. Location: config/routes.rb:49", + "created_at": null + }, + { + "path": "app/assets/javascripts/admin/controllers/admin-group.js.es6", + "line": 16, + "body": "`totalPages` is computed as `Math.floor(user_count / limit) + 1`, which is only correct when user_count is not an exact multiple of limit. When user_count is an exact multiple of limit (e.g. user_count=50, limit=50), this yields 2 instead of the correct 1, causing `showingLast` to never be true on the actual last page and allowing the 'next' pagination control to be enabled for a non-existent, empty extra page. Location: app/assets/javascripts/admin/controllers/admin-group.js.es6:16", + "created_at": null + }, + { + "path": "app/assets/javascripts/admin/routes/admin_group_route.js", + "line": 11, + "body": "`setupController` now calls `model.findMembers();` without returning or chaining the promise (previously `afterModel` returned the promise from `findMembers`, so any AJAX failure would propagate through the route lifecycle). Now any failure of the members fetch is silently swallowed, leaving the member list simply empty with no error surfaced. Location: app/assets/javascripts/admin/routes/admin_group_route.js:11", + "created_at": null + } + ] } ] }, @@ -50865,6 +51648,31 @@ "created_at": "2026-06-28T22:47:11Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "discourse__discourse__commit__5f8a130277dbddc95d133cd2832be639baf89213__shipwright__4701de31", + "pr_url": "https://github.com/discourse/discourse/commit/5f8a130277dbddc95d133cd2832be639baf89213", + "review_comments": [ + { + "path": "app/models/blocked_email.rb", + "line": 11, + "body": "BlockedEmail.should_block? looks up the email with an exact-case query (`BlockedEmail.where(email: email).first`) instead of normalizing case. Since email local/domain parts are typically treated case-insensitively for blocking purposes, a user can bypass a blocked email like 'spammer@bad.com' simply by signing up with 'Spammer@bad.com', defeating the intended block. Location: app/models/blocked_email.rb:11", + "created_at": null + }, + { + "path": "app/models/blocked_email.rb", + "line": 13, + "body": "should_block? increments match_count with a non-atomic read-modify-write (`record.match_count += 1; record.save`) instead of an atomic update (e.g. increment! or update_counters). Concurrent signup attempts against the same blocked email can race and lose increments, producing inaccurate blocking statistics. Location: app/models/blocked_email.rb:13", + "created_at": null + }, + { + "path": "app/assets/javascripts/discourse/controllers/create_account_controller.js", + "line": 274, + "body": "The new client-side handler pushes any rejected email into `rejectedEmails` whenever `result.errors.email` is present, regardless of the actual reason (blocked, domain not allowed, already taken, etc). Subsequent validation of the same address then always reports the generic 'user.email.invalid' reason instead of the real server-provided error, misleading the user about why their email is invalid. Location: app/assets/javascripts/discourse/controllers/create_account_controller.js:274", + "created_at": null + } + ] } ] }, @@ -52089,6 +52897,19 @@ "created_at": "2026-06-28T23:08:26Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "discourse__discourse__commit__5b229316ee4c661836ed1161139692a3e8527444__shipwright__4701de31", + "pr_url": "https://github.com/discourse/discourse/commit/5b229316ee4c661836ed1161139692a3e8527444", + "review_comments": [ + { + "path": "app/assets/stylesheets/common/foundation/mixins.scss", + "line": 126, + "body": "The new `align-items` mixin declares `-ms-align-items: $alignment;`, which is not a real CSS property (the correct IE10 'tweener' syntax property is `-ms-flex-align`, which is already declared in the same mixin). This line is a bogus/no-op declaration that will be silently ignored by all browsers, misleading anyone who assumes it provides IE cross-browser support. Location: app/assets/stylesheets/common/foundation/mixins.scss:126", + "created_at": null + } + ] } ] }, @@ -53315,6 +54136,19 @@ "created_at": "2026-06-28T23:23:17Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "discourse__discourse__commit__267d8be1f556ed59639ced396c885bb44586da19__shipwright__4701de31", + "pr_url": "https://github.com/discourse/discourse/commit/267d8be1f556ed59639ced396c885bb44586da19", + "review_comments": [ + { + "path": "app/serializers/user_serializer.rb", + "line": 140, + "body": "website_name compares URI host values directly without normalizing case: `URI(website.to_s).host` preserves whatever case the user entered (e.g. 'HTTP://Example.com' yields host 'Example.com'), while `Discourse.current_hostname` is typically lowercase. The equality/suffix checks (`website_host == discourse_host`, the stripped-label comparison, and `discourse_host.ends_with?(\".\" << website_host)`) are case-sensitive, so a website URL that is actually the same domain as the instance but differs only in case will incorrectly fall through to the 'different domain' branch and show only the bare host instead of the full path. Location: app/serializers/user_serializer.rb:140", + "created_at": null + } + ] } ] }, @@ -55740,6 +56574,37 @@ "created_at": "2026-06-28T23:03:36Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "discourse__discourse__commit__4f8aed295a29954023b2849c060ef4fb299d1b5d__shipwright__4701de31", + "pr_url": "https://github.com/discourse/discourse/commit/4f8aed295a29954023b2849c060ef4fb299d1b5d", + "review_comments": [ + { + "path": "app/models/post.rb", + "line": 131, + "body": "Post#cook now returns `raw` unmodified for posts with cook_method == raw_html, completely bypassing the markdown/sanitization pipeline (post_analyzer.cook). Combined with app/jobs/scheduled/poll_feed.rb, which stores RSS feed content (`CGI.unescapeHTML(i.content.scrub)`) directly via TopicEmbed.import with cook_method set to raw_html and no HTML sanitization, arbitrary HTML/JS from an external feed is persisted as `raw` and later rendered unescaped (e.g. `<%= raw post.cooked %>` in app/views/embed/best.html.erb). This is a stored XSS vector: content from an untrusted external feed is served to all viewers without sanitization. Location: app/models/post.rb:131", + "created_at": null + }, + { + "path": "lib/tasks/disqus.thor", + "line": 146, + "body": "lib/tasks/disqus.thor previously created imported topics with `created_at: Date.parse(t[:created_at])`, preserving the original Disqus comment/thread date. The new call `TopicEmbed.import_remote(user, t[:link], title: t[:title])` never passes a created_at, and TopicEmbed.import/import_remote have no created_at parameter, so all imported topics will now be timestamped with the current time instead of their true historical creation date, silently corrupting imported data. Location: lib/tasks/disqus.thor:146", + "created_at": null + }, + { + "path": "app/models/topic_embed.rb", + "line": 18, + "body": "TopicEmbed.import uses a check-then-act pattern (`embed = TopicEmbed.where(embed_url: url).first; ... if embed.blank? ... TopicEmbed.create!(...)`) with no locking, even though the embed_url column has a unique DB index (see db/migrate/20131217174004_create_topic_embeds.rb) and TopicRetriever's own comment acknowledges 'another process or job found the embed already'. Two concurrent imports for the same URL (e.g. PollFeed running while a direct TopicRetriever request for the same URL is in flight) can both pass the `embed.blank?` check and race to `create!`, causing an unhandled ActiveRecord::RecordNotUnique to propagate out of the job/transaction. Location: app/models/topic_embed.rb:18", + "created_at": null + }, + { + "path": "spec/jobs/poll_feed_spec.rb", + "line": 2, + "body": "The new spec requires an unrelated dependency (`jobs/regular/process_post`) instead of the job actually under test (`jobs/scheduled/poll_feed`), which is misleading and appears to be a copy-paste leftover in a newly added spec file. Location: spec/jobs/poll_feed_spec.rb:2", + "created_at": null + } + ] } ] }, @@ -57329,6 +58194,43 @@ "created_at": "2026-06-28T22:28:25Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "discourse__discourse__commit__ffbaf8c54269df2ce510de91245760fddce09896__shipwright__4701de31", + "pr_url": "https://github.com/discourse/discourse/commit/ffbaf8c54269df2ce510de91245760fddce09896", + "review_comments": [ + { + "path": "app/models/optimized_image.rb", + "line": 142, + "body": "self.downsize is defined twice back-to-back: first with signature (from, to, max_width, max_height, opts) delegating to optimize with a joined dimension string, then immediately redefined with signature (from, to, dimensions, opts). Ruby silently keeps only the second definition, making the first a dead, unreachable definition. Any existing caller elsewhere that still invokes downsize with separate width/height positional args (matching the old 5-arg signature) will now raise ArgumentError since the effective method only accepts 4 args. Location: app/models/optimized_image.rb:142", + "created_at": null + }, + { + "path": "app/assets/javascripts/discourse/lib/utilities.js", + "line": 182, + "body": "checkImageSize now hardcodes maxSizeKB to 10*1024 (10MB) instead of reading the per-type setting Discourse.SiteSettings['max_' + type + '_size_kb']. This removes the ability to enforce different limits for different upload types (e.g. attachments vs images vs avatars) and will incorrectly block valid uploads that are allowed to be larger than 10MB, or incorrectly allow files that should be limited to less than 10MB for a given type. Location: app/assets/javascripts/discourse/lib/utilities.js:182", + "created_at": null + }, + { + "path": "app/assets/javascripts/discourse/lib/utilities.js", + "line": 246, + "body": "The 413 (entity too large) error handler now hardcodes maxSizeKB to 10*1024 instead of reading Discourse.SiteSettings.max_image_size_kb, so the error message shown to the user no longer reflects the actual server-configured limit that caused the 413, producing a misleading message. Location: app/assets/javascripts/discourse/lib/utilities.js:246", + "created_at": null + }, + { + "path": "app/controllers/uploads_controller.rb", + "line": 55, + "body": "FileHelper.download's max size argument is now hardcoded to 10.megabytes instead of SiteSetting.max_image_size_kb.kilobytes, decoupling the URL-download size cap from the admin-configured max_image_size_kb used later in the same method for the downsize loop. This is inconsistent: an admin who lowers max_image_size_kb below 10MB no longer gets that limit enforced at download time, and an admin who raises it above 10MB will have downloads truncated at a lower, hardcoded value. Location: app/controllers/uploads_controller.rb:55", + "created_at": null + }, + { + "path": "app/controllers/uploads_controller.rb", + "line": 63, + "body": "After the downsize retry loop, there is no check confirming tempfile.size is actually within SiteSetting.max_image_size_kb. If OptimizedImage.downsize fails (e.g. ImageMagick error, unsupported format) or the 5 attempts at 80% still don't bring the file under the limit, the code silently proceeds to Upload.create_for with the still-oversized file, bypassing the max_image_size_kb size restriction without any error surfaced to the caller. Location: app/controllers/uploads_controller.rb:63", + "created_at": null + } + ] } ] }, @@ -58860,6 +59762,37 @@ "created_at": "2026-06-28T22:28:38Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "discourse__discourse__commit__6669a2d94d76eea3b99b8c476d12b1eb66726b07__shipwright__4701de31", + "pr_url": "https://github.com/discourse/discourse/commit/6669a2d94d76eea3b99b8c476d12b1eb66726b07", + "review_comments": [ + { + "path": "app/controllers/topics_controller.rb", + "line": 104, + "body": "`ensure_logged_in` before_filter now excludes `:unsubscribe`, but the action body immediately calls `current_user.id` (`TopicUser.find_by(user_id: current_user.id, ...)`). Since anonymous requests are now allowed to reach this action, `current_user` will be nil and this raises NoMethodError instead of gracefully handling unauthenticated access to the per-topic unsubscribe link. Location: app/controllers/topics_controller.rb:104", + "created_at": null + }, + { + "path": "app/controllers/topics_controller.rb", + "line": 106, + "body": "`tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])` can return nil if no TopicUser row exists for that user/topic. The following lines unconditionally call `tu.notification_level` and `tu.save!` with no nil check, causing a NoMethodError/500 instead of a handled response. Location: app/controllers/topics_controller.rb:106", + "created_at": null + }, + { + "path": "app/controllers/topics_controller.rb", + "line": 109, + "body": "The unsubscribe action mutates `tu.notification_level` directly and calls `tu.save!` instead of going through `TopicUser.change`, which is used by every other notification-level mutation path in this codebase to publish a `notification_level_change` MessageBus event. As a result, other open tabs/websocket clients of the same user won't be notified in real time that the subscription state changed via the unsubscribe link. Location: app/controllers/topics_controller.rb:109", + "created_at": null + }, + { + "path": "app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6", + "line": 6, + "body": "The new Ember controller property is misspelled `stopNotificiationsText` (should be `stopNotificationsText`). The typo is echoed in the template's `{{{stopNotificiationsText}}}` binding, so it still works, but it's a confusing, clearly-introduced naming defect in new public-facing code. Location: app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6:6", + "created_at": null + } + ] } ] }, @@ -60473,6 +61406,31 @@ "created_at": "2026-06-28T23:46:15Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "calcom__cal.com__pull__22532__shipwright__4701de31", + "pr_url": "https://github.com/calcom/cal.com/pull/22532", + "review_comments": [ + { + "path": "packages/trpc/server/routers/viewer/calendars/deleteCache.handler.ts", + "line": 21, + "body": "deleteCache.handler.ts throws a plain `Error(\"Credential not found or access denied\")` instead of a `TRPCError` with an appropriate code (e.g. NOT_FOUND/FORBIDDEN). tRPC will wrap this as a generic INTERNAL_SERVER_ERROR, losing the correct HTTP/error semantics for an authorization failure and making it indistinguishable from unexpected server errors, unlike the rest of the tRPC handlers in this router which return typed errors. Location: packages/trpc/server/routers/viewer/calendars/deleteCache.handler.ts:21", + "created_at": null + }, + { + "path": "packages/features/apps/components/CredentialActionsDropdown.tsx", + "line": 91, + "body": "The cache status dropdown item uses `text-gray-900 dark:text-white` for the title and `text-gray-500 dark:text-white` for the 'last updated' subtitle. In dark mode both elements render as identical white text, eliminating the intended visual hierarchy between the label and the timestamp. Location: packages/features/apps/components/CredentialActionsDropdown.tsx:91", + "created_at": null + }, + { + "path": "packages/platform/atoms/selected-calendars/wrappers/SelectedCalendarsSettingsWebWrapper.tsx", + "line": 70, + "body": "The actions container `
` wrapping `CredentialActionsDropdown` is now rendered unconditionally in both list-item branches, whereas previously the div itself (and its reserved width) was only rendered when the credential could be disconnected. Since `CredentialActionsDropdown` can return `null` internally (delegation credential with no cache), the parent list item now always reserves an extra w-32 flex slot even when there is nothing to show, changing the layout compared to before. Location: packages/platform/atoms/selected-calendars/wrappers/SelectedCalendarsSettingsWebWrapper.tsx:70", + "created_at": null + } + ] } ] }, @@ -61993,6 +62951,31 @@ "created_at": "2026-06-28T23:11:51Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "calcom__cal.com__pull__8330__shipwright__4701de31", + "pr_url": "https://github.com/calcom/cal.com/pull/8330", + "review_comments": [ + { + "path": "packages/trpc/server/routers/viewer/slots.ts", + "line": 146, + "body": "In the new working-hours check inside checkIfIsAvailable, `end` is computed from `slotStartTime` instead of `slotEndTime` (`const end = slotStartTime.hour() * 60 + slotStartTime.minute();`), so the code never actually checks whether the slot's end time is within working hours — it just re-checks the start time twice, allowing slots whose end time falls outside working hours to be marked available. Location: packages/trpc/server/routers/viewer/slots.ts:146", + "created_at": null + }, + { + "path": "packages/trpc/server/routers/viewer/slots.ts", + "line": 121, + "body": "The 'whole day off' override detection compares two dayjs instances with `===` (`dayjs(date.start).add(utcOffset, \"minutes\") === dayjs(date.end).add(utcOffset, \"minutes\")`). Since dayjs objects are always distinct instances, this comparison is always false, so single-day date overrides marking a day fully unavailable (start === end) are never detected and such days are incorrectly treated as available. Location: packages/trpc/server/routers/viewer/slots.ts:121", + "created_at": null + }, + { + "path": "packages/trpc/server/routers/viewer/slots.ts", + "line": 88, + "body": "The new `dateOverrides` parameter type only declares `{ start: Date; end: Date }[]`, dropping the `userId` field that is actually present on the objects passed in (from `availability.dateOverrides.map((override) => ({ userId: availability.user.id, timeZone: availability.timeZone, ...override }))`). The new `dateOverrides.find` logic never filters by `userId`, so for multi-user event types (round robin/collective) a date override belonging to one host incorrectly affects availability calculations for slots belonging to other hosts who have no such override. Location: packages/trpc/server/routers/viewer/slots.ts:88", + "created_at": null + } + ] } ] }, @@ -63231,6 +64214,19 @@ "created_at": "2026-06-28T23:31:37Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "calcom__cal.com__pull__14943__shipwright__4701de31", + "pr_url": "https://github.com/calcom/cal.com/pull/14943", + "review_comments": [ + { + "path": "packages/features/ee/workflows/api/scheduleSMSReminders.ts", + "line": 33, + "body": "The new second OR branch in the deleteMany where-clause (`{ retryCount: { gt: 1 } }`) has no `method: WorkflowMethods.SMS` filter, unlike the first branch. Since `retryCount` is a field on the shared `WorkflowReminder` model (used by SMS, email, and WhatsApp reminders), any reminder of any method that ever accumulates retryCount > 1 will be permanently deleted regardless of method or scheduledDate, which is inconsistent with the intent of this SMS-specific cleanup and risks unintended data loss for non-SMS reminders. Location: packages/features/ee/workflows/api/scheduleSMSReminders.ts:33", + "created_at": null + } + ] } ] }, @@ -64407,6 +65403,19 @@ "created_at": "2026-06-28T22:40:20Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "calcom__cal.com__pull__22345__shipwright__4701de31", + "pr_url": "https://github.com/calcom/cal.com/pull/22345", + "review_comments": [ + { + "path": "packages/lib/server/service/insightsBooking.ts", + "line": 29, + "body": "The constructor's parameter type was changed from the strict `InsightsBookingServiceOptions` (a zod-inferred discriminated union that requires `teamId` when `scope === \"team\"`) to the newly added `InsightsBookingServicePublicOptions`, which declares `teamId` as universally optional regardless of scope. This removes compile-time enforcement that `teamId` must be supplied for team-scoped options, so TypeScript now allows constructing `new InsightsBookingService({ options: { scope: \"team\", userId, orgId } })` without a `teamId`, even though the underlying `insightsBookingServiceOptionsSchema` (still used internally, unchanged) requires it for that scope. This is a genuine signature/schema mismatch introduced by the diff, since the public type no longer reflects the actual runtime contract enforced by the same file's schema. Location: packages/lib/server/service/insightsBooking.ts:29", + "created_at": null + } + ] } ] }, @@ -66706,6 +67715,43 @@ "created_at": "2026-06-28T23:17:49Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "calcom__cal.com__pull__11059__shipwright__4701de31", + "pr_url": "https://github.com/calcom/cal.com/pull/11059", + "review_comments": [ + { + "path": "packages/app-store/_utils/oauth/refreshOAuthTokens.ts", + "line": 6, + "body": "refreshOAuthTokens() returns a raw fetch Response when credential sync is enabled, but the non-sync path returns provider/SDK-shaped objects (e.g. googleapis '{res:{data}}' shape in CalendarService.ts, axios response with `.data` in zoho-bigin/zohocrm, HubSpot SDK token object). Callers such as googlecalendar/lib/CalendarService.ts (`const token = res?.data`), zoho-bigin/lib/CalendarService.ts (`tokenInfo.data.error`) and zohocrm/lib/CalendarService.ts (`zohoCrmTokenInfo.data.error`) assume the non-sync shape, so when APP_CREDENTIAL_SHARING_ENABLED is true they will read `.data` off a raw fetch Response (undefined) and crash or silently store bad tokens. Location: packages/app-store/_utils/oauth/refreshOAuthTokens.ts:6", + "created_at": null + }, + { + "path": "packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts", + "line": 6, + "body": "minimumTokenResponseSchema uses computed property keys `[z.string().toString()]` and `[z.string().optional().toString()]`. Calling `.toString()` on a ZodType instance does not produce a dynamic 'any numeric key' matcher as the comment claims - both keys collapse to the same literal string (object's default toString), so the schema only actually validates `access_token` plus one bogus catch‑all key. The documented intent ('assume any property with a number is the expiry') is not implemented, so expiry validation silently never happens for the credential‑sync refresh path. Location: packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts:6", + "created_at": null + }, + { + "path": "packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts", + "line": 26, + "body": "When a refreshed token response lacks a refresh_token, the code sets `refreshTokenResponse.data.refresh_token = \"refresh_token\"` — a hardcoded placeholder literal string instead of preserving/omitting the original token. This corrupted value is subsequently persisted to the credential's key in DB by every provider (google, office365, zoom, etc.), breaking future OAuth refresh calls that rely on refresh_token. Location: packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts:26", + "created_at": null + }, + { + "path": "packages/app-store/zoho-bigin/lib/CalendarService.ts", + "line": 91, + "body": "refreshOAuthTokens is called with `credentialId` as the third argument (expected to be `userId: number | null`), unlike every other provider in this PR (google, hubspot, office365*, webex, zoomvideo, zohocrm) which pass `credential.userId`. This mismatches the parameter name/purpose in refreshOAuthTokens and will send the wrong identifier (credential id instead of user id) to the CALCOM_CREDENTIAL_SYNC_ENDPOINT when credential sharing is enabled. Location: packages/app-store/zoho-bigin/lib/CalendarService.ts:91", + "created_at": null + }, + { + "path": "packages/app-store/salesforce/lib/CalendarService.ts", + "line": 75, + "body": "New code unconditionally calls the Salesforce OAuth token endpoint and updates the credential in Prisma every time this connection-building code path runs, with no check of token expiry before refreshing (unlike other providers in this PR that gate refresh on expiration). This causes an OAuth refresh + DB write on every Salesforce API interaction, and can invalidate/rotate tokens unnecessarily, potentially breaking concurrent operations relying on the current access token. Location: packages/app-store/salesforce/lib/CalendarService.ts:75", + "created_at": null + } + ] } ] }, @@ -68422,6 +69468,37 @@ "created_at": "2026-06-28T23:04:20Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "calcom__cal.com__pull__7232__shipwright__4701de31", + "pr_url": "https://github.com/calcom/cal.com/pull/7232", + "review_comments": [ + { + "path": "packages/features/ee/workflows/lib/reminders/emailReminderManager.ts", + "line": 210, + "body": "In deleteScheduledEmailReminder, the immediateDelete=true branch only cancels the SendGrid scheduled send and then returns, without deleting or updating the WorkflowReminder row in the database. This differs from deleteScheduledSMSReminder, which always deletes the DB row. Since this function is called with immediateDelete=true when rescheduling bookings (handleNewBooking.ts) and when deleting workflows/steps (workflows.tsx), the corresponding WorkflowReminder rows are never removed from the database after this change, unlike before the diff where prisma.workflowReminder.deleteMany() was explicitly executed and awaited for these cases. Location: packages/features/ee/workflows/lib/reminders/emailReminderManager.ts:210", + "created_at": null + }, + { + "path": "packages/trpc/server/routers/viewer/bookings.tsx", + "line": 494, + "body": "The reschedule handler previously awaited `Promise.all(remindersToDelete)` to ensure workflow reminders (DB rows) for the old booking were deleted before continuing. This await has been removed and not replaced with any await of the new deleteScheduledEmailReminder/deleteScheduledSMSReminder calls, making the reminder cancellation entirely fire-and-forget. The handler can now respond to the client and proceed with rescheduling before the previous booking's reminders are cancelled/deleted, and any rejection from these calls becomes an unhandled promise at this call site. Location: packages/trpc/server/routers/viewer/bookings.tsx:494", + "created_at": null + }, + { + "path": "packages/features/bookings/lib/handleCancelBooking.ts", + "line": 505, + "body": "Previously, cancellation calls to delete external reminders were fire-and-forget but the actual DB row removal (`prisma.workflowReminder.deleteMany`) was collected into `remindersToDelete` and awaited via `Promise.all(prismaPromises...)`. After this change, the DB persistence for cancelled reminders happens inside deleteScheduledEmailReminder/deleteScheduledSMSReminder, but these calls are invoked inside a synchronous forEach and are not added to `prismaPromises`, so the handler no longer waits for reminder cancellation/deletion to complete before responding. Location: packages/features/bookings/lib/handleCancelBooking.ts:505", + "created_at": null + }, + { + "path": "packages/features/ee/workflows/api/scheduleEmailReminders.ts", + "line": 52, + "body": "The newly added cancellation loop wraps the entire `for` loop (including the `client.request` call and reminder deletion) in a single try/catch. If cancelling one reminder via the SendGrid API throws, the exception aborts the loop entirely, leaving all subsequent reminders in `remindersToCancel` unprocessed for that invocation, and `Promise.all(workflowRemindersToDelete)` for reminders already queued before the failure never executes since it's outside the loop but still inside the try block that already threw. Location: packages/features/ee/workflows/api/scheduleEmailReminders.ts:52", + "created_at": null + } + ] } ] }, @@ -70394,6 +71471,43 @@ "created_at": "2026-06-28T23:39:36Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "calcom__cal.com__pull__14740__shipwright__4701de31", + "pr_url": "https://github.com/calcom/cal.com/pull/14740", + "review_comments": [ + { + "path": "packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts", + "line": 38, + "body": "Authorization check uses `&&` instead of `||`: `(await isTeamAdmin(...)) && (await isTeamOwner(...))` requires the user to be BOTH a team admin AND a team owner to be treated as `isTeamAdminOrOwner`. Since ownership normally implies admin status, this effectively excludes team admins who are not owners from managing guests, despite the variable name and apparent intent of granting access to either role. Location: packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts:38", + "created_at": null + }, + { + "path": "packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts", + "line": 63, + "body": "Blacklist check compares the raw (non-normalized) guest email against a lowercased blacklist: `blacklistedGuestEmails` is built via `.toLowerCase()`, but `uniqueGuests` filters using `!blacklistedGuestEmails.includes(guest)` where `guest` is the original, case-sensitive user input. An attacker can bypass the BLACKLISTED_GUEST_EMAILS restriction simply by submitting the blocked email with different casing (e.g. 'Blocked@Example.com'). Location: packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts:63", + "created_at": null + }, + { + "path": "packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts", + "line": 153, + "body": "EventManager is instantiated with `ctx.user` (the currently authenticated actor invoking the mutation) rather than the booking's organizer (`organizer`, fetched separately via `booking.userId`). Since the authorization check permits team admins/owners and attendees (not just the organizer) to add guests, this means calendar sync (`eventManager.updateCalendarAttendees`) will run using the wrong user's calendar credentials whenever a non-organizer adds guests, causing the calendar update to fail or apply against the wrong account. Location: packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts:153", + "created_at": null + }, + { + "path": "packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts", + "line": 170, + "body": "Errors from `sendAddGuestsEmails` are swallowed with a bare `console.log(\"Error sending AddGuestsEmails\")` that discards the actual error object/message, making failures impossible to diagnose and inconsistent with other handlers that log the caught error. Location: packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts:170", + "created_at": null + }, + { + "path": "apps/web/components/dialog/AddGuestsDialog.tsx", + "line": 33, + "body": "In the mutation's onError handler, `message` is built from a template literal (`${err.data?.code}: ${t(err.message)}`) and is therefore always truthy, making the `|| t(\"unable_to_add_guests\")` fallback dead code that can never execute even when `err.data?.code` or `err.message` are undefined (producing a garbled message like 'undefined: undefined_translation_key'). Location: apps/web/components/dialog/AddGuestsDialog.tsx:33", + "created_at": null + } + ] } ] }, @@ -72114,6 +73228,31 @@ "created_at": "2026-06-28T22:29:53Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "calcom__cal.com__pull__10600__shipwright__4701de31", + "pr_url": "https://github.com/calcom/cal.com/pull/10600", + "review_comments": [ + { + "path": "apps/web/components/auth/BackupCode.tsx", + "line": 6, + "body": "The default export function in the new file BackupCode.tsx is still named `TwoFactor`, even though the file/component is now for backup-code entry (label 'backup_code', field name 'backupCode'). This is misleading and inconsistent with its actual purpose, unlike the existing TwoFactor.tsx which correctly names its export. Location: apps/web/components/auth/BackupCode.tsx:6", + "created_at": null + }, + { + "path": "packages/features/auth/lib/next-auth-options.ts", + "line": 131, + "body": "Backup code verification in the credentials authorize() reads user.backupCodes, decrypts it, finds a match, and only afterwards persists the invalidated code via prisma.user.update. Two concurrent login requests using the same backup code will both read the still-valid (un-invalidated) array before either write completes, so both requests can succeed with the same one-time backup code, breaking the intended one-time-use semantics. Location: packages/features/auth/lib/next-auth-options.ts:131", + "created_at": null + }, + { + "path": "apps/web/components/settings/EnableTwoFactorModal.tsx", + "line": 97, + "body": "backupCodesUrl is created with URL.createObjectURL(textBlob) and is only revoked the next time a new object URL is generated (URL.revokeObjectURL(backupCodesUrl) right before creating a new one). If the modal is closed/cancelled or unmounted after backup codes are generated, the final blob URL is never revoked, leaking memory. Location: apps/web/components/settings/EnableTwoFactorModal.tsx:97", + "created_at": null + } + ] } ] }, @@ -74331,6 +75470,43 @@ "created_at": "2026-06-28T22:35:43Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "calcom__cal.com__pull__10967__shipwright__4701de31", + "pr_url": "https://github.com/calcom/cal.com/pull/10967", + "review_comments": [ + { + "path": "packages/trpc/server/routers/viewer/organizations/create.handler.ts", + "line": 151, + "body": "The `slug`/`requestedSlug` condition was inverted: originally `...(!IS_TEAM_BILLING_ENABLED && { slug })` set `slug` directly only when team billing was disabled; the new code `...(IS_TEAM_BILLING_ENABLED ? { slug } : {})` sets `slug` only when team billing IS enabled — the opposite of the intended behavior. This causes newly created organizations to get a persisted `slug` when billing is enabled (which should instead go through the `requestedSlug` approval flow) and to be created with no `slug` when billing is disabled (which previously would have set one immediately). Location: packages/trpc/server/routers/viewer/organizations/create.handler.ts:151", + "created_at": null + }, + { + "path": "packages/core/EventManager.ts", + "line": 118, + "body": "`mainHostDestinationCalendar` is destructured from `evt.destinationCalendar ?? []` and can be `undefined` (e.g. when `destinationCalendar` is `null`/empty), but it is accessed as `mainHostDestinationCalendar.integration` without optional chaining, unlike every other similar destructuring added elsewhere in this same PR (which all use `?.`). This will throw a `TypeError: Cannot read properties of undefined` whenever `evt.destinationCalendar` is empty/null and `evt.location === MeetLocationType`. Location: packages/core/EventManager.ts:118", + "created_at": null + }, + { + "path": "packages/app-store/googlecalendar/lib/CalendarService.ts", + "line": 255, + "body": "In `updateEvent`, when `externalCalendarId` is falsy the fallback does `event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)` — but `externalCalendarId` is guaranteed falsy in that branch, so it compares each calendar's `externalId` against a falsy value, which will (almost) never match a real destination calendar entry. The fallback therefore always resolves to `undefined` instead of picking any destination calendar, breaking event updates that rely on the destinationCalendar fallback. Location: packages/app-store/googlecalendar/lib/CalendarService.ts:255", + "created_at": null + }, + { + "path": "packages/app-store/googlecalendar/lib/CalendarService.ts", + "line": 315, + "body": "Same self-referential comparison bug as in `updateEvent`: `externalCalendarId ? externalCalendarId : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId` in `deleteEvent` compares against the same falsy `externalCalendarId`, so the fallback to `event.destinationCalendar` never resolves a calendar id when `externalCalendarId` isn't supplied, causing deletion to target `undefined`/incorrect calendar. Location: packages/app-store/googlecalendar/lib/CalendarService.ts:315", + "created_at": null + }, + { + "path": "packages/features/bookings/lib/handleNewBooking.ts", + "line": 1084, + "body": "`evt.destinationCalendar` can be `null` (when neither `eventType.destinationCalendar` nor `organizerUser.destinationCalendar` exist). The subsequent `evt.destinationCalendar?.push(...teamDestinationCalendars)` silently no-ops in that case due to optional chaining, so destination calendars collected from non-organizer team members in a COLLECTIVE event are dropped entirely — defeating the PR's stated purpose of handling multiple hosts' destination calendars for collective events. Location: packages/features/bookings/lib/handleNewBooking.ts:1084", + "created_at": null + } + ] } ] }, @@ -75831,6 +77007,31 @@ "created_at": "2026-06-28T22:13:21Z" } ] + }, + { + "tool": "shipwright", + "repo_name": "calcom__cal.com__pull__8087__shipwright__4701de31", + "pr_url": "https://github.com/calcom/cal.com/pull/8087", + "review_comments": [ + { + "path": "packages/app-store/vital/lib/reschedule.ts", + "line": 125, + "body": "bookingRefsFiltered.forEach() callback was changed to `async (bookingRef) => {...}` while still using Array.prototype.forEach, which does not await the returned promises. The surrounding try/catch will no longer catch errors thrown inside the async callback (e.g. from getCalendar/deleteEvent), producing unhandled promise rejections and allowing the outer function to proceed before calendar deletions actually complete. Location: packages/app-store/vital/lib/reschedule.ts:125", + "created_at": null + }, + { + "path": "packages/app-store/wipemycalother/lib/reschedule.ts", + "line": 125, + "body": "Same forEach/async antipattern introduced here: bookingRefsFiltered.forEach(async (bookingRef) => {...}) with an internal await getCalendar(...) call is never awaited by forEach, so the try/catch wrapping it can't catch rejections and the function can return before calendar deletion finishes. Location: packages/app-store/wipemycalother/lib/reschedule.ts:125", + "created_at": null + }, + { + "path": "packages/trpc/server/routers/viewer/bookings.tsx", + "line": 553, + "body": "bookingRefsFiltered.forEach() was converted to an async callback with `await getCalendar(...)` inside, but forEach does not await the callback's promise, so calendar deletions during reschedule are fire-and-forget and any thrown errors become unhandled promise rejections instead of being handled by the enclosing logic. Location: packages/trpc/server/routers/viewer/bookings.tsx:553", + "created_at": null + } + ] } ] }