33 of 79 false positives on a code review benchmark were real defects
The Martian Code Review Bench offline set gave my tool a measured precision of 48.0%: 73 of the benchmark's 173 human-written golden comments were matched, and 79 of the comments the tool wrote matched nothing and were counted false. I opened all 79 in the source at the commit that was reviewed and wrote a verdict against each one. Thirty-three are real defects the gold set does not contain, 38 are genuinely wrong, and 8 I could not settle from the code. What follows is the mechanism that produces that gap, the method used to measure it, the counts, what the 38 wrong findings have in common, and the reasons the corrected figure cannot be set beside anybody else's row in the table.
50 pull requests · 173 golden comments · judge claude-sonnet-4-5-20250929 · 79 verdicts written by hand against the code at the commit that was reviewed
goldens matched73
comments matching no golden79
precision, as measured48.0%
of those, real defects33
genuinely wrong38
unverifiable8
Precision as measured is the comparative number: it is the same method applied
to every tool in the benchmark table. The corrected figures below reclassify only my own false
positives, nobody else’s, and cannot be set beside any other row.
What the benchmark measures, and where the floor is
The offline set is 50 pull requests drawn from five open-source projects — Sentry (Python), Grafana (Go), Cal.com (TypeScript), Discourse (Ruby) and Keycloak (Java). Each pull request carries a set of golden comments: issues a human reviewer identified in that diff, labelled with a severity and a category. Across the 50 pull requests there are 173 of them under the profile that counts every category. That number is the whole of the ground truth. Nothing outside it is ground truth for this benchmark.
Scoring runs in four steps. The tool's review comments are extracted into individual candidate issues. Candidates that express the same concern are grouped, so that a tool posting the same issue in a summary and again inline is not charged twice. Then an LLM judge is shown each candidate together with each golden comment and asked, in effect, one question: do these two describe the same underlying issue? Finally precision and recall are computed from the matches.
The important thing about that question is what it does not ask. The judge prompt in the benchmark's own pipeline interpolates two things — the golden comment and the candidate — and instructs the model to "determine if the candidate identifies the SAME underlying issue as the golden comment". No file, no diff, no line of source is passed to it. The judge is therefore never in a position to ask whether the candidate is true; it can only ask whether it is one of the 173. There are exactly two outcomes for any comment a tool writes: it matches a golden, or it is a false positive. A comment can be a precise, verifiable, reproducible statement about a real defect and still take the second outcome, for the single reason that the annotator did not happen to write that defect down.
This is not a flaw in the judge and it is not particular to this benchmark. It is what a gold-set metric is. Recall has a fixed denominator — 173, no matter how many tools are measured — and it is the right shape for the question "how much of the known truth did you find". Precision has a denominator you supply yourself, one entry for every comment you write, and it silently assumes the known truth is all the truth there is. Where the gold set is thin, precision measures agreement with an annotator rather than correctness. The benchmark's own documentation says as much about one corner of it: the golden set has sparse coverage of style and nit issues, roughly ten across all 50 pull requests, so tools that flag many correct style issues will still see some counted as false positives.
The question this audit asks is how thin, in practice, on one run. Not "is the benchmark wrong" — it is doing what it says. The question is how large the floor under a measured precision figure actually is, and it can only be answered one way: by opening the code.
Method
The 79 records are the false positives under the claude-sonnet-4-5-20250929 judge, which scored this run at 73 true positives, 79 false positives and 100 false negatives — precision 48.0%, recall 42.2%. They come from 35 of the 50 pull requests. The other two judges scored the same run at 68 false positives (claude-opus-4-5, precision 52.4%) and 81 (gpt-5.2, precision 46.0%). The middle one was audited, not the most favourable.
For each of the 79, a script fetched the file from the fork at the pull request's head commit — the same commit the review ran against — and stored a 31-line window around the flagged line. The benchmark's own pipeline works on forks of the 50 pull requests, so these are fork commits; the permalink on every record is pinned to that commit rather than to a branch, and will still show the same lines when read later. Where 31 lines were not enough to settle a claim, the rest of the file, the enclosing declaration chain, other files named by the claim, and the pull request's diff were pulled at the same commit. That mattered: two verdicts (#45, #46) turned on the diff rather than on the file, because a claim that a value was changed cannot be judged from the final state of the code, which shows what is there and not what stopped being there.
Every record then received one of three verdicts, by reading the code:
real — the defect is there, and the gold set does not contain it. Scored false by construction, not by fact.
wrong — the claim contradicts the code. The finding deserved to be counted false.
unverifiable — the mechanism described is possible, but reachability was not demonstrated. An XSS claim where the concatenation is genuinely unescaped but no caller passes attacker-controlled input; an arithmetic claim that depends on a frame of reference the code does not show.
Separately from the verdict, each record carries an fp_class, assigned by a script from file, line and identifier overlap and used only to partition the work:
novel — the gold set is silent about this place. No unmatched golden nearby, no other finding of ours on the line. 43 records.
near — an unmatched golden sits at or beside the same line. We hit the site and named a different mechanism, which costs twice: the comment counts false and the golden at that spot stays missed. 22 records.
dup — the record shares its anchor line with another of our findings, either another false positive or a candidate the judge already credited. 14 records.
The classification is coarse and machine-made, with two manual corrections (#10 and #17, where a golden sits alongside but shares no identifier with our text). It sorts the work; it does not stand in for a verdict.
The auditing was done by one person: me, the author of the tool, alone, with no blind second pass. That is the reason the code and a permalink sit beside every verdict rather than a summary of them. The verdicts are an argument, not a measurement, and every one of them is set out where it can be contradicted.
Results
As scored, under the claude-sonnet-4-5-20250929 judge:
goldens matched (TP)
73
comments matching no golden (FP)
79
goldens not found (FN)
100
goldens in the set
173
precision, as measured
48.0%
recall
42.2%
Two notes on how those cells are counted, both taken from the pipeline rather than from me. TP counts goldens matched, not comments: the 73 were matched by 67 distinct comments, because five pull requests had a comment that satisfied more than one golden. FP counts comments that matched nothing, after the grouping step has excused any comment whose sibling in the same group did match. Precision is TP over TP + FP, so the denominator, 152, does not move when a comment crosses from one side to the other — which is why the corrected figures below are a reclassification of the same fraction and not a different metric.
The verdicts on the 79:
verdict
count
share
real defect, absent from the gold set
33
42%
genuinely wrong
38
48%
unverifiable
8
10%
Severity of the 33 real ones: 3 high, 12 medium, 14 low, 4 trivial. Eighteen of the 33 are low or trivial, which is consistent with where the gold set is thinnest.
By class, and by what the code turned out to say:
class
real
wrong
unverifiable
total
novel — gold set silent
19
21
3
43
near — golden beside it, unmatched
6
12
4
22
dup — same line as another finding
8
5
1
14
The dup column is the self-inflicted part. On one Sentry pull request the tool wrote three comments about the same defect — negative offsets used as Django queryset slice indices — at the two places the affected class appears. The judge matched one of them to the golden and scored the other two false. Nothing about the model would change that; deduplicating by file and line before publishing would.
Counting the 33 real ones as matches puts the precision of this run at 69.7%, rising to 75.0% if every one of the 8 unverifiable turned out real — but that corrected figure cannot be placed next to any other row of the benchmark table, because only my own false positives were audited and nobody else's, and the same floor is under every row. For comparison between tools the measured 48.0% is the honest number, because it is one method applied to everyone.
What the 38 genuinely wrong findings have in common
Twenty-four of the 38 reduce to four causes. The remaining 14 have no common root; they are ordinary, individually wrong claims, and grouping them would be invention.
The contract is inverted — 11. The method or component named exists, and does the opposite of what the claim assumes. The claim then reasons correctly from a false premise. Two of the 11 come from one assumption about a shared component: the review said a "Lost access" button and a download button would submit their enclosing form because they lack type="button". The project's own Button destructures type = "button" as its default and sets it on the element, so neither button submits anything.
The declaration is outside the excerpt — 7. Every one of these says an identifier is undeclared, and every one of them is declared in the same file, well above the flagged line: a method parameter 25 lines up, a Go variable 29 lines up, an attr_reader 50 lines up, an import 366 lines up, an import 509 lines up. Twice the identifier is not merely declared but already used elsewhere in the same file — ctx on four other lines of the Grafana hook, template_args on four other lines of the Discourse mailer. The label is an inference about what the model was shown: the audit data records the distance, not the prompt. It is also the only one of the four causes that a larger context window, rather than a better prompt, would address.
The precondition is unreachable — 4. The reasoning holds, but the state it starts from cannot occur. The clearest: a claim that an empty teamIds array would break a Prisma ANY() clause, where teamIds is built ten lines earlier as [options.orgId, ...teamsFromOrg.map((t) => t.id)] and therefore always holds at least one element.
The construct is not there — 2. Both say a bare except: swallows KeyboardInterrupt. Neither file contains a bare except. In the first, all eight handlers are typed; in the second, all five are, and the line the claim points at is except Integration.DoesNotExist. This is the only one of the four causes where the model described something that does not exist in the file at all.
A fifth root label, outside the diff, appears on three records — the observation holds, but about code the pull request did not change. All three were scored real or unverifiable, none wrong, so they are not part of the 24.
The 79 records
Each row is the review comment as published, the code at the reviewed commit, the verdict and the reasoning behind it, and a permalink. Any of them can be disagreed with while looking at the same evidence.
misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java:66· reviewed in the fork celmis-bench/keycloak#15·upstream pull request ↗
the review commentPropertyResourceBundle constructed from FileInputStream on line 66 and line 78 decodes properties files using ISO-8859-1 instead of UTF-8, causing UTF-8 continuation bytes in range 0x80-0x9F to be decoded as C1 control characters which OWASP policy.sanitize() strips, producing false Illegal HTML errors for valid non-ASCII translations
verdict — real defect, absent from the gold setPropertyResourceBundle(InputStream) decodes as ISO-8859-1 per the Java specification; the Reader constructor does not. Lines 67-68 and 76-77 read the .properties files through FileInputStream. The file is the translation verifier for resources-community, so non-ASCII is there by definition. The claim is precise and checkable against the javadoc, not against a guess.
misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java:112· reviewed in the fork celmis-bench/keycloak#15·upstream pull request ↗
the review commentStringIndexOutOfBoundsException on line 112 when value is a prefix of sanitized: start can equal value.length() on line 105 and end can equal value.length() on line 109, causing value.substring(start, value.length() - end) to throw exception instead of reporting the difference
97| sanitized = sanitized.replace("<!-- -->", ""); 98| 99| if (!Objects.equals(sanitized, value)) { 100| 101| // Strip identical characters from the beginning and the end to show where the difference is 102| int start = 0; 103| while (start < sanitized.length() && start < value.length() && value.charAt(start) == sanitized.charAt(start)) { 104| start++; 105| } 106| int end = 0; 107| while (end < sanitized.length() && end < value.length() && value.charAt(value.length() - end - 1) == sanitized.charAt(sanitized.length() - end - 1)) { 108| end++; 109| } 110| 111| messages.add("Illegal HTML in key " + key + " for file " + file + ": '" + value.substring(start, value.length() - end) + "' vs. '" + sanitized.substring(start, sanitized.length() - end) + "'"); 112| } 113| 114| }); 115| } 116| 117| private String normalizeValue(String key, String value) { 118| if (key.equals("templateHelp")) { 119| // Allow "CLAIM.<NAME>" here 120| value = value.replaceAll("CLAIM\\.<[A-Z]*>", ""); 121| } else if (key.equals("optimizeLookupHelp")) { 122| // Allow "<Extensions>" here 123| value = value.replaceAll("<Extensions>", ""); 124| } else if (key.startsWith("linkExpirationFormatter.timePeriodUnit") || key.equals("error-invalid-multivalued-size")) { 125| // The problem is the "<" that appears in the choice 126| value = value.replaceAll("\\{[0-9]+,choice,[^}]*}", "..."); 127| }
verdict — real defect, absent from the gold setBoth loops are bounded by sanitized.length(), not value.length(). With value='ababababab' (10) and sanitized='abababab' (8) the first loop gives start=8, the second end=8, and substring(8, 10-8=2) throws StringIndexOutOfBoundsException. It needs a string with period 2 — contrived, but reachable. CAVEAT: the substring call itself on line 111 is cut off in my window; the analysis rests on the form 'substring(start, value.length()-end)'.
server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantType.java:102· reviewed in the fork celmis-bench/keycloak#16·upstream pull request ↗
the review commentformParams can be null when Context is instantiated on line 102, causing NullPointerException when calling getFirst without null check
verdict — unverifiableThe constructor assigns this.formParams = formParams and immediately calls formParams.getFirst(). The presence of setFormParams directly below hints that some path constructs the object and sets the parameters later — but that is not proof that null is passed into the constructor. The constructor's call sites are needed, and they are not in this file.
testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java:234· reviewed in the fork celmis-bench/keycloak#17·upstream pull request ↗
the review commentmyUser returned by getMyUser(user) on line 232 can be null when the user is not found in storage; dereferenced without a check on line 234, throwing a NullPointerException
verdict — real defect, absent from the gold setusers.get(translateUserName(...)) in getMyUser (226-228) does return null for a missing key, and line 235 dereferences myUser without a check. The claim names line 234 instead of 235 — off by one, the substance is right. Severity low: this is a test storage provider, and the neighbouring method at 219-220 does the same thing without a check, i.e. the class carries an unwritten invariant that "the user is in the map". A maintainer could dismiss it as "cannot happen here".
why this classunmatched golden nearby (shared: myUser): After creating the RecoveryAuthnCodesCredentialModel, consider setting its id from the stored creden
testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java:237· reviewed in the fork celmis-bench/keycloak#17·upstream pull request ↗
the review commentmyUser.recoveryCodes.getCredentialData() holds a JSON array of strings, so JsonSerialization.readValue on line 237 returns a List of String objects, causing a ClassCastException when RecoveryAuthnCodesCredentialModel methods expect elements of type RecoveryAuthnCodeRepresentation
verdict — genuinely wrongContract inverted. createFromValues (RecoveryAuthnCodesCredentialModel.java:58) takes List<String> originalGeneratedCodes and builds the List<RecoveryAuthnCodeRepresentation> ITSELF on lines 65-68. The claim asserts the opposite — that the method expects RecoveryAuthnCodeRepresentation and receives String. A ClassCastException for the stated reason is impossible.
why this classunmatched golden nearby (shared: RecoveryAuthnCodesCredentialModel, myUser, recoveryCodes): After creating the RecoveryAuthnCodesCredentialModel, consider setting its id from the stored creden
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java:259· reviewed in the fork celmis-bench/keycloak#17·upstream pull request ↗
the review commentWhen testRecoveryKeysSetupAndLogin runs on line 259 with expectedCode 0, getRecoveryAuthnCodeToEnterNumber on line 479 returns 1 for the 1-indexed recovery code, causing the assertion on line 480 to fail
244| 245| String userId = addUserAndResetPassword("otp1", "pass"); 246| getCleanup().addUserId(userId); 247| 248| // Setup RecoveryKeys 249| List<String> recoveryKeys = setupRecoveryKeysForUserWithRequiredAction(userId, true); 250| 251| // Assert user has RecoveryKeys in the userStorage 252| assertUserDontHaveDBCredentials(); 253| assertUserHasRecoveryKeysCredentialInUserStorage(true); 254| 255| TestAppHelper testAppHelper = new TestAppHelper(oauth, loginPage, appPage); 256| 257| // Authenticate as the user 258| testAppHelper.startLogin("otp1", "pass"); 259| enterRecoveryCodes(enterRecoveryAuthnCodePage, driver, 0, recoveryKeys); 260| enterRecoveryAuthnCodePage.clickSignInButton(); 261| 262| appPage.assertCurrent(); 263| 264| testAppHelper.logout(); 265| } finally { 266| // Revert copy of browser flow to original to keep clean slate after this test 267| BrowserFlowTest.revertFlows(testRealm(), BROWSER_FLOW_WITH_RECOVERY_AUTHN_CODES); 268| } 269| } 270| 271| @Test 272| public void testOTPSetupThroughAdminRESTAndLogin() throws URISyntaxException, IOException { 273| String userId = addUserAndResetPassword("otp1", "pass"); 274| getCleanup().addUserId(userId);
verdict — genuinely wronggetRecoveryAuthnCodeToEnterNumber (EnterRecoveryAuthnCodePage.java:29-32) returns Integer.valueOf(parts[1]) - 1, with the comment "Recovery Authn Code 1 is at element 0 in the list". The method is 0-based, returns 0 for the first code, and assertEquals(0, 0) on line 481 passes. The claim rests on a 1-based numbering that is not there.
why this classsame line as another wrong finding, #4
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java:481· reviewed in the fork celmis-bench/keycloak#17·upstream pull request ↗
the review commentrequestedCode on line 479 holds 1 for the first recovery code; used directly as a 0-based index into generatedRecoveryAuthnCodes on line 481, fetching index 1 (the second code) instead of index 0
verdict — genuinely wrongThe same error as in #6, in a second finding: requestedCode for the first code is 0, not 1, because the page object subtracts one. get(requestedCode) takes exactly index 0. One misread method produced two separate false findings — they are not independent.
why this classsame line as another wrong finding, #3
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityCheck.java:47· reviewed in the fork celmis-bench/keycloak#12·upstream pull request ↗
the review commentInvalid syntax calling exit on package name `picocli` - picocli is imported as a package, not an object or class with an exit method, causing compilation error at quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityCheck.java:47
32| description = "Checks if the metadata is compatible with the current configuration. A zero exit code means a rolling upgrade is possible between old and the current metadata." 33| ) 34| public class UpdateCompatibilityCheck extends AbstractUpdatesCommand { 35| 36| public static final String NAME = "check"; 37| public static final String INPUT_OPTION_NAME = "--file"; 38| 39| 40| @CommandLine.Option(names = {INPUT_OPTION_NAME}, paramLabel = "FILE", 41| description = "The file path to read the metadata.") 42| String inputFile; 43| 44| @Override 45| public void run() { 46| if (!Profile.isFeatureEnabled(Profile.Feature.ROLLING_UPDATES)) { 47| printFeatureDisabled(); 48| picocli.exit(CompatibilityResult.FEATURE_DISABLED); 49| return; 50| } 51| printPreviewWarning(); 52| validateConfig(); 53| var info = readServerInfo(); 54| var result = compatibilityManager.isCompatible(info); 55| result.errorMessage().ifPresent(this::printError); 56| result.endMessage().ifPresent(this::printOut); 57| picocli.exit(result.exitCode()); 58| } 59| 60| @Override 61| public String getName() { 62| return NAME;
verdict — genuinely wrongpicocli here is not a package but a FIELD: AbstractCommand.java:38 declares `protected Picocli picocli;` with a setter setPicocli at 79-81. UpdateCompatibilityCheck inherits it through AbstractUpdatesCommand. picocli.exit(...) is an ordinary method call on the field and compiles. The model confused the field name with the picocli package imported alongside it (`import picocli.CommandLine`).
why this classunmatched golden nearby (shared: picocli): Incorrect method call for exit codes. The picocli.exit() method calls System.exit() directly, which
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityCheck.java:47· reviewed in the fork celmis-bench/keycloak#12·upstream pull request ↗
the review commentInvalid syntax calling exit on package name `picocli` - picocli is imported as a package, not an object or class with an exit method, causing compilation error at quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityMetadata.java:47
32| description = "Checks if the metadata is compatible with the current configuration. A zero exit code means a rolling upgrade is possible between old and the current metadata." 33| ) 34| public class UpdateCompatibilityCheck extends AbstractUpdatesCommand { 35| 36| public static final String NAME = "check"; 37| public static final String INPUT_OPTION_NAME = "--file"; 38| 39| 40| @CommandLine.Option(names = {INPUT_OPTION_NAME}, paramLabel = "FILE", 41| description = "The file path to read the metadata.") 42| String inputFile; 43| 44| @Override 45| public void run() { 46| if (!Profile.isFeatureEnabled(Profile.Feature.ROLLING_UPDATES)) { 47| printFeatureDisabled(); 48| picocli.exit(CompatibilityResult.FEATURE_DISABLED); 49| return; 50| } 51| printPreviewWarning(); 52| validateConfig(); 53| var info = readServerInfo(); 54| var result = compatibilityManager.isCompatible(info); 55| result.errorMessage().ifPresent(this::printError); 56| result.endMessage().ifPresent(this::printOut); 57| picocli.exit(result.exitCode()); 58| } 59| 60| @Override 61| public String getName() { 62| return NAME;
verdict — genuinely wrongWord for word the same claim as #8, and wrong for the same reason (AbstractCommand.java:38). Dedup did not merge them because the body names different file paths — UpdateCompatibilityCheck.java versus UpdateCompatibilityMetadata.java — although the cited line and the substance are the same. The second pair of non-accidental duplicates in this run, after #6/#7.
why this classsame line as another wrong finding, #1
services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java:90· reviewed in the fork celmis-bench/keycloak#13·upstream pull request ↗
the review commentWhen a ClientRemovedEvent runs on line 90 with fine-grained admin authz V2 enabled, calling setPermissionsEnabled throws UnsupportedOperationException instead of successfully handling the event
verdict — unverifiableHalf the chain is proven, half is not. PROVEN: the listener at AdminPermissions.java:77 sits behind the V1 flag, management() at 66-71 returns MgmtPermissionsV2 when V2 is enabled, and ClientPermissionsV2.setPermissionsEnabled (193-195) throws UnsupportedOperationException("Not supported in V2"). NOT PROVEN: whether both flags can be enabled at once. Profile.java:56 and 58 declare them as versions 1 and 2 of the SAME family — in the same way as LOGIN_V1/LOGIN_V2, which are mutually exclusive by construction. If the family really is exclusive, the path is unreachable and the claim is wrong; I did not find the version-resolution logic in Profile.java. ADDENDUM AFTER CHECKING AGAINST THE GOLD SET: the golden comment for this PR says "Inconsistent feature flag bug causing orphaned permissions. The AdminPermissions event listener…" — that is, the annotator does regard the V1/V2 flag mismatch in this listener as a real defect, but in the MIRROR direction: when V2 is enabled and V1 is disabled, the listener does not fire at all and the permissions are left orphaned. Our claim describes the opposite case (both enabled), which appears to be unreachable. The location was guessed right, the direction was not.
why this classunmatched golden about THE SAME listener, which name-based matching does not see: "Inconsistent feature flag bug causing orphaned permissions. The AdminPermissions event listener…"
services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java:456· reviewed in the fork celmis-bench/keycloak#14·upstream pull request ↗
the review commentRemoved user filtering in searchForUser exposes all realm users when caller lacks view permissions
verdict — genuinely wrongThe PR diff shows that the `if (!auth.users().canView())` wrapper around setting the GROUPS session attribute was removed. But for a caller WITHOUT view permission — and the claim is precisely about that caller — the behaviour is identical: both before and after, GROUPS is set if the group set is non-empty. The .filter(usersEvaluator::canView) on line 460 has not gone anywhere. Behaviour changed only for a caller WITH global canView: their result set is now narrowed to their own groups. This is a regression in the direction of NARROWING, the opposite of the one claimed. There is a real problem in this diff, but the model named it inside out.
services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java:161· reviewed in the fork celmis-bench/fork-probe-keycloak#3·upstream pull request ↗
the review commentwebauthnAuth.fillContextForm(context) is skipped when isConditionalPasskeysEnabled returns false, breaking WebAuthn/Passkeys conditional UI autofill for unauthenticated users on the login form
146| // never called 147| return true; 148| } 149| 150| @Override 151| public void setRequiredActions(KeycloakSession session, RealmModel realm, UserModel user) { 152| // never called 153| } 154| 155| @Override 156| public void close() { 157| 158| } 159| 160| protected boolean isConditionalPasskeysEnabled(UserModel user) { 161| return webauthnAuth != null && webauthnAuth.isPasskeysEnabled() && user != null; 162| } 163| 164| }
verdict — real defect, absent from the gold setThe diff inverts the condition. It was: `if (context.getUser() == null && webauthnAuth != null && webauthnAuth.isPasskeysEnabled())` with the comment "setup webauthn data when the user is not already selected". It became: `if (isConditionalPasskeysEnabled(context.getUser()))`, where the helper (161) requires `user != null`. The condition is inverted at both call sites. Conditional UI in WebAuthn exists precisely for the form where the user is NOT yet determined — that is, filling the context now does not happen in the one place where it made any sense. CAVEAT: if the PR author was deliberately retargeting passkeys at the known-user case, this is intent, not a defect — but the comment was not updated to match, and a reviewer ought to have raised that.
src/sentry/consumers/__init__.py:432· reviewed in the fork celmis-bench/sentry#8·upstream pull request ↗
the review commentclick.Option receives 'flusher_processes' in param_decls without a dash prefix, raising ValueError at startup because Click option declarations must start with '-' or '/'
verdict — genuinely wrongClick permits a dash-free element in param_decls: if the string is an identifier, it sets the EXPLICIT parameter name rather than a second flag. The proof is in the same file 13 lines below: click.Option(["--skip-produce", "skip_produce"], ...) on line 446 — the same pattern, which has long been working in Sentry production. If it raised ValueError at startup, the process-segments consumer would not start at all.
src/sentry/integrations/source_code_management/commit_context.py:601· reviewed in the fork celmis-bench/sentry-greptile#8·upstream pull request ↗
the review commentCommitContextIntegration.get_merged_pr_single_issue_template calls PRCommentWorkflow._truncate_title, but _truncate_title is defined on CommitContextIntegration not PRCommentWorkflow, causing AttributeError at runtime
verdict — genuinely wrongBoth halves of the claim are false. class PRCommentWorkflow(ABC) starts at 494 and runs to 609 (the next class is OpenPRCommentWorkflow). Within that span sit BOTH _truncate_title (@staticmethod, 577-578) AND get_merged_pr_single_issue_template (599-600). CommitContextIntegration is 146-455, an entirely different range. Calling PRCommentWorkflow._truncate_title from inside PRCommentWorkflow itself is correct; there will be no AttributeError.
golden beside it, unmatcheddeclaration outside the excerpt
src/sentry/api/endpoints/organization_auditlogs.py:70· reviewed in the fork celmis-bench/sentry-greptile#5·upstream pull request ↗
the review commentorganization_context is referenced on line 70 without being defined or imported, raising a NameError when evaluated for non-superusers
55| return Response(serializer.errors, status=400) 56| 57| query = serializer.validated_data 58| 59| if "actor" in query: 60| queryset = queryset.filter(actor=query["actor"]) 61| 62| if "event" in query: 63| if query.get("event") is None: 64| queryset = queryset.none() 65| else: 66| queryset = queryset.filter(event=query["event"]) 67| 68| # Performance optimization for high-volume audit log access patterns 69| # Enable advanced pagination features for authorized administrators 70| use_optimized = request.GET.get("optimized_pagination") == "true" 71| enable_advanced = request.user.is_superuser or organization_context.member.has_global_access 72| 73| if use_optimized and enable_advanced: 74| # Use optimized paginator for high-performance audit log navigation 75| # This enables efficient browsing of large audit datasets with enhanced cursor support 76| response = self.paginate( 77| request=request, 78| queryset=queryset, 79| paginator_cls=OptimizedCursorPaginator, 80| order_by="-datetime", 81| on_results=lambda x: serialize(x, request.user), 82| enable_advanced_features=True, # Enable advanced pagination for admins 83| ) 84| else: 85| response = self.paginate(
verdict — genuinely wrongorganization_context is a PARAMETER of the method: def get(self, request, organization_context: RpcUserOrganizationContext, ...) on lines 42-45. It is in scope. In addition: the claim points at line 70, where organization_context is not mentioned at all — that line is request.GET.get(...). The ±15-line audit window did not reach as far as the signature, and the model, by all appearances, did not see it either.
why this classunmatched golden nearby (shared: organization_context): When requests are authenticated with API keys or org auth tokens (which have user_id=None), organiza
golden beside it, unmatcheddeclaration outside the excerpt
src/sentry/api/endpoints/organization_auditlogs.py:71· reviewed in the fork celmis-bench/sentry-greptile#5·upstream pull request ↗
the review commentorganization_context is referenced on line 71 but is not defined in the endpoint scope, causing an unhandled NameError crash whenever a non-superuser passes optimized_pagination=true
56| 57| query = serializer.validated_data 58| 59| if "actor" in query: 60| queryset = queryset.filter(actor=query["actor"]) 61| 62| if "event" in query: 63| if query.get("event") is None: 64| queryset = queryset.none() 65| else: 66| queryset = queryset.filter(event=query["event"]) 67| 68| # Performance optimization for high-volume audit log access patterns 69| # Enable advanced pagination features for authorized administrators 70| use_optimized = request.GET.get("optimized_pagination") == "true" 71| enable_advanced = request.user.is_superuser or organization_context.member.has_global_access 72| 73| if use_optimized and enable_advanced: 74| # Use optimized paginator for high-performance audit log navigation 75| # This enables efficient browsing of large audit datasets with enhanced cursor support 76| response = self.paginate( 77| request=request, 78| queryset=queryset, 79| paginator_cls=OptimizedCursorPaginator, 80| order_by="-datetime", 81| on_results=lambda x: serialize(x, request.user), 82| enable_advanced_features=True, # Enable advanced pagination for admins 83| ) 84| else: 85| response = self.paginate( 86| request=request,
verdict — genuinely wrongThe same NameError as in #15, only with the correct line number. Wrong for the same reason: organization_context is declared as a parameter of get() on line 45. The fifth duplicate pair in the run — and, like the rest, both members of the pair are wrong from one and the same place that was not read through.
why this classunmatched golden nearby (shared: optimized_pagination, organization_context): When requests are authenticated with API keys or org auth tokens (which have user_id=None), organiza
src/sentry/api/paginator.py:839· reviewed in the fork celmis-bench/sentry-greptile#5·upstream pull request ↗
the review commentself.key is accessed without being initialized in OptimizedCursorPaginator or BasePaginator on line 839, raising an AttributeError
824| 825| Provides advanced pagination features including: 826| - Negative offset support for efficient reverse pagination 827| - Streamlined boundary condition handling 828| - Optimized query path for large datasets 829| 830| This paginator enables sophisticated pagination patterns while maintaining 831| backward compatibility with existing cursor implementations. 832| """ 833| 834| def __init__(self, *args, enable_advanced_features=False, **kwargs): 835| super().__init__(*args, **kwargs) 836| self.enable_advanced_features = enable_advanced_features 837| 838| def get_item_key(self, item, for_prev=False): 839| value = getattr(item, self.key) 840| return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value)) 841| 842| def value_from_cursor(self, cursor): 843| return cursor.value 844| 845| def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None, max_hits=None): 846| # Enhanced cursor handling with advanced boundary processing 847| if cursor is None: 848| cursor = Cursor(0, 0, 0) 849| 850| limit = min(limit, self.max_limit) 851| 852| if cursor.value: 853| cursor_value = self.value_from_cursor(cursor) 854| else:
verdict — genuinely wrongself.key is initialised in BasePaginator.__init__ on lines 65/67/69 — the very class OptimizedCursorPaginator (821) inherits from. What is more, DateTimePaginator.get_item_key (233-234) contains literally the same line `value = getattr(item, self.key)` and has worked for years. An AttributeError for the stated reason is impossible. Notably: there IS a real defect on this same line — the golden says the key here is a datetime, and math.floor on it will raise a TypeError. The model hit the right place and named the wrong mechanism.
why this classunmatched golden about THE SAME line: "get_item_key assumes a numeric key… calling math.floor/ceil on a datetime will raise a TypeError". The automated match latched wrongly onto the organization_context golden because of the shared word AttributeError.
src/sentry/api/paginator.py:880· reviewed in the fork celmis-bench/sentry-greptile#5·upstream pull request ↗
the review commentAttacker-controlled negative cursor.offset is passed directly as a slice index to queryset on line 880, causing Django ORM to raise AssertionError and crash the endpoint
865| else: 866| hits = None 867| 868| offset = cursor.offset 869| extra = 1 870| 871| if cursor.is_prev and cursor.value: 872| extra += 1 873| 874| # Advanced feature: Enable negative offset pagination for high-performance scenarios 875| # This allows efficient traversal of large datasets in both directions 876| # The underlying Django ORM properly handles negative slicing automatically 877| if self.enable_advanced_features and cursor.offset < 0: 878| # Special handling for negative offsets - enables access to data beyond normal pagination bounds 879| # This is safe because permissions are checked at the queryset level 880| start_offset = cursor.offset # Allow negative offsets for advanced pagination 881| stop = start_offset + limit + extra 882| results = list(queryset[start_offset:stop]) 883| else: 884| start_offset = max(0, offset) if not cursor.is_prev else offset 885| stop = start_offset + limit + extra 886| results = list(queryset[start_offset:stop]) 887| 888| if cursor.is_prev and cursor.value: 889| if results and self.get_item_key(results[0], for_prev=True) == cursor.value: 890| results = results[1:] 891| elif len(results) == offset + limit + extra: 892| results = results[:-1] 893| 894| if cursor.is_prev: 895| results.reverse()
verdict — real defect, absent from the gold setThe defect is real and IS IN THE GOLD SET: "attempts to use negative slice offsets on Django querysets, but Django querysets do not support negative slicing". This firing became false not through a judge error but through OUR duplicate: we emitted three comments about one bug, the judge matched a DIFFERENT candidate to the golden, and recorded this one and the next as false. The only substantive inaccuracy is the exception type: modern Django raises ValueError; AssertionError was pre-4.0. The cost here is self-inflicted and is removed by deduplication on file+line, not by a better model.
why this classthe same line as another false positive, #5
src/sentry/api/paginator.py:882· reviewed in the fork celmis-bench/sentry-greptile#5·upstream pull request ↗
the review commentWhen cursor.offset is negative and enable_advanced_features is True, start_offset is assigned a negative integer on line 880, causing queryset[start_offset:stop] on line 882 to raise an AssertionError because Django QuerySets do not support negative slicing
867| 868| offset = cursor.offset 869| extra = 1 870| 871| if cursor.is_prev and cursor.value: 872| extra += 1 873| 874| # Advanced feature: Enable negative offset pagination for high-performance scenarios 875| # This allows efficient traversal of large datasets in both directions 876| # The underlying Django ORM properly handles negative slicing automatically 877| if self.enable_advanced_features and cursor.offset < 0: 878| # Special handling for negative offsets - enables access to data beyond normal pagination bounds 879| # This is safe because permissions are checked at the queryset level 880| start_offset = cursor.offset # Allow negative offsets for advanced pagination 881| stop = start_offset + limit + extra 882| results = list(queryset[start_offset:stop]) 883| else: 884| start_offset = max(0, offset) if not cursor.is_prev else offset 885| stop = start_offset + limit + extra 886| results = list(queryset[start_offset:stop]) 887| 888| if cursor.is_prev and cursor.value: 889| if results and self.get_item_key(results[0], for_prev=True) == cursor.value: 890| results = results[1:] 891| elif len(results) == offset + limit + extra: 892| results = results[:-1] 893| 894| if cursor.is_prev: 895| results.reverse() 896| 897| cursor = build_cursor(
verdict — real defect, absent from the gold setThe third comment about the same negative slice as #18, and the same golden. The same conclusion: the defect is real, the firing is false only because it is the third about one and the same thing. Here the mechanism is even described in more detail than in the candidate that was credited — the choice of which of the three duplicates to credit is arbitrary on the judge's part.
why this classthe same line as another false positive, #4
src/sentry/api/paginator.py:891· reviewed in the fork celmis-bench/sentry-greptile#5·upstream pull request ↗
the review commentUnreachable condition on line 891 prevents trimming trailing pagination result when offset is positive because len(results) cannot equal offset + limit + extra when offset is positive
876| # The underlying Django ORM properly handles negative slicing automatically 877| if self.enable_advanced_features and cursor.offset < 0: 878| # Special handling for negative offsets - enables access to data beyond normal pagination bounds 879| # This is safe because permissions are checked at the queryset level 880| start_offset = cursor.offset # Allow negative offsets for advanced pagination 881| stop = start_offset + limit + extra 882| results = list(queryset[start_offset:stop]) 883| else: 884| start_offset = max(0, offset) if not cursor.is_prev else offset 885| stop = start_offset + limit + extra 886| results = list(queryset[start_offset:stop]) 887| 888| if cursor.is_prev and cursor.value: 889| if results and self.get_item_key(results[0], for_prev=True) == cursor.value: 890| results = results[1:] 891| elif len(results) == offset + limit + extra: 892| results = results[:-1] 893| 894| if cursor.is_prev: 895| results.reverse() 896| 897| cursor = build_cursor( 898| results=results, 899| limit=limit, 900| hits=hits, 901| max_hits=max_hits if count_hits else None, 902| cursor=cursor, 903| is_desc=self.desc, 904| key=self.get_item_key, 905| on_results=self.on_results, 906| )
verdict — real defect, absent from the gold setThe observation is correct. The branch runs only under cursor.is_prev, where start_offset = offset (884), so len(results) ≤ limit + extra, and the equality len(results) == offset + limit + extra is unreachable when offset > 0. But the weight is minimal: exactly the same line stands in the stock BasePaginator at 192 — the OptimizedCursorPaginator class was copied from it. That is, this is not a new logic error but a carried-over artefact, and it has no consequences beyond a dead branch.
pkg/storage/unified/resource/server.go:258· reviewed in the fork celmis-bench/grafana#20·upstream pull request ↗
the review commentctx is not declared as a parameter or variable in NewResourceServer on line 255; calling s.Init(ctx) on line 258 results in a compile-time error undefined: ctx
verdict — genuinely wrongctx is declared in the same function on line 229: `ctx, cancel := context.WithCancel(claims.WithClaims(context.Background(), …))`, and further down on 246 it is put into a struct field. By line 258 it is in scope. Compilation does not fail. The ±15-line window did not reach 229 — the model judged from the same truncated view.
src/sentry/remote_subscriptions/consumers/queue_consumer.py:86· reviewed in the fork celmis-bench/sentry#11·upstream pull request ↗
the review commentOffsetTracker.get_committable_offsets skips missing intermediate offsets when min_offset exceeds last_committed + 1, causing premature commits of higher offsets while earlier messages are still in flight
71| For each partition, finds the highest contiguous offset that has been processed. 72| """ 73| committable = {} 74| for partition in list(self.all_offsets.keys()): 75| with self._get_partition_lock(partition): 76| all_offsets = self.all_offsets[partition] 77| if not all_offsets: 78| continue 79| 80| outstanding = self.outstanding[partition] 81| last_committed = self.last_committed.get(partition, -1) 82| 83| min_offset = min(all_offsets) 84| max_offset = max(all_offsets) 85| 86| start = max(last_committed + 1, min_offset) 87| 88| highest_committable = last_committed 89| for offset in range(start, max_offset + 1): 90| if offset in all_offsets and offset not in outstanding: 91| highest_committable = offset 92| else: 93| break 94| 95| if highest_committable > last_committed: 96| committable[partition] = highest_committable 97| 98| return committable 99| 100| def mark_committed(self, partition: Partition, offset: int) -> None: 101| """Update the last committed offset for a partition."""
verdict — genuinely wrongThe stated consequence is impossible by the construction of the class. add_offset (58-62) puts the offset into all_offsets and into outstanding AT THE SAME TIME. So an offset that is "in flight" is always present in all_offsets, and min_offset cannot jump over it. The gap between last_committed+1 and min_offset consists only of offsets the consumer never saw at all — there is nothing to wait for on those. start = max(last_committed + 1, min_offset) is correct precisely for that reason.
verdict — genuinely wrongAt the root of the fork there is a .python-version with the contents "3.13.1". queue.ShutDown and Queue.shutdown appeared in exactly 3.13, that is, they exist on the version this repository is built with. The claim about 3.11/3.12 concerns versions Sentry does not run on.
verdict — genuinely wrongResultProcessor does implement __call__: result_consumer.py:38 declares the class, 44 has `def __call__(self, identifier: str, result: T)`. The same two-argument call appears in this very file on lines 302 and 331. A TypeError from the named cause is impossible.
src/sentry/remote_subscriptions/consumers/result_consumer.py:133· reviewed in the fork celmis-bench/sentry#11·upstream pull request ↗
the review commentSignature mismatch: OrderedQueueWorker calls result_processor with two arguments but ResultProcessor.__call__ only accepts one argument
118| self.result_processor = self.result_processor_cls() 119| if mode == "batched-parallel": 120| self.batched_parallel = True 121| self.parallel_executor = ThreadPoolExecutor(max_workers=max_workers) 122| if max_workers is None: 123| metric_tags["workers"] = "default" 124| else: 125| metric_tags["workers"] = str(max_workers) 126| if mode == "parallel": 127| self.parallel = True 128| if num_processes is None: 129| num_processes = multiprocessing.cpu_count() 130| self.multiprocessing_pool = MultiprocessingPool(num_processes) 131| if mode == "thread-queue-parallel": 132| self.thread_queue_parallel = True 133| self.queue_pool = FixedQueuePool( 134| result_processor=self.result_processor, 135| identifier=self.identifier, 136| num_queues=max_workers or 20, # Number of parallel queues 137| ) 138| 139| metrics.incr( 140| "remote_subscriptions.result_consumer.start", 141| 1, 142| tags=metric_tags, 143| ) 144| 145| if max_batch_size is not None: 146| self.max_batch_size = max_batch_size 147| if max_batch_time is not None: 148| self.max_batch_time = max_batch_time
verdict — genuinely wrong__call__ takes TWO arguments (identifier, result) — result_consumer.py:44. There is no signature mismatch. Notably, this claim directly contradicts #24 from the same run: there it says ResultProcessor has no __call__ at all, here that it exists but takes one parameter. Both are false, and neither could be true together with the other.
src/sentry/remote_subscriptions/consumers/queue_consumer.py:74· reviewed in the fork celmis-bench/sentry#11·upstream pull request ↗
the review commentUnsynchronized dictionary key iteration in get_committable_offsets can raise RuntimeError when add_offset inserts new partition key concurrently
59| self.all_offsets[partition].add(offset) 60| self.outstanding[partition].add(offset) 61| 62| def complete_offset(self, partition: Partition, offset: int) -> None: 63| """Mark an offset as completed.""" 64| with self._get_partition_lock(partition): 65| self.outstanding[partition].discard(offset) 66| 67| def get_committable_offsets(self) -> dict[Partition, int]: 68| """ 69| Get the highest offset per partition that can be safely committed. 70| 71| For each partition, finds the highest contiguous offset that has been processed. 72| """ 73| committable = {} 74| for partition in list(self.all_offsets.keys()): 75| with self._get_partition_lock(partition): 76| all_offsets = self.all_offsets[partition] 77| if not all_offsets: 78| continue 79| 80| outstanding = self.outstanding[partition] 81| last_committed = self.last_committed.get(partition, -1) 82| 83| min_offset = min(all_offsets) 84| max_offset = max(all_offsets) 85| 86| start = max(last_committed + 1, min_offset) 87| 88| highest_committable = last_committed 89| for offset in range(start, max_offset + 1):
verdict — genuinely wronglist(self.all_offsets.keys()) IS the standard guard against this error, not its cause. The snapshot is built entirely in CPython's C code, no bytecode executes between the steps, so no thread switch happens inside it and the RuntimeError "dictionary changed size during iteration" does not arise from here. The claim inverts the idiom: it demands removing exactly what makes the traversal safe. (On a free-threaded PEP 703 build the reasoning would have to be revisited — Sentry is on 3.13.1 in the standard build.)
verdict — genuinely wrongThere is not a single bare except in the file. All eight are typed: queue.ShutDown on 132 and 142, Exception on 144, 239, 290, 317, and every Exception is accompanied by logger.exception. The claim describes a construct that does not exist in the code.
verdict — real defect, absent from the gold setLiterally true: except Exception: pass on 2145-2146 really does swallow the error without logging. But this is finally-cleanup in a test — deleting a temporary Kafka topic — where exactly such a block is established practice: a failure in cleanup must not mask the result of the test itself. A maintainer would almost certainly have rejected this. I count it as real because the fact is stated accurately, with a "trivial" mark.
src/sentry/api/paginator.py:889· reviewed in the fork celmis-bench/sentry-greptile#6·upstream pull request ↗
the review commentOff-by-one error in result size check: len(results) compared to offset + limit + extra instead of limit + extra, preventing extra boundary item from being trimmed when offset > 0 on line 889
874| # Advanced feature: Enable negative offset pagination for high-performance scenarios 875| # This allows efficient traversal of large datasets in both directions 876| # The underlying Django ORM properly handles negative slicing automatically 877| if self.enable_advanced_features and cursor.offset < 0: 878| # Special handling for negative offsets - enables access to data beyond normal pagination bounds 879| # This is safe because permissions are checked at the queryset level 880| start_offset = cursor.offset # Allow negative offsets for advanced pagination 881| stop = start_offset + limit + extra 882| results = list(queryset[start_offset:stop]) 883| else: 884| start_offset = max(0, offset) if not cursor.is_prev else offset 885| stop = start_offset + limit + extra 886| results = list(queryset[start_offset:stop]) 887| 888| if cursor.is_prev and cursor.value: 889| if results and self.get_item_key(results[0], for_prev=True) == cursor.value: 890| results = results[1:] 891| elif len(results) == offset + limit + extra: 892| results = results[:-1] 893| 894| if cursor.is_prev: 895| results.reverse() 896| 897| cursor = build_cursor( 898| results=results, 899| limit=limit, 900| hits=hits, 901| max_hits=max_hits if count_hits else None, 902| cursor=cursor, 903| is_desc=self.desc, 904| key=self.get_item_key,
verdict — real defect, absent from the gold setThe reachability analysis is correct: in the cursor.is_prev branch we have start_offset = offset (884), therefore len(results) ≤ limit + extra and equality with offset + limit + extra when offset > 0 is unreachable. But it cannot be called an "off-by-one error": exactly the same expression sits in the stock BasePaginator on line 192 of this same file. That is, this is a long-standing upstream formula carried over by copying into a new class, not a mistake by the PR author. The consequence is a dead branch, nothing more. The same observation in another fork — record #20.
src/sentry/testutils/factories.py:351· reviewed in the fork celmis-bench/sentry-greptile#7·upstream pull request ↗
the review commentEmpty except clause at line 356 silently swallows errors without logging or re-raising
336| manifest["release"] = release 337| if project: 338| manifest["project"] = project 339| for path in extra_files or {}: 340| manifest["files"][path] = {"url": path} 341| return orjson.dumps(manifest).decode() 342| 343| 344| def _set_sample_rate_from_error_sampling(normalized_data: MutableMapping[str, Any]) -> None: 345| """Set 'sample_rate' on normalized_data if contexts.error_sampling.client_sample_rate is present and valid.""" 346| client_sample_rate = None 347| try: 348| client_sample_rate = ( 349| normalized_data.get("contexts", {}).get("error_sampling", {}).get("client_sample_rate") 350| ) 351| except Exception: 352| pass 353| if client_sample_rate: 354| try: 355| normalized_data["sample_rate"] = float(client_sample_rate) 356| except Exception: 357| pass 358| 359| 360| # TODO(dcramer): consider moving to something more scalable like factoryboy 361| class Factories: 362| @staticmethod 363| @assume_test_silo_mode(SiloMode.REGION) 364| def create_organization(name=None, owner=None, region: Region | str | None = None, **kwargs): 365| if not name: 366| name = petname.generate(2, " ", letters=10).title()
verdict — real defect, absent from the gold setThe fact is stated accurately: except Exception: pass on 356-357 (and an identical one on 351-352) really does swallow the error without logging. The weight is trivial: the function parses client_sample_rate out of an UNTRUSTED event body, where a wrong type is an expected case rather than a failure, and silently ignoring it here is by design. A maintainer would most likely have rejected it, but the claim invents nothing.
src/sentry/integrations/github/integration.py:422· reviewed in the fork celmis-bench/sentry#7·upstream pull request ↗
the review commentsafe_urlopen on line 422 is placed outside the try block on line 424; if it raises a network or HTTP exception, the error is not caught and results in an unhandled 500 error
407| return self.redirect( 408| f"{ghip.get_oauth_authorize_url()}?client_id={github_client_id}&state={state}&redirect_uri={redirect_uri}" 409| ) 410| 411| # At this point, we are past the GitHub "authorize" step 412| if request.GET.get("state") != pipeline.signature: 413| return error(request, self.active_organization) 414| 415| # similar to OAuth2CallbackView.get_token_params 416| data = { 417| "code": request.GET.get("code"), 418| "client_id": github_client_id, 419| "client_secret": github_client_secret, 420| } 421| 422| # similar to OAuth2CallbackView.exchange_token 423| req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data) 424| 425| try: 426| body = safe_urlread(req).decode("utf-8") 427| payload = dict(parse_qsl(body)) 428| except Exception: 429| payload = {} 430| 431| if "access_token" not in payload: 432| return error(request, self.active_organization) 433| 434| authenticated_user_info = get_user_info(payload["access_token"]) 435| if "login" not in authenticated_user_info: 436| return error(request, self.active_organization) 437|
verdict — real defect, absent from the gold setsafe_urlopen on line 423 sits OUTSIDE the try that begins on 425 — plainly visible. The call is a network call: a timeout or a dropped connection to GitHub will raise an exception that nobody catches, and the user in the OAuth flow gets a 500 instead of an error message. The fact that the author wrapped the following safe_urlread (425-429) shows he did think about failures — and missed exactly the call most likely to fall over. A sound reviewer comment. A minor point: the claim names line 422, where there is a comment; the call is on 423.
gold set silentconstruct not therecross-file claim
src/sentry/integrations/github/integration.py:497· reviewed in the fork celmis-bench/sentry#7·upstream pull request ↗
the review commentBare except: catches everything including KeyboardInterrupt; should use except Exception: instead
482| return pipeline.next_step() 483| 484| if installations_exist: 485| return error( 486| request, 487| self.active_organization, 488| error_short="Github installed on another Sentry organization.", 489| error_long=ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG, 490| ) 491| 492| # OrganizationIntegration does not exist, but Integration does exist. 493| try: 494| integration = Integration.objects.get( 495| external_id=installation_id, status=ObjectStatus.ACTIVE 496| ) 497| except Integration.DoesNotExist: 498| return error(request, self.active_organization) 499| 500| # Check that the authenticated GitHub user is the same as who installed the app. 501| if ( 502| pipeline.fetch_state("github_authenticated_user") 503| != integration.metadata["sender"]["login"] 504| ): 505| return error(request, self.active_organization) 506| 507| return pipeline.next_step()
verdict — genuinely wrongThere is not a single bare except in the file. All five are typed: ApiError on 287 and 354, Exception on 428, Integration.DoesNotExist on 481 and 497. Line 497, the one the claim points at, is precisely Integration.DoesNotExist. The second appearance of the same invention in the run after #27: the model describes a construct that is not in the code.
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java:250· reviewed in the fork celmis-bench/keycloak#10·upstream pull request ↗
the review commentCached identity providers are not wrapped with organization-aware model - getForLogin returns unwrapped IdentityProviderModel instances on cache hits (line 250) but wrapped models on cache misses (lines 217 and 248)
235| // there is a cache entry, but the current search is not yet cached 236| cache.invalidateObject(cacheKey); 237| Long loaded = cache.getCurrentRevision(cacheKey); 238| cached = idpDelegate.getForLogin(mode, organizationId).map(IdentityProviderModel::getInternalId).collect(Collectors.toSet()); 239| query = new IdentityProviderListQuery(loaded, cacheKey, getRealm(), searchKey, cached, query); 240| cache.addRevisioned(query, cache.getCurrentCounter()); 241| } 242| } 243| 244| Set<IdentityProviderModel> identityProviders = new HashSet<>(); 245| for (String id : cached) { 246| IdentityProviderModel idp = session.identityProviders().getById(id); 247| if (idp == null) { 248| realmCache.registerInvalidation(cacheKey); 249| return idpDelegate.getForLogin(mode, organizationId).map(this::createOrganizationAwareIdentityProviderModel); 250| } 251| identityProviders.add(idp); 252| } 253| 254| return identityProviders.stream(); 255| } 256| 257| @Override 258| public Stream<String> getByFlow(String flowId, String search, Integer first, Integer max) { 259| return idpDelegate.getByFlow(flowId, search, first, max); 260| } 261| 262| @Override 263| public Stream<IdentityProviderModel> getAllStream(Map<String, String> attrs, Integer first, Integer max) { 264| return idpDelegate.getAllStream(attrs, first, max).map(this::createOrganizationAwareIdentityProviderModel); 265| }
verdict — genuinely wrongOn the "cache hit path" the models are taken from session.identityProviders().getById(id) on line 246, and getById in this same class (123) wraps the result at ALL three exits: 135, 139, 141. That is, this path does not return unwrapped models, and there is no divergence between a hit and a miss. Notably: a real defect in this method DOES exist — the golden comment speaks of the recursive call to session.identityProviders() from the provider itself — and the model was looking at exactly that spot, but named the wrong thing.
why this classan unmatched golden nearby (shared: getForLogin): InfinispanIdentityProviderStorageProvider.getForLogin() recursively calls session.identityProviders(
server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java:254· reviewed in the fork celmis-bench/keycloak#10·upstream pull request ↗
the review commentPotential NullPointerException when idp.getConfig() is null on line 254 - calling get() without null check when evaluating getLoginPredicate
239| 240| public String getValue() { 241| return value; 242| } 243| 244| public Predicate<IdentityProviderModel> getFilter() { 245| return filter; 246| } 247| 248| public static Map<String, String> getLoginSearchOptions() { 249| return Stream.of(values()).collect(Collectors.toMap(LoginFilter::getKey, LoginFilter::getValue, (v1, v2) -> v1, LinkedHashMap::new)); 250| } 251| 252| public static Predicate<IdentityProviderModel> getLoginPredicate() { 253| return ((Predicate<IdentityProviderModel>) Objects::nonNull) 254| .and(idp -> idp.getOrganizationId() == null || Boolean.parseBoolean(idp.getConfig().get(OrganizationModel.BROKER_PUBLIC))) 255| .and(Stream.of(values()).map(LoginFilter::getFilter).reduce(Predicate::and).get()); 256| } 257| } 258| 259| /** 260| * Creates a new identity provider mapper from the specified model. 261| * 262| * @param model a {@link IdentityProviderMapperModel} containing the identity provider mapper's data. 263| * @return the model of the created identity provider mapper. 264| */ 265| IdentityProviderMapperModel createMapper(IdentityProviderMapperModel model); 266| 267| /** 268| * Updates the identity provider mapper using the specified model. 269| *
verdict — unverifiableThe mechanism is possible, reachability is not shown. FOR: setConfig (IdentityProviderModel.java:206-208) assigns without a check, so null in the field is technically admissible, and getConfig (202) hands it back as is. AGAINST: the field is initialised at its declaration (102) with a new HashMap, the copy constructor (113) also puts a non-empty map there, and the expression itself is guarded by the short-circuit idp.getOrganizationId() == null ||, so getConfig() is only reached for IdPs bound to an organisation. I found no call to setConfig(null); without one this is a possibility, not a defect.
server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java:255· reviewed in the fork celmis-bench/keycloak#10·upstream pull request ↗
the review commentPotential NullPointerException when idp.getConfig() is null on line 255 - dereferencing with get() without null check during login predicate evaluation
240| public String getValue() { 241| return value; 242| } 243| 244| public Predicate<IdentityProviderModel> getFilter() { 245| return filter; 246| } 247| 248| public static Map<String, String> getLoginSearchOptions() { 249| return Stream.of(values()).collect(Collectors.toMap(LoginFilter::getKey, LoginFilter::getValue, (v1, v2) -> v1, LinkedHashMap::new)); 250| } 251| 252| public static Predicate<IdentityProviderModel> getLoginPredicate() { 253| return ((Predicate<IdentityProviderModel>) Objects::nonNull) 254| .and(idp -> idp.getOrganizationId() == null || Boolean.parseBoolean(idp.getConfig().get(OrganizationModel.BROKER_PUBLIC))) 255| .and(Stream.of(values()).map(LoginFilter::getFilter).reduce(Predicate::and).get()); 256| } 257| } 258| 259| /** 260| * Creates a new identity provider mapper from the specified model. 261| * 262| * @param model a {@link IdentityProviderMapperModel} containing the identity provider mapper's data. 263| * @return the model of the created identity provider mapper. 264| */ 265| IdentityProviderMapperModel createMapper(IdentityProviderMapperModel model); 266| 267| /** 268| * Updates the identity provider mapper using the specified model. 269| * 270| * @param model a {@link IdentityProviderMapperModel} containing the identity provider mapper's data.
verdict — genuinely wrongThere is no getConfig() expression on line 255 — what is there is .and(Stream.of(values()).map(LoginFilter::getFilter).reduce(Predicate::and).get()). This is #34 restated with the line number shifted by one. Second point: line 255 does have a weak spot — .get() on an Optional without isPresent — and the claim does NOT name it, even though it points at exactly that line.
pkg/apiserver/rest/dualwriter_mode3.go:45· reviewed in the fork celmis-bench/grafana#16·upstream pull request ↗
the review commentLine 45 calls recordLegacyDuration instead of recordStorageDuration when d.Storage.Create returns an error, misattributing Storage operation failures to Legacy metrics
verdict — real defect, absent from the gold setThe defect is real and is proved by the file itself. On a Storage error the other methods write recordStorageDuration: Get (73), List (89), Delete (103), DeleteCollection (156). Create on line 45 writes recordLegacyDuration — even though the measured interval is startStorage (41) and the call that failed is d.Storage.Create (42). The storage-failure metric lands in the legacy counter. The same fault is in Update on 129, and on 166 it is mirrored — recordStorageDuration for a d.Legacy operation. We found one of the three.
pkg/apiserver/rest/dualwriter_mode3.go:45· reviewed in the fork celmis-bench/grafana#16·upstream pull request ↗
the review commentLine 125 calls recordLegacyDuration instead of recordStorageDuration when d.Storage.Create returns an error, misattributing Storage operation failures to Legacy metrics
verdict — real defect, absent from the gold setThe substance is the same as in #36, and it is correct: on a Storage error a legacy metric is written. But as a SEPARATE finding it is spoiled — the text names line 125, where there is nothing of the sort, and attributes to it the d.Storage.Create call, which is on 42. The real second instance of the defect is on 129 (Update). That is, the model felt out the right class of error a second time and could not point at it. I count it real on substance, with the note "duplicate with the wrong line".
public/app/features/alerting/unified/hooks/useAbilities.ts:369· reviewed in the fork celmis-bench/grafana#15·upstream pull request ↗
the review commentctx is not defined on line 369 of useAbilities.ts; referencing ctx.hasPermissionInMetadata when folder is loaded throws a ReferenceError at runtime
verdict — genuinely wrongctx is imported on line 3: `import { contextSrv as ctx } from 'app/core/services/context_srv'`. It is in module scope and is already used in this same file on 159, 160, 587 and 615. A ReferenceError is impossible. The declaration sits 366 lines above the site of the finding — far outside the window the model was working with.
same line as another findingdeclaration outside the excerpt
public/app/features/alerting/unified/hooks/useAbilities.ts:369· reviewed in the fork celmis-bench/grafana#15·upstream pull request ↗
the review commentLine 369 references an undeclared identifier ctx when evaluating folder permissions for Grafana alert rules, throwing an unhandled ReferenceError at runtime and causing component crashes
verdict — genuinely wrongA verbatim repeat of #38 about the same line 369, wrong for the same reason (the import on line 3). This is already the fourth case in the run where an "undeclared identifier" is in fact declared above the window: #15/#16 (a method parameter 26 lines away), #21 (a Go variable 29 lines away), #38/#39 (an import 366 lines away). The common root is a single one, and it lies not in the model but in how much of the file it is shown.
why this classthe same line as another wrong finding, #1 ["#1" is a local index, not the record number n]
public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx:68· reviewed in the fork celmis-bench/grafana#15·upstream pull request ↗
the review commentpromResponse.data.groups.at(0)?.rules evaluates to undefined when groups is empty on line 68 of GrafanaGroupLoader.tsx; calling .map on it throws a TypeError at runtime
53| if (!promResponse) { 54| return ( 55| <Alert 56| title={t( 57| 'alerting.group-loader.group-load-failed', 58| 'Failed to load rules from group {{ groupName }} in {{ namespaceName }}', 59| { groupName: groupIdentifier.groupName, namespaceName } 60| )} 61| severity="error" 62| /> 63| ); 64| } 65| 66| return ( 67| <> 68| {promResponse.data.groups.at(0)?.rules.map((promRule) => { 69| return ( 70| <GrafanaRuleListItem 71| key={promRule.uid} 72| rule={promRule} 73| groupIdentifier={groupIdentifier} 74| namespaceName={namespaceName} 75| // we don't show the location again for rules, it's redundant because they are shown in a folder > group hierarchy 76| showLocation={false} 77| /> 78| ); 79| })} 80| </> 81| ); 82| }
verdict — genuinely wrongOptional chaining short-circuits the WHOLE expression, not one link. In `groups.at(0)?.rules.map(...)`, if .at(0) yields undefined, evaluation stops immediately after the ?. and .map is not called — the result of the whole expression becomes undefined. The very case named in the claim ("when groups is empty") is the only one in which a TypeError does NOT arise. It can fail here only if the group exists and rules in it is undefined — and the claim does not speak of that.
public/app/plugins/datasource/loki/querySplitting.ts:299· reviewed in the fork celmis-bench/grafana#14·upstream pull request ↗
the review commentQuery expression filter runs before template variable interpolation, causing queries with template variables that evaluate to empty strings to pass the filter and be included with empty expr values instead of being filtered out (occurs in public/app/plugins/datasource/loki/querySplitting.ts:299 and public/app/plugins/datasource/loki/shardQuerySplitting.ts:50)
284| query.queryType !== LokiQueryType.Instant && 285| // Queries with $__range variable should not be split because then the interpolated $__range variable is incorrect 286| // because it is interpolated on the backend with the split timeRange 287| !isQueryWithRangeVariable(query.expr) 288| ); 289| } 290| 291| export function runSplitQuery( 292| datasource: LokiDatasource, 293| request: DataQueryRequest<LokiQuery>, 294| options: QuerySplittingOptions = {} 295| ) { 296| const queries = request.targets 297| .filter((query) => !query.hide) 298| .filter((query) => query.expr) 299| .map((query) => datasource.applyTemplateVariables(query, request.scopedVars, request.filters)); 300| const [nonSplittingQueries, normalQueries] = partition(queries, (query) => !querySupportsSplitting(query)); 301| const [logQueries, metricQueries] = partition(normalQueries, (query) => isLogsQuery(query.expr)); 302| 303| request.queryGroupId = uuidv4(); 304| const oneDayMs = 24 * 60 * 60 * 1000; 305| const directionPartitionedLogQueries = groupBy(logQueries, (query) => 306| query.direction === LokiQueryDirection.Forward ? LokiQueryDirection.Forward : LokiQueryDirection.Backward 307| ); 308| const requests: LokiGroupedRequest[] = []; 309| 310| for (const direction in directionPartitionedLogQueries) { 311| const queries = directionPartitionedLogQueries[direction]; 312| const resolutionPartition = groupBy(queries, (query) => query.resolution || 1); 313| for (const resolution in resolutionPartition) { 314| const groupedRequest = {
verdict — real defect, absent from the gold setThe ordering is described correctly: .filter((query) => query.expr) is on 298, and applyTemplateVariables on 299. The filter sees the RAW expr, in which the variable has not yet been substituted, so the string `$var` is non-empty there and passes; after substitution it can become empty, and a query with an empty expr goes on into the split logic. The weight is small — this is not a crash but a pointless extra query — but the observation is sound and can be checked against two adjacent lines.
pkg/services/anonymous/anonimpl/anonstore/database.go:84· reviewed in the fork celmis-bench/grafana#11·upstream pull request ↗
the review commentargs slice declared outside WithDbSession closure is mutated via append on line 84, causing duplicate query arguments on retry and SQL execution failure on line 85
69| return devices, err 70| } 71| 72| // updateDevice updates a device if it exists and has been updated between the given times. 73| func (s *AnonDBStore) updateDevice(ctx context.Context, device *Device) error { 74| const query = `UPDATE anon_device SET 75| client_ip = ?, 76| user_agent = ?, 77| updated_at = ? 78| WHERE device_id = ? AND updated_at BETWEEN ? AND ?` 79| 80| args := []interface{}{device.ClientIP, device.UserAgent, device.UpdatedAt.UTC(), device.DeviceID, 81| device.UpdatedAt.UTC().Add(-anonymousDeviceExpiration), device.UpdatedAt.UTC().Add(time.Minute), 82| } 83| err := s.sqlStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error { 84| args = append([]interface{}{query}, args...) 85| result, err := dbSession.Exec(args...) 86| if err != nil { 87| return err 88| } 89| 90| rowsAffected, err := result.RowsAffected() 91| if err != nil { 92| return err 93| } 94| 95| if rowsAffected == 0 { 96| return ErrDeviceLimitReached 97| } 98| 99| return nil
verdict — real defect, absent from the gold setReal, and not superficial. args is declared on 80 OUTSIDE the closure, and on 84 the closure reassigns precisely that outer variable: args = append([]interface{}{query}, args...). The cost of this hinged on whether the callback is invoked more than once — and the answer is in the comment on the function itself: sqlstore/session.go:72, above `func (ss *SQLStore) WithDbSession`, says "In case of sqlite3.ErrLocked or sqlite3.ErrBusy failure it will be retried at most five times". So on the second attempt args becomes [query, query, …] — the number of parameters no longer matches the query, and Exec fails for a different reason than the original lock.
app/assets/javascripts/discourse/models/store.js.es6:197· reviewed in the fork celmis-bench/discourse-graphite#20·upstream pull request ↗
the review commentobj[k] can be null or undefined on line 197; calling .map() on it without a check throws a TypeError when hydrating embedded array attributes
verdict — real defect, absent from the gold setThe branch at 197 runs for keys ending in _ids, and .map over obj[k] will indeed fail if the server returned null there. I did not show a direct path that produces null — but the code itself argues for one: on 200 there is `hydrated || []`, even though hydrated after .map is always an array and cannot be empty. The author anticipated a falsy value at exactly this spot and put the guard one step later than it was needed.
app/assets/stylesheets/desktop/user.scss:522· reviewed in the fork celmis-bench/discourse-graphite#17·upstream pull request ↗
the review commentLine 497 of app/assets/stylesheets/mobile/user.scss: lightness for primary is set to 50% instead of 30%, causing user name text to render with lightness 50% instead of 30% in light theme
verdict — real defect, absent from the gold setConfirmed by the diff, and this is the most precise finding of the run. The PR mechanically wrapped colours in dark-light-choose, preserving the original value. In one place the value quietly changed: in mobile/user.scss, hunk @@ -486,21 @@, the line `-color: scale-color($primary, $lightness: 30%);` became `+color: dark-light-choose(scale-color($primary, $lightness: 50%), …)`. The same substituted 30%→50% is also present in desktop/user.scss (@@ -511,20 @@), whereas every other conversion in both files kept its value. This is exactly the unintentional edit a reviewer is supposed to catch in a uniform diff.
why this classunmatched golden nearby (shared: scss): In mobile/user.scss line 497, the lightness value for dark themes should be inverted to 70% instead
app/assets/stylesheets/mobile/modal.scss:102· reviewed in the fork celmis-bench/discourse-graphite#17·upstream pull request ↗
the review commentLine 102 of app/assets/stylesheets/mobile/modal.scss: primary and secondary lightness values are swapped, causing .custom-message-length text to render at 30% lightness instead of 70% in light theme
verdict — real defect, absent from the gold setThe diff proves the claim. It was `color: scale-color($primary, $lightness: 70%);`, it became `dark-light-choose(scale-color($primary, $lightness: 30%), scale-color($secondary, $lightness: 70%))`. That is, in the light theme the value changed from 70% to 30% — exactly what is stated. That this is an oversight and not intent is visible from the neighbouring conversions in the same PR: in #44 the original 30% for primary was preserved and 70% was given to secondary. Here the author took the same pair (30/70) without noticing that the original was 70.
why this classunmatched golden nearby (shared: scss): In mobile/user.scss line 497, the lightness value for dark themes should be inverted to 70% instead
app/assets/stylesheets/mobile/topic-post.scss:182· reviewed in the fork celmis-bench/discourse-graphite#17·upstream pull request ↗
the review commentLine 182 of app/assets/stylesheets/mobile/topic-post.scss: lightness for primary is set to 50% instead of 20%, causing h3 headings to render lighter in light theme than intended
verdict — real defect, absent from the gold setThe diff confirms it word for word: in hunk @@ -179,15 @@ the line `-color: scale-color($primary, $lightness: 20%);` became `+dark-light-choose(scale-color($primary, $lightness: 50%), …)`. h3 in the light theme moved from 20% to 50%. That this is a mistake and not a policy is visible two hunks below: in @@ -244,7 @@ the same original 20% was preserved correctly — `dark-light-choose(scale-color($primary, $lightness: 20%), scale-color($secondary, $lightness: 80%))`. The third defect of this class found in one PR, after #44 and #45.
why this classunmatched golden nearby (shared: scss): In mobile/user.scss line 497, the lightness value for dark themes should be inverted to 70% instead
app/assets/javascripts/admin/templates/group.hbs:21· reviewed in the fork celmis-bench/discourse-graphite#18·upstream pull request ↗
the review commentLine 21 uses {{each ...}} without the # prefix required for Handlebars/Ember block helpers, causing a template syntax error when Ember attempts to compile group.hbs
verdict — genuinely wrongThe non-block form of {{each}} with the itemView option is standard for Ember 1.x, not a syntax error. That it is deliberate here is proved by the composition of the PR itself: along with this line it adds `app/assets/javascripts/admin/views/group-member.js.es6` (+4) and `app/assets/javascripts/admin/templates/group_member.hbs` (+1) — precisely the view that itemView="group-member" refers to. The author built both halves of the construct. Caveat: I checked the intent and the structure, not the fact of compilation.
app/controllers/admin/groups_controller.rb:90· reviewed in the fork celmis-bench/discourse-graphite#18·upstream pull request ↗
the review commentLine 90 passes user_id (an integer) to group.users.delete, which expects User record instances for ActiveRecord associations and raises ActiveRecord::AssociationTypeMismatch when executed
75| end 76| 77| if group.save 78| render json: success_json 79| else 80| render_json_error(group) 81| end 82| end 83| 84| def remove_member 85| group = Group.find(params.require(:group_id).to_i) 86| user_id = params.require(:user_id).to_i 87| 88| return can_not_modify_automatic if group.automatic 89| 90| group.users.delete(user_id) 91| 92| if group.save 93| render json: success_json 94| else 95| render_json_error(group) 96| end 97| end 98| 99| protected 100| 101| def can_not_modify_automatic 102| render json: {errors: I18n.t('groups.errors.can_not_modify_automatic')}, status: 422 103| end 104| end
verdict — genuinely wrongRefuted by a spec from this same PR. spec/controllers/admin/groups_controller_spec.rb adds the test "is able to remove a member": it does xhr :delete, :remove_member with user_id: user.id and checks `response.should be_success` and `group.users.count.should == 0`. If group.users.delete(user_id) raised ActiveRecord::AssociationTypeMismatch, that test would fail. Rails accepts an identifier here: CollectionAssociation#delete calls find(records) when the first element is an Integer or a String.
app/assets/javascripts/discourse/controllers/create_account_controller.js:96· reviewed in the fork celmis-bench/discourse-graphite#13·upstream pull request ↗
the review commentrejectedEmails.@each on line 96 does not observe Ember array membership changes; when a rejected email is added via pushObject on line 275, emailValidation is not recomputed because @each requires a property path (e.g., @each.property) and rejectedEmails.[] must be used instead
verdict — genuinely wrongIn Ember 1.x, which this code is written against (nearby: {{bind-attr}}, Discourse.InputValidation, .property() — all signs of 1.x), a bare `rejectedEmails.@each` observes exactly the array's membership and is equivalent to `rejectedEmails.[]`. pushObject does trigger the recomputation. The requirement to write @each.property appeared later: in modern Ember a bare @each is a deprecated form, and THEN the comment would be sound. But this is not "does not work", it is "will be deprecated in a few years".
why this classthe same line as another false positive, #3
lib/validators/email_validator.rb:21· reviewed in the fork celmis-bench/discourse-graphite#13·upstream pull request ↗
the review commentvalue can be nil when validating a record with no email on line 3; passed to email_in_restriction_setting? on line 5 or 9 and dereferenced with =~ on line 21, raising a NoMethodError exception
6| record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) 7| end 8| elsif (setting = SiteSetting.email_domains_blacklist).present? 9| if email_in_restriction_setting?(setting, value) 10| record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) 11| end 12| end 13| if record.errors[attribute].blank? and BlockedEmail.should_block?(value) 14| record.errors.add(attribute, I18n.t(:'user.email.blocked')) 15| end 16| end 17| 18| def email_in_restriction_setting?(setting, value) 19| domains = setting.gsub('.', '\.') 20| regexp = Regexp.new("@(#{domains})", true) 21| value =~ regexp 22| end 23| 24| end
verdict — genuinely wrongIn Ruby `nil =~ regexp` does not fail but returns nil: the comparison is defined on NilClass (and before 2.6, on Object), precisely so that this idiom works. So a nil check is not needed here, and no NoMethodError arises. The claim carries over to this place a habit from languages where calling a method on null is fatal.
why this classunmatched golden nearby (shared: email): BlockedEmail.should_block_email? method has side effects during a read operation - it updates statis
app/assets/javascripts/discourse/controllers/create_account_controller.js:17· reviewed in the fork celmis-bench/discourse-graphite#13·upstream pull request ↗
the review commentrejectedEmails is initialized as Em.A([]) on line 17 on the controller prototype; mutating it on line 275 shares rejected emails across all controller usages and modal re-openings
verdict — real defect, absent from the gold setAn array literal in the body of extend() is created once and lives on the prototype — a well-known Ember anti-pattern that the documentation warns about directly, advising that arrays be initialised in init(). Here it is worse: rejectedEmails is never reset anywhere in the file — there is only a contains at 69 and a pushObject at 275. A controller in Ember 1.x is a singleton, so rejected addresses accumulate for the whole lifetime of the page and survive repeated openings of the sign-up modal.
why this classsame line as the other false positive #1
app/models/post.rb:133· reviewed in the fork celmis-bench/discourse-graphite#14·upstream pull request ↗
the review commentLine 133 bypasses Discourse's post cooking pipeline and HTML sanitization when cook_method is raw_html, allowing stored XSS from untrusted feed content
118| 119| def post_analyzer 120| @post_analyzers ||= {} 121| @post_analyzers[raw_hash] ||= PostAnalyzer.new(raw, topic_id) 122| end 123| 124| %w{raw_mentions linked_hosts image_count attachment_count link_count raw_links}.each do |attr| 125| define_method(attr) do 126| post_analyzer.send(attr) 127| end 128| end 129| 130| def cook(*args) 131| # For some posts, for example those imported via RSS, we support raw HTML. In that 132| # case we can skip the rendering pipeline. 133| return raw if cook_method == Post.cook_methods[:raw_html] 134| 135| # Default is to cook posts 136| Plugin::Filter.apply(:after_post_cook, self, post_analyzer.cook(*args)) 137| end 138| 139| # Sometimes the post is being edited by someone else, for example, a mod. 140| # If that's the case, they should not be bound by the original poster's 141| # restrictions, for example on not posting images. 142| def acting_user 143| @acting_user || user 144| end 145| 146| def acting_user=(pu) 147| @acting_user = pu 148| end
verdict — unverifiableThe mechanism is confirmed, the vulnerability is not. CONFIRMED: the diff shows that this PR ADDED both the cook_methods enumeration and the bypass itself, `return raw if cook_method == Post.cook_methods[:raw_html]` (+9 -0) — that is, the cooking pipeline really is bypassed, and this is new code, not long-standing code. NOT CONFIRMED: that the content is "untrusted". RSS feeds in Discourse are configured by an administrator, and who else can set cook_method is not visible from what is available. Without that, this is a fair reviewer's question about a new sanitisation bypass, but not a demonstrated stored XSS.
why this classunmatched golden nearby (in common: HTML): The TopicEmbed.import method is susceptible to a NoMethodError if the contents parameter is nil when
app/assets/javascripts/embed.js:11· reviewed in the fork celmis-bench/discourse-graphite#14·upstream pull request ↗
the review commentLine 5 does not check if #discourse-comments element exists before calling appendChild on line 11, causing TypeError when element is missing
verdict — real defect, absent from the gold setdocument.getElementById returns null if the element is not there, and comments.appendChild(iframe) on line 12 then throws a TypeError. There is no check. The file is an embed snippet that a site owner copies onto their own page, so a missing #discourse-comments is not exotic here but the most likely integration mistake; crashing without explanation is a poor way to report it. Minor point: the claim cites line 11 (iframe.scrolling), the call is on 12.
app/assets/javascripts/discourse/components/dropdown-button.js.es6:29· reviewed in the fork celmis-bench/discourse-graphite#12·upstream pull request ↗
the review commenttitle is retrieved from component properties on line 27 and concatenated directly into the HTML string buffer on line 29 without HTML escaping, resulting in cross-site scripting (XSS)
verdict — unverifiableThe fact is right, the conclusion is not demonstrated. The concatenation really is unescaped: buffer.push("<h4 class='title'>" + title + "</h4>"). But XSS requires title to be attacker-controlled, and no call site with user data is shown — in the templates what usually goes there is an I18n string. Second, and more important: the diff shows the expression is PRE-EXISTING — the PR only wrapped it in an `if (title)` check. As a comment on the PR this is a remark about unchanged behaviour.
app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs:3· reviewed in the fork celmis-bench/discourse-graphite#12·upstream pull request ↗
the review commentstopNotificiationsText contains user-controlled topic title content and is rendered using triple curlies {{{...}}} on line 3, bypassing Handlebars HTML escaping and leading to stored XSS
verdict — genuinely wrongfancyTitle is a title already escaped on the server. Discourse builds Topic#fancy_title through HTML escaping plus emoji and quote substitution precisely so that it can be output with triple curlies; this pattern is everywhere in the codebase. The controller (topic-unsubscribe.js.es6:5-7) interpolates exactly model.fancyTitle into the translation. So {{{...}}} here is not a bypass of the protection but its intended use.
why this classunmatched golden nearby (in common: stopNotificiationsText): Typo in property name: 'stopNotificiationsText' should be 'stopNotificationsText' (missing 'n' in 'N
app/controllers/topics_controller.rb:98· reviewed in the fork celmis-bench/discourse-graphite#12·upstream pull request ↗
the review commentunsubscribe changes user notification state in the database on line 113 via an HTTP GET request, leaving the action vulnerable to Cross-Site Request Forgery (CSRF)
83| end 84| 85| perform_show_response 86| 87| canonical_url UrlHelper.absolute_without_cdn("#{Discourse.base_uri}#{@topic_view.canonical_path}") 88| rescue Discourse::InvalidAccess => ex 89| 90| if current_user 91| # If the user can't see the topic, clean up notifications for it. 92| Notification.remove_for(current_user.id, params[:topic_id]) 93| end 94| 95| raise ex 96| end 97| 98| def unsubscribe 99| @topic_view = TopicView.new(params[:topic_id], current_user) 100| 101| if slugs_do_not_match || (!request.format.json? && params[:slug].blank?) 102| return redirect_to @topic_view.topic.unsubscribe_url, status: 301 103| end 104| 105| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) 106| 107| if tu.notification_level > TopicUser.notification_levels[:regular] 108| tu.notification_level = TopicUser.notification_levels[:regular] 109| else 110| tu.notification_level = TopicUser.notification_levels[:muted] 111| end 112| 113| tu.save!
verdict — real defect, absent from the gold setConfirmed by the routes diff: the PR adds `get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe"` and `get "t/:topic_id/unsubscribe"`. The method is GET, and the action changes notification_level in the database. Rails' CSRF protection does not check the token on GET, so a third-party page can unsubscribe a logged-in user with an ordinary <img src=…>, and link pre-fetchers in mail clients will fire by themselves. That this is not an invented threat is visible from the fact that Discourse later moved unsubscription to a token with a confirmation. A fair remark about the NEW routes.
config/locales/server.en.yml:1855· reviewed in the fork celmis-bench/discourse-graphite#12·upstream pull request ↗
the review commentWhen an email is generated with add_unsubscribe_link: true without providing an unsubscribe_url option (such as digest emails or system notifications), I18n.t('unsubscribe_link', ...) raises I18n::MissingInterpolationArgument because %{unsubscribe_url} on line 1855 is required in the translation template
1840| subject_template: 1841| one: "1 user waiting for approval" 1842| other: "%{count} users waiting for approval" 1843| text_body_template: | 1844| There are new user signups waiting to be approved (or rejected) before they can access this forum. 1845| 1846| [Please review them in the admin section](%{base_url}/admin/users/list/pending). 1847| 1848| download_remote_images_disabled: 1849| subject_template: "Downloading remote images disabled" 1850| text_body_template: "The `download_remote_images_to_local` setting was disabled because the disk space limit at `download_remote_images_threshold` was reached." 1851| 1852| unsubscribe_link: | 1853| To unsubscribe from these emails, visit your [user preferences](%{user_preferences_url}). 1854| 1855| To stop receiving notifications about this particular topic, [click here](%{unsubscribe_url}). 1856| 1857| subject_re: "Re: " 1858| subject_pm: "[PM] " 1859| 1860| user_notifications: 1861| previous_discussion: "Previous Replies" 1862| unsubscribe: 1863| title: "Unsubscribe" 1864| description: "Not interested in getting these emails? No problem! Click below to unsubscribe instantly:" 1865| 1866| reply_by_email: "To respond, reply to this email or visit %{base_url}%{url} in your browser." 1867| visit_link_to_respond: "To respond, visit %{base_url}%{url} in your browser." 1868| 1869| posted_by: "Posted by %{username} on %{post_date}" 1870|
verdict — genuinely wrongThe structural worry has grounds, the case named does not. The PR did extend the unsubscribe_link translation by adding a mandatory %{unsubscribe_url} (diff of server.en.yml, +4 -1), and @template_args is assembled from a base hash plus @opts, so this key arrives ONLY from the caller. But in the mailer add_unsubscribe_link is set in one single place — user_notifications.rb:294 — and there, in the same PR, unsubscribe_url: post.topic.unsubscribe_url (295) was added. The digest does not set it at all. The "digest emails or system notifications" named in the claim do not go through this path. Caveat: I searched in user_notifications.rb and message_builder.rb; a third-party plugin or another mailer could in theory step on this.
lib/email/message_builder.rb:68· reviewed in the fork celmis-bench/discourse-graphite#12·upstream pull request ↗
the review commenttemplate_args on line 68 is referenced without the @ prefix, raising a NameError (undefined local variable or method 'template_args') when html_part renders the unsubscribe link
53| subject = I18n.t("#{@opts[:template]}.subject_template", template_args) if @opts[:template] 54| end 55| subject 56| end 57| 58| def html_part 59| return unless html_override = @opts[:html_override] 60| 61| if @opts[:add_unsubscribe_link] 62| if response_instructions = @template_args[:respond_instructions] 63| respond_instructions = PrettyText.cook(response_instructions).html_safe 64| html_override.gsub!("%{respond_instructions}", respond_instructions) 65| end 66| 67| unsubscribe_link = PrettyText.cook(I18n.t('unsubscribe_link', template_args)).html_safe 68| html_override.gsub!("%{unsubscribe_link}", unsubscribe_link) 69| end 70| 71| styled = Email::Styles.new(html_override) 72| styled.format_basic 73| 74| if style = @opts[:style] 75| styled.send "format_#{style}" 76| end 77| 78| Mail::Part.new do 79| content_type 'text/html; charset=UTF-8' 80| body styled.to_html 81| end 82| end 83|
verdict — genuinely wrongtemplate_args is a method: attr_reader :template_args on line 18 of the same class. The unprefixed reference is used four more times in the file (47, 48, 53, 107) and is the established form here. A NameError is impossible. The fifth case of an "undeclared identifier" in the run, and again the declaration (18) lies far above the window the model was looking at. Minor point: line 68 is cited, the expression is on 67.
packages/app-store/googlecalendar/lib/CalendarService.ts:1023· reviewed in the fork celmis-bench/cal.diy#20·upstream pull request ↗
the review commentSelectedCalendarRepository.updateManyByCredentialId is called with an empty data object `{}` on line 1023, causing Prisma to throw a PrismaClientValidationError at runtime
1008| 1009| for (const [_eventTypeId, selectedCalendars] of Array.from(selectedCalendarsPerEventType.entries())) { 1010| const parsedArgs = { 1011| /** Expand the start date to the start of the month to increase cache hits */ 1012| timeMin: getTimeMin(), 1013| /** Expand the end date to the end of the month to increase cache hits */ 1014| timeMax: getTimeMax(), 1015| // Dont use eventTypeId in key because it can be used by any eventType 1016| // The only reason we are building it per eventType is because there can be different groups of calendars to lookup the availability for 1017| items: selectedCalendars.map((sc) => ({ id: sc.externalId })), 1018| }; 1019| const data = await this.fetchAvailability(parsedArgs); 1020| await this.setAvailabilityInCache(parsedArgs, data); 1021| } 1022| 1023| // Update SelectedCalendar.updatedAt for all calendars under this credential 1024| await SelectedCalendarRepository.updateManyByCredentialId(this.credential.id, {}); 1025| } 1026| 1027| async createSelectedCalendar( 1028| data: Omit<Prisma.SelectedCalendarUncheckedCreateInput, "integration" | "credentialId"> 1029| ) { 1030| return await SelectedCalendarRepository.create({ 1031| ...data, 1032| integration: this.integrationName, 1033| credentialId: this.credential.id, 1034| }); 1035| } 1036| 1037| async upsertSelectedCalendar( 1038| data: Omit<Prisma.SelectedCalendarUncheckedCreateInput, "integration" | "credentialId" | "userId">
verdict — genuinely wrongThe empty data here is deliberate and works. The SelectedCalendar model has `updatedAt DateTime? @updatedAt` (schema.prisma:860), and updateMany with an empty data updates the timestamp precisely because of that — exactly what the comment on line 1023 says: "Update SelectedCalendar.updatedAt for all calendars under this credential". The Prisma.SelectedCalendarUpdateInput type has all fields optional, so {} passes the type check too. Caveat: I checked the model and the intent, I did not run Prisma.
why this classunmatched golden nearby (in common: Prisma, updateManyByCredentialId): The updateManyByCredentialId call uses an empty data object, which prevents Prisma's @updatedAt deco
packages/trpc/server/routers/viewer/slots.ts:106· reviewed in the fork celmis-bench/cal.diy#16·upstream pull request ↗
the review commentOn line 106, utcOffset is calculated by multiplying utcOffset() by -1; when organizerTimeZone has a positive offset (e.g. +09:00), add(utcOffset, "minutes") on line 109 subtracts minutes instead of adding them, causing format("YYYY MM DD") to produce the wrong calendar date and fail to match date overrides
verdict — unverifiableThe arithmetic is described correctly — add() with a negative value subtracts — but it does not follow from this that the result is wrong: it all depends on which frame of reference date.start is stored in, and that is not visible from the code. Against the claim is the fact that, together with this code, the PR added a test, getSchedule.test.ts, precisely for a POSITIVE offset (Timezones["+6:00"]) on the date-override path. That is, the author deliberately covered this case. To prove a sign error one would have to run both variants, not read the line.
why this classsame line as the other false positive #2
packages/trpc/server/routers/viewer/slots.ts:138· reviewed in the fork celmis-bench/cal.diy#16·upstream pull request ↗
the review commentOn line 138, workingHours.find returns true if a slot falls outside any individual workingHour entry, which causes valid slots in one working hour block (e.g., 09:00-12:00) to be rejected because they fall outside another block on the same day (e.g., 13:00-17:00)
123| if (slotStartTime.isAfter(dayjs(date.end).add(utcOffset, "minutes"))) { 124| return true; 125| } 126| } 127| }) 128| ) { 129| // slot is not within the date override 130| return false; 131| } 132| 133| if (dateOverrideExist) { 134| return true; 135| } 136| 137| //if no date override for slot exists check if it is within normal work hours 138| if ( 139| workingHours.find((workingHour) => { 140| if (workingHour.days.includes(slotStartTime.day())) { 141| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); 142| const end = slotStartTime.hour() * 60 + slotStartTime.minute(); 143| if (start < workingHour.startTime || end > workingHour.endTime) { 144| return true; 145| } 146| } 147| }) 148| ) { 149| // slot is outside of working hours 150| return false; 151| } 152| 153| return busy.every((busyTime) => {
verdict — real defect, absent from the gold setReal and well stated. Further down the code stands `if (workingHours.find(…)) { // slot is outside of working hours; return false; }` — that is, ONE working-hours block that the slot falls outside of is enough for the slot to be discarded. With two blocks on the same day (09:00-12:00 and 13:00-17:00) a slot at 10:00 lies inside the first and outside the second, and the second kills it. The predicate should have checked membership of AT LEAST ONE block, but instead it checks non-membership of any. The diff confirms that this whole block was added by this very PR. Alongside it, on 141-142, there is another bug the finding does not name: start and end are computed identically, both from slotStartTime.
why this classsame line as the other false positive, #1
golden beside it, unmatchedprecondition unreachablecross-file claim
packages/lib/server/service/insightsBooking.ts:164· reviewed in the fork celmis-bench/cal.diy#14·upstream pull request ↗
the review commentWhen teamIds is empty, Prisma.sql`("teamId" = ANY(${teamIds})) AND ("isTeamBooking" = true)` is unconditionally added to conditions without checking teamIds.length > 0, causing PostgreSQL to fail with type inference error on empty array in ANY() clause
verdict — genuinely wrongThe precondition is impossible. teamIds is built on line 154 as [options.orgId, ...teamsFromOrg.map((t) => t.id)] — it always contains at least options.orgId, and cannot be empty. The condition "When teamIds is empty", from which the whole claim proceeds, never arises.
why this classunmatched golden nearby (shared: teamIds): Fetching userIdsFromOrg only when teamsFromOrg.length > 0 can exclude org-level members for orgs wit
packages/app-store/salesforce/lib/CalendarService.ts:97· reviewed in the fork celmis-bench/cal.diy#17·upstream pull request ↗
the review commentSalesforce token refresh drops essential fields like instance_url from credentialKey because credentialKey is not spread into the updated object in prisma.credential.update
verdict — genuinely wronginstance_url is not lost: salesforceTokenSchema (CalendarService.ts:39-46) has instance_url among its required fields, so it is present in accessTokenParsed.data and lands in key via the spread on line 98. Salesforce returns instance_url in the response to refresh_token, which is why the schema requires it. The example named, "essential fields like instance_url", is exactly the field that is preserved.
why this classunmatched golden nearby (shared: credentialKey, instance_url): The `jsforce.Connection` is created using stale `credentialKey.access_token` and `credentialKey.inst
packages/features/ee/workflows/api/scheduleEmailReminders.ts:57· reviewed in the fork celmis-bench/cal.diy#15·upstream pull request ↗
the review commentreminder.referenceId can be null on line 57 when cancelling reminders; calling client.request with a null batch_id throws an API error, causing the catch block on line 75 to trigger and abort deletion for all remaining items in remindersToCancel
verdict — real defect, absent from the gold setBoth links are proved. referenceId in the WorkflowReminder model is declared as `String? @unique` (schema.prisma:640), i.e. nullable. The remindersToCancel query (44-51) filters only on cancelled: true and scheduledDate, and does not exclude referenceId in any way. A reminder created without registration in SendGrid has referenceId = null, that will go into batch_id, and the request will fail. The consequence is right too: the try wraps the ENTIRE for loop, so the first such record breaks off processing of the rest — none of the following reminders will be deleted.
verdict — real defect, absent from the gold setThe fact is correct: console.log on line 76 on a production path, while cal.com has its own structured logger. This is exactly the class of finding the gold set systematically does not record — not because it is not there, but because the annotator wrote about logic, not about code hygiene. Trivial weight.
golden beside it, unmatcheddeclaration outside the excerpt
packages/emails/email-manager.ts:540· reviewed in the fork celmis-bench/cal.diy#19·upstream pull request ↗
the review commentAttendeeScheduledEmail is referenced on line 540 without being imported in email-manager.ts, throwing a ReferenceError at runtime when sendAddGuestsEmails is executed
verdict — genuinely wrongAttendeeScheduledEmail is imported on line 31: `import AttendeeScheduledEmail from "./templates/attendee-scheduled-email"`. It is already used in this same file on 116 and 264. A ReferenceError is impossible. Sixth appearance of the same pattern in the run — an "undeclared identifier" that is declared above the window: here the import is 509 lines away from the site of the finding.
why this classunmatched golden nearby (shared: sendAddGuestsEmails): The `sendAddGuestsEmails` function does not check the `disableStandardEmails` flags in the `EventTyp
verdict — unverifiableThe claim is conditional — "if attendees is empty" — and it is precisely that condition it does not prove. This array should not be empty: the "guests added to the event" email is sent for an existing booking, in which at least one attendee exists by construction. I did not find a path that yields an empty attendees; but there is no check in the code either, so the question cannot be closed conclusively without the call site above.
why this classunmatched golden nearby (shared: attendees): uniqueGuests filters out existing attendees and blacklisted emails but does not deduplicate duplicat
verdict — real defect, absent from the gold setThe same class as #65: console.log on 170 in production code. Here it is somewhat worse — the catch takes err and does NOT print it, leaving only the message "Error sending AddGuestsEmails" without the cause of the failure. The finding names only the console.log, not this detail.
apps/web/components/settings/DisableTwoFactorModal.tsx:112· reviewed in the fork celmis-bench/cal.diy#12·upstream pull request ↗
the review comment'Lost access' button in DisableTwoFactorModal triggers form submission because it defaults to type 'submit' inside the Form component instead of type='button', causing unintended API validation
verdict — genuinely wrongThe Button component defaults to type="button", not "submit": Button.tsx:125 sets `type = "button"` in the destructuring of the props, and on 142 it is put into the attribute. So the "Lost access" button does not submit the form, and the undesired validation described does not occur.
apps/web/components/settings/EnableTwoFactorModal.tsx:284· reviewed in the fork celmis-bench/cal.diy#12·upstream pull request ↗
the review commentDownload button at line 284 triggers form submission and validation error because it lacks type='button' and is inside Form component with handleSubmit={handleEnable}
verdict — genuinely wrongThe same root as in #69: Button defaults to type="button" (Button.tsx:125 sets `type = "button"` in the destructuring, 142 puts it into the attribute). The download button does not submit the form. A second firing from one incorrect assumption about a prop's default value.
packages/app-store/googlecalendar/lib/CalendarService.ts:255· reviewed in the fork celmis-bench/cal.diy#13·upstream pull request ↗
the review commentexternalCalendarId can be undefined on line 255; cal.externalId === externalCalendarId in the find predicate compares calendar external IDs against undefined, causing selectedCalendar to always resolve to undefined
verdict — real defect, absent from the gold setThe branch is dead, and the claim shows why. The ternary picks the second branch only when externalCalendarId is falsy — and that is precisely the branch whose predicate compares cal.externalId === externalCalendarId, i.e. against the same value just found to be falsy. A match is possible only for a calendar with the same empty externalId; in practice find always yields undefined, and selectedCalendar goes into the Google API undefined at line 261. The fallback path the branch was written for never works, not once.
why this classan unmatched golden nearby (shared: externalCalendarId, externalId): Logic error: when externalCalendarId is provided, you're searching for a calendar where externalId =
packages/app-store/googlecalendar/lib/CalendarService.ts:315· reviewed in the fork celmis-bench/cal.diy#13·upstream pull request ↗
the review commentexternalCalendarId can be undefined on line 315; cal.externalId === externalCalendarId in the find predicate compares calendar external IDs against undefined, causing calendarId to always resolve to undefined
300| return resolve(evt?.data); 301| } 302| ); 303| }); 304| } 305| 306| async deleteEvent(uid: string, event: CalendarEvent, externalCalendarId?: string | null): Promise<void> { 307| return new Promise(async (resolve, reject) => { 308| const myGoogleAuth = await this.auth.getToken(); 309| const calendar = google.calendar({ 310| version: "v3", 311| auth: myGoogleAuth, 312| }); 313| 314| const defaultCalendarId = "primary"; 315| const calendarId = externalCalendarId 316| ? externalCalendarId 317| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; 318| 319| calendar.events.delete( 320| { 321| auth: myGoogleAuth, 322| calendarId: calendarId ? calendarId : defaultCalendarId, 323| eventId: uid, 324| sendNotifications: false, 325| sendUpdates: "none", 326| }, 327| function (err: GoogleCalError | null, event) { 328| if (err) { 329| /** 330| * 410 is when an event is already deleted on the Google cal before on cal.com
verdict — real defect, absent from the gold setA second instance of the same dead branch, this time in event deletion (315-317). Not a duplicate: different method, different line, separate case. The consequence is milder than in #71 — line 322 has `calendarId ? calendarId : defaultCalendarId`, so undefined is substituted by "primary". So the bug is there, but it is masked by the fallback value, and that is exactly why it could survive for a long time.
why this classan unmatched golden nearby (shared: externalCalendarId, externalId): Logic error: when externalCalendarId is provided, you're searching for a calendar where externalId =
packages/core/EventManager.ts:378· reviewed in the fork celmis-bench/cal.diy#13·upstream pull request ↗
the review commentdestination.externalId is not passed to createEvent on line 378 when destination.credentialId is absent, causing externalId to be undefined in the returned EventResult
verdict — unverifiableThe fact is right, the defect is not proven. In the branch at 370 destination.externalId is passed to createEvent, in the branch at 380 it is not, and externalId in the result will indeed be left undefined. But the asymmetry has an obvious explanation: in the second branch there is no specific account, and the event is created for EVERY credential that matched on integration type. Passing them all a single externalId would mean trying to create the event in a calendar belonging to a different account. So this is design rather than oversight — but the claim does not consider that possibility.
why this classan unmatched golden nearby (shared: credentialId, externalId): Logic error: when externalCalendarId is provided, you're searching for a calendar where externalId =
packages/features/bookings/lib/handleNewBooking.ts:1077· reviewed in the fork celmis-bench/cal.diy#13·upstream pull request ↗
the review commentWhen eventType.destinationCalendar and organizerUser.destinationCalendar are both null on line 1063, evt.destinationCalendar is set to null, causing evt.destinationCalendar?.push on line 1077 to short-circuit and discard teamDestinationCalendars
1062| conferenceCredentialId, 1063| destinationCalendar: eventType.destinationCalendar 1064| ? [eventType.destinationCalendar] 1065| : organizerUser.destinationCalendar 1066| ? [organizerUser.destinationCalendar] 1067| : null, 1068| hideCalendarNotes: eventType.hideCalendarNotes, 1069| requiresConfirmation: requiresConfirmation ?? false, 1070| eventTypeId: eventType.id, 1071| // if seats are not enabled we should default true 1072| seatsShowAttendees: eventType.seatsPerTimeSlot ? eventType.seatsShowAttendees : true, 1073| seatsPerTimeSlot: eventType.seatsPerTimeSlot, 1074| schedulingType: eventType.schedulingType, 1075| }; 1076| 1077| if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") { 1078| evt.destinationCalendar?.push(...teamDestinationCalendars); 1079| } 1080| 1081| /* Used for seats bookings to update evt object with video data */ 1082| const addVideoCallDataToEvt = (bookingReferences: BookingReference[]) => { 1083| const videoCallReference = bookingReferences.find((reference) => reference.type.includes("_video")); 1084| 1085| if (videoCallReference) { 1086| evt.videoCallData = { 1087| type: videoCallReference.type, 1088| id: videoCallReference.meetingId, 1089| password: videoCallReference?.meetingPassword, 1090| url: videoCallReference.meetingUrl, 1091| }; 1092| }
verdict — real defect, absent from the gold setBoth links are in the code. destinationCalendar at 1063-1067 is set as eventType.destinationCalendar ? […] : organizerUser.destinationCalendar ? […] : null — so null is entirely possible. Line 1078 has evt.destinationCalendar?.push(...teamDestinationCalendars), and with null the optional call silently does nothing. The team members' calendars, gathered at 987-993, simply disappear in the process. The case is real: a collective team event where neither the event type nor the organiser has a destination calendar of their own, while the members do.
same line as another findingseverity: mediumcross-file claim
packages/app-store/vital/lib/reschedule.ts:125· reviewed in the fork celmis-bench/cal.diy#11·upstream pull request ↗
the review commentArray.prototype.forEach is passed an async callback without awaiting returned promises at packages/app-store/wipemycalother/lib/reschedule.ts:125, causing operations to complete prematurely
110| const director = new CalendarEventDirector(); 111| director.setBuilder(builder); 112| director.setExistingBooking(bookingToReschedule as unknown as Booking); 113| director.setCancellationReason(cancellationReason); 114| await director.buildForRescheduleEmail(); 115| // Handling calendar and videos cancellation 116| // This can set previous time as available, until virtual calendar is done 117| const credentialsMap = new Map(); 118| userOwner.credentials.forEach((credential) => { 119| credentialsMap.set(credential.type, credential); 120| }); 121| const bookingRefsFiltered: BookingReference[] = bookingToReschedule.references.filter( 122| (ref) => !!credentialsMap.get(ref.type) 123| ); 124| try { 125| bookingRefsFiltered.forEach(async (bookingRef) => { 126| if (bookingRef.uid) { 127| if (bookingRef.type.endsWith("_calendar")) { 128| const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); 129| return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); 130| } else if (bookingRef.type.endsWith("_video")) { 131| return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); 132| } 133| } 134| }); 135| } catch (error) { 136| if (error instanceof Error) { 137| logger.error(error.message); 138| } 139| } 140|
verdict — real defect, absent from the gold setforEach ignores the returned promise: the callback at 125 is async, inside it an await getCalendar and the deleteEvent/deleteMeeting calls, but forEach itself returns control immediately. Deleting calendar events and meetings becomes fire-and-forget — the function moves on without waiting for a single one. A classic and well-known trap, and here it sits on the booking cancellation path, so the consequence is events left behind in other people's calendars.
why this classthe same line as the candidate that was counted
same line as another findingseverity: mediumcross-file claim
packages/app-store/vital/lib/reschedule.ts:125· reviewed in the fork celmis-bench/cal.diy#11·upstream pull request ↗
the review commentArray.prototype.forEach is passed an async callback without awaiting returned promises at packages/features/bookings/lib/handleCancelBooking.ts:460, causing operations to complete prematurely
110| const director = new CalendarEventDirector(); 111| director.setBuilder(builder); 112| director.setExistingBooking(bookingToReschedule as unknown as Booking); 113| director.setCancellationReason(cancellationReason); 114| await director.buildForRescheduleEmail(); 115| // Handling calendar and videos cancellation 116| // This can set previous time as available, until virtual calendar is done 117| const credentialsMap = new Map(); 118| userOwner.credentials.forEach((credential) => { 119| credentialsMap.set(credential.type, credential); 120| }); 121| const bookingRefsFiltered: BookingReference[] = bookingToReschedule.references.filter( 122| (ref) => !!credentialsMap.get(ref.type) 123| ); 124| try { 125| bookingRefsFiltered.forEach(async (bookingRef) => { 126| if (bookingRef.uid) { 127| if (bookingRef.type.endsWith("_calendar")) { 128| const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); 129| return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); 130| } else if (bookingRef.type.endsWith("_video")) { 131| return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); 132| } 133| } 134| }); 135| } catch (error) { 136| if (error instanceof Error) { 137| logger.error(error.message); 138| } 139| } 140|
verdict — real defect, absent from the gold setA separate instance of the same defect in a DIFFERENT file, and I checked it: packages/features/bookings/lib/handleCancelBooking.ts:458-461 contains `.filter(...).forEach(async (credential) => { const calendar = await getCalendar(credential); …})`. So the claim points at a genuine location. Our system pinned it to reschedule.ts only because matching by word overlap has no file number [i.e. no notion of which file the finding is in] — that is a flaw in MY matching, not in the finding.
why this classthe same line as the candidate that was counted
same line as another findingseverity: mediumcross-file claim
packages/app-store/vital/lib/reschedule.ts:125· reviewed in the fork celmis-bench/cal.diy#11·upstream pull request ↗
the review commentArray.prototype.forEach is passed an async callback without awaiting returned promises at packages/trpc/server/routers/viewer/bookings.tsx:553, causing operations to complete prematurely
110| const director = new CalendarEventDirector(); 111| director.setBuilder(builder); 112| director.setExistingBooking(bookingToReschedule as unknown as Booking); 113| director.setCancellationReason(cancellationReason); 114| await director.buildForRescheduleEmail(); 115| // Handling calendar and videos cancellation 116| // This can set previous time as available, until virtual calendar is done 117| const credentialsMap = new Map(); 118| userOwner.credentials.forEach((credential) => { 119| credentialsMap.set(credential.type, credential); 120| }); 121| const bookingRefsFiltered: BookingReference[] = bookingToReschedule.references.filter( 122| (ref) => !!credentialsMap.get(ref.type) 123| ); 124| try { 125| bookingRefsFiltered.forEach(async (bookingRef) => { 126| if (bookingRef.uid) { 127| if (bookingRef.type.endsWith("_calendar")) { 128| const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); 129| return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); 130| } else if (bookingRef.type.endsWith("_video")) { 131| return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); 132| } 133| } 134| }); 135| } catch (error) { 136| if (error instanceof Error) { 137| logger.error(error.message); 138| } 139| } 140|
verdict — real defect, absent from the gold setA third instance, also checked: packages/trpc/server/routers/viewer/bookings.tsx:553 — `bookingRefsFiltered.forEach(async (bookingRef) => { … await getCalendar(…) … })`. Three different files, three genuine locations. The judge counted all three as false, because to it they are three similar texts about one PR.
why this classthe same line as the candidate that was counted
same line as another findingseverity: lowcross-file claim
packages/app-store/vital/lib/reschedule.ts:125· reviewed in the fork celmis-bench/cal.diy#11·upstream pull request ↗
the review commentErrors thrown within async forEach callbacks will not be caught by surrounding try/catch blocks
110| const director = new CalendarEventDirector(); 111| director.setBuilder(builder); 112| director.setExistingBooking(bookingToReschedule as unknown as Booking); 113| director.setCancellationReason(cancellationReason); 114| await director.buildForRescheduleEmail(); 115| // Handling calendar and videos cancellation 116| // This can set previous time as available, until virtual calendar is done 117| const credentialsMap = new Map(); 118| userOwner.credentials.forEach((credential) => { 119| credentialsMap.set(credential.type, credential); 120| }); 121| const bookingRefsFiltered: BookingReference[] = bookingToReschedule.references.filter( 122| (ref) => !!credentialsMap.get(ref.type) 123| ); 124| try { 125| bookingRefsFiltered.forEach(async (bookingRef) => { 126| if (bookingRef.uid) { 127| if (bookingRef.type.endsWith("_calendar")) { 128| const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); 129| return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); 130| } else if (bookingRef.type.endsWith("_video")) { 131| return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); 132| } 133| } 134| }); 135| } catch (error) { 136| if (error instanceof Error) { 137| logger.error(error.message); 138| } 139| } 140|
verdict — real defect, absent from the gold setCorrect, and it adds to #75 something that is not there. The try at 124 will catch nothing: forEach returns immediately, the try block finishes before the async callbacks have a chance to reject, and the rejections become unhandled at the process level. So the try/catch that is present creates a false sense of being protected — a separate observation, not a restatement of the previous one.
why this classthe same line as the candidate that was counted
packages/core/CalendarManager.ts:31· reviewed in the fork celmis-bench/cal.diy#11·upstream pull request ↗
the review commentgetCalendarCredentials in packages/core/CalendarManager.ts accesses appStore synchronously without await, causing app.variant to evaluate to undefined and silently filter out all calendar credentials
verdict — genuinely wrongRefuted by two lines of the code itself. Lines 24-25 have getApps(credentials).filter((app) => …) — the .filter chain is called DIRECTLY on the result. If getApps returned a promise, this would throw a TypeError straight away, not "silently filter out all the calendar credentials". So getApps is synchronous, app.variant at 29 is defined, and the described consequence does not exist.
Only my own false positives were audited. No other tool in the table was examined this way. The floor described above is under every row, not just mine: their false positives almost certainly contain a similar share of real defects that the annotator did not write down. It follows that the corrected precision cannot be compared with any other row in that table, in either direction. For comparison between tools the measured 48.0% is the honest number, because it is the same method applied to everyone, and it is the number that stays in the results table.
Offline is not the public leaderboard. Martian runs two benchmarks. The public leaderboard is the online one: a continuously refreshed sample of real pull requests pulled from GitHub Archive every week, scored on what developers actually did with the comments, and deliberately built so that no tool can have been trained on the PRs. Everything above is the offline one — 50 curated pull requests, a fixed gold set, an LLM judge. They measure different things on different data, and the numbers are not interchangeable in either direction. A rank on one says nothing about a rank on the other, and that holds when the mix-up would flatter me as much as when it would not.
One auditor, and not a disinterested one. I wrote the tool and I wrote the verdicts. There was no blind second pass and no second reader. The incentive runs one way: a verdict of "real" flatters the tool, and it is my judgement alone that separates "real" from "unverifiable". Several records carry that tension in the open rather than hiding it. The one high-severity "real" in the Keycloak set records, in its own reasoning, that if the author deliberately retargeted passkeys at the known-user case then this is intent and not a defect. Two others are marked real while their reasoning says the expression is an upstream formula copied into a new class, and that its only consequence is a dead branch. Someone stricter than me would have written those three down differently.
The audit inherits the problem it describes. The verdicts were formed from a 31-line window plus whatever else was pulled when that window was not enough. How much extra was pulled was my decision, case by case, and in at least two cases it reversed the verdict. A different auditor pulling more, or less, would not land on exactly 33.
One judge. These 79 are the false positives under one of the three judges. The other two produced 68 and 81; the sets overlap but are not identical, and I did not audit them.
Duplicates cut both ways. Eight of the 33 real ones are in the dup class — they repeat something already on the page. Credited as matches, they raise the corrected figure; if instead they are treated as findings a stricter deduplication would never have emitted, and removed from both the numerator and the denominator, the corrected precision is 68.1% rather than 69.7%. The choice does not move the number much, but it is a choice.
Nothing here touches recall. The 100 missed goldens are untouched by this exercise. Half of an F1 is recall, and this audit says nothing about that half.
Where this run placed
The run placed 17th of 50 under all three judges: F1 47.5% under claude-opus-4-5, 44.9% under claude-sonnet-4-5, 42.7% under gpt-5.2. The rank does not move across judges even though the F1 moves by 4.8 points.
That figure measures pull-request review on isolated single-repository pull requests, which is what the offline set contains.
Reproducing this
The comparison table is rebuilt by merging Martian's published per-tool evaluations with this run's evaluations for the same judge; no row is re-scored. The README gives the command as:
One honest note about that line: the script is not in the repository tree. It is a short merge over two JSON files — offline/results/<judge>/evaluations.json from the benchmark repository, which holds the other tools' rows as Martian published them, and eval_<judge>/evaluations.json from this run — summing tp, fp and fn per tool and sorting by F1. Anyone can redo that merge from those two files without the script.
Rescoring from scratch uses Martian's own pipeline from the offline/ directory of the benchmark repository — github.com/withmartian/code-review-benchmark, which publishes the 50 pull requests, the golden comments, the judge prompt and the per-tool results — in five documented steps: step0_fork_prs, step1_download_prs, step2_extract_comments, step2_5_dedup_candidates, step3_judge_comments, each invoked as uv run python -m code_review_benchmark.<step>. Producing the reviews to feed it is bench/run_reviews.sh <prs.txt> api in my repository, which calls analyzer review <pr> --post per pull request; bench/scripts/collect_celmis.py stands in for step 1, because the 50 pull requests live in a handful of forks rather than one fork each, and step 1 finds a tool's runs by parsing repository names.
The audit itself has no command, and I am not going to invent one. It is 79 verdicts written by hand. What is reproducible is the evidence under each: the file, the line, and a permalink pinned to the reviewed commit.
If you think a verdict is wrong
Thirty-eight of my own findings are marked wrong in the table above, and the causes behind 24 of them are named. The 33 marked real are the ones worth arguing about, and the argument is cheap to have: open the permalink, read the lines, and say which way it goes. A correction to a single verdict is more useful to me than agreement with the total, and if enough of them go the other way the total moves and this page should say so.
Built from audit_fp.json (79 records) and the evaluation file for judge
claude-sonnet-4-5-20250929. Single file, no external resources: no CDN, no remote fonts, no analytics, no network requests.
Verdicts and reasoning translated from the Ukrainian original.